Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deleting "shipwright-build-webhook" clusterrole and clusterrolebinding #214

Merged

Conversation

ayushsatyam146
Copy link
Contributor

@ayushsatyam146 ayushsatyam146 commented May 27, 2024

Implementing a deleteObjectsIfPresent function and checking for presence of every given resource and then deleting the given resource. Using this to delete "shipwright-build-webhook" clusterrole and clusterrolebinding. Addressing #181

Signed-off by : Ayush Satyam [email protected]

Changes

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Deleting "shipwright-build-webhook" clusterrole and clusterrolebinding from the operator. 
Updating the `kodata/release.yaml` to v0.13.0 https://github.com/shipwright-io/build/releases/tag/v0.13.0) 
and updating all the references of v1alpha1 API to v1beta1 API.

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 27, 2024
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 30, 2024
Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

I added a few suggestions for comments and other nits - the logic to delete the webhook RBAC otherwise looks good to me. Adding "approve" so the rest of the community can add "lgtm" without me.

/hold

This PR needs to update the kodata/release.yaml file to a v0.13.x release version. We currently have a v0.12.x version in-tree which includes the webhook rolebindings. If this code merges, I suspect the operator will hotloop trying to create and delete the RBAC objects.

Comment on lines 255 to 258

err = deleteObjectsIfPresent(ctx, r.Client, []client.Object{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have comment here explaining why this code exists:

Suggested change
err = deleteObjectsIfPresent(ctx, r.Client, []client.Object{
// Builds 0.12.0 created a ClusterRole and ClusterRolebinding for the Build API conversion webhook.
// These were removed in v0.13.0 - when upgrading, these should be removed if present.
err = deleteObjectsIfPresent(ctx, r.Client, []client.Object{

&rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: "shipwright-build-webhook"}},
})
if err != nil {
logger.Error(err, "deleting'shipwright-build-webhook' role and cluster role binding")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - spelling and avoid use of quote characters. Logger tends to use structured format:

Suggested change
logger.Error(err, "deleting'shipwright-build-webhook' role and cluster role binding")
logger.Error(err, "deleting shipwright-build-webhook ClusterRole and ClusterRoleBinding")

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2024
Copy link
Contributor

openshift-ci bot commented Jun 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2024
@adambkaplan
Copy link
Member

@ayushsatyam146 please also fill in the release note in the pull request description. This PR will include significant changes that warrant a release note for end users/admins.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 4, 2024
@adambkaplan
Copy link
Member

adambkaplan commented Jun 4, 2024

I was able to dig into the test failures so far - there's actually a few things going on here:

  1. The sample build strategies are using the v1alpha1 api. These need to be migrated to the v1beta1 API because we switched the default storage to v1beta1, and Envtest does not support conversion webhooks.
    1. The easiest way to migrate the build strategies is to delete the separate sub-directories under kodata/samples/buildstrategies, and replace them with the v0.13.0 sample strategies yaml. If you save this file under kodata/samples/buildstrategies, no code changes are required in the operator.
    2. Doing this is not sufficient - the test code currently has its own mechanisms for extracting the build strategy names. This broke in my experiment - I replaced that logic with manifestival in Parse Build Strategies with Manifestival #215, which has far more robust code for this sort of thing.
  2. The shipwright-io/build dependencies need to be updated to v0.13.0 - this is a standard golang bump go get github.com/shipwright-io/[email protected] && go mod tidy && go mod vendor
  3. Finally, any references to the shipwright-io/build v1alpha1 API should use the v1beta1 APIs instead.

My recommendation - let's review and merge #215, then rebase this PR on top to include the other changes I outlined here.

Implementing a deleteObjectsIfPresent function and checking
for presence of every resource and then deleting the given resource.
Using this to delete "shipwright-build-webhook" clusterrole and clusterrolebinding

Signed-off by : Ayush Satyam<[email protected]>
We currently have a v0.12.x version in-tree which includes
the webhook rolebindings. We are updating the
`kodata/release.yaml`
with the latest v0.13.0 release.

Signed-off by : Ayush Satyam<[email protected]>
of shipwright-io/build. Updating the kodata/samples/buildstrategy files to use
https://github.com/shipwright-io/build/releases/download/v0.13.0/sample-strategies.yaml

Signed-off by : Ayush Satyam<[email protected]>
@pull-request-size pull-request-size bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 10, 2024
@ayushsatyam146
Copy link
Contributor Author

/assign @apoorvajagtap

@adambkaplan
Copy link
Member

/hold cancel

Rebased on top of my dependency PRs.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2024
Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Shipwright build version updated, along with the samples! 🎉 🎉 🎉

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit a35fed8 into shipwright-io:main Jun 10, 2024
4 checks passed
@adambkaplan adambkaplan added this to the release-v0.13.0 milestone Jun 13, 2024
@adambkaplan adambkaplan linked an issue Jun 13, 2024 that may be closed by this pull request
1 task
@adambkaplan adambkaplan added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FEATURE] Handle deletion of roles for webhook deployment
3 participants