Revert "Merge pull request #163 from blooop/claude/npm-pixi-installat… #167
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideSwitches the npm extension from pixi-based installation to an nvm (Node Version Manager)–based installation, wires npm-dependent extensions (codex, gemini) to use the new global npm, and simplifies their test scripts to use a standard non-login shell while removing now-obsolete pixi/npm user snippet and spec files. Class diagram for updated Npm extension configurationclassDiagram
class SimpleRockerExtension
class Npm {
+string name
+tuple depends_on_extension
+list builder_apt_packages
+dict empy_args
}
SimpleRockerExtension <|-- Npm
Npm : name = npm
Npm : depends_on_extension = (curl)
Npm : builder_apt_packages = [curl, ca-certificates, git]
Npm : empy_args[NODE_VERSION] = 24.9.0
Npm : empy_args[NPM_VERSION] = 11.6.1
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The nvm clone step in
npm_builder_snippet.Dockerfileusesgit fetch --tags && git checkout v0.40.3in the cached branch andgit checkout v0.40.0in the fresh-clone branch; these should be made consistent to avoid nondeterministic behavior between builds. - Node/NPM versions are defined both in
Npm.empy_argsand hard-coded asENV NODE_VERSION=24.9.0innpm_snippet.Dockerfile; consider wiring the Dockerfile snippet to use the template variables to prevent version drift. - Given that
builder_apt_packagesfor the npm extension already includesgit, you may be able to drop the explicitapt-get install gitinnpm_builder_snippet.Dockerfileto avoid duplication unless there is a build-stage–specific reason for re-installing it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The nvm clone step in `npm_builder_snippet.Dockerfile` uses `git fetch --tags && git checkout v0.40.3` in the cached branch and `git checkout v0.40.0` in the fresh-clone branch; these should be made consistent to avoid nondeterministic behavior between builds.
- Node/NPM versions are defined both in `Npm.empy_args` and hard-coded as `ENV NODE_VERSION=24.9.0` in `npm_snippet.Dockerfile`; consider wiring the Dockerfile snippet to use the template variables to prevent version drift.
- Given that `builder_apt_packages` for the npm extension already includes `git`, you may be able to drop the explicit `apt-get install git` in `npm_builder_snippet.Dockerfile` to avoid duplication unless there is a build-stage–specific reason for re-installing it.
## Individual Comments
### Comment 1
<location> `deps_rocker/extensions/npm/npm.py:11-13` </location>
<code_context>
+ depends_on_extension = ("curl",)
+ builder_apt_packages = ["curl", "ca-certificates", "git"]
+
+ empy_args = {
+ "NODE_VERSION": "24.9.0",
+ "NPM_VERSION": "11.6.1",
+ }
</code_context>
<issue_to_address>
**suggestion:** Clarify usage of `NPM_VERSION` and keep node/npm versions sourced from a single place.
`NPM_VERSION` is defined but not used in the Dockerfiles, so npm will default to the version bundled with Node. If you intend to pin npm, please pass this through to the Dockerfiles (e.g., `npm install -g npm@${NPM_VERSION}`); otherwise, remove `NPM_VERSION` to avoid confusion. Likewise, `NODE_VERSION` is defined here but the runtime snippet hardcodes `ENV NODE_VERSION=24.9.0`; it would be more robust to source that value from `empy_args` to prevent version drift.
Suggested implementation:
```python
class Npm(SimpleRockerExtension):
"""Install Node and npm using nvm (Node Version Manager).
The Node and npm versions are controlled via ``empy_args`` and should be
treated as the single source of truth for the Docker templates.
"""
name = "npm"
depends_on_extension = ("curl",)
builder_apt_packages = ["curl", "ca-certificates", "git"]
# These values are consumed by the builder/runtime Dockerfile templates.
# Do not hardcode Node/npm versions in the templates; instead, reference
# these variables (e.g. NODE_VERSION, NPM_VERSION).
empy_args = {
"NODE_VERSION": "24.9.0",
"NPM_VERSION": "11.6.1",
}
```
To fully implement your review comment, the following changes are also needed in the Docker/empy templates (filenames may differ slightly in your repo):
1. In the *runtime* Docker snippet for this extension (e.g. `deps_rocker/extensions/npm/Dockerfile.npm.runtime.em`):
- Replace any hardcoded `ENV NODE_VERSION=24.9.0` (or similar) with a reference to the `NODE_VERSION` template variable, e.g.:
- `ENV NODE_VERSION=@NODE_VERSION@` or `${NODE_VERSION}` depending on your existing empy/template style.
- Ensure the Node install step uses that template variable instead of a literal version.
2. If you intend to pin npm:
- In the same runtime/builder Docker template, after Node is installed, add:
- `RUN npm install -g npm@${NPM_VERSION}` (or the equivalent in your template language, referencing the `NPM_VERSION` variable from `empy_args`).
- If you choose *not* to pin npm, remove `"NPM_VERSION": "11.6.1",` from `empy_args` above instead, and update any references accordingly.
3. Verify that whatever mechanism wires `empy_args` into the Docker/empy templates correctly exposes `NODE_VERSION` and `NPM_VERSION` variables; if not, adjust that wiring so the templates see these values.
</issue_to_address>
### Comment 2
<location> `deps_rocker/extensions/npm/npm_builder_snippet.Dockerfile:18-21` </location>
<code_context>
+# Clone nvm from git with caching
+RUN --mount=type=cache,target=/tmp/nvm-git-cache,id=nvm-git-cache \
+ bash -c "set -e && \
+ if [ -d /tmp/nvm-git-cache/.git ]; then \
+ cd /tmp/nvm-git-cache && git fetch --tags && git checkout v0.40.3; \
+ else \
+ git clone https://github.com/nvm-sh/nvm.git /tmp/nvm-git-cache && \
+ cd /tmp/nvm-git-cache && git checkout v0.40.0; \
+ fi && \
</code_context>
<issue_to_address>
**issue (bug_risk):** Align the nvm tag between the cache-hit and fresh-clone paths.
Using v0.40.3 in the cache-hit path and v0.40.0 in the fresh-clone path means builds can produce different environments depending on cache state. Please use the same tag in both branches, ideally defined once (e.g., as a build arg or variable) to prevent future drift.
</issue_to_address>
### Comment 3
<location> `deps_rocker/extensions/npm/npm_snippet.Dockerfile:2-4` </location>
<code_context>
+
+ENV NODE_VERSION=24.9.0
+# Install nvm, node and npm
+ENV NVM_DIR=/usr/local/nvm
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid hardcoding NODE_VERSION in the runtime snippet to prevent drift from the builder configuration.
The builder uses `ARG NODE_VERSION=@NODE_VERSION@` with `NODE_VERSION` defined in `empy_args`, but this runtime snippet hardcodes `ENV NODE_VERSION=24.9.0`. If the version is updated in `empy_args` but not here, PATH will reference the wrong Node version. Please derive this `ENV` from the same `NODE_VERSION` value (e.g., via empy substitution) so there’s a single source of truth.
```suggestion
ENV NODE_VERSION=@NODE_VERSION@
# Install nvm, node and npm
ENV NVM_DIR=/usr/local/nvm
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+18
to
+21
| if [ -d /tmp/nvm-git-cache/.git ]; then \ | ||
| cd /tmp/nvm-git-cache && git fetch --tags && git checkout v0.40.3; \ | ||
| else \ | ||
| git clone https://github.com/nvm-sh/nvm.git /tmp/nvm-git-cache && \ |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Align the nvm tag between the cache-hit and fresh-clone paths.
Using v0.40.3 in the cache-hit path and v0.40.0 in the fresh-clone path means builds can produce different environments depending on cache state. Please use the same tag in both branches, ideally defined once (e.g., as a build arg or variable) to prevent future drift.
Comment on lines
+2
to
+4
| ENV NODE_VERSION=24.9.0 | ||
| # Install nvm, node and npm | ||
| ENV NVM_DIR=/usr/local/nvm |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Avoid hardcoding NODE_VERSION in the runtime snippet to prevent drift from the builder configuration.
The builder uses ARG NODE_VERSION=@NODE_VERSION@ with NODE_VERSION defined in empy_args, but this runtime snippet hardcodes ENV NODE_VERSION=24.9.0. If the version is updated in empy_args but not here, PATH will reference the wrong Node version. Please derive this ENV from the same NODE_VERSION value (e.g., via empy substitution) so there’s a single source of truth.
Suggested change
| ENV NODE_VERSION=24.9.0 | |
| # Install nvm, node and npm | |
| ENV NVM_DIR=/usr/local/nvm | |
| ENV NODE_VERSION=@NODE_VERSION@ | |
| # Install nvm, node and npm | |
| ENV NVM_DIR=/usr/local/nvm |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
+ Coverage 66.63% 66.69% +0.05%
==========================================
Files 41 41
Lines 1232 1234 +2
==========================================
+ Hits 821 823 +2
Misses 411 411
🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…ion-GvBL8"
This reverts commit 2896319, reversing changes made to 38bc59a.
Summary by Sourcery
Switch npm-related extensions from pixi-based installation to an nvm- and npm-based Dockerfile workflow and clean up obsolete artifacts.
New Features:
Enhancements:
Chores: