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

AWS: Extend ${cf} syntax to get output from another region #5579

Merged
merged 6 commits into from Dec 10, 2018

Conversation

Projects
None yet
4 participants
@exoego
Copy link
Contributor

exoego commented Dec 8, 2018

What did you implement:

Closes #5154

How did you implement it:

Extend CloudFormation reference syntax to accept region in the form of ${cf.REGION:stackName.outputKey}, e.g. ${cf.us-east-1:my-service-dev.foobar}.
If region is omitted, default region is used, as same as current ${cf}.

How can we verify it:

  1. Install custom build of serverless from this PR
    npm i -g exoego/serverless#cf-ref-region

  2. Create a service to be referenced: sls create -t aws-nodejs --path my-service

  3. Update serverless.yml of my-service

service: my-service
provider:
  name: aws
  runtime: nodejs8.10
  region: ap-northeast-1
  memorySize: 512
functions:
  hello:
    name: ${self:custom.functionPrefix}hello
    handler: handler.hello
custom:
  functionPrefix: "issue5154-"
resources:
  Outputs:
    functionPrefix:
      Value: ${self:custom.functionPrefix}
      Export:
        Name: functionPrefix
    memorySize:
      Value: ${self:provider.memorySize}
      Export:
        Name: memorySize
  1. Deploy my-service: sls deploy
  2. Create another service: sls create -t aws-nodejs --path another
  3. Update serverless.yml of another
service: another
provider:
  name: aws
  region: us-east-1
  runtime: nodejs8.10
  memorySize: ${cf.ap-northeast-1:myService-dev.memorySize}
functions:
  hello:
    name: ${cf.ap-northeast-1:myService-dev.functionPrefix}hello-usa
    handler: handler.hello
  1. Deploy another service that reference my-service

Todos:

  • Write tests
  • 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

@exoego exoego changed the title [AWS] WIP: Extend ${cf} syntax to get output from another region [AWS] Extend ${cf} syntax to get output from another region Dec 8, 2018

@exoego exoego force-pushed the exoego:cf-ref-region branch from 6831ade to c3203aa Dec 8, 2018

@exoego exoego changed the title [AWS] Extend ${cf} syntax to get output from another region AWS: Extend ${cf} syntax to get output from another region Dec 8, 2018

@exoego exoego referenced this pull request Dec 9, 2018

Closed

${cf} for another region #5154

@horike37
Copy link
Member

horike37 left a comment

@exoego
Thank you very much for working on this! It's a really great addition!

I just tested and it works as expected! Awesome 👍 And just added one comment how to check undefined. LGTM other than that.

const variableStringWithoutSource = variableString.split(':')[1].split('.');
const stackName = variableStringWithoutSource[0];
const outputLogicalId = variableStringWithoutSource[1];
const options = { useCache: true };
if (regionSuffix !== undefined) {

This comment has been minimized.

@horike37

horike37 Dec 9, 2018

Member

Please use _.isUndefined of lodash for consistancy

This comment has been minimized.

@exoego

exoego Dec 10, 2018

Contributor

Updated in 13de2a9

@horike37 horike37 added this to the 1.35.0 milestone Dec 9, 2018

@exoego exoego force-pushed the exoego:cf-ref-region branch from 518b48b to 363dab4 Dec 10, 2018

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Dec 10, 2018

@exoego
Thank you for the updates 👍 LGTM 💯

@dschep
appveyor test failed... can you re-run?

@exoego exoego closed this Dec 10, 2018

@exoego exoego reopened this Dec 10, 2018

@exoego

This comment has been minimized.

Copy link
Contributor

exoego commented Dec 10, 2018

Asynchronous tests fail randomly 🤔
I have already reported #5533

@exoego exoego closed this Dec 10, 2018

@exoego exoego reopened this Dec 10, 2018

@dschep

dschep approved these changes Dec 10, 2018

Copy link
Member

dschep left a comment

LGTM! and ci is now happy @horike37

:shipit:

@dschep dschep merged commit a9e243f into serverless:master Dec 10, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 90.772%
Details
@dschep

This comment has been minimized.

Copy link
Member

dschep commented Dec 10, 2018

btw @horike37 we're gonna move windows builds to travis when I get a chance since it now supports it too

@horike37

This comment has been minimized.

Copy link
Member

horike37 commented Dec 10, 2018

we're gonna move windows builds to travis when I get a chance since it now supports it too

👍 👍 👍

@dschep dschep referenced this pull request Dec 12, 2018

Merged

v1.35.0 release! #5592

@smartinspereira

This comment has been minimized.

Copy link

smartinspereira commented Dec 16, 2018

@dschep @horike37 @exoego
maybe I am missing something here, but this feature doesn't seem to be working as intended.
should do: create stack bar in zone x and reference existing stack foo in zone y
is doing: create stack bar in zone y and reference existing stack foo in zone y

this to stacks foo and bar will cause the error. x=us-east-1; y=eu-central-1;

service: foo

provider:
  name: aws
  region: eu-central-1
  stage: dev
  profile: cndx-dev
  runtime: nodejs8.10
  memory: 1024

functions:
  hello:
    handler: handler.hello

resources:
  Outputs:
    foo:
      Value: foo
      Export:
        Name: foo
service: bar

provider:
  name: aws
  region: us-east-1
  stage: dev
  profile: cndx-dev
  runtime: nodejs8.10
  memory: 1024

functions:
  hello:
    handler: handler.hello

resources:
  Outputs:
    foobar:
      Value: ${self:custom.foobar}
      Export:
        Name: foobar

custom:
  foobar: ${cf.eu-central-1:foo-dev.foo}bar
  # foobar: foobar

steps to reproduce:

version 1:

  1. sls deploy foo
  2. sls deploy bar (with the stack reference in this last two lines enabled)
    -> this will lead to the creation of a new stack bar-dev in the same region as foo-dev
    -> error: no direct error but stack bar-dev should be in us-east-1

version 2:

  1. sls deploy foo
  2. sls deploy bar (without the stack reference in this last two lines enabled)
  3. sls deploy bar (with the stack reference in this last two lines enabled)
    -> this will lead to the creation of a new stack bar-dev in the same region as foo-dev
    -> error: An error occurred: IamRoleLambdaExecution - bar-dev-us-east-1-lambdaRole already exists.

@exoego exoego deleted the exoego:cf-ref-region branch Dec 16, 2018

@smartinspereira

This comment has been minimized.

Copy link

smartinspereira commented Dec 17, 2018

will create an issue

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