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

Commit

Permalink
Replace log.Fatal by return errors in deploy.go (#3116)
Browse files Browse the repository at this point in the history 
* return err for autofill

* replace log fatal with return error

* check for err in unit tests

* fix typo

* lint

* log -> t
  • Loading branch information
Cecile Robert-Michon authored Jun 1, 2018
1 parent b6b0e12 commit f5c08ef
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 22 deletions.
26 changes: 14 additions & 12 deletions cmd/deploy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,20 +159,21 @@ func (dc *deployCmd) load(cmd *cobra.Command, args []string) error {
return fmt.Errorf("failed to get client: %s", err.Error())
}

// autofillApimodel calls log.Fatal() directly and does not return errors
autofillApimodel(dc)
if err = autofillApimodel(dc); err != nil {
return err
}

_, _, err = validateApimodel(apiloader, dc.containerService, dc.apiVersion)
if err != nil {
return fmt.Errorf(fmt.Sprintf("Failed to validate the apimodel after populating values: %s", err))
return fmt.Errorf("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) {
func autofillApimodel(dc *deployCmd) error {
var err error

if dc.containerService.Properties.LinuxProfile != nil {
Expand All @@ -183,11 +184,11 @@ func autofillApimodel(dc *deployCmd) {
}

if dc.dnsPrefix != "" && dc.containerService.Properties.MasterProfile.DNSPrefix != "" {
log.Fatalf("invalid configuration: the apimodel masterProfile.dnsPrefix and --dns-prefix were both specified")
return fmt.Errorf("invalid configuration: the apimodel masterProfile.dnsPrefix and --dns-prefix were both specified")
}
if dc.containerService.Properties.MasterProfile.DNSPrefix == "" {
if dc.dnsPrefix == "" {
log.Fatalf("apimodel: missing masterProfile.dnsPrefix and --dns-prefix was not specified")
return fmt.Errorf("apimodel: missing masterProfile.dnsPrefix and --dns-prefix was not specified")
}
log.Warnf("apimodel: missing masterProfile.dnsPrefix will use %q", dc.dnsPrefix)
dc.containerService.Properties.MasterProfile.DNSPrefix = dc.dnsPrefix
Expand All @@ -203,15 +204,15 @@ func autofillApimodel(dc *deployCmd) {
}

if _, err := os.Stat(dc.outputDirectory); !dc.forceOverwrite && err == nil {
log.Fatalf(fmt.Sprintf("Output directory already exists and forceOverwrite flag is not set: %s", dc.outputDirectory))
return fmt.Errorf("Output directory already exists and forceOverwrite flag is not set: %s", dc.outputDirectory)
}

if dc.resourceGroup == "" {
dnsPrefix := dc.containerService.Properties.MasterProfile.DNSPrefix
log.Warnf("--resource-group was not specified. Using the DNS prefix from the apimodel as the resource group name: %s", dnsPrefix)
dc.resourceGroup = dnsPrefix
if dc.location == "" {
log.Fatal("--resource-group was not specified. --location must be specified in case the resource group needs creation.")
return fmt.Errorf("--resource-group was not specified. --location must be specified in case the resource group needs creation")
}
}

Expand All @@ -223,15 +224,15 @@ func autofillApimodel(dc *deployCmd) {
}
_, publicKey, err := acsengine.CreateSaveSSH(dc.containerService.Properties.LinuxProfile.AdminUsername, dc.outputDirectory, translator)
if err != nil {
log.Fatal("Failed to generate SSH Key")
return fmt.Errorf("Failed to generate SSH Key: %s", err.Error())
}

dc.containerService.Properties.LinuxProfile.SSH.PublicKeys = []api.PublicKey{{KeyData: publicKey}}
}

_, err = dc.client.EnsureResourceGroup(dc.resourceGroup, dc.location, nil)
if err != nil {
log.Fatalln(err)
return err
}

useManagedIdentity := dc.containerService.Properties.OrchestratorProfile.KubernetesConfig != nil &&
Expand Down Expand Up @@ -265,15 +266,15 @@ func autofillApimodel(dc *deployCmd) {
}
applicationID, servicePrincipalObjectID, secret, err := dc.client.CreateApp(appName, appURL, replyURLs, requiredResourceAccess)
if err != nil {
log.Fatalf("apimodel invalid: ServicePrincipalProfile was empty, and we failed to create valid credentials: %q", err)
return fmt.Errorf("apimodel invalid: ServicePrincipalProfile was empty, and we failed to create valid credentials: %q", err)
}
log.Warnf("created application with applicationID (%s) and servicePrincipalObjectID (%s).", applicationID, servicePrincipalObjectID)

log.Warnln("apimodel: ServicePrincipalProfile was empty, assigning role to application...")

err = dc.client.CreateRoleAssignmentSimple(dc.resourceGroup, servicePrincipalObjectID)
if err != nil {
log.Fatalf("apimodel: could not create or assign ServicePrincipal: %q", err)
return fmt.Errorf("apimodel: could not create or assign ServicePrincipal: %q", err)

}

Expand All @@ -289,6 +290,7 @@ func autofillApimodel(dc *deployCmd) {
}
}
}
return nil
}

func validateApimodel(apiloader *api.Apiloader, containerService *api.ContainerService, apiVersion string) (*api.ContainerService, string, error) {
Expand Down
39 changes: 29 additions & 10 deletions cmd/deploy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/Azure/acs-engine/pkg/api"
"github.com/Azure/acs-engine/pkg/armhelpers"
"github.com/satori/go.uuid"
log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -222,7 +221,11 @@ func TestAutoSufixWithDnsPrefixInApiModel(t *testing.T) {

client: &armhelpers.MockACSEngineClient{},
}
autofillApimodel(deployCmd)

err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

defer os.RemoveAll(deployCmd.outputDirectory)

Expand Down Expand Up @@ -261,7 +264,11 @@ func TestAPIModelWithoutServicePrincipalProfileAndClientIdAndSecretInCmd(t *test
}
deployCmd.ClientID = TestClientIDInCmd
deployCmd.ClientSecret = TestClientSecretInCmd
autofillApimodel(deployCmd)

err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

defer os.RemoveAll(deployCmd.outputDirectory)

Expand Down Expand Up @@ -307,7 +314,10 @@ func TestAPIModelWithEmptyServicePrincipalProfileAndClientIdAndSecretInCmd(t *te
}
deployCmd.ClientID = TestClientIDInCmd
deployCmd.ClientSecret = TestClientSecretInCmd
autofillApimodel(deployCmd)
err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

defer os.RemoveAll(deployCmd.outputDirectory)

Expand Down Expand Up @@ -345,7 +355,10 @@ func TestAPIModelWithoutServicePrincipalProfileAndWithoutClientIdAndSecretInCmd(

client: &armhelpers.MockACSEngineClient{},
}
autofillApimodel(deployCmd)
err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

defer os.RemoveAll(deployCmd.outputDirectory)

Expand Down Expand Up @@ -376,7 +389,10 @@ func TestAPIModelWithEmptyServicePrincipalProfileAndWithoutClientIdAndSecretInCm

client: &armhelpers.MockACSEngineClient{},
}
autofillApimodel(deployCmd)
err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

defer os.RemoveAll(deployCmd.outputDirectory)

Expand Down Expand Up @@ -423,25 +439,28 @@ func testAutodeployCredentialHandling(t *testing.T, useManagedIdentity bool, cli
client: &armhelpers.MockACSEngineClient{},
}

autofillApimodel(deployCmd)
err = autofillApimodel(deployCmd)
if err != nil {
t.Fatalf("unexpected error autofilling the example apimodel: %s", err)
}

// cleanup, since auto-populations creates dirs and saves the SSH private key that it might create
defer os.RemoveAll(deployCmd.outputDirectory)

cs, _, err = validateApimodel(apiloader, cs, ver)
if err != nil {
log.Fatalf("unexpected error validating apimodel after populating defaults: %s", err)
t.Fatalf("unexpected error validating apimodel after populating defaults: %s", err)
}

if useManagedIdentity {
if cs.Properties.ServicePrincipalProfile != nil &&
(cs.Properties.ServicePrincipalProfile.ClientID != "" || cs.Properties.ServicePrincipalProfile.Secret != "") {
log.Fatalf("Unexpected credentials were populated even though MSI was active.")
t.Fatalf("Unexpected credentials were populated even though MSI was active.")
}
} else {
if cs.Properties.ServicePrincipalProfile == nil ||
cs.Properties.ServicePrincipalProfile.ClientID == "" || cs.Properties.ServicePrincipalProfile.Secret == "" {
log.Fatalf("Credentials were missing even though MSI was not active.")
t.Fatalf("Credentials were missing even though MSI was not active.")
}
}
}

0 comments on commit f5c08ef

Please sign in to comment.