Fixed args handling in generate and deploy (#1517)

* Added generate test

* Fixed bug in generate.go

Too many args error was not being caught

* Fixed deploy validate method bug

Multiple arg error not caught

* Revert Makefile to original state

* gofmt

cmd/deploy.go

@@ -58,7 +58,9 @@ func newDeployCmd() *cobra.Command {
 		Short: deployShortDescription,
 		Long:  deployLongDescription,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			dc.validate(cmd, args)
+			if err := dc.validate(cmd, args); err != nil {
+				log.Fatalf(fmt.Sprintf("error validating deployCmd: %s", err.Error()))
+			}
 			return dc.run()
 		},
 	}
@@ -78,28 +80,28 @@ func newDeployCmd() *cobra.Command {
 	return deployCmd
 }
 
-func (dc *deployCmd) validate(cmd *cobra.Command, args []string) {
+func (dc *deployCmd) validate(cmd *cobra.Command, args []string) error {
 	var err error
 
 	dc.locale, err = i18n.LoadTranslations()
 	if err != nil {
-		log.Fatalf("error loading translation files: %s", err.Error())
+		return fmt.Errorf(fmt.Sprintf("error loading translation files: %s", err.Error()))
 	}
 
 	if dc.apimodelPath == "" {
-		if len(args) > 0 {
+		if len(args) == 1 {
 			dc.apimodelPath = args[0]
 		} else if len(args) > 1 {
 			cmd.Usage()
-			log.Fatalln("too many arguments were provided to 'deploy'")
+			return fmt.Errorf(fmt.Sprintf("too many arguments were provided to 'deploy'"))
 		} else {
 			cmd.Usage()
-			log.Fatalln("--api-model was not supplied, nor was one specified as a positional argument")
+			return fmt.Errorf(fmt.Sprintf("--api-model was not supplied, nor was one specified as a positional argument"))
 		}
 	}
 
 	if _, err := os.Stat(dc.apimodelPath); os.IsNotExist(err) {
-		log.Fatalf("specified api model does not exist (%s)", dc.apimodelPath)
+		return fmt.Errorf(fmt.Sprintf("specified api model does not exist (%s)", dc.apimodelPath))
 	}
 
 	apiloader := &api.Apiloader{
@@ -110,16 +112,16 @@ func (dc *deployCmd) validate(cmd *cobra.Command, args []string) {
 	// skip validating the model fields for now
 	dc.containerService, dc.apiVersion, err = apiloader.LoadContainerServiceFromFile(dc.apimodelPath, false, nil)
 	if err != nil {
-		log.Fatalf("error parsing the api model: %s", err.Error())
+		return fmt.Errorf(fmt.Sprintf("error parsing the api model: %s", err.Error()))
 	}
 
 	if dc.location == "" {
-		log.Fatalf("--location must be specified")
+		return fmt.Errorf(fmt.Sprintf("--location must be specified"))
 	}
 
 	dc.client, err = dc.authArgs.getClient()
 	if err != nil {
-		log.Fatalf("failed to get client") // TODO: cleanup
+		return fmt.Errorf(fmt.Sprintf("failed to get client")) // TODO: cleanup
 	}
 
 	// autofillApimodel calls log.Fatal() directly and does not return errors
@@ -127,10 +129,12 @@ func (dc *deployCmd) validate(cmd *cobra.Command, args []string) {
 
 	_, _, err = revalidateApimodel(apiloader, dc.containerService, dc.apiVersion)
 	if err != nil {
-		log.Fatalf("Failed to validate the apimodel after populating values: %s", err)
+		return fmt.Errorf(fmt.Sprintf("Failed to validate the apimodel after populating values: %s", err))
 	}
 
 	dc.random = rand.New(rand.NewSource(time.Now().UnixNano()))
+
+	return nil
 }
 
 func autofillApimodel(dc *deployCmd) {

cmd/generate.go

@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -43,7 +44,9 @@ func newGenerateCmd() *cobra.Command {
 		Short: generateShortDescription,
 		Long:  generateLongDescription,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			gc.validate(cmd, args)
+			if err := gc.validate(cmd, args); err != nil {
+				log.Fatalf(fmt.Sprintf("error validating generateCmd: %s", err.Error()))
+			}
 			return gc.run()
 		},
 	}
@@ -60,30 +63,30 @@ func newGenerateCmd() *cobra.Command {
 	return generateCmd
 }
 
-func (gc *generateCmd) validate(cmd *cobra.Command, args []string) {
+func (gc *generateCmd) validate(cmd *cobra.Command, args []string) error {
 	var caCertificateBytes []byte
 	var caKeyBytes []byte
 	var err error
 
 	gc.locale, err = i18n.LoadTranslations()
 	if err != nil {
-		log.Fatalf("error loading translation files: %s", err.Error())
+		return fmt.Errorf(fmt.Sprintf("error loading translation files: %s", err.Error()))
 	}
 
 	if gc.apimodelPath == "" {
-		if len(args) > 0 {
+		if len(args) == 1 {
 			gc.apimodelPath = args[0]
 		} else if len(args) > 1 {
 			cmd.Usage()
-			log.Fatalln("too many arguments were provided to 'generate'")
+			return errors.New("too many arguments were provided to 'generate'")
 		} else {
 			cmd.Usage()
-			log.Fatalln("--api-model was not supplied, nor was one specified as a positional argument")
+			return errors.New("--api-model was not supplied, nor was one specified as a positional argument")
 		}
 	}
 
 	if _, err := os.Stat(gc.apimodelPath); os.IsNotExist(err) {
-		log.Fatalf("specified api model does not exist (%s)", gc.apimodelPath)
+		return fmt.Errorf(fmt.Sprintf("specified api model does not exist (%s)", gc.apimodelPath))
 	}
 
 	apiloader := &api.Apiloader{
@@ -93,7 +96,7 @@ func (gc *generateCmd) validate(cmd *cobra.Command, args []string) {
 	}
 	gc.containerService, gc.apiVersion, err = apiloader.LoadContainerServiceFromFile(gc.apimodelPath, true, nil)
 	if err != nil {
-		log.Fatalf("error parsing the api model: %s", err.Error())
+		return fmt.Errorf(fmt.Sprintf("error parsing the api model: %s", err.Error()))
 	}
 
 	if gc.outputDirectory == "" {
@@ -107,14 +110,14 @@ func (gc *generateCmd) validate(cmd *cobra.Command, args []string) {
 	// consume gc.caCertificatePath and gc.caPrivateKeyPath
 
 	if (gc.caCertificatePath != "" && gc.caPrivateKeyPath == "") || (gc.caCertificatePath == "" && gc.caPrivateKeyPath != "") {
-		log.Fatal("--ca-certificate-path and --ca-private-key-path must be specified together")
+		return errors.New("--ca-certificate-path and --ca-private-key-path must be specified together")
 	}
 	if gc.caCertificatePath != "" {
 		if caCertificateBytes, err = ioutil.ReadFile(gc.caCertificatePath); err != nil {
-			log.Fatal("failed to read CA certificate file:", err)
+			return fmt.Errorf(fmt.Sprintf("failed to read CA certificate file: %s", err.Error()))
 		}
 		if caKeyBytes, err = ioutil.ReadFile(gc.caPrivateKeyPath); err != nil {
-			log.Fatal("failed to read CA private key file:", err)
+			return fmt.Errorf(fmt.Sprintf("failed to read CA private key file: %s", err.Error()))
 		}
 
 		prop := gc.containerService.Properties
@@ -124,6 +127,7 @@ func (gc *generateCmd) validate(cmd *cobra.Command, args []string) {
 		prop.CertificateProfile.CaCertificate = string(caCertificateBytes)
 		prop.CertificateProfile.CaPrivateKey = string(caKeyBytes)
 	}
+	return nil
 }
 
 func (gc *generateCmd) run() error {

cmd/generate_test.go

@@ -0,0 +1,39 @@
+package cmd
+
+import (
+	"testing"
+
+	"github.com/spf13/cobra"
+)
+
+func TestGenerateCmdValidate(t *testing.T) {
+
+	g := &generateCmd{}
+
+	r := &cobra.Command{}
+
+	// validate cmd with 1 arg
+	err := g.validate(r, []string{"../pkg/acsengine/testdata/simple/kubernetes.json"})
+	if err != nil {
+		t.Fatalf("unexpected error validating 1 arg: %s", err.Error())
+	}
+
+	g = &generateCmd{}
+
+	// validate cmd with 0 args
+	err = g.validate(r, []string{})
+	t.Logf(err.Error())
+	if err == nil {
+		t.Fatalf("expected error validating 0 args")
+	}
+
+	g = &generateCmd{}
+
+	// validate cmd with more than 1 arg
+	err = g.validate(r, []string{"../pkg/acsengine/testdata/simple/kubernetes.json", "arg1"})
+	t.Logf(err.Error())
+	if err == nil {
+		t.Fatalf("expected error validating multiple args")
+	}
+
+}