-
Notifications
You must be signed in to change notification settings - Fork 39
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
Rename member webhook cluster scoped objects #562
Rename member webhook cluster scoped objects #562
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #562 +/- ##
==========================================
- Coverage 84.06% 83.09% -0.98%
==========================================
Files 29 29
Lines 2636 2673 +37
==========================================
+ Hits 2216 2221 +5
- Misses 277 307 +30
- Partials 143 145 +2
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add unit tests for the deletion part of the code?
controllers/memberoperatorconfig/memberoperatorconfig_controller.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Matous Jobanek <[email protected]>
Co-authored-by: Matous Jobanek <[email protected]>
Co-authored-by: Matous Jobanek <[email protected]>
… into testcicdscript
@MatousJobanek thanks for your review. I've added a couple of additional unit tests to cover:
|
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please revert the VM-related changes for this PR to ensure it has no impact?
deleted, err := deploy.Delete(ctx, r.Client, r.Client.Scheme(), namespace, false) | ||
if err != nil { | ||
return err | ||
} | ||
if deleted { | ||
logger.Info("Deleted previously deployed webhook app") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a change in behaviour such that if Webhook.Deploy is set to false in the member operator config then the webhook deployment will be deleted. I think the configuration was meant for stopping the member operator from deploying/redeploying the webhook automatically but without deleting any existing ones. I think that's desirable for debugging scenarios. Any reason why we want to change that behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because we now deploy the member operator before we disable the configuration see: https://github.com/codeready-toolchain/toolchain-e2e/pull/950/files#diff-163703aeb25751c1605d305fbfdf39f3a9dfb74d2ac9d8a03f5cf0169d7d41b7R292 .
See also my comment here : https://github.com/codeready-toolchain/toolchain-e2e/pull/950/files#r1580269009
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, I see your point now. I think the webhook provides some pretty important functionality that I'm not sure we'd want to delete unless it's explicitly meant to be deleted but I can't think of a reason why we'd disable the configuration and expect the webhook to stay as it was so I guess it's fine. I'd just suggest we update the comment in the api to make it clear what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just suggest we update the comment in the api to make it clear what happens.
good point, please see https://github.com/codeready-toolchain/api/pull/425/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the webhook provides some pretty important functionality that I'm not sure we'd want to delete unless it's explicitly meant to be deleted but I can't think of a reason why we'd disable the configuration and expect the webhook to stay
I was discussing this with @MatousJobanek, a better approach IMO would be to decouple the deployment of the webhooks from member operator , this would allow to have more flexibility and would avoid such "workarounds" where we deploy something and then we need to delete it. But the decoupling of the webhooks is a bigger effort and not on the priority list atm.
pkg/webhook/deploy/deployment.go
Outdated
@@ -3,11 +3,19 @@ package deploy | |||
import ( | |||
"context" | |||
"encoding/base64" | |||
"encoding/json" | |||
"fmt" | |||
"testing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test functions should be moved to the test file unless I'm missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! done in e07e190 thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, mfrancisc, rajivnathan 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 |
Co-authored-by: Matous Jobanek <[email protected]>
Quality Gate passedIssues Measures |
This PR contains:
old-name
label used to delete the objects using the current name ( since we are redeploying with new name )The renaming of the cluster scoped objects is needed in order to be able to delete the member2 webhook configuration during e2e tests.
We cannot avoid deploying the webhook on member2 anymore since we inverted the order of the make targets in https://github.com/codeready-toolchain/toolchain-e2e/pull/950/files#diff-163703aeb25751c1605d305fbfdf39f3a9dfb74d2ac9d8a03f5cf0169d7d41b7R266 , due to the fact that member operator is now deploying the toolchaincluster-sa and we need to verify and use the toolchaincluster-member SA in
setup-toolchainclusters
only after deploy the member operator.Paired with:
see: https://redhat-internal.slack.com/archives/C06MJ2DVBU4/p1713534677202559