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.

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

Jump to bottom

node 12 -> node 16, grpc -> grpc-js #630

Merged
merged 9 commits into from
Nov 25, 2021
Merged

node 12 -> node 16, grpc -> grpc-js #630

merged 9 commits into from
Nov 25, 2021

Conversation

askmeegs
Copy link
Contributor

@askmeegs askmeegs commented Nov 22, 2021 •
edited
Loading

I came across these upgrades while trying to fix #538 - our Node and grpc JS versions were out of date and/or deprecated. This turned out to not be the cause of the memory leak but we should probably still upgrade.

changelog

  1. Upgrades the node base images for currency and payment service from Node 12 to Node 16. (Node 12 is no longer the active version of Node and will go out of maintenance in April 2022.
  2. Upgrades the grpc package used in currency and payment service from grpc to grpc-js. grpc is deprecated. See Migrating to grpc-js for the bind-async change you see in this PR.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 22, 2021
@askmeegs askmeegs marked this pull request as ready for review November 23, 2021 13:34
@askmeegs askmeegs requested a review from a team as a code owner November 23, 2021 13:34
@github-actions
Copy link

🚲 PR staged at http://34.73.14.146

Shabirmean
Shabirmean suggested changes Nov 23, 2021
View reviewed changes
Copy link
Member

@Shabirmean Shabirmean 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 for the change. Left a comment!

@github-actions
Copy link

🚲 PR staged at http://34.73.14.146

@Shabirmean Shabirmean requested review from a team and bourgeoisor November 25, 2021 15:12
@Shabirmean Shabirmean requested a review from a team November 25, 2021 15:14
@Shabirmean
Copy link
Member

I have updated the PR with some minor improvements. The diff between the last commit from the author and all my commits can be viewed at this link:

96c1a7f...e568cfd

@NimJay NimJay self-requested a review November 25, 2021 18:49
NimJay
NimJay approved these changes Nov 25, 2021
View reviewed changes
Copy link
Collaborator

@NimJay NimJay left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have tried skaffold dev and did a quick manual test of my deployment (on my GKE cluster).
The changes also look good, and yes, Node.js 16 is the way to go today.
Approved!

Thanks, @Shabirmean, for taking over this pull-request and tidying it up. 🙏

src/paymentservice/package.json Show resolved Hide resolved
src/paymentservice/server.js Show resolved Hide resolved
@Shabirmean Shabirmean requested review from Shabirmean and removed request for Shabirmean November 25, 2021 19:36
@Shabirmean Shabirmean merged commit 3f437db into master Nov 25, 2021
@Shabirmean Shabirmean deleted the node-fix-november branch November 25, 2021 19:47
fty4 added a commit to fty4/gcp-microservices-demo that referenced this pull request Dec 14, 2021
@fty4
Revert "node 12 -> node 16, grpc -> grpc-js (GoogleCloudPlatform#630)"
ef09cb0 
This reverts commit 3f437db.

linked to issue 647
fty4 added a commit to fty4/gcp-microservices-demo that referenced this pull request Dec 15, 2021
@fty4
Revert "node 12 -> node 16, grpc -> grpc-js (GoogleCloudPlatform#630)"
f716734 
This reverts commit 3f437db.

linked to issue 647
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shabirmean Shabirmean Shabirmean approved these changes

@NimJay NimJay NimJay approved these changes

@bourgeoisor bourgeoisor Awaiting requested review from bourgeoisor

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Possible memory leak in NodeJS / Python services
3 participants
@askmeegs @Shabirmean @NimJay