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

Invoke local override env #5313

Merged
merged 4 commits into from Sep 26, 2018

Conversation

@robin-norwood
Copy link
Contributor

commented Sep 19, 2018

What did you implement:

Closes #5096

  • sls invoke local should make it possible to override environment variables

How did you implement it:

  • Added --env option to sls invoke local to override environment vars in lib/plugins/invoke/invoke.js
    • Usage: --env NAME1=val1 --env NAME2=val2 ...

How can we verify it:

Run:
npm run test lib/plugins/invoke/
sls invoke local -f helloWorld ---env NAME1=val1 --env NAME2=val2

Todos:

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

Copy link
Member

left a comment

@robin-norwood
Thank you for implementing this feature! I think this is a great stuff with local development 💯
Just reviewed and left one comment. Additionally, could you add the explanation of this feature to invoke-local documentations.

https://github.com/serverless/serverless/blob/master/docs/providers/aws/cli-reference/invoke-local.md
https://github.com/serverless/serverless/blob/master/docs/providers/google/cli-reference/invoke-local.md
https://github.com/serverless/serverless/blob/master/docs/providers/openwhisk/cli-reference/invoke-local.md

@@ -114,6 +119,13 @@ class Invoke {

_.merge(process.env, defaultEnvVars);

// Turn zero or more --env options into an array
// ...then split --env NAME=value and put into process.env.
Array.concat(this.options.env || []).forEach(itm => {

This comment has been minimized.

Copy link
@horike37

horike37 Sep 20, 2018

Member

Is it possible to use lodash library? We make a point of using it for consistency of code base as possible as you can.

@horike37 horike37 added this to the 1.33.0 milestone Sep 20, 2018
@robin-norwood

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

I've added the docs - is this what you meant by using lodash? There may be a better way, but I'm afraid I'm not that familiar with lodash.

@softprops

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

Very cool feature!

@robin-norwood

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@horike37 I think the coverage message is a rounding error in this case. Please do let me know if there are any other changes to make.

@horike37

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

@robin-norwood
Thank you for the updates 👍
Looks perfect! merging...

@horike37 horike37 merged commit 46aa939 into serverless:master Sep 26, 2018
2 of 3 checks passed
2 of 3 checks passed
coverage/coveralls Coverage decreased (-0.001%) to 91.048%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.