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

ci: run corepack enable before installing dependencies #244

Merged
merged 2 commits into from
Apr 29, 2024
Merged

ci: run corepack enable before installing dependencies #244

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5e17fa
ci: run `corepack enable` as part of prepare
legobeat Apr 25, 2024
69e2b16
chore(ci): do redundant setup-node step before checkout to enable yar…
legobeat Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions .github/workflows/build-lint-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,17 @@ jobs:
name: Prepare
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version-file: '.nvmrc'
node-version: 'lts/*'
Copy link
Member

Choose a reason for hiding this comment

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

Should we not prefer the version in .nvmrc?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the PR description, we are now running this first before checking out, so we don't have access to .nvmrc anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figured if we can't use it consistently for a job in a workflow, it's preferred to be consistent within the workflow.

- name: Install Yarn
run: corepack enable
- uses: actions/checkout@v4
- name: Use Node.js and install dependencies
uses: actions/setup-node@v4
with:
node-version: 'lts/*'
Comment on lines 11 to +21
Copy link
Contributor

@mcmire mcmire Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like corepack may be already installed on ubuntu-latest? The reason I say that is that I'm reviewing solutions others have posted in the actions/setup-node ticket, specifically actions/setup-node#480 (comment), and it seems like we could do this without having to install Node twice:

Suggested change
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version-file: '.nvmrc'
node-version: 'lts/*'
- name: Install Yarn
run: corepack enable
- uses: actions/checkout@v4
- name: Use Node.js and install dependencies
uses: actions/setup-node@v4
with:
node-version: 'lts/*'
- uses: actions/checkout@v4
- name: Install Yarn via Corepack
run: corepack enable
- name: Use Node.js and install dependencies
uses: actions/setup-node@v4
with:
node-version: '.nvmrc'

Perhaps you tried this already, but, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also traversing the chain of various attempts at adding Corepack support to setup-node, and apparently according to this comment Yarn is already installed on GitHub runners somehow, so we may wish to remove it so that the shim that Corepack installs doesn't conflict:

Suggested change
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version-file: '.nvmrc'
node-version: 'lts/*'
- name: Install Yarn
run: corepack enable
- uses: actions/checkout@v4
- name: Use Node.js and install dependencies
uses: actions/setup-node@v4
with:
node-version: 'lts/*'
- uses: actions/checkout@v4
- name: Uninstall existing Yarn
run: npm -g uninstall yarn
- name: Install Yarn via Corepack
run: corepack enable
- name: Use Node.js and install dependencies
uses: actions/setup-node@v4
with:
node-version: '.nvmrc'

Then again, this doesn't make any sense since it would imply that npm is always preinstalled on GitHub runners. Just a suggestion though in case it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is directly related to here, in any case? Recall this has always been the case and we rely on @setup/node to override anything present in the local env. I suggest addressing any removal of already installed stuff separately from this PR.

(I also suspect that the corepack enable in the previous step will be done uneffective by the setup-node replacing local nodejs. But you're welcome to try variations as well)

cache: 'yarn'
- name: Install Yarn dependencies
run: yarn --immutable
Expand All @@ -23,11 +29,17 @@ jobs:
needs:
- prepare
steps:
- name: Use Node.js

This comment was marked as duplicate.

Sign in to view

uses: actions/setup-node@v4
with:
node-version: 'lts/*'
- name: Install Yarn
run: corepack enable
- uses: actions/checkout@v4
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version-file: '.nvmrc'
node-version: 'lts/*'
cache: 'yarn'
- run: yarn --immutable --immutable-cache
- run: yarn build
Expand All @@ -45,11 +57,17 @@ jobs:
needs:
- prepare
steps:
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version: 'lts/*'
- name: Install Yarn
run: corepack enable
- uses: actions/checkout@v4
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version-file: '.nvmrc'
node-version: 'lts/*'
cache: 'yarn'
- run: yarn --immutable --immutable-cache
- run: yarn lint
Expand All @@ -76,6 +94,12 @@ jobs:
matrix:
node-version: [18.x, 20.x]
steps:
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
- name: Install Yarn
run: corepack enable
- uses: actions/checkout@v4
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v4
Expand All @@ -101,6 +125,12 @@ jobs:
matrix:
node-version: [18.x, 20.x]
steps:
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
- name: Install Yarn
run: corepack enable
- uses: actions/checkout@v4
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v4
Expand Down