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

Replace log.Fatal by return errors in deploy.go #3116

Merged
merged 6 commits into from
Jun 1, 2018
Merged

Replace log.Fatal by return errors in deploy.go #3116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e6e942d
return err for autofill
May 30, 2018
c1dcc64
replace log fatal with return error
May 31, 2018
8d34236
check for err in unit tests
May 31, 2018
9aa8a31
fix typo
Jun 1, 2018
e67e7a3
lint
Jun 1, 2018
b806348
log -> t
Jun 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions cmd/deploy.go
View file
Open in desktop
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not anymore 😈

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
Open in desktop
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the last line of defense? (i.e., no return err?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a unit test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/me takes off beer goggles

}

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackfrancis you actually just made me realize this should be t not log

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I changed it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O.K. beer googles worked!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍻

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.")
}
}
}