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: nodejs8 runtime #114

Merged
merged 4 commits into from Aug 13, 2018

Conversation

@PatrickHeneise
Copy link
Contributor

commented Jul 31, 2018

Fix issues #113 and #112

BREAKING CHANGE:

  • defaults to nodejs8 runtime
provider:
  name: google
  runtime: nodejs8
  project: myapp
  region: europe-west1
  memorySize: 128

Results in:

screenshot 2018-07-31 at 13 19 41

PatrickHeneise added some commits Jul 31, 2018

feat: add nodejs8 runtime\n\nBREAKING CHANGE: default to nodejs8, ove…
…rwrite with "runtime: nodejs6" in serverless.yml
@PatrickHeneise

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

@pmuens let me know what you think and what's needed to get this merged.

@PatrickHeneise PatrickHeneise referenced this pull request Aug 3, 2018

Closed

Node 8 Runtime #112

@Wolke

This comment has been minimized.

Copy link

commented Aug 4, 2018

when will be merge to master?
need it..

@TimNZ

This comment has been minimized.

Copy link

commented Aug 8, 2018

Functions are now created in the region specified in serverless.yml (asia-northeast1 in my test), but serverless deployment output wrong:

screen shot 2018-08-08 at 12 02 25 pm

screen shot 2018-08-08 at 11 59 13 am

@KeKs0r

This comment has been minimized.

Copy link

commented Aug 8, 2018

@TimNZ I think this PR is not about regions. It is about the Node 8 Runtime, which seem to have worked in your example.

@PatrickHeneise

This comment has been minimized.

@TimNZ

This comment has been minimized.

Copy link

commented Aug 8, 2018

@PatrickHeneise I am using your PR branch, thanks for that.

provider->region is used for deployment and is the region the functions end up in GCloud, it is the region that is reported in Serverless output after deploy that is incorrect.

@adimoraret

This comment has been minimized.

Copy link

commented Aug 9, 2018

Tested locally (only with the NodeJs runtime change from this PR) and it looks like it works.

@PatrickHeneise

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

I've tried to deploy with region 'asia-northeast1' and it didn't work, not in the cloud console nor the output, see also #113

Can remove the region fix and create a second PR if it helps to merge.

@PatrickHeneise

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2018

/cc @horike37 @HyperBrain, anything needed else from my side to get this merged?

@ian

This comment has been minimized.

Copy link

commented Aug 11, 2018

Plz land this.

@LeeCheneler

This comment has been minimized.

Copy link

commented Aug 11, 2018

Any update on when this is due to land? We can't adopt until node8 support is in

@Xplouder

This comment has been minimized.

Copy link

commented Aug 12, 2018

Waiting this as well.

@hamikm

This comment has been minimized.

Copy link

commented Aug 13, 2018

Waiting on this too

@horike37

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Sorry, we don't have commit permission to this repo so can't merge it.

Hey, @pmuens, could you take a look at this PR?

@pmuens

pmuens approved these changes Aug 13, 2018

Copy link
Member

left a comment

Awesome! Thanks for adding / fixing this @PatrickHeneise 👍

Thanks for looking into this @horike37 👍 - Yes, you're right. There might be a permission problem...

LGTM :shipit:

@pmuens pmuens merged commit 8babdf9 into serverless:master Aug 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@TimNZ

This comment has been minimized.

Copy link

commented Aug 13, 2018

Ok - why is no-one using the defaultValue support in _.get()?

@PatrickHeneise PatrickHeneise deleted the PatrickHeneise:feat/nodejs8-runtime branch Aug 13, 2018

@PatrickHeneise

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

Thanks for merging, @pmuens!

@TimNZ I'm not familiar with _.get(), is it worth adding another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.