fix(ci): configure pnpm to use authenticated registry#27699
fix(ci): configure pnpm to use authenticated registry#27699MarioJGMsoft wants to merge 2 commits into
Conversation
Set PNPM_CONFIG_USERCONFIG to the authenticated Azure Artifacts npmrc so pnpm 11 registry commands do not fall back to the public npm registry. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 34c6e107-8597-4cc1-bae3-00a842a57870
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (1 lines, 1 files), I've queued these reviewers:
How this works
|
| set -eu -o pipefail | ||
| echo "Using node $(node --version)" | ||
| echo "##vso[task.setvariable variable=NPM_CONFIG_USERCONFIG]$NPM_CONFIG_USERCONFIG" | ||
| echo "##vso[task.setvariable variable=PNPM_CONFIG_USERCONFIG]$NPM_CONFIG_USERCONFIG" |
There was a problem hiding this comment.
I don't think this belongs here.
I see two reasons:
- This part of this pipeline is setting up npm so we can use npm to install pnpm. See the "Install and configure pnpm" section for where it would go.
- I'm pretty sure pnpm is picking up the existing npm setting already, since it wasn't, our
pnpm installsteps would already be failing to run under networks isolation. Maybe I'm missing something, but I think we have had such enforcement for a long time now, and this has been working as is.
There was a problem hiding this comment.
Another concerning thing looking at this change is we also set NPM_CONFIG_USERCONFIG in tools/pipelines/templates/include-setup-npmrc-for-download.yml : we should not be doing this in multiple places. If a change is need to this copy of the logic, one is likely needed there as well: we should deduplicate them first.
Also note there is additional logic to plumb NPM_CONFIG_USERCONFIG into our docker images, see its use in NPM_CONFIG_USERCONFIG
If there is some other special value we need for this for pnpm reasons, it likely would have to do something inside the docker image to handle it as well.
| set -eu -o pipefail | ||
| echo "Using node $(node --version)" | ||
| echo "##vso[task.setvariable variable=NPM_CONFIG_USERCONFIG]$NPM_CONFIG_USERCONFIG" | ||
| echo "##vso[task.setvariable variable=PNPM_CONFIG_USERCONFIG]$NPM_CONFIG_USERCONFIG" |
There was a problem hiding this comment.
Where is PNPM_CONFIG_USERCONFIG documented to be a thing pnpm looks at? I don't see it in its docs: https://pnpm.io/npmrc#auth-file-locations
There was a problem hiding this comment.
Those docs do state The npm-style NPM_CONFIG_USERCONFIG variable is also honored as a fallback. which implies NPM_CONFIG_USERCONFIG should just work (like think it has been), but nothing in there implies PNPM_CONFIG_USERCONFIG is a thing that has any impact.
PNPM_CONFIG_USERCONFIG is mentioned in the implementation of pnpm, just not in their docs (which is annoying):
https://github.com/search?q=repo%3Apnpm%2Fpnpm%20PNPM_CONFIG_USERCONFIG%20&type=code
The docs in https://github.com/pnpm/pnpm/blob/fb4639f8d2885da64b79cf97d385af97004b65b4/pnpm/crates/config/src/lib.rs#L1125-L1126 imply PNPM_CONFIG_NPMRC_AUTH_FILE / PNPM_CONFIG_USERCONFIG / npm_config_userconfig all do the same thing though thats from a future version of pnpm.
Anyway, I conclude I don't have enough information to know what's going on.
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. Per-bundle deltas
|
Description
The client pipeline already configures an artifct for npm. However, pnpm 11 does not automatically adopt that configuration through
NPM_CONFIG_USERCONFIG.Set
PNPM_CONFIG_USERCONFIGso pnpm 11 adopts the pipeline's existing configuration. This ensures pnpm registry commands, including registry-backed version utility tests, use the appropriate configurationReviewer Guidance
The review process is outlined on this wiki page.
Fixes AB#73158