Yarn Berry: Use appropriate mode for yarn version and repo state

[pavera]

Context

With Yarn Berry there are various modes which can be enabled to get different resulting artifacts. This PR attempts to normalize our usage of these modes to get the correct behavior across a range of yarn versions and potential repo states.

Approach

I've pulled some copy/paste code up into helpers.rb and implemented the logic below

if yarn >= 2 && yarn < 3
  args = ""
  yarn config set enableScripts false
if yarn >= 3 and zero-install
  args = "--mode=skip-build"
if yarn >= 3 and !zero-install
  args = "--mode=update-lockfile"
  yarn config set enableScripts false

This should result in using the much speedier update-lockfile mode if the repository is not setup for zero-install, but will use the correct skip-build mode if the repo is setup for zero install to preserve file permissions while still preventing third party code execution during updates.

[jeffwidman]

Looks fine to me but I'm not very familiar with the underlying yarn logic so you may want a second review from someone else

[jurre]

Should we memoize this? It's not hugely expensive, but we are shelling out and the value would never change I think?

[pavera]

I need to fix this as it is the cause of the updater CI failure. I think we have to determine the version based on a lockfile as we were previously.

[jurre]

Are there any tests we can/should add to cover the bug we're trying to fix here? Overall the changes look great to me, I did spot a few CI failures, rerunning those to make sure they're not flakes

[pavera]

Are there any tests we can/should add

Yes, I think we should make sure we have both zero-install and non zero-install fixtures and confirm the non zero-install does not end up with a .pnp.cjs file after update, and probably confirm permissions of .pnp.cjs on the zero-install case.

I'll be looking at the CI failures today.

[jeffwidman]

Just to doublecheck, the .pnp.cjs and install-state.gz files are required to be checked in for the test spec to work? Or should they be .gitignore'd? Because they're 4+ KB...

[pavera]

.pnp.cjs is the "zero install" configuration file, so yes it needs to be committed to a zero install repo. I'm not sure what the install-state.gz tracks exactly, but generally the .yarn directory needs to be committed to a zero install repo as well.

[merceyz]

I'm not sure what the install-state.gz tracks exactly, but generally the .yarn directory needs to be committed to a zero install repo as well.

To quote the documentation:

.yarn/install-state.gz is an optimization file that you shouldn't ever have to commit. It simply stores the exact state of your project so that the next commands can boot without having to resolve your workspaces all over again.
https://yarnpkg.com/getting-started/qa/#which-files-should-be-gitignored

[gabormagyar]

And to add to what @merceyz said, here's exactly what needs to be gitignored both when using zero installs and when not:

https://yarnpkg.com/getting-started/qa/#which-files-should-be-gitignored

[wozzo]

@pavera Hi there, firstly this is all great work, and we're loving being able to use dependabot now that yarn 2+ is supported.
We are having an issue with the cache not being updated at the moment, and I'm pretty sure it's because we have the nodeLinker setting set to node-modules instead of pnp. The cache can still be committed which reduces the work during an install, but we still need to run the yarn install command to set up the node_modules folder.
We've found fewer problems with this than the pnp approach.

So tldr, I think the pseudo code above should be

if yarn >= 2 && yarn < 3
  args = ""
  yarn config set enableScripts false
if yarn >= 3 and has-yarn-cache
  args = "--mode=skip-build"
if yarn >= 3 and !has-yarn-cache
  args = "--mode=update-lockfile"
  yarn config set enableScripts false

I'm not sure whether the has-yarn-cache value is best obtained from analysing the .gitignore, or just seeing if there is a .yarn/cache directory in the project?
The .pnp.cjs file may or may not exist, but should be updated if it is.