Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

afbjorklund
Copy link
Collaborator

  • Add --install option to minikube kubectl command
  • Add --path option to minikube kubectl command
Usage:
  minikube kubectl [flags]

Flags:
  -h, --help            help for kubectl
      --install         Install kubectl and exit
      --path            Display kubectl path instead of running it
      --prefix string   Installation prefix (default "/usr/local")

For #4697

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 6, 2019
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 6, 2019
func init() {
kubectlCmd.Flags().BoolVar(&kubectlInstallMode, "install", false, "Install kubectl and exit")
kubectlCmd.Flags().StringVar(&kubectlInstallPrefix, "prefix", defaultPrefix, "Installation prefix")
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

@josedonizetti josedonizetti Jul 9, 2019

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.

Copy link
Collaborator Author

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

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")
Copy link
Member

@josedonizetti josedonizetti Jul 6, 2019

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

@afbjorklund
Copy link
Collaborator Author

AFAIK, Windows doesn't have the concept of a default PATH ?

You choose C:\any\random\path and then set up variables and crap.

So I suppose one would have to use --prefix ?

Or another approach, such as running in MinGW.

@afbjorklund afbjorklund added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jul 9, 2019
@tstromberg
Copy link
Contributor

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.

@tstromberg tstromberg closed this Jul 11, 2019
@afbjorklund
Copy link
Collaborator Author

Okay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants