[CFS] Add NPM_CONFIG_REGISTRY arg to docker publish - #11683
[CFS] Add NPM_CONFIG_REGISTRY arg to docker publish#11683Mike Harder (mikeharder) wants to merge 10 commits into
Conversation
|
No changes needing a change description found. |
|
You can try these changes here
|
Reintroduce NPM_CONFIG_REGISTRY argument after pnpm installation.
There was a problem hiding this comment.
Pull request overview
This PR updates the TypeSpec core Docker publish pipeline to better support npm/pnpm installs during image builds by introducing an authenticated npm configuration step and pinning the pnpm version used in the Docker build.
Changes:
- Add a pipeline step to create/authenticate an
.npmrcprior to running the Docker build. - Pin the pnpm version in
docker/Dockerfilebased on the repo’spackageManagerfield.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| eng/tsp-core/pipelines/publish.yml | Adds an authenticated .npmrc creation step before docker build. |
| docker/Dockerfile | Pins pnpm version to the repo-specified packageManager version before running pnpm install/build. |
Suppressed comments (1)
eng/tsp-core/pipelines/publish.yml:106
- The authenticated .npmrc created by
create-authenticated-npmrc.ymlis written to the agent user’s home directory, but the subsequentdocker builddoesn’t pass any registry/auth configuration into the build context. As a result, the Docker build won’t see the intended npm registry settings (and this also doesn’t match the PR’s stated goal of adding anNPM_CONFIG_REGISTRYargument). Consider either removing this step, or explicitly passing registry/auth intodocker build(e.g., using BuildKit--secret ...to mount$(resolvedNpmrcPath)duringRUN pnpm install, or passing a--build-arg NPM_CONFIG_REGISTRY=...and updating the Dockerfile to consume it).
- template: /eng/common/pipelines/templates/steps/create-authenticated-npmrc.yml
- script: |
docker build -f ./docker/Dockerfile \
-t $(imageName):latest \
.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # Install exact version of pnpm to avoid problems with private registries | ||
| RUN npm install -g $(node -p "require('./package.json').packageManager") | ||
|
|
||
| RUN pnpm install --filter "@typespec/compiler..." | ||
| RUN pnpm --filter "@typespec/compiler..." run build |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
docker/Dockerfile:10
- Creating an authenticated
./docker/.npmrcin the pipeline and thenCOPY . /appin the builder stage (plusNPM_CONFIG_USERCONFIG=/app/docker/.npmrc) bakes registry credentials into a Docker layer in thebuilderstage. Even though the final stage doesn’t copy it, the secret can still persist in local build cache or be exposed if the stage is ever exported/debugged.
Consider switching to a BuildKit secret mount for the .npmrc (and .dockerignore it) or otherwise avoid copying the authenticated .npmrc into any image layer.
ENV NPM_CONFIG_USERCONFIG=/app/docker/.npmrc
# Upgrade all packages per https://eng.ms/docs/more/containers-secure-supply-chain/updating.
| - template: /eng/common/pipelines/templates/steps/create-authenticated-npmrc.yml | ||
| parameters: | ||
| npmrcPath: ./docker/.npmrc | ||
|
|
||
| - script: | |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
eng/tsp-core/pipelines/publish.yml:106
- The pipeline creates an authenticated
docker/.npmrcand the Dockerfile doesCOPY . /app, so the auth token written bynpmAuthenticate@0can end up in the Docker build context and in the builder-stage image layers. Even though the final stage doesn’t copy it, this is still a secret-handling risk. Prefer BuildKit secrets (docker build --secret …+RUN --mount=type=secret) or otherwise ensure the generated.npmrcis excluded from the build context (e.g., create it outside.and/or add it to.dockerignore).
- template: /eng/common/pipelines/templates/steps/create-authenticated-npmrc.yml
parameters:
npmrcPath: ./docker/.npmrc
- script: |
docker build -f ./docker/Dockerfile \
docker/Dockerfile:9
ENV NPM_CONFIG_USERCONFIG=/app/docker/.npmrccombined withCOPY . /appmeans an authenticated.npmrcgenerated during CI can be captured in the builder-stage filesystem/layers. Consider switching to BuildKit secret mounts for npm config (so tokens never enter the image layers) and keep the generated.npmrcout of the build context.
COPY . /app
ENV NPM_CONFIG_USERCONFIG=/app/docker/.npmrc
docker/Dockerfile:9
- PR title mentions adding an
NPM_CONFIG_REGISTRYarg, but the change actually introduces a generated authenticated.npmrcplusNPM_CONFIG_USERCONFIG. If the intent is only to make Docker publishing work with the Azure Artifacts registry, consider updating the PR title/description to match the implemented approach (or implement the build-arg approach described by the title).
ENV NPM_CONFIG_USERCONFIG=/app/docker/.npmrc
…spec into mikeharder/docker
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docker/Dockerfile:37
- The authenticated
.npmrccreated bynpmAuthenticate@0contains credentials.COPYing it into the final stage bakes those credentials into the image layer history (even if the file is later deleted in a RUN step). This can leak feed tokens to anyone who can pull the image. Consider switching to Docker BuildKit secrets (e.g.,docker build --secret ...+RUN --mount=type=secret ...) or performing the npm install in the builder stage and copying only the installed artifacts into the final image.
COPY --from=builder /app/docker/.npmrc /tmp/.npmrc
ENV NPM_CONFIG_USERCONFIG=/tmp/.npmrc
RUN npm install --verbose -g /tmp/compiler.tgz && rm /tmp/compiler.tgz && rm /tmp/.npmrc
eng/tsp-core/pipelines/publish.yml:108
- This creates an authenticated
.npmrc(vianpmAuthenticate@0) inside the Docker build context (./docker/.npmrc) and the Dockerfile then COPYs it into the image. That combination risks publishing registry credentials in the image layer history. Prefer using Docker BuildKit secrets (DOCKER_BUILDKIT=1 docker build --secret id=npmrc,src=...) and updating the Dockerfile to read the npmrc viaRUN --mount=type=secretso the token never enters the image filesystem/layers.
- template: /eng/common/pipelines/templates/steps/create-authenticated-npmrc.yml
parameters:
npmrcPath: ./docker/.npmrc
- script: |
docker build -f ./docker/Dockerfile \
-t $(imageName):latest \
.
docker/Dockerfile:37
ENV NPM_CONFIG_USERCONFIG=/tmp/.npmrcis set in the final image but the file is deleted in the same layer. This leaves the runtime container with an env var pointing at a non-existent config file, which can cause confusing npm/pnpm behavior for users (or any future npm usage in the image). Prefer scoping the config to the install command instead of persisting it as an ENV.
This issue also appears on line 34 of the same file.
COPY --from=builder /app/docker/.npmrc /tmp/.npmrc
ENV NPM_CONFIG_USERCONFIG=/tmp/.npmrc
RUN npm install --verbose -g /tmp/compiler.tgz && rm /tmp/compiler.tgz && rm /tmp/.npmrc
No description provided.