Skip to content

ci: update node version#230

Open
stay-foolish-forever wants to merge 2 commits into
alibaba:mainfrom
stay-foolish-forever:ci/update-node-version
Open

ci: update node version#230
stay-foolish-forever wants to merge 2 commits into
alibaba:mainfrom
stay-foolish-forever:ci/update-node-version

Conversation

@stay-foolish-forever

Copy link
Copy Markdown
Contributor

Description

Update Node version used by Github Actions to 24.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)
    Running local build for package tarball and pages to check that node 24 is compatible.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA

Related Issues

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔍 OpenCodeReview found 3 issue(s) in this PR.

  • ✅ 3 posted as inline comment(s)
  • 📝 0 posted as summary

runs-on: self-hosted
container:
image: node:20
image: node:24

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reliability concern: node:24 is a non-LTS (Current) release line. For production CI/CD workflows, it's recommended to use an LTS version (e.g., node:22 which should be the Active LTS around this time). Non-LTS releases have shorter support windows and may receive breaking changes more frequently.

Additionally, consider pinning to a more specific version tag (e.g., node:24.x.y) rather than just the major version to ensure reproducible builds. The node:24 tag will silently update on every minor/patch release, which could cause unexpected build failures.

Suggestion:

Suggested change
image: node:24
image: node:22

runs-on: self-hosted
container:
image: node:20
image: node:24

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Container image tag precision: Using a bare major version tag (node:24) means the underlying image can change silently when new minor/patch releases are published, potentially introducing breaking changes or inconsistencies between runs. Consider pinning to a specific minor or patch version (e.g., node:24.1 or node:24.1.0) for more reproducible builds, especially on self-hosted runners where image caching behavior may vary.

runs-on: self-hosted
container:
image: node:20
image: node:24

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Potential stability concern: Node.js 24 is likely still in "Current" (non-LTS) status as of now. For a release/publish workflow, using a non-LTS Node version may introduce unexpected breaking changes or instability. Consider using node:22 (the active LTS) instead, or if Node 24 is specifically required, pin to a specific minor version (e.g., node:24.1) to ensure reproducible builds.

Also note that this same change is applied consistently across deploy-pages.yml and ocr-review.yml, so any compatibility issue will affect all three workflows.

@lizhengfeng101 lizhengfeng101 left a comment

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.

Review: ci: update node version

Changes: Updates Node.js container image from node:20 to node:24 across all three GitHub Actions workflows (deploy-pages.yml, ocr-review.yml, release.yml). Also removes an outdated trigger comment in ocr-review.yml.

Findings

Non-blocking suggestions:

  1. Consider pinning to a specific minor version (e.g., node:24.1) instead of the rolling node:24 tag. Rolling tags can introduce unexpected behavior when a new patch is published, which may affect build reproducibility.

  2. Node 24 is currently in "Current" status, not LTS. LTS is scheduled for October 2025. This is generally fine for CI usage, but worth noting — if any dependencies rely on native addons, there may be compatibility surprises down the road.

Verdict

Clean, straightforward change. All node:20 references in workflows are covered — no missed spots. The outdated comment removal in ocr-review.yml is a nice cleanup.

LGTM ✅ — good to merge.

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.

2 participants