You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "bikash-barnwal (via GitHub)" <gi...@apache.org> on 2026/08/14 15:14:43 UTC
[PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
bikash-barnwal opened a new pull request, #43168:
URL: https://github.com/apache/superset/pull/43168
### SUMMARY
`docker/docker-frontend.sh` installed the frontend with a bare `npm install`. When `package.json` and `package-lock.json` disagree, `npm install` re-resolves versions and rewrites the lockfile, so a dev image can be built against dependency versions that were never reviewed — and the repository has no `.npmrc` cooldown backing the lockfile up.
This switches that install to `npm ci`, which installs exactly what the lockfile pins and fails loudly when the lockfile is out of sync with `package.json`. The `Dockerfile` already uses `npm ci` for the equivalent install, so the two paths are now consistent.
Trade-off worth noting for reviewers: `npm ci` removes `node_modules` before installing, so with the dev bind mount the first container start after this change reinstalls from scratch. `NPM_RUN_PRUNE` (the existing knob for that directory) is unaffected.
Fixes #42979
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — build script change.
### TESTING INSTRUCTIONS
1. Set `BUILD_SUPERSET_FRONTEND_IN_DOCKER=true` in `docker/.env-local`.
2. `docker compose up --detach`.
3. The frontend container logs `Running "npm ci"` and the dev server comes up on http://localhost:9000.
4. To see the guard work, edit a dependency version in `superset-frontend/package.json` without updating the lockfile and restart: the install now fails with npm's lockfile-out-of-sync error instead of silently resolving a different version.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #42979
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43168:
URL: https://github.com/apache/superset/pull/43168#issuecomment-5294998845
<!-- Bito Reply -->
The flagged issue is correct. Running `npm ci` inside a container that bind-mounts the host's `node_modules` directory will delete and recreate the directory, which can lead to permission issues (root ownership) and binary incompatibility with the host OS. To resolve this, you should mount a separate volume for `node_modules` in your Docker Compose configuration to isolate the container's dependencies from the host's filesystem.
To implement this, update your `docker-compose.yml` to include a volume mapping for the frontend dependencies:
```yaml
services:
superset-app:
volumes:
- ./superset-frontend:/app/superset-frontend
- /app/superset-frontend/node_modules
```
This ensures that `npm ci` operates within an isolated volume rather than modifying the host's directory. Regarding other comments on this PR, the `pr_comments.csv` file is currently empty, so there are no additional comments to address.
**docker-compose.yml**
```
services:
superset-app:
volumes:
- ./superset-frontend:/app/superset-frontend
- /app/superset-frontend/node_modules
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43168:
URL: https://github.com/apache/superset/pull/43168#issuecomment-5606394680
@hainenber you might have opinions on this. Feel free to merge if you don't see any wrinkles/problems I'm not spotting :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43168:
URL: https://github.com/apache/superset/pull/43168#discussion_r3785008638
##########
docker/docker-frontend.sh:
##########
@@ -32,8 +32,10 @@ if [ "$BUILD_SUPERSET_FRONTEND_IN_DOCKER" = "true" ]; then
npm run prune
fi
- echo "Running \"npm install\""
- npm install
+ # Install from the committed lockfile so a dev image build resolves the same
+ # versions that were reviewed, matching the `npm ci` used in the Dockerfile.
+ echo "Running \"npm ci\""
+ npm ci
Review Comment:
**Suggestion:** Because `/app/superset-frontend` is a host bind mount and the compose files do not provide a separate `node_modules` volume, `npm ci` deletes and recreates the host's `superset-frontend/node_modules` directory. Running the container can therefore replace host-installed modules, including OS-specific native binaries, and may leave the host tree owned by the container's root user. Mount an anonymous or named volume at `/app/superset-frontend/node_modules` before running the install. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Host frontend dependencies are deleted on container startup.
- ⚠️ Native modules may become incompatible with the host OS.
- ⚠️ Root-owned files can impede host-side development.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=41ed03cec54341e5aa900a0a40ba8754&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=41ed03cec54341e5aa900a0a40ba8754&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docker/docker-frontend.sh
**Line:** 37:38
**Comment:**
*Stale Reference: Because `/app/superset-frontend` is a host bind mount and the compose files do not provide a separate `node_modules` volume, `npm ci` deletes and recreates the host's `superset-frontend/node_modules` directory. Running the container can therefore replace host-installed modules, including OS-specific native binaries, and may leave the host tree owned by the container's root user. Mount an anonymous or named volume at `/app/superset-frontend/node_modules` before running the install.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43168&comment_hash=48edf92d63b65b8ef096124029c92567b0ff41b06b3398bd59bb4f0decbca597&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43168&comment_hash=48edf92d63b65b8ef096124029c92567b0ff41b06b3398bd59bb4f0decbca597&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on PR #43168:
URL: https://github.com/apache/superset/pull/43168#issuecomment-5301102562
@bikash-barnwal I do have a concern in the linked issue. Can we please address that before any action? It's indeed that the docker-build steps are now ~1 minutes longer than usual builds, which can increase the project's resource utilization due to economy of scales on human and bot's PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43168:
URL: https://github.com/apache/superset/pull/43168#issuecomment-5294952622
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "bikash-barnwal (via GitHub)" <gi...@apache.org>.
bikash-barnwal commented on PR #43168:
URL: https://github.com/apache/superset/pull/43168#issuecomment-5550370297
Fixed — superset-node bind-mounts the whole `superset-frontend` dir straight from the host with no node_modules exclusion (unlike `superset-websocket`, which already masks node_modules and dist the same way), and has no `user: *superset-user` override, so it runs as the image's default root user. `npm ci`'s delete+recreate was landing directly on the host copy and leaving it root-owned. Added the same anonymous-volume mask superset-websocket uses. Verified with `docker compose config` that it resolves to a container-managed volume over `/app/superset-frontend/node_modules`, same as the websocket service.
@hainenber as a side effect this should also help the build-time concern: the volume persists across `docker compose up` restarts, so after the first run `npm ci` reinstalls into an already-populated volume instead of a bind mount it just emptied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #43168:
URL: https://github.com/apache/superset/pull/43168
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43168:
URL: https://github.com/apache/superset/pull/43168#issuecomment-5550379818
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 3cf0711ff1beb57d57bf1b81fcd3b164adf8d988 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a9bc84947e01a000883c67d |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43168--superset-docs-preview.netlify.app](https://deploy-preview-43168--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgEADQu8w6yUj8dIOOoTNKUn4QF0V099y9xXuhtpJqhw1gAtdsxmM4xHumgqdvLLSSu8_Eck0M41jk61WencaT1jjUwgjl-WI551qvKBXB9wM) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43168--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(docker): install the frontend from the lockfile in docker-frontend.sh [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43168:
URL: https://github.com/apache/superset/pull/43168#issuecomment-5310849035
Codeant's thread here seems accurate. `superset-node` runs as root with no `node_modules` volume like `superset-websocket` has, so `npm ci` would wipe the host's bind-mounted `node_modules` and leave it root-owned. Worth fixing before merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org