-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Follow up fixes for #4713 #4754
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
Follow up fixes for #4713 #4754
Conversation
…ank you!) Simple oversight that deep variables can reference overwrites too. This fix acknowledges this oversight by using the standard logic for using overwrite instead of getValueFromSource in this case.
|
@laardee Can you give this PR a try and see if the problem is solved? |
76b7d58 to
ec61500
Compare
…(DV) cases
Solve for these cases. The bug was that DVs can render to DVs when a value is being obtained from them (i.e. they are being de-referenced in getValueFromDeep). This is particularly problematic in the case that the original DV being rendered has a deep reference into the rendered DV. In that case the DV returned by rendering must be replaced by yet another DV.
While accomplishing the above, various bits of cleanup were implemented and I added some commentary around getDeeperValue for future engineers.
This case is particularly relevant if you have:
`serverless.yml`:
```
service: serverless-hello-world
provider:
name: aws
runtime: nodejs6.10
stage: dev
environment:
SECRET: ${self:custom.secrets.SECRET}
functions:
helloWorld:
handler: handler.helloWorld
events:
- http:
path: hello-world
method: get
cors: true
custom:
stage: ${opt:stage, self:provider.stage}
secrets: ${file(secrets.${self:custom.stage}.yml)}
```
AND
`secrets.dev.yml`:
```
SECRETS: secrets
```
Populating this service should result in the following sequence of rendering phases:
#########################
# PHASE 1
#
# ${self:custom.secrets.SECRET}
# <- ${deep:0.SECRET}
# deep[0] = ${file(secrets.${self:custom.stage}.yml)}
#
# ${opt:stage, self:provider.stage}
# <- 'dev'
#
# ${file(secrets.${self:custom.stage}.yml)}
# <- ${file(secrets.${deep:1}.yml)}
# deep[1] = ${opt:stage, self:provider.stage}
#
# RESULT
#
# service: serverless-hello-world
#
# provider:
# name: aws
# runtime: nodejs6.10
# stage: dev
# environment:
# SECRET: ${deep:0.SECRET}
#
# functions:
# helloWorld:
# handler: handler.helloWorld
# events:
# - http:
# path: hello-world
# method: get
# cors: true
#
# custom:
# stage: dev
# secrets: ${file(secrets.${deep:1}.yml)}
#########################
#########################
# PHASE 2
#
# ${deep:0.SECRET}
# <- this.populateValue('${file(secrets.${self:custom.stage}.yml)}') => '${file(secrets.${deep:1}.yml)}' => '${deep:2}' => '${deep:2.SECRET}'
# deep[2] = ${file(secrets.${deep:1}.yml)}
#
# ${file(secrets.${deep:1}.yml)}
# <- this.populateValue('${file(secrets.${deep:1}.yml)}') => '${file(secrets.dev.yml)}' => '${deep:3}'
# deep[3] = ${file(secrets.dev.yml)}
#
# RESULT
#
# service: serverless-hello-world
#
# provider:
# name: aws
# runtime: nodejs6.10
# stage: dev
# environment:
# SECRET: ${deep:2.SECRET}
#
# functions:
# helloWorld:
# handler: handler.helloWorld
# events:
# - http:
# path: hello-world
# method: get
# cors: true
#
# custom:
# stage: dev
# secrets: ${deep:3}
#########################
#########################
# PHASE 3
#
# ${deep:2.SECRET}
# <- this.populateValue('${file(secrets.${deep:1}.yml)}') => '${file(secrets.dev.yml)}' => '${deep:3}' => '${deep:3.SECRET}'
# deep[3] is already set to ${file(secrets.dev.yml)}
#
# ${deep:3}
# <- this.populateValue('${file(secrets.dev.yml)}') => { SECRET: 'secret' }
#
# RESULT
#
# service: serverless-hello-world
#
# provider:
# name: aws
# runtime: nodejs6.10
# stage: dev
# environment:
# SECRET: ${deep:3.SECRET}
#
# functions:
# helloWorld:
# handler: handler.helloWorld
# events:
# - http:
# path: hello-world
# method: get
# cors: true
#
# custom:
# stage: dev
# secrets:
# SECRET: secret
#########################
#########################
# PHASE 4
#
# ${deep:3}
# <- this.populateValue('${file(secrets.dev.yml)}') => this.getDeeperValue(['SECRET'], { SECRET: 'secret' }) => 'secret'
#
# RESULT
#
# service: serverless-hello-world
#
# provider:
# name: aws
# runtime: nodejs6.10
# stage: dev
# environment:
# SECRET: secret
#
# functions:
# helloWorld:
# handler: handler.helloWorld
# events:
# - http:
# path: hello-world
# method: get
# cors: true
#
# custom:
# stage: dev
# secrets:
# SECRET: secret
#########################
ec61500 to
8c7db12
Compare
|
Yes, now it packages our project at least as good as before. This, or the previous PR, fixes also the issue that we faced with @medikoo where the region variable was not resolved early enough for SSM variables. |
|
I have detected a returned cyclic error here. So many configs to check... |
|
This cyclic issue is restricted to the print command. Continuing investigation. |
Fix `print` The print command is highly linked to the `Variables` and `Service` codebases, keep those in sync and leave reminders about the link. Made these explicit and separately implemented to avoid complexity. Additionally, the print command re-populates an object with the *very similar* content as the previously pre-populated service (just not augmented as just mentioned). This can lead to cross contamination between the two. As such, all caches must be cleared per unique invocation of service/object/property population. Add tests for some expected but previously unverified behaviors. Clean pre-population The previous implementation worked okay but was unnecessary and would have been a maintenance problem. Instead, just knock out the population of variables depending on those config dependent services and use the standard means of resolution. Fix cyclic bug (resulting from running print against a self-referencing serverless.yml) The caching of values could lead to a cyclic object remaining in the caches for variable population. This causes crashes and pain. Solved by the cache cleaning logic.
The framework's Service class modifies a user's service, mutating a given `service: { name: 'string' }` to `service: 'string'` but doesn't account for this in the variables system. This is the basis for serverless#4744.
While working this area, I discovered that the Service class also does this to the provider, accepting `provider: 'aws'` and replacing it with `provider: { name: 'aws' }`, causing the natural `${self:provider}` to fail.
Catching either 'self:service.name' or 'self:provider' and replacing them with the mutated reference solves this user confusion.
|
This should be ready for merge. |
|
@erikerikson Wow. Thank you. Can you add all issues that are closed by this PR (there seem to be a whole lot of them 😄 ) as |
HyperBrain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice stuff in here 🎉 . I have added only some minor comments.
| const logWarning = require('./Error').logWarning; | ||
| const PromiseTracker = require('./PromiseTracker'); | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
| let variable = variableString; | ||
| // The loaded service is altered during load in ~/lib/classes/Service (see loadServiceFileParam) | ||
| // Account for these so that user's reference to their file populate properly | ||
| if (variable === 'self:service.name') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add a comment to the Service class at the proper location to warn people, that any changes done there that affect the service property, have to be checked with this implementation too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good point. Missed commenting this. Adding additional comment here too.
| if (_.isString(this.cache.serviceProvider)) { | ||
| service.provider = { name: this.cache.serviceProvider }; | ||
| } | ||
| service.provider = _.merge({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be only done if the provider is set to aws, shouldn't it? The print command is located at top level (outside of the aws plugin) and might be called for arbitrary provider types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected so too. However, the code in ~/lib/classes/Service.js (
serverless/lib/classes/Service.js
Line 19 in bf5a8c9
| this.provider = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed 🤔 . So it seems to be just an initial value there (that will be overridden by the incoming service configuration). In theory, a service.yml without provider definition would work then, which is a bit strange, but so be it.
However, here in the print command I'm not really sure if that, what we do here, is really just a preliminary intialization that will be overwritten or will influence subsequent behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was weird to see at first but I faulted on the side of parity to avoid bugs resulting from dissonance. Thank you very much for paying attention here!
|
Thanks for the kick to actually read docs on how the title "closes #..."/keywords feature :D I have added comments as discussed. If you agree with my analysis of the print.js, line 41 comment, this should be ready to merge but otherwise, I'll fix it. |
|
@erikerikson I meant in the bug description, not subject 😆 |
HyperBrain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, good to go 👍
|
Merged. Hopefully some people will test with master and verify that the covered cases now work as expected. |
|
Some people here 😀 I quickly tested with the master and now I get some errors with SSM parameters and provider/region. I have a provider:
name: aws
runtime: nodejs6.10
stage: dev
environment:
MY_SSM_PARAM: ${ssm:my-ssm-parameter}But it fails with error Adding the region to the provider doesn't help either and packaging with provider:
name: aws
runtime: nodejs6.10
stage: dev
region: eu-west-1
environment:
MY_SSM_PARAM: ${ssm:my-ssm-parameter}But when I add the region to custom and refer to it in provider, packaging works as expected. provider:
name: aws
runtime: nodejs6.10
stage: dev
region: ${self:custom.region}
environment:
MY_SSM_PARAM: ${ssm:my-ssm-parameter}
custom:
region: eu-west-1This last setup is about the same that we have. We have the region in a file, but somewhat similar. I used that setup to test the PR earlier so I didn't catch this. 😞 @HyperBrain @erikerikson can you quickly check if my findings are valid. |
|
Thanks for reporting/finding this @laardee, we're working from an unfortunate deficit of test cases. I'll look into it shortly. |
|
I've reproduced this and discovered the error. I created a timing issue based on the resolution order of the promises but on a positive note, it should be simple to test for and fix. Incoming... |
|
Just commited tests for the case you encountered and a fix. PR #4769 I also ran the new code against your provided config using the cmd line you gave. I got the expected error from SSM rather than this codebase. Apologies for the train of issues. |
|
That PR #4769 fixes all of the issues I encountered earlier with the SSM. |
|
Thanks for all your work in testing and verification @laardee! |
|
Ok. I'll merge the update/fix PR now. Then master can be given a second try and further round of testing 😄 |
|
All my use cases work now with the master branch 👏 |
|
What about |
|
What's the issue @dschep? Link? |
|
It doesn't get populated: Note that the above is with |
|
Thank you. I'll look into it. |
|
It turns out that I don't have GNU sed. To verify, your with right? It looks like we need to rewrite |
What did you implement:
Fixes:
serverless.ymlwith the previously populated and adorned version processed by the framework. OLD, painful, and previously unreported.${self:}leads to crash in the print command #4556, Closes All variables should resolve, timeout, or be identified #4687."${self:service.name}"and"${self:provider}"which users expect to be valid because they are present in theirserverless.ymlbut are mutated by theServiceclass. Closes A valid service attribute to satisfy the declaration 'self:service.name' could not be found #4744.How did you implement it:
getValueFromDeepmethod uses the standard logic for detecting and resolving overwrites rather than assuming that all deep variables are standard single variable strings.getValueFromDeep, detecting recursive resolution to a new variable containing string.How can we verify it:
npm run testTodos:
Is this ready for review?: YES
Is it a breaking change?: NO