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.

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

Prevent configureAWS from being called multiple times for create-stack #136

Merged
merged 1 commit into from
Sep 18, 2018

Conversation

jpb
Copy link
Contributor

@jpb jpb commented Sep 18, 2018

Continuing my notes from #134 (comment), it appears that loadStackArgs already used argv.region:

iidy/src/cfn/index.ts

Lines 950 to 953 in 7404bc2

const cliOptionOverrides = _.pickBy(argv, (v: any, k: string) => !_.isEmpty(v) && _.includes(['region', 'profile', 'assumeRoleArn'], k));
const argsfileSettings = {profile: argsdata.Profile, assumeRoleArn: argsdata.AssumeRoleARN, region: argsdata.Region};
const mergedAWSSettings = _.merge(argsfileSettings, cliOptionOverrides);
await configureAWS(mergedAWSSettings); // cliOptionOverrides trump argsfile

This pull requests removes the extra, unnecessary call to configureAWS.

Fixes #134

Copy link
Contributor

@cspicer cspicer left a comment

Choose a reason for hiding this comment

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

I've reviewed the PR and the issue notes investigating the bug – the changes look good to me.

@@ -950,6 +949,11 @@ export async function _loadStackArgs(argsfile: string, argv: GenericCLIArguments
const cliOptionOverrides = _.pickBy(argv, (v: any, k: string) => !_.isEmpty(v) && _.includes(['region', 'profile', 'assumeRoleArn'], k));
const argsfileSettings = {profile: argsdata.Profile, assumeRoleArn: argsdata.AssumeRoleARN, region: argsdata.Region};
const mergedAWSSettings = _.merge(argsfileSettings, cliOptionOverrides);

logger.debug(`loadStackArgs cliOptionOverrides`, cliOptionOverrides);

Choose a reason for hiding this comment

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

Was it intended to leave these debug logs in?

@unbounceRabbit unbounceRabbit merged commit d24773c into master Sep 18, 2018
@tavisrudd tavisrudd deleted the create-stack-multiple-mfa-prompt branch April 26, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants