-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 Fix clusterctl delete when deleting providers with cluster-wide resources #5420
🐛 Fix clusterctl delete when deleting providers with cluster-wide resources #5420
Conversation
@fabriziopandini PTAL when you've got some time. If it's valid to add a new func to the interface, I think this would be the cleaner solution and I would extend the fakeProxy accordingly and add new test cases to the If it's not, I only see the option to add magic to the ListResource func. This would work roughly work like that:
P.S. Also fine for me if we want to implement the second option just to avoid the additional func in the interface. |
ab56cff
to
36d6804
Compare
f420262
to
1677e60
Compare
/test pull-cluster-api-e2e-workload-upgrade-1-22-latest-main @fabriziopandini Ready for review. Adding unit tests would have been too much effort as ListResources creates clients based on a kubeconfig. So I think I would have had to use testenv. I added a delete in the clusterctl upgrade test to ensure it still works. Once CAPA picks up this change, CAPA also verifies that our issue is fixed (I also tested it manually with the fix as described in the linked issue) |
resources Signed-off-by: Stefan Büringer [email protected]
1677e60
to
f5a9d76
Compare
/test pull-cluster-api-e2e-workload-upgrade-1-22-latest-main |
/lgtm |
I will take a look tomorrow :) |
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.
/approve
/hold for @ykakarap
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
/lgtm |
@vincepri I reviewed and LGTM's the PR. You can go ahead and cancel the hold. |
/hold cancel |
What this PR does / why we need it:
WIP: we can't add new types of the cluster package to the signature as this leads to cyclic dependencies with the fake proxy implementation.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #5417