Fixed pack.js and Dockerfile references to deleted .npmrc#28205
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR clarifies pnpm detection comments in the CI enforcement script, removes copying the root .npmrc from Docker pre-install COPY steps (production and dev) so only package.json, pnpm-lock.yaml, and pnpm-workspace.yaml are used for installs, and updates ghost/core/scripts/pack.js to import js-yaml, emit a trimmed deploy pnpm-workspace.yaml, avoid writing pnpm.overrides into the packaged package.json, regenerate the deploy lockfile, and require pnpm-workspace.yaml (with a non-empty default catalog), pnpm-lock.yaml, and package.json in deploy output. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f26407d to
39400ce
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/scripts/pack.js (1)
131-131:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate log message to reflect write operation instead of copy.
The message says "Copied .npmrc" but line 125 uses
writeFileSyncto write the file directly rather than copying it. The message should say "Wrote .npmrc" or "Wrote .npmrc and copied pnpm-workspace.yaml" to accurately describe what the code does.📝 Suggested fix for log message accuracy
-console.log('Copied .npmrc (with frozen-lockfile=false) and pnpm-workspace.yaml'); +console.log('Wrote .npmrc (with frozen-lockfile=false) and copied pnpm-workspace.yaml');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/scripts/pack.js` at line 131, The console log message inaccurately says "Copied .npmrc" even though the file is created via writeFileSync; update the log in the pack.js console.log call to reflect a write operation (e.g., "Wrote .npmrc (with frozen-lockfile=false) and copied pnpm-workspace.yaml"). Locate the writeFileSync usage that writes .npmrc and the console.log statement and change the text to mention "Wrote .npmrc" while still noting that pnpm-workspace.yaml was copied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ghost/core/scripts/pack.js`:
- Line 131: The console log message inaccurately says "Copied .npmrc" even
though the file is created via writeFileSync; update the log in the pack.js
console.log call to reflect a write operation (e.g., "Wrote .npmrc (with
frozen-lockfile=false) and copied pnpm-workspace.yaml"). Locate the
writeFileSync usage that writes .npmrc and the console.log statement and change
the text to mention "Wrote .npmrc" while still noting that pnpm-workspace.yaml
was copied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2e92ea0-c8d6-4f86-8baf-5afde2891190
📒 Files selected for processing (4)
.github/scripts/enforce-package-manager.jsDockerfile.productiondocker/ghost-dev/Dockerfileghost/core/scripts/pack.js
39400ce to
25a37ec
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28205 +/- ##
==========================================
- Coverage 73.88% 73.88% -0.01%
==========================================
Files 1531 1531
Lines 129865 129873 +8
Branches 15585 15586 +1
==========================================
+ Hits 95956 95961 +5
- Misses 32946 32949 +3
Partials 963 963
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bf012ce to
6ce05f3
Compare
The pnpm 11 upgrade in #28197 deleted the root .npmrc, which removed both a file reference (broke Docker COPY) and the `frozen-lockfile=false` directive that had been silently absorbing several lockfile/workspace mismatches in published-tarball installs (pnpm 11 ignores .npmrc keys other than auth/registry, so it couldn't carry the directive anyway). Five distinct issues across the release pipeline: 1. `Dockerfile.production` and `docker/ghost-dev/Dockerfile` listed .npmrc in their COPY directives. Docker fails on missing sources — devcontainer image build went red on first push to main. 2. `pack.js` read root .npmrc to build the published tarball. Would throw on the next `pnpm archive` / release. 3. pnpm 11 strict-validates `catalog:` refs in workspace package.jsons, so `pnpm pack` of @tryghost/i18n etc. needs pnpm-workspace.yaml in scope. pack.js wrote it AFTER the pack loop. Reordered to write it first. 4. The packaged pnpm-workspace.yaml shipped overrides + packageExtensions verbatim from root, but `pnpm deploy` generates a lockfile that doesn't record them (they're already applied to the resolved package.json). pnpm 11's frozen-lockfile install (CI=true default for ghost-cli) compares config to lockfile as raw strings and fails ERR_PNPM_LOCKFILE_CONFIG_MISMATCH. Solution: pack.js writes a trimmed workspace.yaml carrying only catalog/catalogs/allowBuilds/ strictDepBuilds — the things end-user installs actually need. 5. After pack.js's package.json post-processing (strip peer suffixes, rewrite file: refs to component tarballs), the deploy lockfile no longer matched. Under pnpm 10 this was masked by frozen-lockfile=false in the shipped .npmrc. Solution: pack.js regenerates the lockfile inside the deploy dir with `pnpm install --lockfile-only` so the shipped tarball is internally consistent. Also re-applies the comment softening from the #28197 review of .github/scripts/enforce-package-manager.js (was lost to a rebase autostash on my end before merge).
6ce05f3 to
7331fbf
Compare
refs #28197, #28205. This fixes the `ghost-dev` compose for pnpm 11. The dev Dockerfile (`docker/ghost-dev/Dockerfile`) only stages `ghost/{core,i18n,parse-email-address}` `package.json`s at build time. Compose then mounts `./ghost` into the container at runtime, which exposes `ghost/admin`'s `workspace:*` deps (e.g. `@tryghost/admin-x-framework` → lives in `apps/admin-x-framework/`). pnpm 10 was lenient about the missing workspace packages; pnpm 11 strict-validates the workspace graph and `pnpm dev` fails instead of being tolerated.
Hotfix for #28197. Five distinct release-pipeline files broke after the
.npmrcdeletion, but the deeper story is that.npmrcwas also carryingfrozen-lockfile=false, which was silently absorbing several pnpm-deploy-vs-lockfile mismatches under pnpm 10. pnpm 11 ignores.npmrcfor that directive, so the mismatches surface.What was broken
Dockerfile.production:28— production image builds.COPYfails on missing source.docker/ghost-dev/Dockerfile:21— devcontainer + local dev. Already failed on main: run 26525156893.ghost/core/scripts/pack.js:124— read root.npmrcto build the published Ghost tarball; would throw on next release.pack.js—pnpm packof@tryghost/i18n/etc. needspnpm-workspace.yamlin scope (pnpm 11 strict-validatescatalog:refs); pack.js wrote it AFTER the pack loop.pack.js— the shipped tarball'spnpm-workspace.yamlcarriedoverrides+packageExtensionsverbatim from root, butpnpm deploy's generated lockfile doesn't record them (they're already applied to the resolved package.json). pnpm 11's frozen-lockfile install (CI=true default for ghost-cli) failedERR_PNPM_LOCKFILE_CONFIG_MISMATCH.pack.js— after the existing peer-suffix-stripping and component-file: rewriting on the deployed package.json, the deploy lockfile no longer matched. Under pnpm 10 this was masked byfrozen-lockfile=falsein.npmrc; pnpm 11 failsERR_PNPM_OUTDATED_LOCKFILE.pack.js—minimumReleaseAgepolicy in the shipped workspace.yaml 404'd on the private component tarballs and failed install insideDockerfile.production.What changed
.npmrcfrom theCOPYline in both. Single-token edit per file.pack.js— stop reading the deleted root.npmrc.pack.js— copypnpm-workspace.yamlBEFORE the pack loop (so workspacecatalog:refs resolve forpnpm packof components).pack.js— ship a trimmedpnpm-workspace.yamlcarrying onlycatalog,catalogs,allowBuilds,strictDepBuilds(the things end-user installs need). Dropoverrides,packageExtensions,packages, supply-chain policies. Explicitly setminimumReleaseAge: 0since the published tarball is itself a release artifact and the policy 404s on private component tarballs.pack.js— regenerate the deploy lockfile inside the deploy dir withpnpm install --lockfile-onlyafter package.json post-processing, so the shipped tarball is internally consistent under pnpm 11's strict checks.enforce-package-manager.js— re-applies the comment softening from the Updated pnpm to 11.4.0 #28197 review (lost to a rebase autostash on my end).Verification
End-to-end through the full release pipeline, locally, with
CI=trueto match production install behavior:pnpm archive(inghost/core/)docker build --target core -t … .(from extracted tarball)CI=true ghost install local --archive …How this slipped through #28197
Two layers of cause:
.npmrcreferences at the start of Updated pnpm to 11.4.0 #28197 and noted the two DockerfileCOPYlines andpack.js's read, but didn't follow through with edits when I deleted the file. Should have grepped post-delete as a final sweep.CI=true, which silently reconciled the lockfile mismatches via the defaultfrozen-lockfile=false. CI runs withCI=true→frozen-lockfile=true→ strict check tripped. AddingCI=trueto my local verification checklist for any release-pipeline change.And a third:
Build & Publish Artifacts(which runspnpm archive) was failing on #28197's CI but the merge gate didn't see it because that job wasn't injob_required_tests.needs. Fixed separately in #28206.