Skip to content

Allow to run functions with env variables correctly populated #27

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.

Sign up for GitHub

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

Open
marcosnils wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow to run functions with env variables correctly populated #27

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,6 @@ module.exports = function(S) {

return functions.forEach(function(fun) {
if( -1 !== fun.getRuntime().getName().indexOf('nodejs') ) {
// Override s-function.json defined environment!
Object.getPrototypeOf( fun.getRuntime() ).getEnvVars = ()=> BbPromise.resolve( {} );
_this.handlers[ fun.name ] = fun;

@tszajna0 tszajna0 Apr 21, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It may be made optional, but not removed, because it's really useful to pass local config (different form AWS) via env and not have it overriden by SLS Function.run, e.g. https://github.com/serverless/serverless/blob/master/lib/RuntimeNode.js#L48

@marcosnils marcosnils Apr 22, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@tszajna0 I see what you mean but shouldn't the serve plugin work the same was as serverless core does?. In serverless core you can't override the variables defined in your s-function.json file. You need to properly configure your stage variables to make them work.

@tszajna0 tszajna0 Apr 22, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@marcosnils let me put it opposite way: I wish I could easily specify my own env when locally running functions in SLS, unfortunately I have to properly configure stage variables in a different way than I'd deploy to AWS.

IMO it's useful to mimic sls behavior as close as possible and it's also useful to be able to alter it slightly. Then having a switch between these two would make everyone happy, I suppose. If you agree, then the question is shall I do it or would you?

@marcosnils marcosnils Apr 22, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@tszajna0 I'll do it. No worries :)

fun.endpoints.forEach(function(endpoint){
Expand Down Expand Up @@ -174,7 +172,8 @@ module.exports = function(S) {
return response || responses['default'];
}

fun.run( _this.evt.stage, _this.evt.region, event ).then( (res)=> {
let runtime = fun.getRuntime();
runtime.run(fun, _this.evt.stage, _this.evt.region, event).then( (res)=> {

@tszajna0 tszajna0 Apr 21, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does it change anything?
https://github.com/serverless/serverless/blob/master/lib/Function.js#L237-L239

@marcosnils marcosnils Apr 22, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right, I can rollback this.

if( res.status == 'success' ) {
resolve(Object.assign({
result: res.response
Expand Down