Skip to content

Conversation

@razz1k
Copy link
Contributor

@razz1k razz1k commented May 7, 2025

No description provided.

@razz1k razz1k requested a review from zameo94 May 7, 2025 14:37
@ain ain requested a review from Copilot May 7, 2025 14:47
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the Docker image documentation to reflect the new supported Node.js version.

  • Update Node.js version from v18 to v22 in the README.md file.
Files not reviewed (2)
  • Dockerfile: Language not supported
  • package.json: Language not supported

Copy link
Member

@ain ain left a comment

Choose a reason for hiding this comment

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

Thanks for the bump! Some points made for considerations.

apt-get install -y google-cloud-sdk nodejs yarn google-chrome-unstable --no-install-recommends && \
apt-get autoremove && \
rm -rf /var/lib/apt/lists/*
apt-get autoclean && \
Copy link
Member

Choose a reason for hiding this comment

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

👍

package.json Outdated
Comment on lines 11 to 14
"build": "docker build -t gcp-ruby/test -f ./Dockerfile .",
"run": "docker run --rm -it --entrypoint sh gcp-ruby/test:latest",
"clean": "docker image rm gcp-ruby/test:latest",
"full-test": "npm run test && npm run build && npm run run && npm run clean"
Copy link
Member

Choose a reason for hiding this comment

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

I'd avoid adding this over NPM, since it adds unnecessary complexity and failure layer as well as confusion (test vs full-test).

GitHub Actions is already testing against this and instead of local tests you could simply push the PR up and get it tested by Actions for you.

dockerlint was only added here because it's a Node module that does the linting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, make sense

@razz1k razz1k requested a review from ain May 8, 2025 16:38
Copy link
Member

@ain ain left a comment

Choose a reason for hiding this comment

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

Thanks for the work here Igor!

@ain ain merged commit 0741e88 into main May 9, 2025
2 checks passed
@ain ain deleted the bump-node-to-v22 branch May 9, 2025 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants