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

Conversation

Projects
5 participants
@dschep
Copy link
Member

dschep commented Jan 23, 2019

What did you implement:

closes #4311
closes #4725

How did you implement it:

  • added profile to list of variables that must resolve to a constant before SSM (and cfn & s3) variables are attempted to be resolved.
  • updated AwsProvider to check the aws-profile option instead of profile since that's what's actually implemented(afaik).
  • updated getProfileSourceValue return value to be more like that of the getStageSourceValue and getRegionSourceValue

How can we verify it:

set profile to a variable and try to sls print with stable install and then again with this branch

Todos:

  • Write tests
  • n/a Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens added the pr/in-review label Jan 24, 2019

@dschep dschep force-pushed the fix-sls-4311 branch 2 times, most recently from 1374dd0 to 318296e Jan 24, 2019

@@ -102,6 +102,10 @@ class Variables {
const requiredConfigs = [
_.assign({ name: 'region' }, provider.getRegionSourceValue()),
_.assign({ name: 'stage' }, provider.getStageSourceValue()),
_.assign({ name: 'profile' }, {
value: this.service.provider.profile,
path: 'serverless.service.provider.profile'

This comment has been minimized.

@j0k3r

j0k3r Jan 25, 2019

Contributor

And test will be green!

Suggested change Beta
path: 'serverless.service.provider.profile'
path: 'serverless.service.provider.profile',

@dschep dschep force-pushed the fix-sls-4311 branch from 318296e to e1f9b83 Jan 25, 2019

@pmuens
Copy link
Member

pmuens left a comment

Looks good from a code perspective 👍

I read through the comments re: breaking changes, but it looks like this is just an addition, so we should be fine here.

I'm not sure how important this PR is. We could merge it right away but I'd love to have a unit test covering this (won't "Request changes") to not block here...

(Or is this already covered by a test 🤔...)

@erikerikson

This comment has been minimized.

Copy link
Member

erikerikson commented Jan 25, 2019

Agreed, the switch to be additive rather than replacing avoidsthis being a breaking change except in extreme circumstances that I think we accept.

Agreed there's a need for tests. Something like a variable resolving to a valid value then being used by an SSM, S3, whatever variable which will fail but we can catch that expected failure and ensure the profile resolved properly.

@erikerikson

This comment has been minimized.

Copy link
Member

erikerikson commented Jan 25, 2019

Otherwise, this is a really nice change. Lifting the limitation on profile is super useful and increases the consistency for users. Great work @dschep!

@dschep dschep dismissed stale reviews from pmuens and j0k3r via 92f4f67 Jan 25, 2019

@dschep

This comment has been minimized.

Copy link
Member Author

dschep commented Jan 25, 2019

Note, the test coverage switches from setting the option to setting the config. AFAIK variables in options aren't actually a supported (or documented) feature of sls (and was explicitly not implemented for this profile change)

@pmuens

pmuens approved these changes Jan 28, 2019

Copy link
Member

pmuens left a comment

Looks good! Thanks for fixing the tests @dschep 👍

LGTM :shipit:

@dschep dschep merged commit 03d8ceb into master Jan 28, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@dschep dschep deleted the fix-sls-4311 branch Jan 28, 2019

@dschep dschep added this to the 1.36.4 milestone Feb 5, 2019

@dschep dschep added this to In progress in Serverless via automation Feb 5, 2019

@shortjared shortjared added the bug label Feb 6, 2019

@pmuens pmuens moved this from In progress to Done in Serverless Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment