-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
example [lambda_safe_deployments] use nodejs8.10 for the PreTraffic hook #605
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much neater. Thank you for this update!
awesome |
d995564
to
d76f981
Compare
d76f981
to
3c1be7c
Compare
Codecov Report
@@ Coverage Diff @@
## develop #605 +/- ##
========================================
Coverage 94.65% 94.65%
========================================
Files 69 69
Lines 3011 3011
Branches 559 559
========================================
Hits 2850 2850
Misses 85 85
Partials 76 76 Continue to review full report at Codecov.
|
1c61e43
to
030c16f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sullis Thanks so much for contributing this change! There are still a few things to address:
- All changes should be made against the
develop
branch, not master. Please rebase your changes onto the latestdevelop
and switch the base of this PR todevelop
. - Please address @brettstack's comment on properly returning an error in the exception case. Currently, it's returning a string which Lambda will take as success. Previously, the example returned a response that indicated failure.
examples/2016-10-31/lambda_safe_deployments/src/preTrafficHook.js
Outdated
Show resolved
Hide resolved
WDYT? @brettstack |
@sullis Thanks for the update! I changed the base branch of this PR to develop instead of master. Can you rebase your changes onto latest develop so it can be merged? |
af27b79
to
f664535
Compare
Done. |
Issue #, if available:
n/a
Description of changes:
PreTrafficHook example now uses NodeJS 8.10 runtime
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.