From 994d093766c018f0f501d04cf2d85d9c49966cf7 Mon Sep 17 00:00:00 2001 From: Nanik T Date: Thu, 24 Oct 2019 22:24:42 +1100 Subject: [PATCH] Add boolean to disable opening browser during test The WaitAndMaybeOpenService(..) method allow user to open the service url in a browser when found, this create issue during testing as it's opening browser and consuming resources. The fix is to introduce a boolean flag allowing the caller to specify whether to just print out the url or open a browser window --- cmd/minikube/cmd/config/open.go | 16 +++++++++++- cmd/minikube/cmd/service.go | 20 ++++++++++++++- pkg/minikube/service/service.go | 27 +++++++++----------- pkg/minikube/service/service_test.go | 37 +++++++++++++++++++++++----- 4 files changed, 77 insertions(+), 23 deletions(-) diff --git a/cmd/minikube/cmd/config/open.go b/cmd/minikube/cmd/config/open.go index 8a852729cebf..458950b91728 100644 --- a/cmd/minikube/cmd/config/open.go +++ b/cmd/minikube/cmd/config/open.go @@ -17,9 +17,12 @@ limitations under the License. package config import ( + "fmt" "os" "text/template" + "github.com/pkg/browser" + "github.com/spf13/cobra" "k8s.io/minikube/pkg/minikube/assets" "k8s.io/minikube/pkg/minikube/cluster" @@ -95,9 +98,20 @@ You can add one by annotating a service with the label {{.labelName}}:{{.addonNa } for i := range serviceList.Items { svc := serviceList.Items[i].ObjectMeta.Name - if err := service.WaitAndMaybeOpenService(api, namespace, svc, addonsURLTemplate, addonsURLMode, https, wait, interval); err != nil { + var urlString []string + + if urlString, err = service.WaitForService(api, namespace, svc, addonsURLTemplate, addonsURLMode, https, wait, interval); err != nil { exit.WithCodeT(exit.Unavailable, "Wait failed: {{.error}}", out.V{"error": err}) } + + if len(urlString) != 0 { + out.T(out.Celebrate, "Opening kubernetes service {{.namespace_name}}/{{.service_name}} in default browser...", out.V{"namespace_name": namespace, "service_name": svc}) + for _, url := range urlString { + if err := browser.OpenURL(url); err != nil { + exit.WithError(fmt.Sprintf("browser failed to open url %s", url), err) + } + } + } } }, } diff --git a/cmd/minikube/cmd/service.go b/cmd/minikube/cmd/service.go index d0068a23da32..66e7fa8335c6 100644 --- a/cmd/minikube/cmd/service.go +++ b/cmd/minikube/cmd/service.go @@ -17,9 +17,14 @@ limitations under the License. package cmd import ( + "fmt" "os" "text/template" + "github.com/pkg/browser" + + "k8s.io/minikube/pkg/minikube/out" + "github.com/spf13/cobra" "k8s.io/minikube/pkg/minikube/cluster" "k8s.io/minikube/pkg/minikube/exit" @@ -68,11 +73,24 @@ var serviceCmd = &cobra.Command{ if !cluster.IsMinikubeRunning(api) { os.Exit(1) } - err = service.WaitAndMaybeOpenService(api, namespace, svc, + + var urlString []string + + urlString, err = service.WaitForService(api, namespace, svc, serviceURLTemplate, serviceURLMode, https, wait, interval) if err != nil { exit.WithError("Error opening service", err) } + + if len(urlString) != 0 { + out.T(out.Celebrate, "Opening kubernetes service {{.namespace_name}}/{{.service_name}} in default browser...", out.V{"namespace_name": namespace, "service_name": svc}) + + for _, url := range urlString { + if err := browser.OpenURL(url); err != nil { + exit.WithError(fmt.Sprintf("browser failed to open url %s", url), err) + } + } + } }, } diff --git a/pkg/minikube/service/service.go b/pkg/minikube/service/service.go index 58a7264731b3..388ab9fc92fd 100644 --- a/pkg/minikube/service/service.go +++ b/pkg/minikube/service/service.go @@ -29,7 +29,6 @@ import ( "github.com/docker/machine/libmachine" "github.com/golang/glog" "github.com/olekukonko/tablewriter" - "github.com/pkg/browser" "github.com/pkg/errors" "github.com/spf13/viper" core "k8s.io/api/core/v1" @@ -265,9 +264,11 @@ func PrintServiceList(writer io.Writer, data [][]string) { table.Render() } -// WaitAndMaybeOpenService waits for a service, and opens it when running -func WaitAndMaybeOpenService(api libmachine.API, namespace string, service string, urlTemplate *template.Template, urlMode bool, https bool, - wait int, interval int) error { +// WaitForService waits for a service, and return the urls when available +func WaitForService(api libmachine.API, namespace string, service string, urlTemplate *template.Template, urlMode bool, https bool, + wait int, interval int) ([]string, error) { + + var urlList []string // Convert "Amount of time to wait" and "interval of each check" to attempts if interval == 0 { interval = 1 @@ -275,12 +276,12 @@ func WaitAndMaybeOpenService(api libmachine.API, namespace string, service strin chkSVC := func() error { return CheckService(namespace, service) } if err := retry.Expo(chkSVC, time.Duration(interval)*time.Second, time.Duration(wait)*time.Second); err != nil { - return errors.Wrapf(err, "Service %s was not found in %q namespace. You may select another namespace by using 'minikube service %s -n ", service, namespace, service) + return urlList, errors.Wrapf(err, "Service %s was not found in %q namespace. You may select another namespace by using 'minikube service %s -n ", service, namespace, service) } serviceURL, err := GetServiceURLsForService(api, namespace, service, urlTemplate) if err != nil { - return errors.Wrap(err, "Check that minikube is running and that you have specified the correct namespace") + return urlList, errors.Wrap(err, "Check that minikube is running and that you have specified the correct namespace") } if !urlMode { @@ -295,22 +296,18 @@ func WaitAndMaybeOpenService(api libmachine.API, namespace string, service strin if len(serviceURL.URLs) == 0 { out.T(out.Sad, "service {{.namespace_name}}/{{.service_name}} has no node port", out.V{"namespace_name": namespace, "service_name": service}) - return nil + return urlList, nil } for _, bareURLString := range serviceURL.URLs { - urlString, isHTTPSchemedURL := OptionallyHTTPSFormattedURLString(bareURLString, https) + url, isHTTPSchemedURL := OptionallyHTTPSFormattedURLString(bareURLString, https) if urlMode || !isHTTPSchemedURL { - out.T(out.Empty, urlString) - } else { - out.T(out.Celebrate, "Opening kubernetes service {{.namespace_name}}/{{.service_name}} in default browser...", out.V{"namespace_name": namespace, "service_name": service}) - if err := browser.OpenURL(urlString); err != nil { - out.ErrT(out.Empty, "browser failed to open url: {{.error}}", out.V{"error": err}) - } + out.T(out.Empty, url) + urlList = append(urlList, url) } } - return nil + return urlList, nil } // GetServiceListByLabel returns a ServiceList by label diff --git a/pkg/minikube/service/service_test.go b/pkg/minikube/service/service_test.go index 0d4d13fe571e..01426ddfac62 100644 --- a/pkg/minikube/service/service_test.go +++ b/pkg/minikube/service/service_test.go @@ -885,6 +885,15 @@ func TestWaitAndMaybeOpenService(t *testing.T) { api: defaultAPI, urlMode: true, https: true, + expected: []string{"https://127.0.0.1:1111", "https://127.0.0.1:2222"}, + }, + { + description: "correctly return serviceURLs, http, url mode", + namespace: "default", + service: "mock-dashboard", + api: defaultAPI, + urlMode: true, + https: false, expected: []string{"http://127.0.0.1:1111", "http://127.0.0.1:2222"}, }, { @@ -903,12 +912,28 @@ func TestWaitAndMaybeOpenService(t *testing.T) { servicesMap: serviceNamespaces, endpointsMap: endpointNamespaces, } - err := WaitAndMaybeOpenService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0) + + var urlList []string + urlList, err := WaitForService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0) if test.err && err == nil { - t.Fatalf("WaitAndMaybeOpenService expected to fail for test: %v", test) + t.Fatalf("WaitForService expected to fail for test: %v", test) } if !test.err && err != nil { - t.Fatalf("WaitAndMaybeOpenService not expected to fail but got err: %v", err) + t.Fatalf("WaitForService not expected to fail but got err: %v", err) + } + + if test.urlMode { + // check the size of the url slices + if len(urlList) != len(test.expected) { + t.Fatalf("WaitForService returned [%d] urls while expected is [%d] url", len(urlList), len(test.expected)) + } + + // check content of the expected url + for i, v := range test.expected { + if v != urlList[i] { + t.Fatalf("WaitForService returned [%s] urls while expected is [%s] url", urlList[i], v) + } + } } }) @@ -954,12 +979,12 @@ func TestWaitAndMaybeOpenServiceForNotDefaultNamspace(t *testing.T) { servicesMap: serviceNamespaceOther, endpointsMap: endpointNamespaces, } - err := WaitAndMaybeOpenService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0) + _, err := WaitForService(test.api, test.namespace, test.service, defaultTemplate, test.urlMode, test.https, 1, 0) if test.err && err == nil { - t.Fatalf("WaitAndMaybeOpenService expected to fail for test: %v", test) + t.Fatalf("WaitForService expected to fail for test: %v", test) } if !test.err && err != nil { - t.Fatalf("WaitAndMaybeOpenService not expected to fail but got err: %v", err) + t.Fatalf("WaitForService not expected to fail but got err: %v", err) } })