Skip to content

Conversation

@medikoo
Copy link
Contributor

@medikoo medikoo commented Feb 23, 2021

Addresses 2.2.0 from #8364 and major part of #7207

  • New implementation of Variables parser and resolver, backed with 100% tests coverage
  • New implementation of variable sources: self, opt, env, strToBool and file
  • Configuration of first variable resolution phase, which attempts to resolve from configured sources before we resolve eventual additional environment variables from .env files
  • Rename old variables fixture to variables-legacy, and introduce new variables fixture

I mark it more as enhancement than refactor, as it's not a 1:1 refactor, but vastly improved resolution logic which changed how some edge cases are handled (e.g. we will no longer support self: variable, and there's built-in reliable prevention of circular references). Additionally it fixes many reported bugs, which were more design bugs of so currently implemented resolver

Fixes: #7921, #7807, #6214, #6022, #6014, #5828, #5759, #5278, #5136, #4837, #4817

Additionally @serverless/test was updated to rely on new resolver: https://github.com/serverless/test/pull/82

@medikoo medikoo self-assigned this Feb 23, 2021
@medikoo medikoo force-pushed the 0122-new-variables-engine branch 2 times, most recently from cb7c95f to 5b88695 Compare February 23, 2021 12:48
@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #8987 (ee259bd) into master (839c2cc) will increase coverage by 0.64%.
The diff coverage is 98.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8987      +/-   ##
==========================================
+ Coverage   86.31%   86.96%   +0.64%     
==========================================
  Files         274      283       +9     
  Lines       10247    10839     +592     
==========================================
+ Hits         8845     9426     +581     
- Misses       1402     1413      +11     
Impacted Files Coverage Δ
lib/configSchema.js 100.00% <ø> (ø)
scripts/serverless.js 74.64% <54.16%> (-10.46%) ⬇️
...iguration/variables/humanize-property-path-keys.js 100.00% <100.00%> (ø)
lib/configuration/variables/parse.js 100.00% <100.00%> (ø)
lib/configuration/variables/resolve-meta.js 100.00% <100.00%> (ø)
lib/configuration/variables/resolve.js 100.00% <100.00%> (ø)
lib/configuration/variables/sources/env.js 100.00% <100.00%> (ø)
lib/configuration/variables/sources/file.js 100.00% <100.00%> (ø)
lib/configuration/variables/sources/opt.js 100.00% <100.00%> (ø)
lib/configuration/variables/sources/self.js 100.00% <100.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 839c2cc...44d588f. Read the comment docs.

@medikoo medikoo requested a review from pgrzesik February 23, 2021 12:55
@medikoo medikoo force-pushed the 0122-new-variables-engine branch 3 times, most recently from ad3352d to dfef0c6 Compare February 23, 2021 15:22
Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What can I say - amazing job @medikoo 🎉 I've tried my best while reviewing, I've added a few questions and comments, but nothing major I believe

@medikoo medikoo force-pushed the 0122-new-variables-engine branch from dfef0c6 to 44d588f Compare February 24, 2021 10:30
@medikoo medikoo requested a review from pgrzesik February 24, 2021 10:59
Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome 🥳

@saumilsdk
Copy link

Great work :)

@medikoo
Copy link
Contributor Author

medikoo commented Mar 9, 2021

Thank you @saumilsdk

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid variable references as ${source:prop:invalid} are not communicated with error

4 participants