-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add 'cluster', 'namespace', and 'user' options to delete command. Enhance delete to load provider components from cluster. #384
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,12 @@ Usage: | |
clusterctl delete cluster [flags] | ||
|
||
Flags: | ||
--cluster string The name of the kubeconfig cluster to use | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This help text isn't super clear without reading the code. Maybe "The name of the cluster to use in the kubeconfig file"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We get this help text for 'free' from the libraries so it matches what kubectl has for help text (and actually the entire ecosystem of CLIs that are using the same client-go libraries). I do agree the message isn't super clear. However, there are some nice things about providing a consistent experience as users who are familiar with kubectl would know exactly what this means. So I see three options:
Of these options I think I like #2 the best as it is the least work and the only users right now are early adopters. It will be less work because we don't have to remember to remove our custom message later once client-go picks up the new messaging. However, I'm happy to do #1 if you think that is best. What do you think? (or do you have another idea?). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your second option seems reasonable to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will follow up with another PR in kube core -- I have an open issue here: #397 |
||
-h, --help help for cluster | ||
--kubeconfig string Path to the kubeconfig file to use for connecting to the cluster to be deleted, if empty, the default KUBECONFIG load path is used. | ||
-n, --namespace string If present, the namespace scope for this CLI request | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is namespace for the kubectl apply commands, which isn't super clear from the help text. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is for kubectl apply true, but will also be the namespace for everything |
||
-p, --provider-components string A yaml file containing cluster api provider controllers and supporting objects, if empty the value is loaded from the cluster's configuration store. | ||
--user string The name of the kubeconfig user to use | ||
|
||
Global Flags: | ||
--alsologtostderr log to standard error as well as files | ||
|
This file was deleted.
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.
Does createTempFile handle removing the file automatically? I'm guessing not since we call os.Remove here. Which means that if there is an error creating the client we leak the file. We need to defer removing the file so that it gets cleaned up even on the error return paths.
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 created a PR here: #390, which addresses this issue in two places. Note that there is still an opportunity to leak files if the user prematurely terminates the program or an automated system kills the VM / container. So in any sort of automated system we will need cleanup processes that handle leaked 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.
PR #390 merged and I have rebased this commit on top.