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

Resolve profile before performing aws-sdk dependent actions #5744

Merged
merged 2 commits into from Jan 28, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+11 −5
Diff settings

Always

Just for now

Copy path View file
@@ -102,6 +102,10 @@ class Variables {
const requiredConfigs = [ const requiredConfigs = [
_.assign({ name: 'region' }, provider.getRegionSourceValue()), _.assign({ name: 'region' }, provider.getRegionSourceValue()),
_.assign({ name: 'stage' }, provider.getStageSourceValue()), _.assign({ name: 'stage' }, provider.getStageSourceValue()),
_.assign({ name: 'profile' }, {
value: this.service.provider.profile,
path: 'serverless.service.provider.profile',
}),
]; ];
return this.disableDepedentServices(() => { return this.disableDepedentServices(() => {
const prepopulations = requiredConfigs.map(config => const prepopulations = requiredConfigs.map(config =>
Copy path View file
@@ -147,11 +147,12 @@ describe('Variables', () => {
const prepopulatedProperties = [ const prepopulatedProperties = [
{ name: 'region', getter: (provider) => provider.getRegion() }, { name: 'region', getter: (provider) => provider.getRegion() },
{ name: 'stage', getter: (provider) => provider.getStage() }, { name: 'stage', getter: (provider) => provider.getStage() },
{ name: 'profile', getter: (provider) => provider.getProfile() },
]; ];
describe('basic population tests', () => { describe('basic population tests', () => {
prepopulatedProperties.forEach((property) => { prepopulatedProperties.forEach((property) => {
it(`should populate variables in ${property.name} values`, () => { it(`should populate variables in ${property.name} values`, () => {
awsProvider.options[property.name] = '${self:foobar, "default"}'; awsProvider.serverless.service.provider[property.name] = '${self:foobar, "default"}';
return serverless.variables.populateService().should.be.fulfilled return serverless.variables.populateService().should.be.fulfilled
.then(() => expect(property.getter(awsProvider)).to.be.eql('default')); .then(() => expect(property.getter(awsProvider)).to.be.eql('default'));
}); });
@@ -167,15 +168,15 @@ describe('Variables', () => {
prepopulatedProperties.forEach(property => { prepopulatedProperties.forEach(property => {
dependentConfigs.forEach(config => { dependentConfigs.forEach(config => {
it(`should reject ${config.name} variables in ${property.name} values`, () => { it(`should reject ${config.name} variables in ${property.name} values`, () => {
awsProvider.options[property.name] = config.value; awsProvider.serverless.service.provider[property.name] = config.value;
return serverless.variables.populateService() return serverless.variables.populateService()
.should.be.rejectedWith('Variable dependency failure'); .should.be.rejectedWith('Variable dependency failure');
}); });
it(`should reject recursively dependent ${config.name} service dependencies`, () => { it(`should reject recursively dependent ${config.name} service dependencies`, () => {
serverless.variables.service.custom = { serverless.variables.service.custom = {
settings: config.value, settings: config.value,
}; };
awsProvider.options.region = '${self:custom.settings.region}'; awsProvider.serverless.service.provider.region = '${self:custom.settings.region}';
return serverless.variables.populateService() return serverless.variables.populateService()
.should.be.rejectedWith('Variable dependency failure'); .should.be.rejectedWith('Variable dependency failure');
}); });
@@ -196,8 +197,8 @@ describe('Variables', () => {
{ name: 'getValueFromS3', original: serverless.variables.getValueFromS3 }, { name: 'getValueFromS3', original: serverless.variables.getValueFromS3 },
{ name: 'getValueFromSsm', original: serverless.variables.getValueFromSsm }, { name: 'getValueFromSsm', original: serverless.variables.getValueFromSsm },
]; ];
awsProvider.options.region = combination.region; awsProvider.serverless.service.provider.region = combination.region;
awsProvider.options.state = combination.state; awsProvider.serverless.service.provider.state = combination.state;
return serverless.variables.populateService().should.be.fulfilled return serverless.variables.populateService().should.be.fulfilled
.then(() => { .then(() => {
dependentMethods.forEach((method) => { dependentMethods.forEach((method) => {
@@ -410,6 +410,7 @@ class AwsProvider {


getProfileSourceValue() { getProfileSourceValue() {
const values = this.getValues(this, [ const values = this.getValues(this, [
['options', 'aws-profile'],
This conversation was marked as resolved by dschep

This comment has been minimized.

@erikerikson

erikerikson Jan 23, 2019

Member

Is this option being renamed? This strikes me as a breaking change.

This comment has been minimized.

@erikerikson

erikerikson Jan 23, 2019

Member

The option remains profile according to https://serverless.com/framework/docs/providers/aws/cli-reference/config-credentials/ though I did find https://serverless.com/framework/docs/providers/aws/guide/credentials#using-the-aws-profile-option which suggests that aws-profile is correct. However, that's never been supported so I'd suggest this is a documentation bug.

This comment has been minimized.

@dschep

dschep Jan 24, 2019

Author Member

Yeah. I wasn't 100% sure about this. I was only familiar with the later option, which is global. I see that it's an option specifically on the config subcommand 😕

I've got an idea for how to resolve this. (the real issue is that options.profile shouldn't be checked while doing variable substitution because it shadows the standard way of setting variables from options)

This comment has been minimized.

@erikerikson

erikerikson Jan 24, 2019

Member

Can you please explain how that's the real issue? I'm a bit confused about the assertion and change.

That part of the code is only about prioritizing the resolution/variables of specific attributes to avoid dependency related issues in non-variable usage (e.g. use in the aws-sdk).

My expectation, taking the assumption that their is some improper handling of --aws-profile, would be something like you would add the --aws-profile option rather than contingently using the existing option.

This comment has been minimized.

@dschep

dschep Jan 24, 2019

Author Member

discussed in slack and completely changed anyway

['options', 'profile'], ['options', 'profile'],
['serverless', 'config', 'profile'], ['serverless', 'config', 'profile'],
['serverless', 'service', 'provider', 'profile'], ['serverless', 'service', 'provider', 'profile'],
ProTip! Use n and p to navigate between commits in a pull request.