-
Notifications
You must be signed in to change notification settings - Fork 14
test: manage yarn release/runtime outside repo #510
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
base: main
Are you sure you want to change the base?
Conversation
although this adds an extra step in the setup process, it's done only once and it's needed to fix the issue reported by Checkmarx
f61313e to
b1b39fa
Compare
| # Yarn Zero-Install cache (only needed if using PnP — you're not) | ||
| .yarn/ | ||
| # Yarn versioned binaries (kept out of git) | ||
| .yarnrc.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .yarnrc.yml file is still tracked in git (modified in the PR), but it's also being added to .gitignore. Hmm...
.yarnrc.yml should remain tracked in git for team consistency (it defines nodeLinker: node-modules). Only the binary runtime .yarn/releases/ should be ignored..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to keep the file in the repo but having it in the gitignore avoids showing the file as modified when you install yarn the first time
| pushd qa/tools/block-scanner | ||
| bun install | ||
| bun run generate:data | ||
| popd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought POSIX standard requires files to end with a newline character?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we were not using bun? let's not use all the package managers there are.
| run: | | ||
| npm install -g yarn@1 | ||
| yarn set version 3.6.4 | ||
| yarn install --immutable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corepack looks better here..
e.g. :
run: |
corepack enable
yarn install --immutable
npm install -g yarn gives you one version system-wide. Corepack is simpler. Built Into Node.js.
quote from https://yarnpkg.com/corepack :
You may notice by reading our [installation guide](https://yarnpkg.com/getting-started/install) that we don't tell you to run npm install -g yarn to install Yarn - we even recommend against it. The reason is simple: just like your project dependencies must be locked, so should be the package manager itself.
Installing Yarn as a global binary meant you always used whatever was the latest version published. Most of the time it worked fine, but every once in a while something was shipped that could impact the way your project was installed - be it a bugfix, new bug, or breaking change.
To counter that, Yarn joined forces with the Node.js project to start the development of [Corepack](https://nodejs.org/api/corepack.html), an official Node.js tool letting you define which package manager version you want to use on a per-project basis, just like your lockfile does for your project dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is corepack available in the github executor or should I use a specific action for that?
Yes npm install -g yarn@1 is system wide but this is confined to the machine that is running this workflow and will be gone after execution
|
|
||
| ### 🔄 Prepare Yarn (one-time per machine) | ||
|
|
||
| The repository no longer ships the Yarn runtime in `.yarn/releases/` (to keep secrets out of git). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we need 'why'.
What do you think of :
The repository no longer ships the Yarn runtime in .yarn/releases/ to address a Checkmarx security finding. The Yarn binary was flagged as potentially containing embedded credentials or secrets, so we now download it locally on each machine instead of committing it to the repository.
|
|
||
| ## 🧰 Install Dependencies | ||
|
|
||
| From the **QA tests folder**, install all required dependencies: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor- since cd qa/tests is already in the 'Prepare Yarn (one-time per machine)' section, no need to repeat it below, just have it as:
yarn install --immutable
Although this adds an extra step in the setup process, it's done only once and it's needed to fix the issue reported by Checkmarx