refactor: update devcontainer #2025
Conversation
WalkthroughA new Dockerfile was added to the Changes
Estimated code review effort2 (30–90 minutes) Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.devcontainer/devcontainer.json (3)
70-71: Mounting host $HOME over container $HOME may clobber dot-files
Binding the entire host home directory onto/home/ubuntuwipes whatever the Dockerfile (or features) placed there and risks leaking local secrets (SSH keys, git creds) into the container image cache. Unless you absolutely need the whole home, mount only the specific sub-paths you care about (e.g.,.ssh,.gitconfig).
43-45: Interpreter path assumes a pre-created venv
/home/ubuntu/.venv/bin/pythonwill break on first open if the venv isn’t created in the Dockerfile or apostCreateCommand. Either create the venv during image build/post-create or switch to the system Python.
63-64:postCreateCommandcommented out – build/install steps lost
The old image ran a post-create script to compile Rust and pip-install packages. With it disabled the workspace may open half-configured. Confirm that these steps are no longer needed or re-enable the command with the new path.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.devcontainer/Dockerfile(1 hunks).devcontainer/devcontainer.json(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
.devcontainer/devcontainer.json (1)
Learnt from: julienmancuso
PR: #1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1302-1306
Timestamp: 2025-06-11T21:18:00.425Z
Learning: In the Dynamo operator, the project’s preferred security posture is to set a Pod-level PodSecurityContext with runAsUser, runAsGroup, and fsGroup all set to 1000, and then selectively override the user at the individual container level (e.g., RunAsUser: 0 for Kaniko) when root is required.
| "features": { | ||
| "ghcr.io/devcontainers/features/common-utils:2": { | ||
| "userUid": "${localEnv:UID-1000}", | ||
| "userGid": "${localEnv:UID-1000}", | ||
| "upgradePackages": "false" | ||
| } |
There was a problem hiding this comment.
Malformed feature options – UID env var & boolean type
"${localEnv:UID-1000}" is not a valid environment-variable key (dash not allowed). This will resolve to an empty string and the feature will fail.
Also upgradePackages expects a JSONC boolean, not a string.
- "userUid": "${localEnv:UID-1000}",
- "userGid": "${localEnv:UID-1000}",
- "upgradePackages": "false"
+ "userUid": "${localEnv:UID}",
+ "userGid": "${localEnv:UID}",
+ "upgradePackages": false📝 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.
| "features": { | |
| "ghcr.io/devcontainers/features/common-utils:2": { | |
| "userUid": "${localEnv:UID-1000}", | |
| "userGid": "${localEnv:UID-1000}", | |
| "upgradePackages": "false" | |
| } | |
| "features": { | |
| "ghcr.io/devcontainers/features/common-utils:2": { | |
| "userUid": "${localEnv:UID}", | |
| "userGid": "${localEnv:UID}", | |
| "upgradePackages": false | |
| } |
🤖 Prompt for AI Agents
In .devcontainer/devcontainer.json around lines 72 to 77, the userUid and
userGid options use an invalid environment variable key with a dash, causing
them to resolve to empty strings; replace "${localEnv:UID-1000}" with a valid
key like "${localEnv:UID_1000}" or another valid environment variable without
dashes. Also, change the upgradePackages value from the string "false" to the
boolean false without quotes to match the expected JSONC boolean type.
| RUN apt-get update && \ | ||
| apt-get install -y sudo wget lsb-release software-properties-common gnupg ca-certificates && \ | ||
| # Install latest stable LLVM using official script | ||
| bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)" && \ | ||
| apt-get clean && \ | ||
| rm -rf /var/lib/apt/lists/* && \ | ||
| # Add all users to sudo group and enable passwordless sudo | ||
| echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Slim down the image and harden sudo setup
- Add
--no-install-recommendsto theapt-get installline – it cuts bloat by skipping docs and ancillary packages. - The comment says “Add all users to sudo group” but in reality you only relax
/etc/sudoers; if theubuntuuser (or any other) is not already in thesudogroup it will still lack sudo rights. Either ensure the user is added (usermod -aG sudo ubuntu) or re-phrase the comment. - Fetching a remote install script (
llvm.sh) at build-time without pinning a checksum makes reproducibility brittle. At minimum, pin the expected SHA256 or an explicit LLVM version (LLVM_VERSION=17 bash …) so future upstream changes don’t silently break the build.
- apt-get install -y sudo wget lsb-release software-properties-common gnupg ca-certificates && \
+ apt-get install -y --no-install-recommends \
+ sudo wget lsb-release software-properties-common gnupg ca-certificates && \
+ usermod -aG sudo ubuntu && \📝 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.
| RUN apt-get update && \ | |
| apt-get install -y sudo wget lsb-release software-properties-common gnupg ca-certificates && \ | |
| # Install latest stable LLVM using official script | |
| bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)" && \ | |
| apt-get clean && \ | |
| rm -rf /var/lib/apt/lists/* && \ | |
| # Add all users to sudo group and enable passwordless sudo | |
| echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers | |
| RUN apt-get update && \ | |
| apt-get install -y --no-install-recommends \ | |
| sudo wget lsb-release software-properties-common gnupg ca-certificates && \ | |
| usermod -aG sudo ubuntu && \ | |
| # Install latest stable LLVM using official script | |
| bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)" && \ | |
| apt-get clean && \ | |
| rm -rf /var/lib/apt/lists/* && \ | |
| # Add all users to sudo group and enable passwordless sudo | |
| echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers |
🤖 Prompt for AI Agents
In .devcontainer/Dockerfile lines 4 to 11, add --no-install-recommends to the
apt-get install command to reduce image size by skipping unnecessary packages.
Ensure the user (e.g., ubuntu) is added to the sudo group with usermod -aG sudo
ubuntu or update the comment to accurately reflect that only sudoers file is
modified. Also, modify the llvm.sh installation to specify an explicit LLVM
version or verify the script's SHA256 checksum to improve build reproducibility
and security.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR has been closed due to inactivity. If you believe this PR is still relevant, please feel free to reopen it with additional context or information. |
Summary by CodeRabbit