cmd package refactor and unit tests

cmd/deploy.go

@@ -130,9 +130,13 @@ func (dc *deployCmd) validate(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf(fmt.Sprintf("--location does not match api model location"))
 	}
 
+	if err = dc.authArgs.validateAuthArgs(); err != nil {
+		return fmt.Errorf("%s", err)
+	}
+
 	dc.client, err = dc.authArgs.getClient()
 	if err != nil {
-		return fmt.Errorf(fmt.Sprintf("failed to get client")) // TODO: cleanup
+		return fmt.Errorf("failed to get client: %s", err.Error())
 	}
 
 	// autofillApimodel calls log.Fatal() directly and does not return errors

cmd/root.go

@@ -1,6 +1,8 @@
 package cmd
 
 import (
+	"fmt"
+
 	"github.com/Azure/acs-engine/pkg/armhelpers"
 	"github.com/Azure/go-autorest/autorest/azure"
 	uuid "github.com/satori/go.uuid"
@@ -70,31 +72,38 @@ func addAuthFlags(authArgs *authArgs, f *flag.FlagSet) {
 	f.StringVar(&authArgs.language, "language", "en-us", "language to return error messages in")
 }
 
-func (authArgs *authArgs) getClient() (*armhelpers.AzureClient, error) {
+func (authArgs *authArgs) validateAuthArgs() error {
 	authArgs.ClientID, _ = uuid.FromString(authArgs.rawClientID)
 	authArgs.SubscriptionID, _ = uuid.FromString(authArgs.rawSubscriptionID)
 
 	if authArgs.AuthMethod == "client_secret" {
 		if authArgs.ClientID.String() == "00000000-0000-0000-0000-000000000000" || authArgs.ClientSecret == "" {
-			log.Fatal(`--client-id and --client-secret must be specified when --auth-method="client_secret"`)
+			return fmt.Errorf(`--client-id and --client-secret must be specified when --auth-method="client_secret"`)
 		}
 		// try parse the UUID
 	} else if authArgs.AuthMethod == "client_certificate" {
 		if authArgs.ClientID.String() == "00000000-0000-0000-0000-000000000000" || authArgs.CertificatePath == "" || authArgs.PrivateKeyPath == "" {
-			log.Fatal(`--client-id and --certificate-path, and --private-key-path must be specified when --auth-method="client_certificate"`)
+			return fmt.Errorf(`--client-id and --certificate-path, and --private-key-path must be specified when --auth-method="client_certificate"`)
 		}
 	}
 
 	if authArgs.SubscriptionID.String() == "00000000-0000-0000-0000-000000000000" {
-		log.Fatal("--subscription-id is required (and must be a valid UUID)")
+		return fmt.Errorf("--subscription-id is required (and must be a valid UUID)")
 	}
 
-	env, err := azure.EnvironmentFromName(authArgs.RawAzureEnvironment)
+	_, err := azure.EnvironmentFromName(authArgs.RawAzureEnvironment)
 	if err != nil {
-		log.Fatal("failed to parse --azure-env as a valid target Azure cloud environment")
+		return fmt.Errorf("failed to parse --azure-env as a valid target Azure cloud environment")
 	}
+	return nil
+}
 
+func (authArgs *authArgs) getClient() (*armhelpers.AzureClient, error) {
 	var client *armhelpers.AzureClient
+	env, err := azure.EnvironmentFromName(authArgs.RawAzureEnvironment)
+	if err != nil {
+		return nil, err
+	}
 	switch authArgs.AuthMethod {
 	case "device":
 		client, err = armhelpers.NewAzureClientWithDeviceAuth(env, authArgs.SubscriptionID.String())
@@ -103,8 +112,7 @@ func (authArgs *authArgs) getClient() (*armhelpers.AzureClient, error) {
 	case "client_certificate":
 		client, err = armhelpers.NewAzureClientWithClientCertificateFile(env, authArgs.SubscriptionID.String(), authArgs.ClientID.String(), authArgs.CertificatePath, authArgs.PrivateKeyPath)
 	default:
-		log.Fatalf("--auth-method: ERROR: method unsupported. method=%q.", authArgs.AuthMethod)
-		return nil, nil // unreachable
+		return nil, fmt.Errorf("--auth-method: ERROR: method unsupported. method=%q", authArgs.AuthMethod)
 	}
 	if err != nil {
 		return nil, err

cmd/scale.go

@@ -109,6 +109,10 @@ func (sc *scaleCmd) validate(cmd *cobra.Command, args []string) {
 		log.Fatal("--new-node-count must be specified")
 	}
 
+	if err = sc.authArgs.validateAuthArgs(); err != nil {
+		log.Fatal("%s", err)
+	}
+
 	if sc.client, err = sc.authArgs.getClient(); err != nil {
 		log.Error("Failed to get client:", err)
 	}

cmd/upgrade.go

@@ -33,12 +33,12 @@ type upgradeCmd struct {
 	resourceGroupName   string
 	deploymentDirectory string
 	upgradeVersion      string
-	containerService    *api.ContainerService
-	apiVersion          string
 	location            string
 	timeoutInMinutes    int
 
 	// derived
+	containerService    *api.ContainerService
+	apiVersion          string
 	client              armhelpers.ACSEngineClient
 	locale              *gotext.Locale
 	nameSuffix          string
@@ -60,37 +60,36 @@ func newUpgradeCmd() *cobra.Command {
 	}
 
 	f := upgradeCmd.Flags()
-	f.StringVarP(&uc.location, "location", "l", "", "location the cluster is deployed in")
-	f.StringVarP(&uc.resourceGroupName, "resource-group", "g", "", "the resource group where the cluster is deployed")
-	f.StringVar(&uc.deploymentDirectory, "deployment-dir", "", "the location of the output from `generate`")
-	f.StringVar(&uc.upgradeVersion, "upgrade-version", "", "desired kubernetes version")
+	f.StringVarP(&uc.location, "location", "l", "", "location the cluster is deployed in (required)")
+	f.StringVarP(&uc.resourceGroupName, "resource-group", "g", "", "the resource group where the cluster is deployed (required)")
+	f.StringVar(&uc.deploymentDirectory, "deployment-dir", "", "the location of the output from `generate` (required)")
+	f.StringVar(&uc.upgradeVersion, "upgrade-version", "", "desired kubernetes version (required)")
 	f.IntVar(&uc.timeoutInMinutes, "vm-timeout", -1, "how long to wait for each vm to be upgraded in minutes")
 	addAuthFlags(&uc.authArgs, f)
 
 	return upgradeCmd
 }
 
-func (uc *upgradeCmd) validate(cmd *cobra.Command, args []string) {
+func (uc *upgradeCmd) validate(cmd *cobra.Command) error {
 	log.Infoln("validating...")
 
 	var err error
 
 	uc.locale, err = i18n.LoadTranslations()
 	if err != nil {
-		log.Fatalf("error loading translation files: %s", err.Error())
+		return fmt.Errorf("error loading translation files: %s", err.Error())
 	}
 
 	if uc.resourceGroupName == "" {
 		cmd.Usage()
-		log.Fatal("--resource-group must be specified")
+		return fmt.Errorf("--resource-group must be specified")
 	}
 
 	if uc.location == "" {
 		cmd.Usage()
-		log.Fatal("--location must be specified")
-	} else {
-		uc.location = helpers.NormalizeAzureRegion(uc.location)
+		return fmt.Errorf("--location must be specified")
 	}
+	uc.location = helpers.NormalizeAzureRegion(uc.location)
 
 	if uc.timeoutInMinutes != -1 {
 		timeout := time.Duration(uc.timeoutInMinutes) * time.Minute
@@ -100,28 +99,32 @@ func (uc *upgradeCmd) validate(cmd *cobra.Command, args []string) {
 	// TODO(colemick): add in the cmd annotation to help enable autocompletion
 	if uc.upgradeVersion == "" {
 		cmd.Usage()
-		log.Fatal("--upgrade-version must be specified")
-	}
-
-	if uc.client, err = uc.authArgs.getClient(); err != nil {
-		log.Error("Failed to get client:", err)
+		return fmt.Errorf("--upgrade-version must be specified")
 	}
 
 	if uc.deploymentDirectory == "" {
 		cmd.Usage()
-		log.Fatal("--deployment-dir must be specified")
+		return fmt.Errorf("--deployment-dir must be specified")
+	}
+
+	if err = uc.authArgs.validateAuthArgs(); err != nil {
+		return fmt.Errorf("%s", err)
 	}
+	return nil
+}
 
+func (uc *upgradeCmd) loadCluster(cmd *cobra.Command) error {
+	var err error
 	_, err = uc.client.EnsureResourceGroup(uc.resourceGroupName, uc.location, nil)
 	if err != nil {
-		log.Fatalln(err)
+		return fmt.Errorf("Error ensuring resource group: %s", err)
 	}
 
 	// load apimodel from the deployment directory
 	apiModelPath := path.Join(uc.deploymentDirectory, "apimodel.json")
 
 	if _, err = os.Stat(apiModelPath); os.IsNotExist(err) {
-		log.Fatalf("specified api model does not exist (%s)", apiModelPath)
+		return fmt.Errorf("specified api model does not exist (%s)", apiModelPath)
 	}
 
 	apiloader := &api.Apiloader{
@@ -131,19 +134,19 @@ func (uc *upgradeCmd) validate(cmd *cobra.Command, args []string) {
 	}
 	uc.containerService, uc.apiVersion, err = apiloader.LoadContainerServiceFromFile(apiModelPath, true, true, nil)
 	if err != nil {
-		log.Fatalf("error parsing the api model: %s", err.Error())
+		return fmt.Errorf("error parsing the api model: %s", err.Error())
 	}
 
 	if uc.containerService.Location == "" {
 		uc.containerService.Location = uc.location
 	} else if uc.containerService.Location != uc.location {
-		log.Fatalf("--location does not match api model location")
+		return fmt.Errorf("--location does not match api model location")
 	}
 
 	// get available upgrades for container service
 	orchestratorInfo, err := api.GetOrchestratorVersionProfile(uc.containerService.Properties.OrchestratorProfile)
 	if err != nil {
-		log.Fatalf("error getting list of available upgrades: %s", err.Error())
+		return fmt.Errorf("error getting list of available upgrades: %s", err.Error())
 	}
 	// add the current version if upgrade has failed
 	orchestratorInfo.Upgrades = append(orchestratorInfo.Upgrades, &api.OrchestratorProfile{
@@ -160,12 +163,11 @@ func (uc *upgradeCmd) validate(cmd *cobra.Command, args []string) {
 		}
 	}
 	if !found {
-		log.Fatalf("version %s is not supported", uc.upgradeVersion)
+		return fmt.Errorf("version %s is not supported", uc.upgradeVersion)
 	}
 
-	uc.client, err = uc.authArgs.getClient()
-	if err != nil {
-		log.Fatalf("failed to get client") // TODO: cleanup
+	if uc.client, err = uc.authArgs.getClient(); err != nil {
+		return fmt.Errorf("Failed to get client: %s", err)
 	}
 
 	// Read name suffix to identify nodes in the resource group that belong
@@ -191,10 +193,19 @@ func (uc *upgradeCmd) validate(cmd *cobra.Command, args []string) {
 	for _, agentPool := range uc.containerService.Properties.AgentPoolProfiles {
 		uc.agentPoolsToUpgrade = append(uc.agentPoolsToUpgrade, agentPool.Name)
 	}
+	return nil
 }
 
 func (uc *upgradeCmd) run(cmd *cobra.Command, args []string) error {
-	uc.validate(cmd, args)
+	err := uc.validate(cmd)
+	if err != nil {
+		log.Fatalf("error validating upgrade command: %v", err)
+	}
+
+	err = uc.loadCluster(cmd)
+	if err != nil {
+		log.Fatalf("error loading existing cluster: %v", err)
+	}
 
 	upgradeCluster := kubernetesupgrade.UpgradeCluster{
 		Translator: &i18n.Translator{

cmd/upgrade_test.go

@@ -0,0 +1,135 @@
+package cmd
+
+import (
+	"fmt"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"github.com/spf13/cobra"
+)
+
+var _ = Describe("the upgrade command", func() {
+
+	It("should create an upgrade command", func() {
+		output := newUpgradeCmd()
+
+		Expect(output.Use).Should(Equal(upgradeName))
+		Expect(output.Short).Should(Equal(upgradeShortDescription))
+		Expect(output.Long).Should(Equal(upgradeLongDescription))
+		Expect(output.Flags().Lookup("location")).NotTo(BeNil())
+		Expect(output.Flags().Lookup("resource-group")).NotTo(BeNil())
+		Expect(output.Flags().Lookup("deployment-dir")).NotTo(BeNil())
+		Expect(output.Flags().Lookup("upgrade-version")).NotTo(BeNil())
+	})
+
+	It("should validate an upgrade command", func() {
+		r := &cobra.Command{}
+
+		cases := []struct {
+			uc          *upgradeCmd
+			expectedErr error
+		}{
+			{
+				uc: &upgradeCmd{
+					resourceGroupName:   "",
+					deploymentDirectory: "_output/test",
+					upgradeVersion:      "1.8.9",
+					location:            "centralus",
+					timeoutInMinutes:    60,
+					authArgs: authArgs{
+						rawSubscriptionID: "99999999-0000-0000-0000-000000000000",
+					},
+				},
+				expectedErr: fmt.Errorf("--resource-group must be specified"),
+			},
+			{
+				uc: &upgradeCmd{
+					resourceGroupName:   "test",
+					deploymentDirectory: "_output/test",
+					upgradeVersion:      "1.8.9",
+					location:            "",
+					timeoutInMinutes:    60,
+					authArgs: authArgs{
+						rawSubscriptionID: "99999999-0000-0000-0000-000000000000",
+					},
+				},
+				expectedErr: fmt.Errorf("--location must be specified"),
+			},
+			{
+				uc: &upgradeCmd{
+					resourceGroupName:   "test",
+					deploymentDirectory: "_output/test",
+					upgradeVersion:      "",
+					location:            "southcentralus",
+					timeoutInMinutes:    60,
+					authArgs: authArgs{
+						rawSubscriptionID: "99999999-0000-0000-0000-000000000000",
+					},
+				},
+				expectedErr: fmt.Errorf("--upgrade-version must be specified"),
+			},
+			{
+				uc: &upgradeCmd{
+					resourceGroupName:   "test",
+					deploymentDirectory: "",
+					upgradeVersion:      "1.9.0",
+					location:            "southcentralus",
+					timeoutInMinutes:    60,
+					authArgs: authArgs{
+						rawSubscriptionID: "99999999-0000-0000-0000-000000000000",
+					},
+				},
+				expectedErr: fmt.Errorf("--deployment-dir must be specified"),
+			},
+			{
+				uc: &upgradeCmd{
+					resourceGroupName:   "test",
+					deploymentDirectory: "",
+					upgradeVersion:      "1.9.0",
+					location:            "southcentralus",
+					timeoutInMinutes:    60,
+					authArgs: authArgs{
+						rawSubscriptionID: "99999999-0000-0000-0000-000000000000",
+					},
+				},
+				expectedErr: fmt.Errorf("--deployment-dir must be specified"),
+			},
+			{
+				uc: &upgradeCmd{
+					resourceGroupName:   "test",
+					deploymentDirectory: "_output/mydir",
+					upgradeVersion:      "1.9.0",
+					location:            "southcentralus",
+					authArgs:            authArgs{},
+				},
+				expectedErr: fmt.Errorf("--subscription-id is required (and must be a valid UUID)"),
+			},
+			{
+				uc: &upgradeCmd{
+					resourceGroupName:   "test",
+					deploymentDirectory: "_output/mydir",
+					upgradeVersion:      "1.9.0",
+					location:            "southcentralus",
+					authArgs: authArgs{
+						rawSubscriptionID:   "99999999-0000-0000-0000-000000000000",
+						RawAzureEnvironment: "AzurePublicCloud",
+						AuthMethod:          "device",
+					},
+				},
+				expectedErr: nil,
+			},
+		}
+
+		for _, c := range cases {
+			err := c.uc.validate(r)
+			if c.expectedErr != nil && err != nil {
+				Expect(err.Error()).To(Equal(c.expectedErr.Error()))
+			} else {
+				Expect(err).To(BeNil())
+				Expect(c.expectedErr).To(BeNil())
+			}
+		}
+
+	})
+
+})