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

chore(ci): use nodeVersion with environment variables #74576

Open
wants to merge 5 commits into
base: canary
Choose a base branch
from

Conversation

nnnnoel
Copy link
Contributor

@nnnnoel nnnnoel commented Jan 7, 2025

No description provided.

@ijjk
Copy link
Member

ijjk commented Jan 7, 2025

Allow CI Workflow Run

  • approve CI run for commit: 1ca3b1d

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

This is going in the right direction, thank you. But some semantics are now off.

jobs:
testExamples:
# Don't execute using cron on forks
if: (github.repository == 'vercel/next.js') || (inputs.is_dispatched == true)
if: (github.repository_owner == 'vercel') || (inputs.is_dispatched == true)
Copy link
Member

Choose a reason for hiding this comment

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

Matches other checks e.g. in trigger_release 👍🏻

@@ -244,7 +244,7 @@ jobs:
react: ['']
uses: ./.github/workflows/build_reusable.yml
with:
nodeVersion: 18.18.2
nodeVersion: ${{ env.NODE_LTS_VERSION }}
Copy link
Member

Choose a reason for hiding this comment

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

The Node.js version has a different meaning here. We want to test with our lowest supported Node.js version not whatever is the highest maintenance release line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see. Thanks for your explain.
I fixed that and specify as environment variable with env.NODE_MAINTENANCE_VERSION.
But I don't know when we can use matrix and build/test for the every supported node versions like 18, 20, 22.
my changes are only port magic numbers to environment variable and didn't use matrix (:

Copy link
Member

Choose a reason for hiding this comment

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

This PR shouldn't add matrix testing nor change the used Node.js version

@@ -95,7 +95,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: 18
node-version: ${{ env.NODE_LTS_VERSION }}
Copy link
Member

Choose a reason for hiding this comment

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

This should be

Suggested change
node-version: ${{ env.NODE_LTS_VERSION }}
nodeVersion: ${{ env.NODE_MAINTENANCE_VERSION }}

to match the prior behavior. Same for the other instances. I'm all for using named variables over magic numbers but changing the used Node.js version should be in a follow-up in case we need to revert what Node.js version is used.

@nnnnoel nnnnoel requested a review from eps1lon January 7, 2025 10:05
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