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

adding kubectl test for minikube #398

Closed
wants to merge 4 commits into from

Conversation

ramitsurana
Copy link

No description provided.

@ramitsurana ramitsurana changed the title adding installation test for kubectl adding kubectl test for minikube Jul 26, 2016
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

fmt.Printf("Checking for Kubectl ...\n")
path, err := exec.LookPath("kubectl")
if err != nil {
log.Fatal("Kubectl is not installed.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you switch this to something like:

glog.Errorln("Kubectl is not installed.")
os.Exit(1)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually lets just leave this as a warning, kubectl isn't the only way to interact with kubernetes so we shouldn't error if the user doesn't have it installed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if we can add a better help message to direct where to download it from?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jimmidyson for the suggestion.I will add some nice suggestion for kubectl.

@dlorenc
Copy link
Contributor

dlorenc commented Jul 26, 2016

Thanks for the PR! I left a few comments.

@ramitsurana
Copy link
Author

Thanks @dlorenc I knew it needed improvement ! I will make the changes accordingly.

fmt.Printf("Checking for Kubectl ...\n")
if _, err := exec.LookPath("kubectl") {
glog.Errorln("For more info on kubectl visit http://kubernetes.io/docs/getting-started-guides/minikube/#download-kubectl")
os.Exit(1)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if the url can be shortened.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you drop the os.Exit(1) here? I think this should just log a warning instead of an error.

@@ -102,6 +103,14 @@ func runStart(cmd *cobra.Command, args []string) {
kubeHost = strings.Replace(kubeHost, ":2376", ":"+strconv.Itoa(constants.APIServerPort), -1)
fmt.Printf("Kubernetes is available at %s.\n", kubeHost)

//Checking for kubectl
fmt.Printf("Checking for Kubectl ...\n")
if _, err := exec.LookPath("kubectl") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, you'll need:

if _, err := exec.LookPath("kubectl"); err != nil {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still Failing :( .Could you please check again ?

@jberkus
Copy link

jberkus commented Jul 27, 2016

This is a good first step. However:

  1. we also need to verify the version of Kubectl.
  2. I'll argue that we should simply download and install it instead of forcing users to take multiple steps to do it themselves.

@aaron-prindle
Copy link
Contributor

aaron-prindle commented Jul 27, 2016

@ramitsurana the travis test is failing due to the file format not being compliant with gofmt:

From Travis:

Checking gofmt...
--- /dev/fd/63  2016-07-27 16:37:29.696912317 +0000
+++ /dev/fd/62  2016-07-27 16:37:29.696912317 +0000
@@ -0,0 +1 @@
+cmd/minikube/cmd/start.go
make: *** [test] Error 1

The file difference:

aprindle@aprindle:~/go/src/k8s.io/minikube/cmd/minikube/cmd$ gofmt -d start.go
diff start.go gofmt/start.go
--- /tmp/gofmt123843528 2016-07-27 15:07:32.963388318 -0700
+++ /tmp/gofmt412009607 2016-07-27 15:07:32.963388318 -0700
@@ -106,11 +106,10 @@
        //Checking for kubectl
        fmt.Printf("Checking for Kubectl ...\n")
        if _, err := exec.LookPath("kubectl"); err != nil {
-               glog.Errorln("For more info on kubectl visit http://kubernetes.io/docs/getting-started-guides/minikube/#download-kubectl")
+               glog.Errorln("For more info on kubectl visit http://kubernetes.io/docs/getting-started-guides/minikube/#download-kubectl")
        }
        fmt.Printf("Kubectl is configured and installed.")
-
-
+
        // setup kubeconfig
        name := constants.MinikubeContext
        certAuth := constants.MakeMiniPath("ca.crt")

To fix this, run gofmt on start.go with:

gofmt -w start.go

@dlorenc
Copy link
Contributor

dlorenc commented Jul 28, 2016

This is a good first step. However:

we also need to verify the version of Kubectl.

I'll argue that we should simply download and install it instead of forcing users to take multiple steps to do it themselves.

Yeah, we plan on bundling kubectl. Right now we're holding off until we get our installer story straightened out, that work is tracked here: #402

@dlorenc
Copy link
Contributor

dlorenc commented Aug 3, 2016

I'm going to close this for now. Please re-open if you get some time to finish it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants