feat: add profile flag to root init command #642

bvtujo · 2020-02-07T01:07:51Z

Resolves #317

Adds a profile flag to root init command to be used by the implicit call to env init.
If no profile is specified and the user opts to deploy a test env, prompt them to select from the list of available profiles. If a profile is specified, deploy the test env into that profile.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This commit adds a profile flag to `ecs-preview init` which is passed into the implicit call to `ecs-preview env init`. If the user does not specify a profile, ask them to select a profile.

bvtujo · 2020-02-07T01:37:34Z

More unit tests forthcoming once I dig more into how mocks work

iamhopaul123 · 2020-02-11T18:02:46Z

internal/pkg/cli/init.go

@@ -221,6 +227,9 @@ func (o *initOpts) deployEnv() error {
 		// User chose not to deploy the application, exit.
 		return nil
 	}
+	if err := o.initEnv.Ask(); err != nil {


Not sure if we need this. initEnv.Ask() is used to get EnvName and EnvProfile in an interactive way. However, init by default (if not pass in value using flags) is supposed to provide an easy way for users to spin up their apps easily. That's the reason why we default EnvName to be "test" and EnvProfile to be "default" since in most cases for new projects they are set to be these values.

Also we always have value for EnvName, the Ask() command is just used to get the profile name (asking for env name will be skipped).

iamhopaul123 · 2020-02-11T18:04:02Z

internal/pkg/cli/init.go

@@ -274,6 +283,7 @@ func BuildInitCmd() *cobra.Command {
 			return nil
 		}),
 	}
+	cmd.Flags().StringVar(&vars.profile, profileFlag, "", profileFlagDescription)


Would it be nice to give default value for the flag as "default" so that we can remove the initEnv.Ask() and simplify the workflow?

That's a good idea, we can set that default in init but leave it empty in env init

But if we set the default value for the flag to be "default" then in the initEnv.Ask() it won't ask user to select since o.EnvProfile will not be an empty string.

I can make this change, but I'd like to make sure we document it somewhere. Is there a good spot that we can stick a line in the env init calls that will remind the user "hey, this will deploy to your default profile; if that's not cool, choose no and then run env deploy later.

I could simply modify the initShouldDeployPrompt to the following:
"Would you like to deploy a test environment? (This will deploy to your default profile if not otherwise specified with the --profile flag)" but that's not dynamic, and it seems like we could do better by only printing off a warning line when the user hasn't specified a profile.

efekarakus · 2020-02-11T17:57:55Z

internal/pkg/cli/init.go

@@ -77,10 +78,15 @@ func newInitOpts(vars initVars) (*initOpts, error) {
 	if err != nil {
 		return nil, err
 	}
+	profileSess, err := sessProvider.FromProfile(vars.profile)


I think the reason why this doesn't error when vars.profile="" is this:

// If not set and environment variables are not set the "default"
// (DefaultSharedConfigProfile) will be used as the profile to load the
// session config from.

So right now it's creating a session with the default profile which is slightly misleading.

My intuition says that we should just not set this field until o.initEnv.Ask() is executed.

efekarakus · 2020-02-11T18:09:58Z

internal/pkg/cli/init.go

@@ -221,6 +227,9 @@ func (o *initOpts) deployEnv() error {
 		// User chose not to deploy the application, exit.
 		return nil
 	}
+	if err := o.initEnv.Ask(); err != nil {
+		return err
+	}
 	return o.initEnv.Execute()


While trying to understand how come this worked, I realized that we have a bug in "env init" :|

Right now the value from askEnvProfile in env_init.go gets ignored inside Execute. So even if you choose a different profile than "default", we pick up only "default".
https://github.com/aws/amazon-ecs-cli-v2/blob/b223142e566e3bd123b541ff8cc500d4b3a113fb/internal/pkg/cli/env_init.go#L120

We should change the code so that we initialize envDeployer and envIdentity only after the EnvProfile value is set.

Here is a sample way of how to do it in env_delete.go:
https://github.com/aws/amazon-ecs-cli-v2/blob/b223142e566e3bd123b541ff8cc500d4b3a113fb/internal/pkg/cli/env_delete.go#L60

bvtujo · 2020-02-15T00:35:23Z

Behavior has been updated to not ask for a profile when running ecs-preview init without a profile flag specified.

I've also refactored the existing env deploy code to only initialize profile clients AFTER the user is prompted for a profile to deploy to, right at the beginning of Execute.

This seems to work; I was able to get ecs-preview init --profile personal to deploy test to my personal aws account.

Is there a way to avoid repeating myself in the initProfileClients method?

iamhopaul123

Overall LGTM except that we need to find a good way to avoid the repetition.

iamhopaul123 · 2020-02-17T17:37:46Z

internal/pkg/cli/init.go

 		profileConfig: cfg,
 		prog:          spin,
 		identity:      id,
+
+		initProfileClients: func(o *initEnvOpts) error {


Not quite sure but maybe we can define this function outside of the struct to make it a variable so that we can reuse that?

👍 yeah like @iamhopaul123 says, we can do:

var initEnvProfileClients = func(o *initEnvOpts) error { // your function }

and then while initializing the structs:

initProfileClients: initEnvProfileClients,

Done in the latest commit.

efekarakus

Looks good thank you!

efekarakus · 2020-02-17T17:47:22Z

internal/pkg/cli/env_init.go

+		initProfileClients: func(o *initEnvOpts) error {
+			profileSess, err := session.NewProvider().FromProfile(o.EnvProfile)
+			if err != nil {
+				return fmt.Errorf("Cannot create session from profile %s: %w", o.EnvProfile, err)


Suggested change

return fmt.Errorf("Cannot create session from profile %s: %w", o.EnvProfile, err)

return fmt.Errorf("create session from profile %s: %w", o.EnvProfile, err)

The recommended guideline for error strings from Go:

Error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context.

https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Another one that we follow is avoiding words like "failed" or "cannot" in error strings as usually they're redundant in the context of errors :)

https://github.com/uber-go/guide/blob/master/style.md#error-wrapping

Gotcha, thanks for the links!

efekarakus

🚀

iamhopaul123

feat: Add profile flag to init command

0717c4d

This commit adds a profile flag to `ecs-preview init` which is passed into the implicit call to `ecs-preview env init`. If the user does not specify a profile, ask them to select a profile.

bvtujo requested a review from a team as a code owner February 7, 2020 01:07

bvtujo requested review from clareliguori and efekarakus February 7, 2020 01:07

Remove TODO

91f2d62

iamhopaul123 reviewed Feb 11, 2020

View reviewed changes

iamhopaul123 requested a review from a team February 11, 2020 18:05

efekarakus reviewed Feb 11, 2020

View reviewed changes

Refactor env init to get profile clients only after asking for profile

d9c5804

efekarakus changed the title ~~feat: Add profile flag to root init command~~ feat: add profile flag to root init command Feb 17, 2020

iamhopaul123 approved these changes Feb 17, 2020

View reviewed changes

efekarakus approved these changes Feb 17, 2020

View reviewed changes

bvtujo added 3 commits February 17, 2020 11:22

Move initEnvProfileClients function to its own variable

bbbc58c

Merge branch master into bvtujo/global-init-profile-flag

16fe1b1

Reword error message in envInitProfileClients

d1a1557

bvtujo requested review from efekarakus and iamhopaul123 February 17, 2020 19:32

efekarakus approved these changes Feb 17, 2020

View reviewed changes

iamhopaul123 approved these changes Feb 17, 2020

View reviewed changes

bvtujo merged commit b369d1e into aws:master Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add profile flag to root init command #642

feat: add profile flag to root init command #642

bvtujo commented Feb 7, 2020

bvtujo commented Feb 7, 2020

iamhopaul123 Feb 11, 2020 •

edited

Loading

iamhopaul123 Feb 11, 2020

efekarakus Feb 11, 2020

iamhopaul123 Feb 11, 2020

bvtujo Feb 11, 2020

bvtujo Feb 11, 2020

efekarakus Feb 11, 2020

efekarakus Feb 11, 2020

efekarakus Feb 11, 2020

bvtujo commented Feb 15, 2020

iamhopaul123 left a comment

iamhopaul123 Feb 17, 2020

efekarakus Feb 17, 2020

bvtujo Feb 17, 2020

efekarakus left a comment

efekarakus Feb 17, 2020

bvtujo Feb 17, 2020

efekarakus left a comment

iamhopaul123 left a comment

	return fmt.Errorf("Cannot create session from profile %s: %w", o.EnvProfile, err)
	return fmt.Errorf("create session from profile %s: %w", o.EnvProfile, err)

feat: add profile flag to root init command #642

feat: add profile flag to root init command #642

Conversation

bvtujo commented Feb 7, 2020

bvtujo commented Feb 7, 2020

iamhopaul123 Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvtujo commented Feb 15, 2020

iamhopaul123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efekarakus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efekarakus left a comment

Choose a reason for hiding this comment

iamhopaul123 left a comment

Choose a reason for hiding this comment

iamhopaul123 Feb 11, 2020 •

edited

Loading