Skip to content

revert: undo Yarn 4 upgrade and danger 13.0.10 bump - #81

Merged
iskounen merged 1 commit into
mainfrom
iskounen/revert/to-pre-danger-upgrade
Jun 25, 2026
Merged

revert: undo Yarn 4 upgrade and danger 13.0.10 bump#81
iskounen merged 1 commit into
mainfrom
iskounen/revert/to-pre-danger-upgrade

Conversation

@iskounen

Copy link
Copy Markdown
Contributor

Summary

Why

After upgrading Danger and then Yarn 4, CI is failing on consumer repos (e.g. Eigen) because actions/setup-node activates the consumer's Yarn version (e.g. 4.10.3) before the install step runs in .tooling, causing a lockfile format mismatch in hardened mode. Root cause not yet fully diagnosed.

🤖 Generated with Claude Code

Reverts PRs #76, #77, #78, #79, #80 to restore the pre-upgrade state
while we investigate the Corepack version resolution issue.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

- name: Install tooling dependencies
run: yarn install --immutable
run: yarn install --frozen-lockfile

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.

🟡 --frozen-lockfile will fail against a Yarn Berry .tooling.

This step runs in .tooling, which is checked out from artsy/duchamp@main (ref: main, line 46). At the moment, main still pins packageManager: "yarn@4.17.0", and with Corepack enabled (line 55) yarn --version resolves to Yarn 4 inside .tooling. Yarn Berry (2+) does not accept the Yarn 1 flag --frozen-lockfile — it errors with Usage Error: Unsupported option name ("--frozen-lockfile") — so this install will fail until the revert actually lands on main (including this PR's own review CI).

Note that .github/actions/setup-and-install/action.yml in this same PR added Yarn-version detection precisely to bridge this Yarn 1 ↔ Yarn 4 ambiguity, but this workflow doesn't use it. Consider reusing that action / detection here, or keeping the install tolerant of both, e.g.:

        run: |
          if [[ "$(yarn --version)" =~ ^[2-9] ]]; then
            yarn install --immutable
          else
            yarn install --frozen-lockfile
          fi

Same applies to line 175.

@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🤖 Code Review

Summary

Reverts the root project from Yarn 4 (Berry) back to Yarn 1.22.22 and danger from 13.0.10 to 13.0.4, undoing PRs #76#80. This regenerates yarn.lock in Yarn 1 (lockfile v1) format, deletes the Berry-only .yarnrc.yml, drops the .yarn/install-state.gz ignore, and reworks setup-and-install/action.yml to use setup-node's built-in yarn cache plus runtime Yarn-version detection. It also flips two .tooling installs in claude-review.yml from --immutable to --frozen-lockfile.

The action.yml change is the right call — detecting Berry vs Classic and choosing --no-immutable/--frozen-lockfile accordingly makes installs robust during the Yarn 4 -> Yarn 1 transition. My one substantive concern is that claude-review.yml was not given the same treatment.

Issues Found

🟡 Important — claude-review.yml hardcodes --frozen-lockfile for a .tooling that may still be Yarn Berry (.github/workflows/claude-review.yml:58 and :175)

- name: Install tooling dependencies
  run: yarn install --frozen-lockfile
  working-directory: .tooling

.tooling is checked out from artsy/duchamp@main (ref: main, lines 46 / 163) and Corepack is enabled just above (lines 55 / 172). Until this revert lands on main, that checkout pins yarn@4.17.0, so yarn --version inside .tooling is Yarn 4. Yarn Berry rejects --frozen-lockfile (Usage Error: Unsupported option name ("--frozen-lockfile")), so these steps fail — including, potentially, this PR's own review CI run, since it checks out .tooling from current main.

This is exactly the ambiguity the new action.yml solves with version detection. Recommend reusing the shared action here, or applying the same detect-and-branch logic, rather than hardcoding the Yarn 1 flag. (Inline suggestion posted on line 58.) Post-merge, once main is on Yarn 1, the hardcoded flag becomes correct — so this is mainly a transition-window / self-CI hazard, but a real one.

🟢 Suggestion — version-detection regex won't match Yarn >= 10 (.github/actions/setup-and-install/action.yml:38)

if [[ "\$YARN_VERSION" =~ ^[234] ]]; then

^[234] matches a leading 2, 3, or 4. A future yarn@10/yarn@20 would start with 1/2 and be misclassified (10.x -> Classic, 20.x -> Berry). Not a problem today, but ^[2-9] (or a numeric >= 2 check) would be more future-proof for "anything Berry."

Areas Reviewed

  • Architecture & Design — Migrating caching from a manual actions/cache@v4 step to setup-node's built-in cache: yarn with cache-dependency-path covering both yarn.lock and .tooling/yarn.lock is clean and reduces config. Adding runtime Yarn-version detection to the shared action is a good resilience choice for the transition period.
  • Consistency — The root revert is internally consistent: package.json (yarn@1.22.22), regenerated Yarn 1 yarn.lock (# yarn lockfile v1), deleted .yarnrc.yml, and removed the .yarn/install-state.gz ignore all line up with Yarn 1. The gap is that claude-review.yml opted into hardcoded flags instead of the shared, version-aware action (see Important above).
  • Security — No security impact. danger 13.0.10 -> 13.0.4 is a downgrade; assuming no known advisory in 13.0.4, this is fine as a deliberate revert.

Questions for Author

  1. Is main being reverted to Yarn 1 before or together with this PR? If .tooling (always ref: main) is still on yarn@4.17.0 when these workflows run, the --frozen-lockfile steps in claude-review.yml will fail. Confirming the merge ordering would resolve the Important item.
  2. This PR doesn't touch tests.yml (yarn install --immutable) or npm-audit.yml (yarn npm audit — a Berry-only command). Those run against the caller's checkout rather than duchamp's root, so they're likely intentionally out of scope — but is the revert meant to be limited to the root package + the two claude-review install steps, or should those Berry-isms be revisited too?

Reviewed the full diff plus related workflows (tests.yml, npm-audit.yml, danger.yml), the .tooling package manifest, and both lockfile formats. The revert is well-formed; the only actionable concern is the hardcoded --frozen-lockfile in claude-review.yml.

@iskounen
iskounen merged commit 45d4884 into main Jun 25, 2026
6 checks passed
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.

1 participant