Rename member webhook cluster scoped objects #562
Conversation
mfrancisc
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19 and
sbryzak
as code owners
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
|
controllers/memberoperatorconfig/memberoperatorconfig_controller.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Matous Jobanek <mjobanek@redhat.com>
Co-authored-by: Matous Jobanek <mjobanek@redhat.com>
Co-authored-by: Matous Jobanek <mjobanek@redhat.com>
… into testcicdscript
@MatousJobanek thanks for your review. I've added a couple of additional unit tests to cover:
|
|
/retest |
| 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.
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.
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.
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.
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.
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.
The test functions should be moved to the test file unless I'm missing something
There was a problem hiding this comment.
Good point! done in e07e190 thanks!
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
This PR contains:
old-namelabel 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-toolchainclustersonly after deploy the member operator.Paired with:
see: https://redhat-internal.slack.com/archives/C06MJ2DVBU4/p1713534677202559