-
Notifications
You must be signed in to change notification settings - Fork 413
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
feat(cli): implement platform destroy command #2261
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for vcluster-docs canceled.Built without sensitive environment variables
|
474e568
to
fa9d5e1
Compare
Signed-off-by: Rohan CJ <[email protected]>
fa9d5e1
to
4b4293b
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.
Why are we putting the delete command into the start folder?
That seems a bit counter-intuitive 😅
@@ -141,7 +161,7 @@ func (l *LoftStarter) prepare() error { | |||
contextToLoad = l.Context | |||
} else if platformConfig.LastInstallContext != "" && platformConfig.LastInstallContext != contextToLoad { | |||
contextToLoad, err = l.Log.Question(&survey.QuestionOptions{ | |||
Question: product.Replace("Seems like you try to use 'loft start' with a different kubernetes context than before. Please choose which kubernetes context you want to use"), | |||
Question: product.Replace(fmt.Sprintf("Seems like you try to use 'loft %s' with a different kubernetes context than before. Please choose which kubernetes context you want to use", l.CommandName)), |
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 we also rename the command from 'loft %s'
to 'vcluster platform %s'
here?
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.
will fix!
}, | ||
} | ||
|
||
startCmd := &cobra.Command{ |
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 guess you meant destroyCmd
😉
|
||
startCmd.Flags().StringVar(&cmd.Context, "context", "", "The kube context to use for installation") | ||
startCmd.Flags().StringVar(&cmd.Namespace, "namespace", "", "The namespace vCluster platform is installed in") | ||
startCmd.Flags().BoolVar(&cmd.DeleteNamespace, "delete-namespace", true, "Whether to delete the namespace or not") |
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.
Feel free to ignore: Might be handy to have something like --ignore-not-found
, which does not error when there's no platfrom.
@@ -0,0 +1,248 @@ | |||
package start |
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 guess this should be in a destroy package!?
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.
yeah, makes sense! I can put the client initializers and validation in the clihelper
) | ||
|
||
// define the order of resource deletion | ||
var resourceOrder = []string{ |
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.
Just stating the obvious, but this means we always have to keep the resources in sync manually here because we have to respect the ordering!?
Do you know what would happen if we would just delete all resources coming from discoveryClient.ServerResourcesForGroupVersion("storage.loft.sh/v1")
with a Background deletion policy without a specific order?!
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.
Yeah, I think we need to do some e2e where we run vclusterctl commands against a platform instance.
I think the deletion would hang in most cases if we did an unordered delete because many things like accesskeys don't have finalizers
I'll move it into a destroy folder and move some of the common options out into clihelper |
What issue type does this pull request address? (keep at least one, remove the others)
/kind feature
What does this pull request do? Which issues does it resolve? (use
resolves #<issue_number>
if possible)resolves ENG-3980
Please provide a short message that should be published in the vcluster release notes
Add a new cli command: vcluster platform destroy
What else do we need to know?
The command will wait for each resource type to finish cleaning up before proceeding to the next, so that resource types that depend on each other will not prevent cleanup. (Remove a cluster object before a virtualclusterinstance running on it, etc)
vcluster platform destroy
(no args) on v4.0.0:vcluster platform destroy
(no args) on v3.4.9: