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

Require provider.credentials vars to be resolved before s3/ssm/cf vars #5763

Merged
merged 2 commits into from Jan 30, 2019

Conversation

Projects
3 participants
@dschep
Copy link
Member

dschep commented Jan 29, 2019

What did you implement:

Require provider.credentials vars to be resolved before s3/ssm/cf vars, similar to long standing region/stage and new profile preresolve.

test uses the service definition instead of getCredentials because
getCredentials is much more complex than getStage/getProfile/getRegion
and we only need to test that the config is updated, not all the
credential setting logic

How did you implement it:

added credentials in the prepopulate step of variable system

How can we verify it:

Create an SSM var called sls-5763 containing the value sls-5763
Create a service with the following config, replacing the key id & secret key of course:

service: sls-5763
custom:
  creds:
    sls-5763:
      accessKeyId: YOURACCESSKEYID
      secretAccessKey: YOURSECRETACCESSKEY
provider:
  credentials: ${self:custom.creds.${ssm:sls-5763}}
  name: aws
  runtime: nodejs8.10
functions:
  hello:
    handler: handler.hello

then when you run sls print with the stable version of serverless you'll get:

$ sls print
 Serverless Information ----------------------------------

  ##########################################################################################
  # 2501: 2 of 3 promises have settled
  # 2501: 1 unsettled promises:
  # 2501:   ssm:sls-4638 waited on by: ${self:custom.creds.${ssm:sls-4638}} ${ssm:sls-4638}
  # This can result from latent connections but may represent a cyclic variable dependency
  ##########################################################################################


 Serverless Information ----------------------------------

  ##########################################################################################
  # 5006: 2 of 3 promises have settled
  # 5006: 1 unsettled promises:
  # 5006:   ssm:sls-4638 waited on by: ${self:custom.creds.${ssm:sls-4638}} ${ssm:sls-4638}
  # This can result from latent connections but may represent a cyclic variable dependency
  ##########################################################################################


 Serverless Information ----------------------------------

  ##########################################################################################
  # 7507: 2 of 3 promises have settled
  # 7507: 1 unsettled promises:
  # 7507:   ssm:sls-4638 waited on by: ${self:custom.creds.${ssm:sls-4638}} ${ssm:sls-4638}
  # This can result from latent connections but may represent a cyclic variable dependency
  ##########################################################################################
^C

then install this branch with npm i -g serverless/serverless#credentials-preresolve and you should get:

$ sls print

  Error --------------------------------------------------

  Variable dependency failure: variable 'ssm:sls-4638' references service SSM but using that service requires a concrete value to be ca
lled.

     For debugging logs, run again after setting the "SLS_DEBUG=*" environment variable.

  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Issues:        forum.serverless.com

  Your Environment Information -----------------------------
     OS:                     darwin
     Node Version:           10.15.0
     Serverless Version:     1.36.3

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

Require provider.credentials vars to be resolved before s3/ssm/cf vars
Similar to long standing region/stage and new profile preresolve.

test uses the service definition instead of `getCredentials` because
`getCredentials` is much more complex than `getStage`/`getProfile`/`getRegion`
and we only need to test that the config is updated, not all the
credential setting logic

@dschep dschep changed the title Require provider.credentials vars to be resolved before s3/ssm/cf vars [WIP] Require provider.credentials vars to be resolved before s3/ssm/cf vars Jan 29, 2019

@dschep dschep requested review from eahefnawy , alexdebrie and pmuens Jan 29, 2019

@dschep dschep changed the title [WIP] Require provider.credentials vars to be resolved before s3/ssm/cf vars Require provider.credentials vars to be resolved before s3/ssm/cf vars Jan 29, 2019

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

@pmuens

pmuens approved these changes Jan 30, 2019

Copy link
Member

pmuens left a comment

Nice 👍 Added a comment about a function name that caught my attention.

Other than that I think it should be GTM :shipit:

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

This comment has been minimized.

@pmuens

pmuens Jan 30, 2019

Member

Does this requiredConfigs above mean that those are required to be present (e.g. users need to put in region, stage, porfile, etc.)? At least it reads like that. Pretty sure that's not the case. Just want to make sure that this is not a breaking change...

This comment has been minimized.

@dschep

dschep Jan 30, 2019

Author Member

yeah, the name is a little misleading. It works just fine when they're not set. They'r required to be prepopulated if they're set.

@dschep dschep merged commit 9cd8c41 into master Jan 30, 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 credentials-preresolve branch Jan 30, 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