Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add profile flag to root init command #642

Merged
merged 6 commits into from
Feb 17, 2020
Merged

feat: add profile flag to root init command #642

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0717c4d
feat: Add profile flag to init command
bvtujo Feb 7, 2020
91f2d62
Remove TODO
bvtujo Feb 7, 2020
d9c5804
Refactor env init to get profile clients only after asking for profile
bvtujo Feb 14, 2020
bbbc58c
Move initEnvProfileClients function to its own variable
bvtujo Feb 17, 2020
16fe1b1
Merge branch master into bvtujo/global-init-profile-flag
bvtujo Feb 17, 2020
d1a1557
Reword error message in envInitProfileClients
bvtujo Feb 17, 2020
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
16 changes: 13 additions & 3 deletions internal/pkg/cli/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type initVars struct {
appType string
appName string
dockerfilePath string
profile string
}

type initOpts struct {
Expand Down Expand Up @@ -77,10 +78,15 @@ func newInitOpts(vars initVars) (*initOpts, error) {
if err != nil {
return nil, err
}
profileSess, err := sessProvider.FromProfile(vars.profile)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if err != nil {
return nil, err
}
prompt := prompt.New()
spin := termprogress.NewSpinner()
id := identity.New(sess)
deployer := cloudformation.New(sess)
envDeployer := cloudformation.New(profileSess)
cfg, err := profile.NewConfig()
if err != nil {
return nil, err
Expand Down Expand Up @@ -115,13 +121,13 @@ func newInitOpts(vars initVars) (*initOpts, error) {
initEnvVars: initEnvVars{
GlobalOpts: NewGlobalOpts(),
EnvName: defaultEnvironmentName,
EnvProfile: "default",
EnvProfile: vars.profile,
IsProduction: false,
},
envCreator: ssm,
projectGetter: ssm,
envDeployer: deployer,
projDeployer: deployer, // TODO #317
envDeployer: envDeployer,
projDeployer: deployer,
profileConfig: cfg,
prog: spin,
identity: id,
Expand Down Expand Up @@ -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 {
Copy link
Contributor

@iamhopaul123 iamhopaul123 Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

return err
}
return o.initEnv.Execute()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

cmd.Flags().StringVarP(&vars.projectName, projectFlag, projectFlagShort, "", projectFlagDescription)
cmd.Flags().StringVarP(&vars.appName, appFlag, appFlagShort, "", appFlagDescription)
cmd.Flags().StringVarP(&vars.appType, appTypeFlag, appTypeFlagShort, "", appTypeFlagDescription)
Expand Down
2 changes: 2 additions & 0 deletions internal/pkg/cli/init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ func TestInitOpts_Run(t *testing.T) {
opts.prompt.(*climocks.Mockprompter).EXPECT().Confirm(initShouldDeployPrompt, initShouldDeployHelpPrompt).
Return(true, nil)
opts.initEnv.(*climocks.MockactionCommand).EXPECT().Execute().Return(nil)
opts.initEnv.(*climocks.MockactionCommand).EXPECT().Ask().Return(nil)
opts.appDeploy.(*climocks.MockactionCommand).EXPECT().Ask().Return(nil)
opts.appDeploy.(*climocks.MockactionCommand).EXPECT().Execute().Return(nil)
},
Expand All @@ -119,6 +120,7 @@ func TestInitOpts_Run(t *testing.T) {
opts.prompt.(*climocks.Mockprompter).EXPECT().Confirm(initShouldDeployPrompt, initShouldDeployHelpPrompt).
Return(true, nil)
opts.initEnv.(*climocks.MockactionCommand).EXPECT().Execute().Return(nil)
opts.initEnv.(*climocks.MockactionCommand).EXPECT().Ask().Return(nil)
opts.appDeploy.(*climocks.MockactionCommand).EXPECT().Ask().Return(nil)
opts.appDeploy.(*climocks.MockactionCommand).EXPECT().Execute().Return(nil)
},
Expand Down