Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

Commit

Permalink
fix: commands return errors rather than exit (#693)
Browse files Browse the repository at this point in the history 
* fix: don't double-log errors and ensure return code of 1

* ref: return err from version cmd and update tests

* fix: work around ginkgo flags.Parse() issue

* test: execute root command

* test: add cases for default args and "completion" command

* test: execute the orchestrators command

* fix: return error instead of calling log.Fatalf()

* test: add root command debug flag case

* refactor: return errors instead of calling log.Fatalf()

* fix: return error when generating bash completion script

* style: code cleanup

* test: add basic no-args command invocations

* ref: use more readable HaveOccurred() matcher
  • Loading branch information
@mboersma
mboersma committed Mar 11, 2019
1 parent 3d01072 commit a290ab8
Show file tree
Hide file tree
Showing 15 changed files with 178 additions and 116 deletions.
36 changes: 16 additions & 20 deletions cmd/deploy.go
View file
Expand Up @@ -72,16 +72,16 @@ func newDeployCmd() *cobra.Command {
Long: deployLongDescription,
RunE: func(cmd *cobra.Command, args []string) error {
if err := dc.validateArgs(cmd, args); err != nil {
log.Fatalf("error validating deployCmd: %s", err.Error())
return errors.Wrap(err, "validating deployCmd")
}
if err := dc.mergeAPIModel(); err != nil {
log.Fatalf("error merging API model in deployCmd: %s", err.Error())
return errors.Wrap(err, "merging API model in deployCmd")
}
if err := dc.loadAPIModel(cmd, args); err != nil {
log.Fatalf("failed to load apimodel: %s", err.Error())
return errors.Wrap(err, "loading API model")
}
if _, _, err := dc.validateApimodel(); err != nil {
log.Fatalf("Failed to validate the apimodel after populating values: %s", err.Error())
return errors.Wrap(err, "validating API model after populating values")
}
return dc.run()
},
Expand Down Expand Up @@ -109,7 +109,7 @@ func (dc *deployCmd) validateArgs(cmd *cobra.Command, args []string) error {

dc.locale, err = i18n.LoadTranslations()
if err != nil {
return errors.Wrap(err, "error loading translation files")
return errors.Wrap(err, "loading translation files")
}

if dc.apimodelPath == "" {
Expand Down Expand Up @@ -383,29 +383,27 @@ func (dc *deployCmd) run() error {

templateGenerator, err := engine.InitializeTemplateGenerator(ctx)
if err != nil {
log.Fatalf("failed to initialize template generator: %s", err.Error())
return errors.Wrap(err, "initializing template generator")
}

certsgenerated, err := dc.containerService.SetPropertiesDefaults(false, false)
if err != nil {
log.Fatalf("error in SetPropertiesDefaults template %s: %s", dc.apimodelPath, err.Error())
os.Exit(1)
return errors.Wrapf(err, "in SetPropertiesDefaults template %s", dc.apimodelPath)
}

template, parameters, err := templateGenerator.GenerateTemplate(dc.containerService, engine.DefaultGeneratorCode, BuildTag)
//TODO enable GenerateTemplateV2 when new template generation flow has been validated!
//template, parameters, err := templateGenerator.GenerateTemplateV2(dc.containerService, engine.DefaultGeneratorCode, BuildTag)
if err != nil {
log.Fatalf("error generating template %s: %s", dc.apimodelPath, err.Error())
os.Exit(1)
return errors.Wrapf(err, "generating template %s", dc.apimodelPath)
}

if template, err = transform.PrettyPrintArmTemplate(template); err != nil {
log.Fatalf("error pretty printing template: %s \n", err.Error())
return errors.Wrap(err, "pretty-printing template")
}
var parametersFile string
if parametersFile, err = transform.BuildAzureParametersFile(parameters); err != nil {
log.Fatalf("error pretty printing template parameters: %s \n", err.Error())
return errors.Wrap(err, "pretty-printing template parameters")
}

writer := &engine.ArtifactWriter{
Expand All @@ -414,20 +412,18 @@ func (dc *deployCmd) run() error {
},
}
if err = writer.WriteTLSArtifacts(dc.containerService, dc.apiVersion, template, parametersFile, dc.outputDirectory, certsgenerated, dc.parametersOnly); err != nil {
log.Fatalf("error writing artifacts: %s \n", err.Error())
return errors.Wrap(err, "writing artifacts")
}

templateJSON := make(map[string]interface{})
parametersJSON := make(map[string]interface{})

err = json.Unmarshal([]byte(template), &templateJSON)
if err != nil {
log.Fatalln(err)
if err = json.Unmarshal([]byte(template), &templateJSON); err != nil {
return err
}

err = json.Unmarshal([]byte(parameters), &parametersJSON)
if err != nil {
log.Fatalln(err)
if err = json.Unmarshal([]byte(parameters), &parametersJSON); err != nil {
return err
}

deploymentSuffix := dc.random.Int31()
Expand All @@ -446,7 +442,7 @@ func (dc *deployCmd) run() error {
body, _ := ioutil.ReadAll(res.Body)
log.Errorf(string(body))
}
log.Fatalln(err)
return err
}

return nil
Expand Down
13 changes: 9 additions & 4 deletions cmd/deploy_test.go
View file
Expand Up @@ -94,17 +94,22 @@ func getAPIModelWithoutServicePrincipalProfile(baseAPIModel string, useManagedId
}

func TestNewDeployCmd(t *testing.T) {
output := newDeployCmd()
if output.Use != deployName || output.Short != deployShortDescription || output.Long != deployLongDescription {
t.Fatalf("deploy command should have use %s equal %s, short %s equal %s and long %s equal to %s", output.Use, deployName, output.Short, deployShortDescription, output.Long, versionLongDescription)
command := newDeployCmd()
if command.Use != deployName || command.Short != deployShortDescription || command.Long != deployLongDescription {
t.Fatalf("deploy command should have use %s equal %s, short %s equal %s and long %s equal to %s", command.Use, deployName, command.Short, deployShortDescription, command.Long, versionLongDescription)
}

expectedFlags := []string{"api-model", "dns-prefix", "auto-suffix", "output-directory", "ca-private-key-path", "resource-group", "location", "force-overwrite"}
for _, f := range expectedFlags {
if output.Flags().Lookup(f) == nil {
if command.Flags().Lookup(f) == nil {
t.Fatalf("deploy command should have flag %s", f)
}
}

command.SetArgs([]string{})
if err := command.Execute(); err == nil {
t.Fatalf("expected an error when calling deploy with no arguments")
}
}

func TestValidate(t *testing.T) {
Expand Down
20 changes: 9 additions & 11 deletions cmd/generate.go
View file
Expand Up @@ -49,15 +49,15 @@ func newGenerateCmd() *cobra.Command {
Long: generateLongDescription,
RunE: func(cmd *cobra.Command, args []string) error {
if err := gc.validate(cmd, args); err != nil {
log.Fatalf(fmt.Sprintf("error validating generateCmd: %s", err.Error()))
return errors.Wrap(err, "validating generateCmd")
}

if err := gc.mergeAPIModel(); err != nil {
log.Fatalf(fmt.Sprintf("error merging API model in generateCmd: %s", err.Error()))
return errors.Wrap(err, "merging API model in generateCmd")
}

if err := gc.loadAPIModel(cmd, args); err != nil {
log.Fatalf(fmt.Sprintf("error loading API model in generateCmd: %s", err.Error()))
return errors.Wrap(err, "loading API model in generateCmd")
}

return gc.run()
Expand Down Expand Up @@ -179,13 +179,12 @@ func (gc *generateCmd) run() error {
}
templateGenerator, err := engine.InitializeTemplateGenerator(ctx)
if err != nil {
log.Fatalf("failed to initialize template generator: %s", err.Error())
return errors.Wrap(err, "initializing template generator")
}

certsGenerated, err := gc.containerService.SetPropertiesDefaults(false, false)
if err != nil {
log.Fatalf("error in SetPropertiesDefaults template %s: %s", gc.apimodelPath, err.Error())
os.Exit(1)
return errors.Wrapf(err, "in SetPropertiesDefaults template %s", gc.apimodelPath)
}

//TODO remove these debug statements when we're new template generation implementation is enabled!
Expand All @@ -196,16 +195,15 @@ func (gc *generateCmd) run() error {
//TODO enable GenerateTemplateV2 when new template generation flow has been validated!
//template, parameters, err := templateGenerator.GenerateTemplateV2(gc.containerService, engine.DefaultGeneratorCode, BuildTag)
if err != nil {
log.Fatalf("error generating template %s: %s", gc.apimodelPath, err.Error())
os.Exit(1)
return errors.Wrapf(err, "generating template %s", gc.apimodelPath)
}

if !gc.noPrettyPrint {
if template, err = transform.PrettyPrintArmTemplate(template); err != nil {
log.Fatalf("error pretty printing template: %s \n", err.Error())
return errors.Wrap(err, "pretty-printing template")
}
if parameters, err = transform.BuildAzureParametersFile(parameters); err != nil {
log.Fatalf("error pretty printing template parameters: %s \n", err.Error())
return errors.Wrap(err, "pretty-printing template parameters")
}
}

Expand All @@ -215,7 +213,7 @@ func (gc *generateCmd) run() error {
},
}
if err = writer.WriteTLSArtifacts(gc.containerService, gc.apiVersion, template, parameters, gc.outputDirectory, certsGenerated, gc.parametersOnly); err != nil {
log.Fatalf("error writing artifacts: %s \n", err.Error())
return errors.Wrap(err, "writing artifacts")
}

return nil
Expand Down
13 changes: 9 additions & 4 deletions cmd/generate_test.go
View file
Expand Up @@ -10,17 +10,22 @@ import (
)

func TestNewGenerateCmd(t *testing.T) {
output := newGenerateCmd()
if output.Use != generateName || output.Short != generateShortDescription || output.Long != generateLongDescription {
t.Fatalf("generate command should have use %s equal %s, short %s equal %s and long %s equal to %s", output.Use, generateName, output.Short, generateShortDescription, output.Long, generateLongDescription)
command := newGenerateCmd()
if command.Use != generateName || command.Short != generateShortDescription || command.Long != generateLongDescription {
t.Fatalf("generate command should have use %s equal %s, short %s equal %s and long %s equal to %s", command.Use, generateName, command.Short, generateShortDescription, command.Long, generateLongDescription)
}

expectedFlags := []string{"api-model", "output-directory", "ca-certificate-path", "ca-private-key-path", "set", "no-pretty-print", "parameters-only"}
for _, f := range expectedFlags {
if output.Flags().Lookup(f) == nil {
if command.Flags().Lookup(f) == nil {
t.Fatalf("generate command should have flag %s", f)
}
}

command.SetArgs([]string{})
if err := command.Execute(); err == nil {
t.Fatalf("expected an error when calling generate with no arguments")
}
}

func TestGenerateCmdValidate(t *testing.T) {
Expand Down
22 changes: 13 additions & 9 deletions cmd/get_versions_test.go
View file
Expand Up @@ -10,13 +10,17 @@ import (

var _ = Describe("The get-versions command", func() {
It("should create a get-versions command", func() {
output := newGetVersionsCmd()
command := newGetVersionsCmd()

Expect(output.Use).Should(Equal(getVersionsName))
Expect(output.Short).Should(Equal(getVersionsShortDescription))
Expect(output.Long).Should(Equal(getVersionsLongDescription))
Expect(output.Flags().Lookup("orchestrator")).To(BeNil())
Expect(output.Flags().Lookup("version")).NotTo(BeNil())
Expect(command.Use).Should(Equal(getVersionsName))
Expect(command.Short).Should(Equal(getVersionsShortDescription))
Expect(command.Long).Should(Equal(getVersionsLongDescription))
Expect(command.Flags().Lookup("orchestrator")).To(BeNil())
Expect(command.Flags().Lookup("version")).NotTo(BeNil())

command.SetArgs([]string{})
err := command.Execute()
Expect(err).NotTo(HaveOccurred())
})

It("should support JSON output", func() {
Expand All @@ -26,7 +30,7 @@ var _ = Describe("The get-versions command", func() {
output: "json",
}
err := command.run(nil, nil)
Expect(err).To(BeNil())
Expect(err).NotTo(HaveOccurred())
})

It("should support human-readable output", func() {
Expand All @@ -36,7 +40,7 @@ var _ = Describe("The get-versions command", func() {
output: "human",
}
err := command.run(nil, nil)
Expect(err).To(BeNil())
Expect(err).NotTo(HaveOccurred())
})

It("should error on an invalid output option", func() {
Expand All @@ -46,7 +50,7 @@ var _ = Describe("The get-versions command", func() {
output: "yaml",
}
err := command.run(nil, nil)
Expect(err).NotTo(BeNil())
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("output format \"yaml\" is not supported"))
})
})
24 changes: 14 additions & 10 deletions cmd/orchestrators_test.go
View file
Expand Up @@ -10,13 +10,17 @@ import (

var _ = Describe("The orchestrators command", func() {
It("should create an orchestrators command", func() {
output := newOrchestratorsCmd()
command := newOrchestratorsCmd()

Expect(output.Use).Should(Equal(orchestratorsName))
Expect(output.Short).Should(Equal(orchestratorsShortDescription))
Expect(output.Long).Should(Equal(orchestratorsLongDescription))
Expect(output.Flags().Lookup("orchestrator")).NotTo(BeNil())
Expect(output.Flags().Lookup("version")).NotTo(BeNil())
Expect(command.Use).Should(Equal(orchestratorsName))
Expect(command.Short).Should(Equal(orchestratorsShortDescription))
Expect(command.Long).Should(Equal(orchestratorsLongDescription))
Expect(command.Flags().Lookup("orchestrator")).NotTo(BeNil())
Expect(command.Flags().Lookup("version")).NotTo(BeNil())

command.SetArgs([]string{})
err := command.Execute()
Expect(err).NotTo(HaveOccurred())
})

It("should fail on unsupported orchestrator", func() {
Expand All @@ -25,7 +29,7 @@ var _ = Describe("The orchestrators command", func() {
}

err := command.run(nil, nil)
Expect(err).NotTo(BeNil())
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("Unsupported orchestrator 'unsupported'"))
})

Expand All @@ -35,7 +39,7 @@ var _ = Describe("The orchestrators command", func() {
}

err := command.run(nil, nil)
Expect(err).NotTo(BeNil())
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("Must specify orchestrator for version '1.1.1'"))
})

Expand All @@ -46,7 +50,7 @@ var _ = Describe("The orchestrators command", func() {
}

err := command.run(nil, nil)
Expect(err).NotTo(BeNil())
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("Kubernetes version 1.1.1 is not supported"))
})

Expand All @@ -58,6 +62,6 @@ var _ = Describe("The orchestrators command", func() {
}

err := command.run(nil, nil)
Expect(err).To(BeNil())
Expect(err).NotTo(HaveOccurred())
})
})
4 changes: 2 additions & 2 deletions cmd/root.go
View file
Expand Up @@ -235,8 +235,8 @@ func getCompletionCmd(root *cobra.Command) *cobra.Command {
# ~/.bashrc or ~/.profile
source <(aks-engine completion)
`,
Run: func(cmd *cobra.Command, args []string) {
root.GenBashCompletion(os.Stdout)
RunE: func(cmd *cobra.Command, args []string) error {
return root.GenBashCompletion(os.Stdout)
},
}
return completionCmd
Expand Down
46 changes: 41 additions & 5 deletions cmd/root_test.go
View file
Expand Up @@ -12,17 +12,53 @@ import (
)

func TestNewRootCmd(t *testing.T) {
output := NewRootCmd()
if output.Use != rootName || output.Short != rootShortDescription || output.Long != rootLongDescription {
t.Fatalf("root command should have use %s equal %s, short %s equal %s and long %s equal to %s", output.Use, rootName, output.Short, rootShortDescription, output.Long, rootLongDescription)
command := NewRootCmd()
if command.Use != rootName || command.Short != rootShortDescription || command.Long != rootLongDescription {
t.Fatalf("root command should have use %s equal %s, short %s equal %s and long %s equal to %s", command.Use, rootName, command.Short, rootShortDescription, command.Long, rootLongDescription)
}
expectedCommands := []*cobra.Command{getCompletionCmd(output), newDeployCmd(), newGenerateCmd(), newGetVersionsCmd(), newOrchestratorsCmd(), newScaleCmd(), newUpgradeCmd(), newVersionCmd()}
rc := output.Commands()
expectedCommands := []*cobra.Command{getCompletionCmd(command), newDeployCmd(), newGenerateCmd(), newGetVersionsCmd(), newOrchestratorsCmd(), newScaleCmd(), newUpgradeCmd(), newVersionCmd()}
rc := command.Commands()
for i, c := range expectedCommands {
if rc[i].Use != c.Use {
t.Fatalf("root command should have command %s", c.Use)
}
}

command.SetArgs([]string{"--debug"})
err := command.Execute()
if err != nil {
t.Fatal(err)
}
}

func TestShowDefaultModelArg(t *testing.T) {
command := NewRootCmd()
command.SetArgs([]string{"--show-default-model"})
err := command.Execute()
if err != nil {
t.Fatal(err)
}
// TODO: examine command output
}

func TestDebugArg(t *testing.T) {
command := NewRootCmd()
command.SetArgs([]string{"--show-default-model"})
err := command.Execute()
if err != nil {
t.Fatal(err)
}
// TODO: examine command output
}

func TestCompletionCommand(t *testing.T) {
command := getCompletionCmd(NewRootCmd())
command.SetArgs([]string{})
err := command.Execute()
if err != nil {
t.Fatal(err)
}
// TODO: examine command output
}

func TestGetSelectedCloudFromAzConfig(t *testing.T) {
Expand Down

0 comments on commit a290ab8

Please sign in to comment.