-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add an install option to kubectl command #4698
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afbjorklund 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 |
func init() { | ||
kubectlCmd.Flags().BoolVar(&kubectlInstallMode, "install", false, "Install kubectl and exit") | ||
kubectlCmd.Flags().StringVar(&kubectlInstallPrefix, "prefix", defaultPrefix, "Installation prefix") |
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.
won't this default prefix be a problem for windows?
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.
Probably, but haven't used anything but MSYS myself.
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'm not a windows user as well. It worries me that downloading kubeclt
currently works on all platforms, and now considering this default prefix it will break in windows. Does it make sense to only use the prefix when in non-windows systems? It basically does the previous behavior for windows, and we add an issue for someone else to implement the windows support.
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 can add another prefix (already have a suffix...), just not sure what it would be
cmd/minikube/cmd/kubectl.go
Outdated
func init() { | ||
kubectlCmd.Flags().BoolVar(&kubectlInstallMode, "install", false, "Install kubectl and exit") | ||
kubectlCmd.Flags().StringVar(&kubectlInstallPrefix, "prefix", defaultPrefix, "Installation prefix") | ||
kubectlCmd.Flags().BoolVar(&kubectlPathMode, "path", false, "Display kubectl path instead of running it") |
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.
without reading the help command I would consider that path
would be the place to install, pretty much like prefix
.
what do you think about --which
cos it does the same thing as which kubectl
, right? probably wouldn't make the same sense for a windows user.
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, that is one idea. I copied --url
(from dashboard), and then it became --path
.
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.
Think --which
can be misleading, since it is normally associated with PATH (which it is not in). Will go for something like --print
, and hope that there is no confusion.
AFAIK, Windows doesn't have the concept of a default PATH ? You choose So I suppose one would have to use Or another approach, such as running in MinGW. |
I prefer that minikube not be in the business of installing system binaries when possible. I don't mind downloading kubectl to our own folder, but I feel that this duty is better left to a package manager. |
Okay |
For #4697