Changed command runner

Fixed test Print error message Change minikube running check to after check if exists Updates Set and adds test for disabling
kubernetes · Oct 11, 2019 · 007342d · 007342d
1 parent 03c241d
commit 007342d
Show file tree

Hide file tree

Showing 10 changed files with 77 additions and 32 deletions.
diff --git a/cmd/minikube/cmd/config/disable_test.go b/cmd/minikube/cmd/config/disable_test.go
@@ -16,10 +16,24 @@ limitations under the License.
 
 package config
 
-import "testing"
+import (
+	"testing"
+
+	"gotest.tools/assert"
+	pkgConfig "k8s.io/minikube/pkg/minikube/config"
+	"k8s.io/minikube/pkg/minikube/localpath"
+)
 
 func TestDisableUnknownAddon(t *testing.T) {
 	if err := Set("InvalidAddon", "false"); err == nil {
 		t.Fatalf("Disable did not return error for unknown addon")
 	}
 }
+
+func TestDisableAddon(t *testing.T) {
+	if err := Set("dashboard", "false"); err != nil {
+		t.Fatalf("Diable returned unexpected error: " + err.Error())
+	}
+	config, _ := pkgConfig.ReadConfig(localpath.ConfigFile)
+	assert.Equal(t, config["dashboard"], false)
+}
diff --git a/cmd/minikube/cmd/config/enable_test.go b/cmd/minikube/cmd/config/enable_test.go
@@ -16,10 +16,24 @@ limitations under the License.
 
 package config
 
-import "testing"
+import (
+	"testing"
+
+	"gotest.tools/assert"
+	pkgConfig "k8s.io/minikube/pkg/minikube/config"
+	"k8s.io/minikube/pkg/minikube/localpath"
+)
 
 func TestEnableUnknownAddon(t *testing.T) {
 	if err := Set("InvalidAddon", "false"); err == nil {
 		t.Fatalf("Enable did not return error for unknown addon")
 	}
 }
