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: Add fallback support in ${cf} and ${s3} #5758

Merged
merged 2 commits into from Jan 31, 2019

Conversation

Projects
4 participants
@exoego
Copy link
Contributor

exoego commented Jan 27, 2019

What did you implement:

Add fallback support in ${cf} and ${s3}, so every variable syntaxes support fallback.

Closes #4940.
Closes #5756.

How did you implement it:

Currently, ${cf} and ${s3} will throw error if AWS request fails.
This behavior is supposed to be expected since it can prevent deploying broken service.
However, since current implementation of fallback (Variable#overwrite) requires all of Promises is fulfilled, ${cf} and ${s3} does not support fallback.

After this PR, #overwrite rid rejected promises, so first fulfilled value is used as fallback.

Note: I think this change makes it easier to address #3367, #5057, #5369 and #5757.

How can we verify it:

  1. npm install -g exoego/serverless#default-values
  2. sls deploy below and confirm it complete successfully.
service: aws-nodejs

provider:
  name: aws
  runtime: nodejs8.10

functions:
  hello:
    handler: handler.hello

custom:
  envVar: ${env:hoge, 'fallback'}
  optVar: ${opt:hoge, 'fallback'}
  selfVar: ${self:custom.xxx, 'fallback'}
  fileVar: ${file(./not-found):nooo, 'fallback'}
  cfVar: ${cf:no-such-stuck-exists.UNDEFINED_KEY, 'fallback'} 
  s3Var: ${s3:no/such/key, 'fallback'}
  ssmVar: ${ssm:/path/to/service/myParam, 'fallback'}
  1. sls deploy below and confirm it aborts due to lack of fallback.
service: aws-nodejs

provider:
  name: aws
  runtime: nodejs8.10

functions:
  hello:
    handler: handler.hello

custom:
  envVar: ${env:hoge, 'fallback'}
  optVar: ${opt:hoge, 'fallback'}
  selfVar: ${self:custom.xxx, 'fallback'}
  fileVar: ${file(./not-found):nooo, 'fallback'}
  cfVar: ${cf:no-such-stuck-exists.UNDEFINED_KEY}  # no fallback !
  s3Var: ${s3:no/such/key} # no fallback !
  ssmVar: ${ssm:/path/to/service/myParam, 'fallback'}

Todos:

  • Write tests
  • Write documentation N/A
  • 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

@eahefnawy eahefnawy self-assigned this Jan 28, 2019

@eahefnawy eahefnawy self-requested a review Jan 28, 2019

@eahefnawy eahefnawy added this to In progress in Serverless via automation Jan 28, 2019

@eahefnawy

This comment has been minimized.

Copy link
Member

eahefnawy commented Jan 31, 2019

@exoego with this change, if you reference invalid cf or s3 source without fallback, would you still get an error? or it'd just be caught and ignored?

@exoego

This comment has been minimized.

Copy link
Contributor Author

exoego commented Jan 31, 2019

@exoego with this change, if you reference invalid cf or s3 source without fallback, would you still get an error? or it'd just be caught and ignored?

It raises error if no fallback, as same as current.
There are already test codes for ${s3} and ${cf}.

Added the cases in How can we verify it too.

Serverless automation moved this from In progress to Reviewer approved Jan 31, 2019

@eahefnawy
Copy link
Member

eahefnawy left a comment

Beautiful! Working on my end! Thanks @exoego 🙌

LG2M

@eahefnawy eahefnawy added this to the 1.36.4 milestone Jan 31, 2019

@eahefnawy eahefnawy merged commit 8c53de8 into serverless:master Jan 31, 2019

1 check passed

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

Serverless automation moved this from Reviewer approved to Done Jan 31, 2019

@exoego exoego deleted the exoego:default-values branch Jan 31, 2019

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