Refactor to remove opening browser and just return url(s)

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)
+					}
+				}
+			}
 		}
 	},
 }

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)
+				}
+			}
+		}
 	},
 }
 

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,22 +264,24 @@ 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
 	}
 	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 <namespace>", 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 <namespace>", 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

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)
 			}
 
 		})