+
+func TestEnableAddon(t *testing.T) {
+	if err := Set("ingress", "true"); err != nil {
+		t.Fatalf("Enable returned unexpected error: " + err.Error())
+	}
+	config, _ := pkgConfig.ReadConfig(localpath.ConfigFile)
+	assert.Equal(t, config["ingress"], true)
+}
diff --git a/cmd/minikube/cmd/config/open.go b/cmd/minikube/cmd/config/open.go
@@ -21,7 +21,6 @@ import (
 
 	"github.com/spf13/cobra"
 	"k8s.io/minikube/pkg/minikube/assets"
-	"k8s.io/minikube/pkg/minikube/cluster"
 	"k8s.io/minikube/pkg/minikube/exit"
 	"k8s.io/minikube/pkg/minikube/machine"
 	"k8s.io/minikube/pkg/minikube/out"
@@ -62,7 +61,6 @@ var addonsOpenCmd = &cobra.Command{
 		}
 		defer api.Close()
 
-		cluster.EnsureMinikubeRunningOrExit(api, 1)
 		addon, ok := assets.Addons[addonName] // validate addon input
 		if !ok {
 			exit.WithCodeT(exit.Data, `addon '{{.name}}' is not a valid addon packaged with minikube.

diff --git a/cmd/minikube/cmd/config/util.go b/cmd/minikube/cmd/config/util.go
@@ -111,20 +111,37 @@ func EnableOrDisableAddon(name string, val string) error {
 	if err != nil {
 		return errors.Wrapf(err, "parsing bool: %s", name)
 	}
+	addon := assets.Addons[name]
+
+	// check addon status before enabling/disabling it
+	alreadySet, err := isAddonAlreadySet(addon, enable)
+	if err != nil {
+		out.ErrT(out.Conflict, "{{.error}}", out.V{"error": err})
+		return err
+	}
+	//if addon is already enabled or disabled, do nothing
+	if alreadySet {
+		return nil
+	}
 
 	// TODO(r2d4): config package should not reference API, pull this out
 	api, err := machine.NewAPIClient()
 	if err != nil {
 		return errors.Wrap(err, "machine client")
 	}
 	defer api.Close()
-	cluster.EnsureMinikubeRunningOrExit(api, 0)
 
-	addon := assets.Addons[name]
+	//if minikube is not running, we return and simply update the value in the addon
+	//config and rewrite the file
+	if !cluster.IsMinikubeRunning(api) {
+		return nil
+	}
+
 	host, err := cluster.CheckIfHostExistsAndLoad(api, config.GetMachineName())
 	if err != nil {
 		return errors.Wrap(err, "getting host")
 	}
+
 	cmd, err := machine.CommandRunner(host)
 	if err != nil {
 		return errors.Wrap(err, "command runner")
@@ -139,30 +156,24 @@ func EnableOrDisableAddon(name string, val string) error {
 	return enableOrDisableAddonInternal(addon, cmd, data, enable)
 }
 
-func isAddonAlreadySet(addon *assets.Addon, enable bool) error {
-
+func isAddonAlreadySet(addon *assets.Addon, enable bool) (bool, error) {
 	addonStatus, err := addon.IsEnabled()
 
 	if err != nil {
-		return errors.Wrap(err, "get the addon status")
+		return false, errors.Wrap(err, "get the addon status")
 	}
 
 	if addonStatus && enable {
-		return fmt.Errorf("addon %s was already enabled", addon.Name())
+		return true, nil
 	} else if !addonStatus && !enable {
-		return fmt.Errorf("addon %s was already disabled", addon.Name())
+		return true, nil
 	}
 
-	return nil
+	return false, nil
 }
 
 func enableOrDisableAddonInternal(addon *assets.Addon, cmd command.Runner, data interface{}, enable bool) error {
 	var err error
-	// check addon status before enabling/disabling it
-	if err := isAddonAlreadySet(addon, enable); err != nil {
-		out.ErrT(out.Conflict, "{{.error}}", out.V{"error": err})
-		os.Exit(0)
-	}
 
 	if enable {
 		for _, addon := range addon.Assets {

diff --git a/cmd/minikube/cmd/config/util_test.go b/cmd/minikube/cmd/config/util_test.go
@@ -85,29 +85,29 @@ func TestSetBool(t *testing.T) {
 func TestIsAddonAlreadySet(t *testing.T) {
 	testCases := []struct {
 		addonName string
-		expectErr string
 	}{
 		{
 			addonName: "ingress",
-			expectErr: "addon ingress was already ",
 		},
 		{
 			addonName: "heapster",
-			expectErr: "addon heapster was already ",
 		},
 	}
 
 	for _, test := range testCases {
 		addon := assets.Addons[test.addonName]
 		addonStatus, _ := addon.IsEnabled()
 
-		expectMsg := test.expectErr + "enabled"
-		if !addonStatus {
-			expectMsg = test.expectErr + "disabled"
+		alreadySet, err := isAddonAlreadySet(addon, addonStatus)
+		if !alreadySet {
+			if addonStatus {
+				t.Errorf("Did not get expected status, \n\n expected %+v already enabled", test.addonName)
+			} else {
+				t.Errorf("Did not get expected status, \n\n expected %+v already disabled", test.addonName)
+			}
 		}
-		err := isAddonAlreadySet(addon, addonStatus)
-		if err.Error() != expectMsg {
-			t.Errorf("Did not get expected error, \n\n expected: %+v \n\n actual: %+v", expectMsg, err)
+		if err != nil {
+			t.Errorf("Got unexpected error: %+v", err)
 		}
 	}
 }
diff --git a/cmd/minikube/cmd/dashboard.go b/cmd/minikube/cmd/dashboard.go
@@ -93,7 +93,9 @@ var dashboardCmd = &cobra.Command{
 			exit.WithCodeT(exit.NoInput, "kubectl not found in PATH, but is required for the dashboard. Installation guide: https://kubernetes.io/docs/tasks/tools/install-kubectl/")
 		}
 
-		cluster.EnsureMinikubeRunningOrExit(api, 1)
+		if cluster.IsMinikubeRunning(api) {
+			os.Exit(1)
+		}
 
 		// Check dashboard status before enabling it
 		dashboardAddon := assets.Addons["dashboard"]

diff --git a/cmd/minikube/cmd/service.go b/cmd/minikube/cmd/service.go
@@ -17,6 +17,7 @@ limitations under the License.
 package cmd
 
 import (
+	"os"
 	"text/template"
 
 	"github.com/spf13/cobra"
@@ -64,7 +65,9 @@ var serviceCmd = &cobra.Command{
 		}
 		defer api.Close()
 
-		cluster.EnsureMinikubeRunningOrExit(api, 1)
+		if cluster.IsMinikubeRunning(api) {
+			os.Exit(1)
+		}
 		err = service.WaitAndMaybeOpenService(api, namespace, svc,
 			serviceURLTemplate, serviceURLMode, https, wait, interval)
 		if err != nil {

diff --git a/go.mod b/go.mod
@@ -76,6 +76,7 @@ require (
 	gopkg.in/airbrake/gobrake.v2 v2.0.9 // indirect
 	gopkg.in/gemnasium/logrus-airbrake-hook.v2 v2.1.2 // indirect
 	gopkg.in/mgo.v2 v2.0.0-20190816093944-a6b53ec6cb22 // indirect
+	gotest.tools v2.2.0+incompatible
 	k8s.io/api v0.0.0
 	k8s.io/apimachinery v0.0.0
 	k8s.io/client-go v0.0.0

diff --git a/pkg/minikube/bootstrapper/kubeadm/kubeadm.go b/pkg/minikube/bootstrapper/kubeadm/kubeadm.go
@@ -19,6 +19,7 @@ package kubeadm
 import (
 	"bytes"
 	"crypto/tls"
+	"k8s.io/minikube/pkg/minikube/cluster"
 
 	"fmt"
 	"net"
@@ -127,7 +128,7 @@ func NewKubeadmBootstrapper(api libmachine.API) (*Bootstrapper, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "getting api client")
 	}
-	runner, err := machine.CommandRunner(h)
+	runner, err := machine.CommandRunner(h, cluster.IsMinikubeRunning(api))
 	if err != nil {
 		return nil, errors.Wrap(err, "command runner")
 	}

diff --git a/pkg/minikube/cluster/cluster.go b/pkg/minikube/cluster/cluster.go
@@ -604,12 +604,13 @@ func CreateSSHShell(api libmachine.API, args []string) error {
 
 // EnsureMinikubeRunningOrExit checks that minikube has a status available and that
 // the status is `Running`, otherwise it will exit
-func EnsureMinikubeRunningOrExit(api libmachine.API, exitStatus int) {
+func IsMinikubeRunning(api libmachine.API) bool {
 	s, err := GetHostStatus(api)
 	if err != nil {
-		exit.WithError("Error getting machine status", err)
+		return false
 	}
 	if s != state.Running.String() {
-		exit.WithCodeT(exit.Unavailable, "minikube is not running, so the service cannot be accessed")
+		return false
 	}
+	return true
 }