fix: fix cf-pages gh pkgs sdk install#7638
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR deactivates two unused translation strings and refactors the install script's GitHub Packages authentication mechanism. Instead of passing auth credentials via environment variables, the script now writes a temporary ChangesTranslation String Deactivation
Install Script GitHub Packages Auth Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Deploying explorer-dev with
|
| Latest commit: |
82457b6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e5f56652.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://fix-cf-pages-sdk-pkgs-build.explorer-dev-dxz.pages.dev |
Deploying swap-dev with
|
| Latest commit: |
82457b6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://86f682be.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://fix-cf-pages-sdk-pkgs-build.swap-dev-5u6.pages.dev |
| // pnpm follows the npm convention of reading config from `npm_config_<key>` env vars. | ||
| // Passing the registry/auth this way keeps the token off disk entirely. | ||
| const childEnv = { | ||
| ...process.env, | ||
| 'npm_config_@cowprotocol:registry': 'https://npm.pkg.github.com', | ||
| 'npm_config_//npm.pkg.github.com/:_authToken': authToken, | ||
| } |
There was a problem hiding this comment.
This had no effect on cf-pages
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/scripts/install.test.mjs (1)
7-105: ⚡ Quick winAdd a failure-path cleanup case for the temp npmrc.
This only exercises the successful install path. The important guarantee added in
install.jsis thefinallycleanup afterexecSyncthrows; without a test for that branch, a regression can leak the temp auth file without CI noticing.🤖 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 `@tools/scripts/install.test.mjs` around lines 7 - 105, Add a test exercising the failure path so the temp .npmrc is still cleaned up when execSync throws: in the existing test block (or add a new it) change the sandbox.require('child_process') stub so execSync throws an Error (e.g., execSync() { throw new Error('fail') }) and wrap vm.runInNewContext invocation in a try/catch expecting that error; after catching, assert that writes contains the temp .npmrc write and removals contains the temp dir removal (same assertions as the success case). Use the same fakeFs, sandbox.process.env, and checks of writes[0].path/content/options and removals[0].path/options to verify finally cleanup ran.
🤖 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.
Nitpick comments:
In `@tools/scripts/install.test.mjs`:
- Around line 7-105: Add a test exercising the failure path so the temp .npmrc
is still cleaned up when execSync throws: in the existing test block (or add a
new it) change the sandbox.require('child_process') stub so execSync throws an
Error (e.g., execSync() { throw new Error('fail') }) and wrap vm.runInNewContext
invocation in a try/catch expecting that error; after catching, assert that
writes contains the temp .npmrc write and removals contains the temp dir removal
(same assertions as the success case). Use the same fakeFs, sandbox.process.env,
and checks of writes[0].path/content/options and removals[0].path/options to
verify finally cleanup ran.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d12e1558-dd7a-405b-b491-b234d2174457
📒 Files selected for processing (3)
apps/cowswap-frontend/src/locales/en-US.potools/scripts/install.jstools/scripts/install.test.mjs
3 participants
Summary
Fix cf-pages installation of gh pkgs sdk.
The way the npm auth token was configured previously did not work on cf-pages: https://dash.cloudflare.com/4a1862f09bd4939397c99f925ef3eeaf/pages/view/swap-dev/da5e2aa5-297e-4250-ac02-0d7b45f34992
Now the auth token is stored in a temp file.
Works for both vercel and cf-pages.
To Test
Vercel and cf-pages builds should succeed
Summary by CodeRabbit