-
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
🌱 Allows delelting multiple providers at once #3617
🌱 Allows delelting multiple providers at once #3617
Conversation
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.
@gab-satchi question
Looking at the implementation I see we are fixing the issue of clusterctl deleting one provider for each run, but I can't see this part of the issue being addressed:
Perhaps it would be convenient to do something like clusterctl delete --all --namespace foobaz to delete all the providers within the specified namespace.
If we are not going to address this, might be we should prevent--all and --namespace to be used together. WDYT?
I missed the all case. /hold |
e0cdf74
to
88870a6
Compare
/unhold |
88870a6
to
4675d87
Compare
failing due to known flakes. /test pull-cluster-api-test |
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.
some nits but otherwise lgtm
@@ -68,6 +68,18 @@ var deleteCmd = &cobra.Command{ | |||
# Cluster API Providers are orphaned and there might be ongoing costs incurred as a result of this. | |||
clusterctl delete --all | |||
|
|||
# Deletes all the providers in a namespace | |||
# Important! As a consequence of this operation, all the corresponding resources managed by |
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 goes beyond this PR but I think we might be diluting the importance of "Important!" by saying it 6 times 😄
4675d87
to
ff71a62
Compare
/test pull-cluster-api-make |
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
- allows for deleting all providers in a specific namespace
ff71a62
to
b315908
Compare
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
/assign @wfernandes |
Reviewing |
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 ran it locally and it seems to work as expected including the --all
flag.
/lgtm
What this PR does / why we need it:
Clusterctl delete currently would break out of loop after finding one provider to delete and only delete that provider. The cli has grown to provide flags for core, bootstrap and controlplane providers along with infrastructure. With these changes, any combination of those providers can be deleted.
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 #3558