feat(global): INFRA-1507 add local npm registry#7496
feat(global): INFRA-1507 add local npm registry#7496Gladioluss wants to merge 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request adds npm registry server configuration and authentication support across the CI/CD pipeline. A workflow-level environment variable and Docker build arguments are introduced in the GitHub Actions workflow, Yarn is configured with registry and authentication settings, and the Dockerfile is updated to accept and use these registry credentials during package installation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4410d1dbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| fs.writeFileSync(npmrcPath, [ | ||
| `registry=${NPM_REGISTRY_SERVER}`, | ||
| `//${registryUrl.host}${registryUrl.pathname === '/' ? '' : registryUrl.pathname}/:_authToken=${NPM_AUTH_TOKEN}`, |
There was a problem hiding this comment.
If NPM_REGISTRY_SERVER includes a path with a trailing slash (for example https://registry.example.com/npm/), this builds an npmrc auth entry like //registry.example.com/npm//:_authToken=... with a double slash before :_authToken. In that case npm may not apply the token to registry requests, and npm unpublish will fail with auth errors even though the token is valid. Trim the trailing slash (or conditionally append /) when building this key.
Useful? React with 👍 / 👎.
| DOCKER_IMAGE: condo/condo-image:${{ github.event.pull_request.head.sha || github.sha }} | ||
| DOCKER_IMAGE_FULL: ${{ secrets.DOCKER_REGISTRY }}/condo/condo-image:${{ github.event.pull_request.head.sha || github.sha }} | ||
| CONDO_TEST_SHARD_TOTAL: 20 | ||
| NPM_ALWAYS_AUTH: false |
There was a problem hiding this comment.
isn't it deprecated....and right now seems like a security issue
There was a problem hiding this comment.
Authorization is always required when configuring a local npm repository
There was a problem hiding this comment.
nope, we won't store anything like this in repo
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
bin/npm-prune-unpublished.js (2)
1-4: Import order nit.Per ESLint
import/order, builtin imports should be alphabetized case-insensitively;child_processshould come beforefs.🔧 Proposed diff
-const fs = require('fs') -const os = require('os') -const path = require('path') -const { spawnSync } = require('child_process') +const { spawnSync } = require('child_process') +const fs = require('fs') +const os = require('os') +const path = require('path')As per coding guidelines: "Enforce import order groups ... Alphabetize imports case-insensitively within each import group".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/npm-prune-unpublished.js` around lines 1 - 4, Import order is reversed for builtins: alphabetize the built-in requires case-insensitively so child_process comes before fs; update the top-level requires (the const declarations referencing spawnSync, fs, os, path) to list builtins in order: require('child_process') (keep destructuring spawnSync), then require('fs'), require('os'), require('path') so the identifiers spawnSync, fs, os, path remain unchanged and linting passes.
52-76: Sequential public-registry fetches will dominate runtime.For a monorepo lockfile with thousands of unique package names, awaiting each
fetchin series (one round-trip per package) will take minutes on cold cache. Consider batching with a bounded concurrency (e.g.p-limitor a simple N-at-a-time worker loop) so the daily job stays well under the job timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/npm-prune-unpublished.js` around lines 52 - 76, getMissingVersions currently performs each fetch sequentially causing long runtimes; change it to perform the metadata fetches with bounded concurrency (e.g., use p-limit or implement an N-at-a-time worker) so many package lookups run in parallel but capped. Specifically, keep the metadataByPackage cache logic and dedup by pkg.name, collect the unique package names to fetch, then run fetch(`${PUBLIC_REGISTRY}/${encodePackageName(name)}`) for those names with a concurrency limit (and set metadataByPackage to null on 404 or parsed JSON on success, throw on non-ok), and finally rebuild the missing array using metadataByPackage as the existing code does. Ensure error handling and the same return shape of getMissingVersions remain unchanged.Dockerfile (1)
28-40: MissingNPM_AUTH_TOKENwiring for authenticated private registries.
.yarnrc.ymlreadsnpmAuthToken: "${NPM_AUTH_TOKEN:-}", but the installer stage never declaresARG NPM_AUTH_TOKEN/ENV NPM_AUTH_TOKEN.yarn install --immutablewill therefore run with an empty token. Today this works only because the configured registry is anonymously readable andNPM_ALWAYS_AUTH=false; the moment the private registry requires auth (or you flipNPM_ALWAYS_AUTH=true), container builds will 401.🔧 Suggested wiring
ARG NPM_ALWAYS_AUTH=false ARG NPM_REGISTRY_SERVER=https://registry.npmjs.org +ARG NPM_AUTH_TOKEN= WORKDIR /app # Copy pruned monorepo (only package.json + yarn.lock) COPY --chown=app:app ./out /app # Copy yarn berry COPY --chown=app:app ./.yarn /app/.yarn COPY --chown=app:app ./.yarnrc.yml /app/.yarnrc.yml ENV NPM_ALWAYS_AUTH=$NPM_ALWAYS_AUTH ENV NPM_REGISTRY_SERVER=$NPM_REGISTRY_SERVER -RUN --mount=type=cache,target=/usr/local/share/.cache/yarn \ +ENV NPM_AUTH_TOKEN=$NPM_AUTH_TOKEN +RUN --mount=type=cache,target=/usr/local/share/.cache/yarn \ + --mount=type=secret,id=npm_auth_token,env=NPM_AUTH_TOKEN \ yarn install --immutable --inline-buildsPrefer
--mount=type=secretover plainARG/ENVso the token does not end up in the image history/layers. You'd pass it through the workflow'sdocker/build-push-actionwith thesecrets:input instead ofbuild-args.Does your target private registry allow unauthenticated reads from the build host? If yes, this is currently latent; if not, installer builds inside the container will fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 28 - 40, The Dockerfile installer stage doesn't provision the NPM_AUTH_TOKEN referenced by .yarnrc.yml (npmAuthToken: "${NPM_AUTH_TOKEN:-}") so yarn install --immutable runs with an empty token; update the Dockerfile to mount the secret into the build with --mount=type=secret (not ARG/ENV) and consume it during the RUN yarn install step (pass the secret into the build context so the npmAuthToken is available to yarn without adding the token to image layers), ensuring the secret name matches NPM_AUTH_TOKEN used by .yarnrc.yml and the RUN --mount=type=cache,... yarn install --immutable --inline-builds command is updated to read that secret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/npm.prune.unpublished.yml:
- Around line 31-37: The DRY_RUN environment expression currently defaults
scheduled runs to false causing real npm unpublish actions; change the workflow
so scheduled/cron executions default to a dry-run instead of real unpublishes
(e.g. make DRY_RUN default to 'true' for non-workflow_dispatch events or invert
the boolean logic), and ensure this aligns with how bin/npm-prune-unpublished.js
reads DRY_RUN (the DRY_RUN env var and its truthy check). Update the DRY_RUN
assignment in the job env so manual dispatch can still pass a real value but
scheduled runs remain safe by default.
In `@bin/npm-prune-unpublished.js`:
- Around line 18-30: The semver regex in parseNpmResolution rejects versions
that have both prerelease and build metadata (e.g., 1.0.0-alpha.1+build.7);
update the validation to allow an optional prerelease segment followed by an
optional build segment by replacing the single combined group
(?:[-+][0-9A-Za-z-.]+)? with two groups
(?:-[0-9A-Za-z-.]+)?(?:\+[0-9A-Za-z-.]+)?, keeping the same start/end anchors so
parseNpmResolution still returns null for truly invalid versions.
- Around line 52-76: getMissingVersions currently treats any package absent from
PUBLIC_REGISTRY as deletable which will unconditionally unpublish private scoped
packages; change the workflow so you (1) add an allowlist check using an env var
NPM_MIRRORED_SCOPES (comma-separated scopes) and only consider packages whose
scope matches that allowlist before attempting to unpublish, (2) keep
getMissingVersions (and its use of PUBLIC_REGISTRY, encodePackageName,
metadataByPackage, and the missing array) purely for detection and move the
safety checks to the main loop that performs npm unpublish against
NPM_REGISTRY_SERVER, (3) implement a confirmation threshold via an env var
MAX_MISSING_DELETE and abort with an error if missing.length >
MAX_MISSING_DELETE, and (4) re-order the imports at the top of the file to
follow guidelines: child_process, fs, os, path (alphabetical within builtins).
---
Nitpick comments:
In `@bin/npm-prune-unpublished.js`:
- Around line 1-4: Import order is reversed for builtins: alphabetize the
built-in requires case-insensitively so child_process comes before fs; update
the top-level requires (the const declarations referencing spawnSync, fs, os,
path) to list builtins in order: require('child_process') (keep destructuring
spawnSync), then require('fs'), require('os'), require('path') so the
identifiers spawnSync, fs, os, path remain unchanged and linting passes.
- Around line 52-76: getMissingVersions currently performs each fetch
sequentially causing long runtimes; change it to perform the metadata fetches
with bounded concurrency (e.g., use p-limit or implement an N-at-a-time worker)
so many package lookups run in parallel but capped. Specifically, keep the
metadataByPackage cache logic and dedup by pkg.name, collect the unique package
names to fetch, then run fetch(`${PUBLIC_REGISTRY}/${encodePackageName(name)}`)
for those names with a concurrency limit (and set metadataByPackage to null on
404 or parsed JSON on success, throw on non-ok), and finally rebuild the missing
array using metadataByPackage as the existing code does. Ensure error handling
and the same return shape of getMissingVersions remain unchanged.
In `@Dockerfile`:
- Around line 28-40: The Dockerfile installer stage doesn't provision the
NPM_AUTH_TOKEN referenced by .yarnrc.yml (npmAuthToken: "${NPM_AUTH_TOKEN:-}")
so yarn install --immutable runs with an empty token; update the Dockerfile to
mount the secret into the build with --mount=type=secret (not ARG/ENV) and
consume it during the RUN yarn install step (pass the secret into the build
context so the npmAuthToken is available to yarn without adding the token to
image layers), ensuring the secret name matches NPM_AUTH_TOKEN used by
.yarnrc.yml and the RUN --mount=type=cache,... yarn install --immutable
--inline-builds command is updated to read that secret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc28a1ae-2be9-4085-b9dc-4502e64cddcb
📒 Files selected for processing (8)
.github/workflows/nodejs.condo.ci.yml.github/workflows/nodejs.condo.k6.yml.github/workflows/npm.prune.unpublished.yml.github/workflows/packages.ui.review.yml.github/workflows/packages.ui.sync-baseline.yml.yarnrc.ymlDockerfilebin/npm-prune-unpublished.js
| - name: Prune unpublished package versions | ||
| run: node bin/npm-prune-unpublished.js | ||
| env: | ||
| DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs.dry_run || 'false' }} | ||
| NPM_PUBLIC_REGISTRY: https://registry.npmjs.org | ||
| NPM_REGISTRY_SERVER: ${{ secrets.NPM_REGISTRY_SERVER }} | ||
| NPM_AUTH_TOKEN: ${{ secrets.NPM_AUTH_TOKEN }} |
There was a problem hiding this comment.
Confirm that a daily scheduled run should perform real npm unpublish calls.
With the current expression the cron (line 4-5) always resolves to DRY_RUN=false, so every day at 02:30 UTC this job will irreversibly unpublish every locked version not found on the public registry. That's fine if the private registry only mirrors public packages, but:
- Any package present only in the private registry (e.g. an
@open-condo/*release that was not yet mirrored publicly, or any truly private package that ever lands inyarn.lock) will be deleted — see related comment onbin/npm-prune-unpublished.js. - The
&& ... || 'false'expression is easy to misread. Consider either inverting to be explicit, or flipping the default to dry-run on schedule until confidence is built.
🔧 Safer / clearer default
- DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs.dry_run || 'false' }}
+ # dry_run defaults to true on manual runs; scheduled runs perform real unpublishes
+ DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs.dry_run }}(true/false stringification of a bool expression is well-defined; empty string for non-dispatch paths evaluates as dry-run via !== 'false' in the script.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Prune unpublished package versions | |
| run: node bin/npm-prune-unpublished.js | |
| env: | |
| DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs.dry_run || 'false' }} | |
| NPM_PUBLIC_REGISTRY: https://registry.npmjs.org | |
| NPM_REGISTRY_SERVER: ${{ secrets.NPM_REGISTRY_SERVER }} | |
| NPM_AUTH_TOKEN: ${{ secrets.NPM_AUTH_TOKEN }} | |
| - name: Prune unpublished package versions | |
| run: node bin/npm-prune-unpublished.js | |
| env: | |
| # dry_run defaults to true on manual runs; scheduled runs perform real unpublishes | |
| DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs.dry_run }} | |
| NPM_PUBLIC_REGISTRY: https://registry.npmjs.org | |
| NPM_REGISTRY_SERVER: ${{ secrets.NPM_REGISTRY_SERVER }} | |
| NPM_AUTH_TOKEN: ${{ secrets.NPM_AUTH_TOKEN }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/npm.prune.unpublished.yml around lines 31 - 37, The
DRY_RUN environment expression currently defaults scheduled runs to false
causing real npm unpublish actions; change the workflow so scheduled/cron
executions default to a dry-run instead of real unpublishes (e.g. make DRY_RUN
default to 'true' for non-workflow_dispatch events or invert the boolean logic),
and ensure this aligns with how bin/npm-prune-unpublished.js reads DRY_RUN (the
DRY_RUN env var and its truthy check). Update the DRY_RUN assignment in the job
env so manual dispatch can still pass a real value but scheduled runs remain
safe by default.
| function parseNpmResolution (resolution) { | ||
| const marker = '@npm:' | ||
| const markerIndex = resolution.lastIndexOf(marker) | ||
|
|
||
| if (markerIndex === -1) return null | ||
|
|
||
| const name = resolution.slice(0, markerIndex) | ||
| const version = resolution.slice(markerIndex + marker.length).split('::')[0] | ||
|
|
||
| if (!/^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z-.]+)?$/.test(version)) return null | ||
|
|
||
| return { name, version } | ||
| } |
There was a problem hiding this comment.
Semver regex rejects valid pre-release+build combinations.
The character class [0-9A-Za-z-.] does not include +, so versions with both prerelease and build metadata — e.g. 1.0.0-alpha.1+build.7 — fail the test at line 27 and are silently dropped from the pruning set. That's the safe direction (they'll never be unpublished), but it also means such versions will accumulate forever in the private registry even when legitimately gone upstream.
🔧 Proposed fix
- if (!/^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z-.]+)?$/.test(version)) return null
+ if (!/^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(version)) return null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseNpmResolution (resolution) { | |
| const marker = '@npm:' | |
| const markerIndex = resolution.lastIndexOf(marker) | |
| if (markerIndex === -1) return null | |
| const name = resolution.slice(0, markerIndex) | |
| const version = resolution.slice(markerIndex + marker.length).split('::')[0] | |
| if (!/^\d+\.\d+\.\d+(?:[-+][0-9A-Za-z-.]+)?$/.test(version)) return null | |
| return { name, version } | |
| } | |
| function parseNpmResolution (resolution) { | |
| const marker = '@npm:' | |
| const markerIndex = resolution.lastIndexOf(marker) | |
| if (markerIndex === -1) return null | |
| const name = resolution.slice(0, markerIndex) | |
| const version = resolution.slice(markerIndex + marker.length).split('::')[0] | |
| if (!/^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(version)) return null | |
| return { name, version } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/npm-prune-unpublished.js` around lines 18 - 30, The semver regex in
parseNpmResolution rejects versions that have both prerelease and build metadata
(e.g., 1.0.0-alpha.1+build.7); update the validation to allow an optional
prerelease segment followed by an optional build segment by replacing the single
combined group (?:[-+][0-9A-Za-z-.]+)? with two groups
(?:-[0-9A-Za-z-.]+)?(?:\+[0-9A-Za-z-.]+)?, keeping the same start/end anchors so
parseNpmResolution still returns null for truly invalid versions.
| async function getMissingVersions (packages) { | ||
| const metadataByPackage = new Map() | ||
| const missing = [] | ||
|
|
||
| for (const pkg of packages) { | ||
| if (!metadataByPackage.has(pkg.name)) { | ||
| const response = await fetch(`${PUBLIC_REGISTRY}/${encodePackageName(pkg.name)}`) | ||
|
|
||
| if (response.status === 404) { | ||
| metadataByPackage.set(pkg.name, null) | ||
| } else if (!response.ok) { | ||
| throw new Error(`Failed to fetch ${pkg.name} metadata from ${PUBLIC_REGISTRY}: ${response.status} ${response.statusText}`) | ||
| } else { | ||
| metadataByPackage.set(pkg.name, await response.json()) | ||
| } | ||
| } | ||
|
|
||
| const metadata = metadataByPackage.get(pkg.name) | ||
| if (!metadata || !metadata.versions || !metadata.versions[pkg.version]) { | ||
| missing.push(pkg) | ||
| } | ||
| } | ||
|
|
||
| return missing | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full file structure
fd npm-prune-unpublished.js --type fRepository: open-condo-software/condo
Length of output: 1884
🏁 Script executed:
# Read the full file to understand context and intent
cat -n bin/npm-prune-unpublished.jsRepository: open-condo-software/condo
Length of output: 5931
🏁 Script executed:
# Also check if there are any related configuration files or documentation
fd -e md -e json -e yaml | grep -i pruneRepository: open-condo-software/condo
Length of output: 51
This script will unconditionally delete any private package not found on the public npm registry, with no scope allowlist or safety threshold.
The getMissingVersions function marks a package as "missing" if the public registry returns 404 or if the specific version is absent from public metadata. The main loop then calls npm unpublish for every missing entry against NPM_REGISTRY_SERVER without any scope filter, allowlist, or confirmation mechanism.
Concrete failure modes:
- Any genuinely private package (e.g.,
@open-condo/*,@app/*, or scoped previews) that lives only in the private registry will be deleted on the next run, since it returns 404 on public npm. - There is no distinction between "intentionally unpublished upstream" and "first-party private code" — both appear identical from the public registry side.
Recommend restricting the unpublish set to known-safe scopes via an allowlist (e.g., an NPM_MIRRORED_SCOPES env var), and/or requiring a confirmation threshold (e.g., abort if missing.length > N) before deleting.
Separately: Import order at lines 1–4 violates the coding guidelines. Expected order (alphabetical within builtins): child_process, fs, os, path instead of fs, os, path, child_process.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/npm-prune-unpublished.js` around lines 52 - 76, getMissingVersions
currently treats any package absent from PUBLIC_REGISTRY as deletable which will
unconditionally unpublish private scoped packages; change the workflow so you
(1) add an allowlist check using an env var NPM_MIRRORED_SCOPES (comma-separated
scopes) and only consider packages whose scope matches that allowlist before
attempting to unpublish, (2) keep getMissingVersions (and its use of
PUBLIC_REGISTRY, encodePackageName, metadataByPackage, and the missing array)
purely for detection and move the safety checks to the main loop that performs
npm unpublish against NPM_REGISTRY_SERVER, (3) implement a confirmation
threshold via an env var MAX_MISSING_DELETE and abort with an error if
missing.length > MAX_MISSING_DELETE, and (4) re-order the imports at the top of
the file to follow guidelines: child_process, fs, os, path (alphabetical within
builtins).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nodejs.condo.ci.yml:
- Line 26: The workflow sets NPM_REGISTRY_SERVER from secrets unconditionally
which can become empty for external forks and override the Dockerfile ARG;
update the workflow so the build-arg is only provided when non-empty (e.g., use
a fallback or conditional so NPM_REGISTRY_SERVER is not passed as an empty
value), or remove the build-arg entirely and rely on the Dockerfile ARG
NPM_REGISTRY_SERVER default; alternatively adjust .yarnrc.yml's
npmRegistryServer handling to not treat an empty variable as set (use a
conditional that only applies the value when non-empty) — change references
around the workflow env NPM_REGISTRY_SERVER, the build-image job build-arg
usage, the Dockerfile ARG NPM_REGISTRY_SERVER, and .yarnrc.yml npmRegistryServer
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72a137f1-4b44-4baa-bfbf-2d208608803d
📒 Files selected for processing (3)
.github/workflows/nodejs.condo.ci.yml.yarnrc.ymlDockerfile
✅ Files skipped from review due to trivial changes (1)
- .yarnrc.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
| DOCKER_IMAGE: condo/condo-image:${{ github.event.pull_request.head.sha || github.sha }} | ||
| DOCKER_IMAGE_FULL: ${{ secrets.DOCKER_REGISTRY }}/condo/condo-image:${{ github.event.pull_request.head.sha || github.sha }} | ||
| CONDO_TEST_SHARD_TOTAL: 20 | ||
| NPM_REGISTRY_SERVER: ${{ secrets.NPM_REGISTRY_SERVER }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "nodejs.condo.ci.yml" -o -name "Dockerfile" -o -name ".yarnrc.yml" | head -20Repository: open-condo-software/condo
Length of output: 135
🏁 Script executed:
cat -n .github/workflows/nodejs.condo.ci.yml | head -120Repository: open-condo-software/condo
Length of output: 5113
🏁 Script executed:
find . -name "Dockerfile" | head -10Repository: open-condo-software/condo
Length of output: 81
🏁 Script executed:
find . -name ".yarnrc.yml" | head -10Repository: open-condo-software/condo
Length of output: 82
🏁 Script executed:
cat -n Dockerfile | grep -A 5 -B 5 "NPM_REGISTRY_SERVER"Repository: open-condo-software/condo
Length of output: 833
🏁 Script executed:
cat -n .yarnrc.ymlRepository: open-condo-software/condo
Length of output: 1014
NPM_REGISTRY_SERVER from undefined secret will override Dockerfile default with empty string, breaking yarn install in external PRs.
If secrets.NPM_REGISTRY_SERVER is not defined (common in external forks), the workflow-level env: at line 26 resolves to empty. The build-image job lacks an environment: declaration, so it reads repository-scoped secrets instead of environment-scoped ones. This results in env.NPM_REGISTRY_SERVER being empty, which line 97 passes as a build-arg NPM_REGISTRY_SERVER= — an empty string that overrides the Dockerfile's ARG NPM_REGISTRY_SERVER=https://registry.npmjs.org default.
The Dockerfile ENV then becomes empty, and .yarnrc.yml line 25 (npmRegistryServer: "${NPM_REGISTRY_SERVER:-https://registry.npmjs.org}") treats the empty string as a set variable, not unset, so the :- fallback does not apply. Yarn ends up with a blank registry and yarn install fails.
Fix:
- Only set the build-arg when
secrets.NPM_REGISTRY_SERVERis non-empty:NPM_REGISTRY_SERVER: ${{ secrets.NPM_REGISTRY_SERVER || 'https://registry.npmjs.org' }}at line 26, or - Omit the build-arg entirely and rely on the Dockerfile default, or
- Update
.yarnrc.ymlto use:+(only if set and non-empty) or leave the variable unset in the Dockerfile when empty.
Also verify that NPM_REGISTRY_SERVER and NPM_REGISTRY_AUTH_TOKEN secrets exist at repository scope or in an 'external' environment, so external PR builds access them correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nodejs.condo.ci.yml at line 26, The workflow sets
NPM_REGISTRY_SERVER from secrets unconditionally which can become empty for
external forks and override the Dockerfile ARG; update the workflow so the
build-arg is only provided when non-empty (e.g., use a fallback or conditional
so NPM_REGISTRY_SERVER is not passed as an empty value), or remove the build-arg
entirely and rely on the Dockerfile ARG NPM_REGISTRY_SERVER default;
alternatively adjust .yarnrc.yml's npmRegistryServer handling to not treat an
empty variable as set (use a conditional that only applies the value when
non-empty) — change references around the workflow env NPM_REGISTRY_SERVER, the
build-image job build-arg usage, the Dockerfile ARG NPM_REGISTRY_SERVER, and
.yarnrc.yml npmRegistryServer accordingly.
|
2 participants
Summary by CodeRabbit