Variables: Handle all sources with a new resolver #9200
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9200 +/- ##
==========================================
- Coverage 87.24% 86.96% -0.29%
==========================================
Files 310 315 +5
Lines 11485 11651 +166
==========================================
+ Hits 10020 10132 +112
- Misses 1465 1519 +54
Continue to review full report at Codecov.
|
9b965ed to
dd7db91
Compare
dd7db91 to
70a2f9d
Compare
pgrzesik
left a comment
There was a problem hiding this comment.
Looks great overall 👏 I only have a few minor comments/questions, please let me know what do you think
| The above plugin will add support for variables like `${echo:foobar}` and resolve to the key. EG: | ||
| `${echo:foobar}` will resolve to `'foobar'`. | ||
| this.configurationVariablesSources = { |
There was a problem hiding this comment.
Just to be extra sure - the old way of defining variables won't work and plugins will have to migrate to new approach, correct?
There was a problem hiding this comment.
Yes, but that'll happen with a new major (no support is dropped with this PR)
it's dictated simply by fact that old variables resolver will be removed with next major, and same way old extensions will stop working.
However I don't think this feature is that used. It was created to aid dashboard plugin, and I'm not sure if any other plugins really benefited from that (and if they did migration will be really easy, you may check, how it looks in enterprise plugin PR)
There was a problem hiding this comment.
I just saw that in the related plugin PR - thanks for clarification - that's what I thought but wanted to make sure I'm not missing something here 👍
docs/providers/aws/guide/plugins.md
Outdated
| // Note: they're passed if they're configured into variable | ||
|
|
||
| // `options` is CLI options | ||
| // `resolveConfigurationProperty` allows to access other confuguration properties, |
There was a problem hiding this comment.
small typo: confuguration -> configuration
| Error: ServerlessError, | ||
| errorMessage: 'Non-string address argument in variable "cf" source: %v', | ||
| }); | ||
| const separatorIndex = address.indexOf('.'); |
There was a problem hiding this comment.
What do you think of splitting the address on . character and then verifying if it has length of 2 and destructuring the resulting array to stack name and logical id? In the current situation someone can provide by mistake an address like stackName.output.something which will be invalid but won't be caught here.
There was a problem hiding this comment.
I thnk it's fine to assume then that output.something is target output name. Is such naming not supported by AWS?
There was a problem hiding this comment.
The output names have to be alphanumeric only and don't allow . in name, but I guess it's an edge case here
There was a problem hiding this comment.
Yes, I think we may leave this (at least at this point) to AWS validation. User should anyway be presented with meaningful message.
70a2f9d to
e88c49c
Compare
Closes: #8364
Accompanied by https://github.com/serverless/enterprise-plugin/pull/558 (ideally if it's merged right after this one, and released before we release new Framework version)
Configures all remaining variables sources in new variable resolver