From 2c4420c0f92d8a56cadce5be498f40f30431464b Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Wed, 16 May 2018 16:50:12 -0700 Subject: [PATCH 01/12] new deploy cmd test --- cmd/deploy_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 7181f53456..f98004787a 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -72,6 +72,20 @@ func getAPIModelWithoutServicePrincipalProfile(baseAPIModel string, useManagedId strconv.FormatBool(useManagedIdentity)) } +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) + } + + 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 { + t.Fatalf("deploy command should have flag %s", f) + } + } +} + func TestAutofillApimodelWithoutManagedIdentityCreatesCreds(t *testing.T) { testAutodeployCredentialHandling(t, false, "", "") } From 8d7232fd7ed537cc34f4f4c99d7de3b8fa3b5bd2 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Wed, 16 May 2018 18:15:14 -0700 Subject: [PATCH 02/12] add validate test deploy --- cmd/deploy.go | 16 ++++--- cmd/deploy_test.go | 104 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 5 deletions(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index 31420bb959..c16ebe0610 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -118,17 +118,23 @@ func (dc *deployCmd) validate(cmd *cobra.Command, args []string) error { return fmt.Errorf(fmt.Sprintf("specified api model does not exist (%s)", dc.apimodelPath)) } + if dc.location == "" { + return fmt.Errorf(fmt.Sprintf("--location must be specified")) + } + dc.location = helpers.NormalizeAzureRegion(dc.location) + + return nil +} + +func (dc *deployCmd) load(cmd *cobra.Command, args []string) error { + var err error + apiloader := &api.Apiloader{ Translator: &i18n.Translator{ Locale: dc.locale, }, } - if dc.location == "" { - return fmt.Errorf(fmt.Sprintf("--location must be specified")) - } - dc.location = helpers.NormalizeAzureRegion(dc.location) - dc.containerService, dc.apiVersion, err = apiloader.LoadContainerServiceFromFile(dc.apimodelPath, true, false, nil) if err != nil { return fmt.Errorf(fmt.Sprintf("error parsing the api model: %s", err.Error())) diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index f98004787a..d1fc269432 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/acs-engine/pkg/armhelpers" "github.com/satori/go.uuid" log "github.com/sirupsen/logrus" + "github.com/spf13/cobra" ) const ExampleAPIModel = `{ @@ -86,6 +87,109 @@ func TestNewDeployCmd(t *testing.T) { } } +func TestValidate(t *testing.T) { + r := &cobra.Command{} + apimodelPath := "apimodel-unit-test.json" + + _, err := os.Create(apimodelPath) + if err != nil { + t.Fatalf("unable to create test apimodel path: %s", err.Error()) + } + defer os.Remove(apimodelPath) + + cases := []struct { + dc *deployCmd + expectedErr error + args []string + }{ + { + dc: &deployCmd{ + apimodelPath: "", + dnsPrefix: "test", + outputDirectory: "output/test", + forceOverwrite: false, + caCertificatePath: "test", + caPrivateKeyPath: "test", + }, + args: []string{}, + expectedErr: fmt.Errorf("--api-model was not supplied, nor was one specified as a positional argument"), + }, + { + dc: &deployCmd{ + apimodelPath: "", + dnsPrefix: "test", + outputDirectory: "output/test", + caCertificatePath: "test", + caPrivateKeyPath: "test", + }, + args: []string{"wrong/path"}, + expectedErr: fmt.Errorf("specified api model does not exist (wrong/path)"), + }, + { + dc: &deployCmd{ + apimodelPath: "", + dnsPrefix: "test", + outputDirectory: "output/test", + caCertificatePath: "test", + caPrivateKeyPath: "test", + }, + args: []string{"test/apimodel.json", "some_random_stuff"}, + expectedErr: fmt.Errorf("too many arguments were provided to 'deploy'"), + }, + { + dc: &deployCmd{ + apimodelPath: "", + dnsPrefix: "test", + outputDirectory: "output/test", + caCertificatePath: "test", + caPrivateKeyPath: "test", + }, + args: []string{apimodelPath}, + expectedErr: fmt.Errorf("--location must be specified"), + }, + { + dc: &deployCmd{ + apimodelPath: "", + dnsPrefix: "test", + outputDirectory: "output/test", + caCertificatePath: "test", + caPrivateKeyPath: "test", + location: "west europe", + }, + args: []string{apimodelPath}, + expectedErr: nil, + }, + { + dc: &deployCmd{ + apimodelPath: apimodelPath, + dnsPrefix: "test", + outputDirectory: "output/test", + caCertificatePath: "test", + caPrivateKeyPath: "test", + location: "canadaeast", + }, + args: []string{}, + expectedErr: nil, + }, + } + + for _, c := range cases { + err = c.dc.validate(r, c.args) + if err != nil && c.expectedErr != nil { + if err.Error() != c.expectedErr.Error() { + t.Fatalf("expected validate deploy command to return error %s, but instead got %s", c.expectedErr.Error(), err.Error()) + } + } else { + if c.expectedErr != nil { + t.Fatalf("expected validate deploy command to return error %s, but instead got no error", c.expectedErr.Error()) + } else if err != nil { + t.Fatalf("expected validate deploy command to return no error, but instead got no error", err.Error()) + } + + } + } +} + func TestAutofillApimodelWithoutManagedIdentityCreatesCreds(t *testing.T) { testAutodeployCredentialHandling(t, false, "", "") } From 6aac6a23956a8357928b0ffa5638e4b0c5209514 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Wed, 16 May 2018 18:18:30 -0700 Subject: [PATCH 03/12] fix lint --- cmd/deploy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index d1fc269432..18c3ab7a6b 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -183,7 +183,7 @@ func TestValidate(t *testing.T) { if c.expectedErr != nil { t.Fatalf("expected validate deploy command to return error %s, but instead got no error", c.expectedErr.Error()) } else if err != nil { - t.Fatalf("expected validate deploy command to return no error, but instead got no error", err.Error()) + t.Fatalf("expected validate deploy command to return no error, but instead got %s", err.Error()) } } From 46aa80c8f4cf61d2bbe97bfb34eab73d7e91b391 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 10:47:46 -0700 Subject: [PATCH 04/12] added newcmd tests --- cmd/generate_test.go | 14 ++++++++++++++ cmd/orchestrators.go | 6 +++--- cmd/orchestrators_test.go | 10 ++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/cmd/generate_test.go b/cmd/generate_test.go index 24eb7196bc..bbd0c55ad2 100644 --- a/cmd/generate_test.go +++ b/cmd/generate_test.go @@ -6,6 +6,20 @@ import ( "github.com/spf13/cobra" ) +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) + } + + expectedFlags := []string{"api-model", "output-directory", "ca-certificate-path", "ca-private-key-path", "set", "classic-mode", "no-pretty-print", "parameters-only"} + for _, f := range expectedFlags { + if output.Flags().Lookup(f) == nil { + t.Fatalf("generate command should have flag %s", f) + } + } +} + func TestGenerateCmdValidate(t *testing.T) { g := &generateCmd{} r := &cobra.Command{} diff --git a/cmd/orchestrators.go b/cmd/orchestrators.go index bce5a4169d..6c40d4f6a8 100644 --- a/cmd/orchestrators.go +++ b/cmd/orchestrators.go @@ -9,9 +9,9 @@ import ( ) const ( - cmdName = "orchestrators" - cmdShortDescription = "Display info about supported orchestrators" - cmdLongDescription = "Display supported versions and upgrade versions for each orchestrator" + orchestratorsName = "orchestrators" + orchestratorsShortDescription = "Display info about supported orchestrators" + orchestratorsLongDescription = "Display supported versions and upgrade versions for each orchestrator" ) type orchestratorsCmd struct { diff --git a/cmd/orchestrators_test.go b/cmd/orchestrators_test.go index 7c7d53eb1a..21e0da7c76 100644 --- a/cmd/orchestrators_test.go +++ b/cmd/orchestrators_test.go @@ -6,6 +6,16 @@ import ( ) var _ = Describe("The orchestrators command", func() { + It("should create an orchestrators command", func() { + output := 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()) + }) + It("should fail on unsupported orchestrator", func() { command := &orchestratorsCmd{ orchestrator: "unsupported", From 1b234769f7dcec6b96e2954e4f5c5b6969cada69 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 10:51:00 -0700 Subject: [PATCH 05/12] fix var --- cmd/orchestrators.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/orchestrators.go b/cmd/orchestrators.go index 6c40d4f6a8..c19c214531 100644 --- a/cmd/orchestrators.go +++ b/cmd/orchestrators.go @@ -24,9 +24,9 @@ func newOrchestratorsCmd() *cobra.Command { oc := orchestratorsCmd{} command := &cobra.Command{ - Use: cmdName, - Short: cmdShortDescription, - Long: cmdLongDescription, + Use: orchestratorsName, + Short: orchestratorsShortDescription, + Long: orchestratorsLongDescription, RunE: func(cmd *cobra.Command, args []string) error { return oc.run(cmd, args) }, From f6578c1e689005d8d0d87169cea757aafe93e490 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 14:43:10 -0700 Subject: [PATCH 06/12] add load function in deploy run --- cmd/deploy.go | 4 ++++ cmd/deploy_test.go | 1 - 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/deploy.go b/cmd/deploy.go index c16ebe0610..2947e91de3 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -74,6 +74,10 @@ func newDeployCmd() *cobra.Command { if err := dc.validate(cmd, args); err != nil { log.Fatalf(fmt.Sprintf("error validating deployCmd: %s", err.Error())) } + err = sc.load(cmd, args) + if err != nil { + log.Fatalln("failed to load apimodel: %s", err.Error()) + } return dc.run() }, } diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go index 18c3ab7a6b..9b7d9c2cf3 100644 --- a/cmd/deploy_test.go +++ b/cmd/deploy_test.go @@ -185,7 +185,6 @@ func TestValidate(t *testing.T) { } else if err != nil { t.Fatalf("expected validate deploy command to return no error, but instead got %s", err.Error()) } - } } } From f8e7f6a1219308491d77a1f61388eb96914915f4 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 14:57:34 -0700 Subject: [PATCH 07/12] scale validate tests --- cmd/deploy.go | 3 +- cmd/scale.go | 82 +++++++++++++++++++++---------------- cmd/scale_test.go | 101 ++++++++++++++++++++++++++++++++++++++++++++++ cmd/upgrade.go | 8 ++-- 4 files changed, 153 insertions(+), 41 deletions(-) create mode 100644 cmd/scale_test.go diff --git a/cmd/deploy.go b/cmd/deploy.go index 2947e91de3..83d9b88041 100644 --- a/cmd/deploy.go +++ b/cmd/deploy.go @@ -74,8 +74,7 @@ func newDeployCmd() *cobra.Command { if err := dc.validate(cmd, args); err != nil { log.Fatalf(fmt.Sprintf("error validating deployCmd: %s", err.Error())) } - err = sc.load(cmd, args) - if err != nil { + if err := dc.load(cmd, args); err != nil { log.Fatalln("failed to load apimodel: %s", err.Error()) } return dc.run() diff --git a/cmd/scale.go b/cmd/scale.go index 5c5adbb319..ee2ece1ad7 100644 --- a/cmd/scale.go +++ b/cmd/scale.go @@ -34,21 +34,21 @@ type scaleCmd struct { resourceGroupName string deploymentDirectory string newDesiredAgentCount int - containerService *api.ContainerService - apiVersion string location string agentPoolToScale string classicMode bool + masterFQDN string // derived - apiModelPath string - agentPool *api.AgentPoolProfile - client armhelpers.ACSEngineClient - locale *gotext.Locale - nameSuffix string - agentPoolIndex int - masterFQDN string - logger *log.Entry + containerService *api.ContainerService + apiVersion string + apiModelPath string + agentPool *api.AgentPoolProfile + client armhelpers.ACSEngineClient + locale *gotext.Locale + nameSuffix string + agentPoolIndex int + logger *log.Entry } const ( @@ -84,56 +84,62 @@ func newScaleCmd() *cobra.Command { return scaleCmd } -func (sc *scaleCmd) validate(cmd *cobra.Command, args []string) { +func (sc *scaleCmd) validate(cmd *cobra.Command) error { log.Infoln("validating...") - sc.logger = log.New().WithField("source", "scaling command line") var err error sc.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 sc.resourceGroupName == "" { cmd.Usage() - log.Fatal("--resource-group must be specified") + return fmt.Errorf("--resource-group must be specified") } if sc.location == "" { cmd.Usage() - log.Fatal("--location must be specified") - } else { - sc.location = helpers.NormalizeAzureRegion(sc.location) + return fmt.Errorf("--location must be specified") } + sc.location = helpers.NormalizeAzureRegion(sc.location) + if sc.newDesiredAgentCount == 0 { cmd.Usage() - log.Fatal("--new-node-count must be specified") + return fmt.Errorf("--new-node-count must be specified") } - if err = sc.authArgs.validateAuthArgs(); err != nil { - log.Fatal("%s", err) + if sc.deploymentDirectory == "" { + cmd.Usage() + return fmt.Errorf("--deployment-dir must be specified") } - if sc.client, err = sc.authArgs.getClient(); err != nil { - log.Error("Failed to get client:", err) + if err = sc.authArgs.validateAuthArgs(); err != nil { + return fmt.Errorf("%s", err.Error()) } - if sc.deploymentDirectory == "" { - cmd.Usage() - log.Fatal("--deployment-dir must be specified") + return nil +} + +func (sc *scaleCmd) load(cmd *cobra.Command) error { + sc.logger = log.New().WithField("source", "scaling command line") + var err error + + if sc.client, err = sc.authArgs.getClient(); err != nil { + return fmt.Errorf("failed to get client: %s", err.Error()) } _, err = sc.client.EnsureResourceGroup(sc.resourceGroupName, sc.location, nil) if err != nil { - log.Fatalln(err) + return fmt.Errorf("%s", err.Error()) } // load apimodel from the deployment directory sc.apiModelPath = path.Join(sc.deploymentDirectory, "apimodel.json") if _, err = os.Stat(sc.apiModelPath); os.IsNotExist(err) { - log.Fatalf("specified api model does not exist (%s)", sc.apiModelPath) + return fmt.Errorf("specified api model does not exist (%s)", sc.apiModelPath) } apiloader := &api.Apiloader{ @@ -143,25 +149,25 @@ func (sc *scaleCmd) validate(cmd *cobra.Command, args []string) { } sc.containerService, sc.apiVersion, err = apiloader.LoadContainerServiceFromFile(sc.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 sc.containerService.Location == "" { sc.containerService.Location = sc.location } else if sc.containerService.Location != sc.location { - log.Fatalf("--location does not match api model location") + return fmt.Errorf("--location does not match api model location") } if sc.agentPoolToScale == "" { agentPoolCount := len(sc.containerService.Properties.AgentPoolProfiles) if agentPoolCount > 1 { - log.Fatal("--node-pool is required if more than one agent pool is defined in the container service") + return fmt.Errorf("--node-pool is required if more than one agent pool is defined in the container service") } else if agentPoolCount == 1 { sc.agentPool = sc.containerService.Properties.AgentPoolProfiles[0] sc.agentPoolIndex = 0 sc.agentPoolToScale = sc.containerService.Properties.AgentPoolProfiles[0].Name } else { - log.Fatal("No node pools found to scale") + return fmt.Errorf("No node pools found to scale") } } else { agentPoolIndex := -1 @@ -173,7 +179,7 @@ func (sc *scaleCmd) validate(cmd *cobra.Command, args []string) { } } if agentPoolIndex == -1 { - log.Fatalf("node pool %s wasn't in the deployed api model", sc.agentPoolToScale) + return fmt.Errorf("node pool %s was not found in the deployed api model", sc.agentPoolToScale) } } @@ -188,11 +194,17 @@ func (sc *scaleCmd) validate(cmd *cobra.Command, args []string) { nameSuffixParam := templateParameters["nameSuffix"].(map[string]interface{}) sc.nameSuffix = nameSuffixParam["defaultValue"].(string) - log.Infoln(fmt.Sprintf("Name suffix: %s", sc.nameSuffix)) + log.Infoln("Name suffix: %s", sc.nameSuffix) + return nil } func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { - sc.validate(cmd, args) + if err := sc.validate(cmd); err != nil { + log.Fatalln("failed to validate scale command: %s", err.Error()) + } + if err := sc.load(cmd); err != nil { + log.Fatalln("failed to load existing container service: %s", err.Error()) + } orchestratorInfo := sc.containerService.Properties.OrchestratorProfile var currentNodeCount, highestUsedIndex int @@ -204,7 +216,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { if err != nil { log.Fatalln("failed to get vms in the resource group. Error: %s", err.Error()) } else if len(*vms.Value) < 1 { - log.Fatalln("The provided resource group does not contain any vms.") + log.Fatalln("The provided resource group does not contain any vms") } for _, vm := range *vms.Value { diff --git a/cmd/scale_test.go b/cmd/scale_test.go new file mode 100644 index 0000000000..3c94b54ec6 --- /dev/null +++ b/cmd/scale_test.go @@ -0,0 +1,101 @@ +package cmd + +import ( + "fmt" + "testing" + + "github.com/spf13/cobra" +) + +func TestNewScaleCmd(t *testing.T) { + output := newScaleCmd() + if output.Use != scaleName || output.Short != scaleShortDescription || output.Long != scaleLongDescription { + t.Fatalf("scale command should have use %s equal %s, short %s equal %s and long %s equal to %s", output.Use, scaleName, output.Short, scaleShortDescription, output.Long, scaleLongDescription) + } + + expectedFlags := []string{"location", "resource-group", "deployment-dir", "new-node-count", "classic-mode", "node-pool", "master-FQDN"} + for _, f := range expectedFlags { + if output.Flags().Lookup(f) == nil { + t.Fatalf("scale command should have flag %s", f) + } + } +} + +func TestScaleCmdValidate(t *testing.T) { + r := &cobra.Command{} + + cases := []struct { + sc *scaleCmd + expectedErr error + }{ + { + sc: &scaleCmd{ + location: "centralus", + resourceGroupName: "", + deploymentDirectory: "_output/test", + agentPoolToScale: "agentpool1", + newDesiredAgentCount: 5, + masterFQDN: "test", + }, + expectedErr: fmt.Errorf("--resource-group must be specified"), + }, + { + sc: &scaleCmd{ + location: "", + resourceGroupName: "testRG", + deploymentDirectory: "_output/test", + agentPoolToScale: "agentpool1", + newDesiredAgentCount: 5, + masterFQDN: "test", + }, + expectedErr: fmt.Errorf("--location must be specified"), + }, + { + sc: &scaleCmd{ + location: "centralus", + resourceGroupName: "testRG", + deploymentDirectory: "_output/test", + agentPoolToScale: "agentpool1", + masterFQDN: "test", + }, + expectedErr: fmt.Errorf("--new-node-count must be specified"), + }, + { + sc: &scaleCmd{ + location: "centralus", + resourceGroupName: "testRG", + deploymentDirectory: "", + agentPoolToScale: "agentpool1", + newDesiredAgentCount: 5, + masterFQDN: "test", + }, + expectedErr: fmt.Errorf("--deployment-dir must be specified"), + }, + { + sc: &scaleCmd{ + location: "centralus", + resourceGroupName: "testRG", + deploymentDirectory: "_output/test", + agentPoolToScale: "agentpool1", + newDesiredAgentCount: 5, + masterFQDN: "test", + }, + expectedErr: nil, + }, + } + + for _, c := range cases { + err := c.sc.validate(r) + if err != nil && c.expectedErr != nil { + if err.Error() != c.expectedErr.Error() { + t.Fatalf("expected validate scale command to return error %s, but instead got %s", c.expectedErr.Error(), err.Error()) + } + } else { + if c.expectedErr != nil { + t.Fatalf("expected validate scale command to return error %s, but instead got no error", c.expectedErr.Error()) + } else if err != nil { + t.Fatalf("expected validate scale command to return no error, but instead got %s", err.Error()) + } + } + } +} diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 993724d365..b648e6adb4 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -106,16 +106,16 @@ func (uc *upgradeCmd) validate(cmd *cobra.Command) error { cmd.Usage() 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 + if err = uc.authArgs.validateAuthArgs(); err != nil { + return fmt.Errorf("%s", err) + } + if uc.client, err = uc.authArgs.getClient(); err != nil { return fmt.Errorf("Failed to get client: %s", err) } From c122886872a99cd43fed1602d4da927d47ddeb10 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 15:05:00 -0700 Subject: [PATCH 08/12] fix upgrade test --- cmd/upgrade.go | 6 +++--- cmd/upgrade_test.go | 30 ------------------------------ 2 files changed, 3 insertions(+), 33 deletions(-) diff --git a/cmd/upgrade.go b/cmd/upgrade.go index b648e6adb4..36e458abca 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -113,16 +113,16 @@ func (uc *upgradeCmd) loadCluster(cmd *cobra.Command) error { var err error if err = uc.authArgs.validateAuthArgs(); err != nil { - return fmt.Errorf("%s", err) + return fmt.Errorf("%s", err.Error()) } if uc.client, err = uc.authArgs.getClient(); err != nil { - return fmt.Errorf("Failed to get client: %s", err) + return fmt.Errorf("Failed to get client: %s", err.Error()) } _, err = uc.client.EnsureResourceGroup(uc.resourceGroupName, uc.location, nil) if err != nil { - return fmt.Errorf("Error ensuring resource group: %s", err) + return fmt.Errorf("Error ensuring resource group: %s", err.Error()) } // load apimodel from the deployment directory diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index ce809dedc9..2874a2739a 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -36,9 +36,6 @@ var _ = Describe("the upgrade command", func() { upgradeVersion: "1.8.9", location: "centralus", timeoutInMinutes: 60, - authArgs: authArgs{ - rawSubscriptionID: "99999999-0000-0000-0000-000000000000", - }, }, expectedErr: fmt.Errorf("--resource-group must be specified"), }, @@ -49,9 +46,6 @@ var _ = Describe("the upgrade command", func() { upgradeVersion: "1.8.9", location: "", timeoutInMinutes: 60, - authArgs: authArgs{ - rawSubscriptionID: "99999999-0000-0000-0000-000000000000", - }, }, expectedErr: fmt.Errorf("--location must be specified"), }, @@ -62,9 +56,6 @@ var _ = Describe("the upgrade command", func() { upgradeVersion: "", location: "southcentralus", timeoutInMinutes: 60, - authArgs: authArgs{ - rawSubscriptionID: "99999999-0000-0000-0000-000000000000", - }, }, expectedErr: fmt.Errorf("--upgrade-version must be specified"), }, @@ -75,9 +66,6 @@ var _ = Describe("the upgrade command", func() { upgradeVersion: "1.9.0", location: "southcentralus", timeoutInMinutes: 60, - authArgs: authArgs{ - rawSubscriptionID: "99999999-0000-0000-0000-000000000000", - }, }, expectedErr: fmt.Errorf("--deployment-dir must be specified"), }, @@ -88,9 +76,6 @@ var _ = Describe("the upgrade command", func() { upgradeVersion: "1.9.0", location: "southcentralus", timeoutInMinutes: 60, - authArgs: authArgs{ - rawSubscriptionID: "99999999-0000-0000-0000-000000000000", - }, }, expectedErr: fmt.Errorf("--deployment-dir must be specified"), }, @@ -100,21 +85,6 @@ var _ = Describe("the upgrade command", func() { 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, }, From b88c05304101786e198cf5bd7f932e56cd92b8c4 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 15:08:50 -0700 Subject: [PATCH 09/12] move authargs validation --- cmd/scale.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/scale.go b/cmd/scale.go index ee2ece1ad7..4d3935fbaf 100644 --- a/cmd/scale.go +++ b/cmd/scale.go @@ -115,10 +115,6 @@ func (sc *scaleCmd) validate(cmd *cobra.Command) error { return fmt.Errorf("--deployment-dir must be specified") } - if err = sc.authArgs.validateAuthArgs(); err != nil { - return fmt.Errorf("%s", err.Error()) - } - return nil } @@ -126,6 +122,10 @@ func (sc *scaleCmd) load(cmd *cobra.Command) error { sc.logger = log.New().WithField("source", "scaling command line") var err error + if err = sc.authArgs.validateAuthArgs(); err != nil { + return fmt.Errorf("%s", err.Error()) + } + if sc.client, err = sc.authArgs.getClient(); err != nil { return fmt.Errorf("failed to get client: %s", err.Error()) } From 78f470c94f26e5aeaa50eaea5416768d2a418f1e Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 17:01:57 -0700 Subject: [PATCH 10/12] fixed scale.go error handling --- cmd/scale.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/cmd/scale.go b/cmd/scale.go index 4d3935fbaf..baca787dd3 100644 --- a/cmd/scale.go +++ b/cmd/scale.go @@ -123,7 +123,7 @@ func (sc *scaleCmd) load(cmd *cobra.Command) error { var err error if err = sc.authArgs.validateAuthArgs(); err != nil { - return fmt.Errorf("%s", err.Error()) + return err } if sc.client, err = sc.authArgs.getClient(); err != nil { @@ -132,7 +132,7 @@ func (sc *scaleCmd) load(cmd *cobra.Command) error { _, err = sc.client.EnsureResourceGroup(sc.resourceGroupName, sc.location, nil) if err != nil { - return fmt.Errorf("%s", err.Error()) + return err } // load apimodel from the deployment directory @@ -200,10 +200,10 @@ func (sc *scaleCmd) load(cmd *cobra.Command) error { func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { if err := sc.validate(cmd); err != nil { - log.Fatalln("failed to validate scale command: %s", err.Error()) + return fmt.Errorf("failed to validate scale command: %s", err.Error()) } if err := sc.load(cmd); err != nil { - log.Fatalln("failed to load existing container service: %s", err.Error()) + return fmt.Errorf("failed to load existing container service: %s", err.Error()) } orchestratorInfo := sc.containerService.Properties.OrchestratorProfile @@ -214,9 +214,9 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { //TODO handle when there is a nextLink in the response and get more nodes vms, err := sc.client.ListVirtualMachines(sc.resourceGroupName) if err != nil { - log.Fatalln("failed to get vms in the resource group. Error: %s", err.Error()) + return fmt.Errorf("failed to get vms in the resource group. Error: %s", err.Error()) } else if len(*vms.Value) < 1 { - log.Fatalln("The provided resource group does not contain any vms") + return fmt.Errorf("The provided resource group does not contain any vms") } for _, vm := range *vms.Value { @@ -243,7 +243,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { if currentNodeCount > sc.newDesiredAgentCount { if sc.masterFQDN == "" { cmd.Usage() - log.Fatal("master-FQDN is required to scale down a kubernetes cluster's agent pool") + return fmt.Errorf("master-FQDN is required to scale down a kubernetes cluster's agent pool") } vmsToDelete := make([]string, 0) @@ -255,12 +255,12 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { case api.Kubernetes: kubeConfig, err := acsengine.GenerateKubeConfig(sc.containerService.Properties, sc.location) if err != nil { - log.Errorf("failed to generate kube config: %v", err) + return fmt.Errorf("failed to generate kube config: %v", err) return err } err = sc.drainNodes(kubeConfig, vmsToDelete) if err != nil { - log.Errorf("Got error %+v, while draining the nodes to be deleted", err) + return fmt.Errorf("Got error %+v, while draining the nodes to be deleted", err) return err } case api.OpenShift: @@ -297,7 +297,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { } else { vmssList, err := sc.client.ListVirtualMachineScaleSets(sc.resourceGroupName) if err != nil { - log.Fatalln("failed to get vmss list in the resource group. Error: %s", err.Error()) + return fmt.Errorf("failed to get vmss list in the resource group. Error: %s", err.Error()) } for _, vmss := range *vmssList.Value { poolName, nameSuffix, err := utils.VmssNameParts(*vmss.Name) @@ -316,19 +316,19 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { } templateGenerator, err := acsengine.InitializeTemplateGenerator(ctx, sc.classicMode) if err != nil { - log.Fatalln("failed to initialize template generator: %s", err.Error()) + return fmt.Errorf("failed to initialize template generator: %s", err.Error()) } sc.containerService.Properties.AgentPoolProfiles = []*api.AgentPoolProfile{sc.agentPool} template, parameters, _, err := templateGenerator.GenerateTemplate(sc.containerService, acsengine.DefaultGeneratorCode, false, BuildTag) if err != nil { - log.Fatalf("error generating template %s: %s", sc.apiModelPath, err.Error()) + return fmt.Errorf("error generating template %s: %s", sc.apiModelPath, err.Error()) os.Exit(1) } if template, err = transform.PrettyPrintArmTemplate(template); err != nil { - log.Fatalf("error pretty printing template: %s \n", err.Error()) + return fmt.Errorf("error pretty printing template: %s \n", err.Error()) } templateJSON := make(map[string]interface{}) @@ -336,12 +336,12 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { err = json.Unmarshal([]byte(template), &templateJSON) if err != nil { - log.Fatalln(err) + return err } err = json.Unmarshal([]byte(parameters), ¶metersJSON) if err != nil { - log.Fatalln(err) + retur err } transformer := transform.Transformer{Translator: ctx.Translator} @@ -365,7 +365,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { case api.Kubernetes: err = transformer.NormalizeForK8sVMASScalingUp(sc.logger, templateJSON) if err != nil { - log.Fatalf("error tranforming the template for scaling template %s: %s", sc.apiModelPath, err.Error()) + return fmt.Errorf("error tranforming the template for scaling template %s: %s", sc.apiModelPath, err.Error()) os.Exit(1) } if sc.agentPool.IsAvailabilitySets() { @@ -375,7 +375,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { case api.SwarmMode: case api.DCOS: if sc.agentPool.IsAvailabilitySets() { - log.Fatalf("scaling isn't supported for orchestrator %s, with availability sets", orchestratorInfo.OrchestratorType) + return fmt.Errorf("scaling isn't supported for orchestrator %s, with availability sets", orchestratorInfo.OrchestratorType) os.Exit(1) } transformer.NormalizeForVMSSScaling(sc.logger, templateJSON) @@ -391,7 +391,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { parametersJSON, nil) if err != nil { - log.Fatalln(err) + return err } apiloader := &api.Apiloader{ From a12af93e46756902ee3d62c15712726136e40b95 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 17:04:06 -0700 Subject: [PATCH 11/12] fix typo --- cmd/scale.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/scale.go b/cmd/scale.go index baca787dd3..dbb5c60fdf 100644 --- a/cmd/scale.go +++ b/cmd/scale.go @@ -341,7 +341,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { err = json.Unmarshal([]byte(parameters), ¶metersJSON) if err != nil { - retur err + return err } transformer := transform.Transformer{Translator: ctx.Translator} From 441a7f9f9a521dd4a6f3a7e5b6abc463928fe8e8 Mon Sep 17 00:00:00 2001 From: CecileRobertMichon Date: Thu, 17 May 2018 17:09:19 -0700 Subject: [PATCH 12/12] remove unreachable code --- cmd/scale.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/cmd/scale.go b/cmd/scale.go index dbb5c60fdf..c10a89f8fd 100644 --- a/cmd/scale.go +++ b/cmd/scale.go @@ -256,12 +256,10 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { kubeConfig, err := acsengine.GenerateKubeConfig(sc.containerService.Properties, sc.location) if err != nil { return fmt.Errorf("failed to generate kube config: %v", err) - return err } err = sc.drainNodes(kubeConfig, vmsToDelete) if err != nil { return fmt.Errorf("Got error %+v, while draining the nodes to be deleted", err) - return err } case api.OpenShift: bundle := bytes.NewReader(sc.containerService.Properties.OrchestratorProfile.OpenShiftConfig.ConfigBundles["master"]) @@ -324,11 +322,10 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { template, parameters, _, err := templateGenerator.GenerateTemplate(sc.containerService, acsengine.DefaultGeneratorCode, false, BuildTag) if err != nil { return fmt.Errorf("error generating template %s: %s", sc.apiModelPath, err.Error()) - os.Exit(1) } if template, err = transform.PrettyPrintArmTemplate(template); err != nil { - return fmt.Errorf("error pretty printing template: %s \n", err.Error()) + return fmt.Errorf("error pretty printing template: %s", err.Error()) } templateJSON := make(map[string]interface{}) @@ -366,7 +363,6 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { err = transformer.NormalizeForK8sVMASScalingUp(sc.logger, templateJSON) if err != nil { return fmt.Errorf("error tranforming the template for scaling template %s: %s", sc.apiModelPath, err.Error()) - os.Exit(1) } if sc.agentPool.IsAvailabilitySets() { addValue(parametersJSON, fmt.Sprintf("%sOffset", sc.agentPool.Name), highestUsedIndex+1) @@ -376,7 +372,6 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { case api.DCOS: if sc.agentPool.IsAvailabilitySets() { return fmt.Errorf("scaling isn't supported for orchestrator %s, with availability sets", orchestratorInfo.OrchestratorType) - os.Exit(1) } transformer.NormalizeForVMSSScaling(sc.logger, templateJSON) }