Skip to content
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

feat(CLI): Automatically expand loaded env variables #9615

Merged
merged 4 commits into from Jul 15, 2021

Conversation

d-asensio
Copy link
Contributor

@d-asensio d-asensio commented Jun 16, 2021

Closes: #9689

@pgrzesik
Copy link
Contributor

pgrzesik commented Jun 17, 2021

Hello @d-asensio - could you please include motivation behind this PR proposal? I believe there is not corresponding issue created for that, correct?

@d-asensio d-asensio changed the title [WIP] Expand env variables Expand env variables Jul 4, 2021
@d-asensio
Copy link
Contributor Author

Hey @pgrzesik sure! I found some time to write an issue with the motivations of this. I referenced it in my first comment.

Thank you!

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.

Thanks a lot @d-asensio - it looks great overall and thanks so much for writing up a motivation in a linked issue. I think it's a good idea to allow such expansions, but it would also be good to document it e.g. here: https://github.com/serverless/serverless/blob/master/docs/environment-variables.md

Btw - it looks like there's a conflicting file - could you please look into that and rebase your branch?

@d-asensio
Copy link
Contributor Author

d-asensio commented Jul 5, 2021

Hey, @pgrzesik I rebased it onto master and added a comment with a little example in the docs.

Thank you!

@d-asensio
Copy link
Contributor Author

Hi @pgrzesik, is there something missing here?

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.

Thanks a lot 🙇 Sorry for the delay in review - I missed the notification and then I was off for a few days. I have one minor suggestion related to docs - also, let's wait for opinion from @medikoo if we should introduce such extensions.

docs/environment-variables.md Show resolved Hide resolved
@pgrzesik pgrzesik requested a review from medikoo July 12, 2021 11:05
@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #9615 (1a44ea0) into master (7078449) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 1a44ea0 differs from pull request most recent head fd4b4ff. Consider uploading reports for the commit fd4b4ff to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9615   +/-   ##
=======================================
  Coverage   86.10%   86.10%           
=======================================
  Files         328      328           
  Lines       12590    12594    +4     
=======================================
+ Hits        10840    10844    +4     
  Misses       1750     1750           
Impacted Files Coverage Δ
lib/cli/load-dotenv.js 95.23% <100.00%> (+1.12%) ⬆️

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 7078449...fd4b4ff. Read the comment docs.

medikoo
medikoo previously approved these changes Jul 14, 2021
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@d-asensio @pgrzesik it feels as great addition to me. It mimicks how things will work in a shell, which is a nice improvement.

@pgrzesik After adjustment to docs is made, feel free to merge

@pgrzesik
Copy link
Contributor

Thanks @medikoo 🙇 @d-asensio so the only thing to address is one comment about attributing what library is used for expansions and we're good to go 👍

@d-asensio
Copy link
Contributor Author

Done! Thank you both @pgrzesik and @medikoo! 🥳 🎉

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.

Thank you @d-asensio, well done 👏

@pgrzesik pgrzesik changed the title Expand env variables feat(CLI): Automatically expand loaded env variables Jul 15, 2021
@pgrzesik pgrzesik merged commit 1864969 into serverless:master Jul 15, 2021
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.

Expand environment variables when using useDotenv: true in serverless configuration
3 participants