add init container to prepare workspace pod#184
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a workspace init container that prepares the PVC (dirs, SSH key, optional Jumpstarter client config) and unifies capability selection; the main container command/workingDir is set only for the configured toolchain image. Also updates an SCC-related error message and adds tests for SecurityContext behavior. Changes
Sequence DiagramsequenceDiagram
participant Pod as Pod Creation
participant Init as workspace-init (Init Container)
participant PVC as PVC (/workspace)
participant Main as toolchain (Main Container)
participant JS as Jumpstarter Client
Pod->>Init: start init container
Init->>PVC: create dirs, set permissions
Init->>PVC: generate Ed25519 SSH key if missing
Init->>PVC: copy /jumpstarter/client.yaml (if mounted)
Init->>JS: run jumpstarter client config (best-effort)
Init-->>Pod: exit successfully
Pod->>Main: start main container (entrypoint depends on image)
Main->>PVC: use prepared workspace
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/workspace/controller.go`:
- Around line 340-343: The copied client.yaml is placed at
/workspace/.config/jumpstarter/clients/workspace.yaml but the activation step is
missing; after the cp in the init path ensure you run the Jumpstarter activation
command (jmp config client use workspace) so the copied client becomes the
active client (replicate what hack/workspace-entrypoint.sh did previously) —
update the init logic to execute the activation command (or the equivalent
config toggle) immediately after creating/copying workspace.yaml.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dfe7b237-058b-41ed-abca-b5b139882cc6
📒 Files selected for processing (1)
internal/controller/workspace/controller.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/workspace/controller_test.go (1)
500-500: Guard container indexing to avoid panic in test failures.Directly indexing
pod.Spec.Containers[0]can panic and obscure the root cause when pod construction regresses.Suggested hardening
- secCtx := pod.Spec.Containers[0].SecurityContext + if len(pod.Spec.Containers) == 0 { + t.Fatal("expected at least one container in pod spec") + } + secCtx := pod.Spec.Containers[0].SecurityContext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/workspace/controller_test.go` at line 500, The test directly indexes pod.Spec.Containers[0], which can panic if the pod has no containers; update the test around where secCtx is set to first verify containers exist (e.g. check len(pod.Spec.Containers) > 0) and fail the test with a clear message if not, then safely assign secCtx := pod.Spec.Containers[0].SecurityContext; this guards the test from panics and makes failures clearer (referencing pod.Spec.Containers and secCtx).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/workspace/controller_test.go`:
- Line 500: The test directly indexes pod.Spec.Containers[0], which can panic if
the pod has no containers; update the test around where secCtx is set to first
verify containers exist (e.g. check len(pod.Spec.Containers) > 0) and fail the
test with a clear message if not, then safely assign secCtx :=
pod.Spec.Containers[0].SecurityContext; this guards the test from panics and
makes failures clearer (referencing pod.Spec.Containers and secCtx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55f7f920-1eba-452d-9841-f2c93adb812d
📒 Files selected for processing (3)
internal/controller/operatorconfig/controller.gointernal/controller/workspace/controller.gointernal/controller/workspace/controller_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/controller/operatorconfig/controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/workspace/controller.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/common/tasks/scripts/build_image.sh (1)
391-467:⚠️ Potential issue | 🔴 CriticalDon't let the split-build path publish a container after a later failure.
Line 460 starts the external
docker://$CONTAINER_PUSHwrite in the background, but Line 474 (aib to-disk-image) and Line 701 (artifact compression) can still fail afterward. That means a failed Task can still leave a published container behind, and the final progress/result updates are emitted before the push outcome is known. Keep the local OCI export/annotation async if you want the overlap, but move the final registryskopeo copybehind the success boundary, or push to a staging tag and only promote it once the step is otherwise complete.Also applies to: 470-482, 699-701, 730-742
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` around lines 391 - 467, The background subshell currently performs the final registry push ("skopeo copy ... docker://$CONTAINER_PUSH") which can publish a container even if later steps fail; preserve the async OCI export/annotation (use symbols PUSH_SRC, OCI_DIR, AIB_METADATA_PID, ANNOTATE_DONE) but remove the remote push from that subshell and instead wait for the task success boundary before performing the final push. Concretely: let the subshell only create and annotate the OCI layout and exit (exporting OCI_DIR and new manifest info), keep CONTAINER_PUSH_PID for the background work that completes early, then after the disk-image and compression steps succeed, perform the registry upload (use CONTAINER_PUSH or a staging tag and promote it) synchronously and only on success; ensure any temporary OCI_DIR cleanup happens after the registry push and that failures do not trigger publishing.
🧹 Nitpick comments (2)
internal/controller/workspace/controller.go (1)
320-342: Consider sharing the workspace bootstrap code instead of embedding a second copy.
initScriptnow overlaps withhack/workspace-entrypoint.sh:1-56on directory creation, SSH key generation, and Jumpstarter setup. Extracting a shared script, or adding an init-only mode to the existing entrypoint, would make these two paths much harder to drift apart.internal/common/tasks/scripts/build_image.sh (1)
263-278: Use the configured auth file when pinning the AIB image reference.Line 267 does an unauthenticated
skopeo inspect, even though the script has already preparedREGISTRY_AUTH_FILE. If$(params.automotive-image-builder)points at an authenticated mirror, Lines 275 and 557 will silently fall back to the mutable ref instead of the digest-pinned one, which weakens the recorded provenance. Please verify this path against your private-registry AIB override case.🔍 Suggested adjustment
( aib --version 2>&1 | head -1 | tr -d '\r\n' | sed 's/^aib //' > /tmp/aib-version.txt 2>/dev/null || true - AIB_DIGEST=$(skopeo inspect --format '{{.Digest}}' "docker://${AIB_IMAGE_REF}" 2>/dev/null || echo "") + if [ -n "${REGISTRY_AUTH_FILE:-}" ]; then + AIB_DIGEST=$(skopeo inspect --authfile="$REGISTRY_AUTH_FILE" --format '{{.Digest}}' "docker://${AIB_IMAGE_REF}" 2>/dev/null || echo "") + else + AIB_DIGEST=$(skopeo inspect --format '{{.Digest}}' "docker://${AIB_IMAGE_REF}" 2>/dev/null || echo "") + fi if [ -n "$AIB_DIGEST" ]; thenAlso applies to: 553-559
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` around lines 263 - 278, The skopeo inspect call used to compute AIB_DIGEST for AIB_IMAGE_REF is unauthenticated and should use the configured registry auth file; update the skopeo inspect invocation (the one assigning AIB_DIGEST) to include the --authfile "$REGISTRY_AUTH_FILE" option (and do the same for the other skopeo inspect occurrence later in the script) so authenticated private-registry AIB overrides produce a digest-pinned value written to /tmp/aib-pinned.txt; keep the existing fallback behavior if inspect fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 550-552: The human-readable "AIB build phase" timing currently
subtracts BUILD_START_TIME which re-includes setup time; change the subtraction
to use AIB_INVOKE_TIME when computing the readable duration (i.e., use
AIB_END_TIME - AIB_INVOKE_TIME) so it matches the structured metric; update both
places that print the AIB build phase duration (any echo/log that uses
AIB_END_TIME minus BUILD_START_TIME) to use AIB_INVOKE_TIME instead so the two
outputs stay consistent.
In `@internal/controller/workspace/controller.go`:
- Around line 347-350: The container spec currently hardcodes WorkingDir to
"/workspace" for all images which overrides custom images' built-in WORKDIR;
change the logic that builds the container spec so that WorkingDir is set to
"/workspace" only when image == configuredImage (the same condition used for
setting the command variable), and leave WorkingDir unset/empty for any other
image to preserve the image's default WORKDIR; adjust where WorkingDir is
assigned in the container creation near the Command/command handling to check
configuredImage before setting it.
---
Outside diff comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 391-467: The background subshell currently performs the final
registry push ("skopeo copy ... docker://$CONTAINER_PUSH") which can publish a
container even if later steps fail; preserve the async OCI export/annotation
(use symbols PUSH_SRC, OCI_DIR, AIB_METADATA_PID, ANNOTATE_DONE) but remove the
remote push from that subshell and instead wait for the task success boundary
before performing the final push. Concretely: let the subshell only create and
annotate the OCI layout and exit (exporting OCI_DIR and new manifest info), keep
CONTAINER_PUSH_PID for the background work that completes early, then after the
disk-image and compression steps succeed, perform the registry upload (use
CONTAINER_PUSH or a staging tag and promote it) synchronously and only on
success; ensure any temporary OCI_DIR cleanup happens after the registry push
and that failures do not trigger publishing.
---
Nitpick comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 263-278: The skopeo inspect call used to compute AIB_DIGEST for
AIB_IMAGE_REF is unauthenticated and should use the configured registry auth
file; update the skopeo inspect invocation (the one assigning AIB_DIGEST) to
include the --authfile "$REGISTRY_AUTH_FILE" option (and do the same for the
other skopeo inspect occurrence later in the script) so authenticated
private-registry AIB overrides produce a digest-pinned value written to
/tmp/aib-pinned.txt; keep the existing fallback behavior if inspect fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 720fb2de-df1b-4212-ac7f-61b04f117459
📒 Files selected for processing (12)
cmd/main.goconfig/manager/manager.yamlinternal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/common.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.gointernal/controller/imagebuild/metrics.gointernal/controller/imagebuild/metrics_test.gointernal/controller/operatorconfig/controller.gointernal/controller/operatorconfig/resources.gointernal/controller/workspace/controller.gointernal/controller/workspace/controller_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/controller/operatorconfig/controller.go
- internal/controller/workspace/controller_test.go
| AIB_END_TIME=$(date +%s) | ||
| echo "⏱ AIB build phase: $((AIB_END_TIME - BUILD_START_TIME))s" | ||
|
|
There was a problem hiding this comment.
Fix the human-readable build-phase timing.
Line 551 subtracts BUILD_START_TIME, so the "AIB build phase" log includes setup time again. The structured result at Line 750 already uses AIB_INVOKE_TIME, so the two outputs disagree.
🛠️ Suggested fix
AIB_END_TIME=$(date +%s)
-echo "⏱ AIB build phase: $((AIB_END_TIME - BUILD_START_TIME))s"
+echo "⏱ AIB build phase: $((AIB_END_TIME - AIB_INVOKE_TIME))s"Also applies to: 748-750
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/build_image.sh` around lines 550 - 552, The
human-readable "AIB build phase" timing currently subtracts BUILD_START_TIME
which re-includes setup time; change the subtraction to use AIB_INVOKE_TIME when
computing the readable duration (i.e., use AIB_END_TIME - AIB_INVOKE_TIME) so it
matches the structured metric; update both places that print the AIB build phase
duration (any echo/log that uses AIB_END_TIME minus BUILD_START_TIME) to use
AIB_INVOKE_TIME instead so the two outputs stay consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/controller/workspace/controller.go (1)
344-350:⚠️ Potential issue | 🟠 MajorLeave
WorkingDirunset for custom images.Line 377 still hardcodes
/workspace, which overrides an image-definedWORKDIReven though this branch now preserves customENTRYPOINT/CMD. Images that rely on their own startup directory can still fail immediately.💡 Proposed fix
- var command []string + var ( + command []string + workingDir string + ) if image == configuredImage { command = []string{"/usr/local/bin/workspace-entrypoint.sh"} + workingDir = "/workspace" } @@ - Command: command, - WorkingDir: "/workspace", + Command: command, + WorkingDir: workingDir,Also applies to: 372-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/workspace/controller.go` around lines 344 - 350, WorkingDir is being set to "/workspace" even for custom images, which overrides an image-defined WORKDIR; change the logic so WorkingDir is only assigned when image == configuredImage (the toolchain image) — e.g., move or add the WorkingDir = "/workspace" assignment into the same branch that sets command = []string{"/usr/local/bin/workspace-entrypoint.sh"} so that for custom images (image != configuredImage) WorkingDir remains unset and the container's own WORKDIR is preserved.
🧹 Nitpick comments (1)
internal/controller/workspace/controller_test.go (1)
416-552: Cover the arbitrary-image startup contract in this table.This matrix checks
SecurityContext, but it doesn't assert the behavior that actually makes custom images usable:Commandshould stay nil,WorkingDirshould preserve the image default, and the init container should be present. Adding those expectations here would catch the current custom-image regression immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/workspace/controller_test.go` around lines 416 - 552, Update TestBuildPod_SecurityContext to also assert the arbitrary-image startup contract: when the test case uses a custom image (image == customImage or image != "" && image != automotivev1alpha1.DefaultToolchainImage) assert pod.Spec.Containers[0].Command is nil (so the image CMD is preserved), pod.Spec.Containers[0].WorkingDir is nil (so image default working directory is preserved), and that the init container(s) exist (pod.Spec.InitContainers is non-empty). Add these assertions alongside the existing SecurityContext checks in the loop that calls r.buildPod(ws, operatorConfig) so the table covers both security and startup-contract expectations for custom vs toolchain images.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/workspace/controller.go`:
- Around line 344-350: isWorkspaceActive currently relies on execing "/bin/sh
-c" with a shell pipeline (ls | grep | ps | awk) which fails in
minimal/distroless custom images; update isWorkspaceActive to avoid shell
pipelines by invoking binaries directly as command slices and parsing outputs in
Go (e.g. run exec command ["ps","-o","pid,cmd"] and parse for the workspace
process, and run ["ls","-A","/some/path"] when listing is needed) instead of
using "/bin/sh -c"; additionally, treat missing binaries as non-fatal (if exec
returns ENOENT, assume active or use alternative marker) so probes do not
falsely fail for shell-less images — change references inside isWorkspaceActive
and any helper that builds the shell command to use direct command/args and
Go-side parsing.
---
Duplicate comments:
In `@internal/controller/workspace/controller.go`:
- Around line 344-350: WorkingDir is being set to "/workspace" even for custom
images, which overrides an image-defined WORKDIR; change the logic so WorkingDir
is only assigned when image == configuredImage (the toolchain image) — e.g.,
move or add the WorkingDir = "/workspace" assignment into the same branch that
sets command = []string{"/usr/local/bin/workspace-entrypoint.sh"} so that for
custom images (image != configuredImage) WorkingDir remains unset and the
container's own WORKDIR is preserved.
---
Nitpick comments:
In `@internal/controller/workspace/controller_test.go`:
- Around line 416-552: Update TestBuildPod_SecurityContext to also assert the
arbitrary-image startup contract: when the test case uses a custom image (image
== customImage or image != "" && image !=
automotivev1alpha1.DefaultToolchainImage) assert pod.Spec.Containers[0].Command
is nil (so the image CMD is preserved), pod.Spec.Containers[0].WorkingDir is nil
(so image default working directory is preserved), and that the init
container(s) exist (pod.Spec.InitContainers is non-empty). Add these assertions
alongside the existing SecurityContext checks in the loop that calls
r.buildPod(ws, operatorConfig) so the table covers both security and
startup-contract expectations for custom vs toolchain images.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc02b4d5-e3ab-407d-829e-213d95a9f305
📒 Files selected for processing (3)
internal/controller/operatorconfig/controller.gointernal/controller/workspace/controller.gointernal/controller/workspace/controller_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/controller/operatorconfig/controller.go
| // For the toolchain image, run the entrypoint (handles user creation, overlayfs, | ||
| // subuid/subgid setup). For custom images, don't override Command so the | ||
| // image's own ENTRYPOINT/CMD runs — the init container already prepared the PVC. | ||
| var command []string | ||
| if image == configuredImage { | ||
| command = []string{"/usr/local/bin/workspace-entrypoint.sh"} | ||
| } |
There was a problem hiding this comment.
Custom images still fail the activity-probe contract.
Once Command is left unset, isWorkspaceActive() on Lines 558-563 still execs /bin/sh -c and assumes ls, grep, ps, and awk exist in the workspace image. Minimal/distroless images will now start, but every auto-pause probe will fail, so they never auto-pause and the controller will keep retrying.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/workspace/controller.go` around lines 344 - 350,
isWorkspaceActive currently relies on execing "/bin/sh -c" with a shell pipeline
(ls | grep | ps | awk) which fails in minimal/distroless custom images; update
isWorkspaceActive to avoid shell pipelines by invoking binaries directly as
command slices and parsing outputs in Go (e.g. run exec command
["ps","-o","pid,cmd"] and parse for the workspace process, and run
["ls","-A","/some/path"] when listing is needed) instead of using "/bin/sh -c";
additionally, treat missing binaries as non-fatal (if exec returns ENOENT,
assume active or use alternative marker) so probes do not falsely fail for
shell-less images — change references inside isWorkspaceActive and any helper
that builds the shell command to use direct command/args and Go-side parsing.
in order to make it possible for arbitrary containers to work Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assited-by: claude-sonnet-4.6
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/controller/workspace/controller.go (1)
344-352:⚠️ Potential issue | 🟠 MajorCustom images still break the auto-pause probe.
Leaving
Commandunset here is the right startup behavior, butisWorkspaceActive()on Lines 560-565 still execs["/bin/sh", "-c", ...]and assumesls,grep,ps, andawkexist in the workspace image. Minimal/distroless custom images will start, then every activity check fails and the controller requeues forever instead of honoring auto-pause. Either skip that probe forimage != configuredImageor make missing probe commands a non-fatal path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/workspace/controller.go` around lines 344 - 352, isWorkspaceActive() currently execs a shell with ls/grep/ps/awk which fails on minimal/distroless custom images; modify the controller so that when image != configuredImage it either skips the auto-pause activity probe entirely or treats command-not-found from the probe as non-fatal (return "unknown" or "active" rather than erroring/requeuing). Locate isWorkspaceActive (and the code path constructing [" /bin/sh","-c", ...]) and add an early check against the configuredImage/ image comparison to skip running the probe for custom images, or wrap the exec result/error handling to detect shell/command-not-found and return a benign result so auto-pause is honored.
🧹 Nitpick comments (1)
internal/controller/workspace/controller.go (1)
320-342: Extract the shared workspace bootstrap into one place.This raw-shell block now partially duplicates the directory/ownership setup in
hack/workspace-entrypoint.sh, Lines 4-42. Keeping the init path and the toolchain-entrypoint path aligned will get brittle quickly; moving the common prep into a shared script/helper would reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/workspace/controller.go` around lines 320 - 342, The initScript literal in controller.go duplicates setup logic found in hack/workspace-entrypoint.sh; extract the common mkdir/sshkey/chown/bootstrap steps into a single shared bootstrap script (e.g., workspace-bootstrap.sh) and update both places to invoke that shared script instead of inlining the shell block. Concretely: create the shared script with the existing directory, ssh-keygen, jumpstarter-copy and chown commands, add it to the image or deliver it via a ConfigMap/Volume, replace the initScript variable usage in controller.go (initScript) to run the shared script, and update hack/workspace-entrypoint.sh to source/run the same shared script so both paths stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/controller/workspace/controller.go`:
- Around line 344-352: isWorkspaceActive() currently execs a shell with
ls/grep/ps/awk which fails on minimal/distroless custom images; modify the
controller so that when image != configuredImage it either skips the auto-pause
activity probe entirely or treats command-not-found from the probe as non-fatal
(return "unknown" or "active" rather than erroring/requeuing). Locate
isWorkspaceActive (and the code path constructing [" /bin/sh","-c", ...]) and
add an early check against the configuredImage/ image comparison to skip running
the probe for custom images, or wrap the exec result/error handling to detect
shell/command-not-found and return a benign result so auto-pause is honored.
---
Nitpick comments:
In `@internal/controller/workspace/controller.go`:
- Around line 320-342: The initScript literal in controller.go duplicates setup
logic found in hack/workspace-entrypoint.sh; extract the common
mkdir/sshkey/chown/bootstrap steps into a single shared bootstrap script (e.g.,
workspace-bootstrap.sh) and update both places to invoke that shared script
instead of inlining the shell block. Concretely: create the shared script with
the existing directory, ssh-keygen, jumpstarter-copy and chown commands, add it
to the image or deliver it via a ConfigMap/Volume, replace the initScript
variable usage in controller.go (initScript) to run the shared script, and
update hack/workspace-entrypoint.sh to source/run the same shared script so both
paths stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f03f4c4d-23fe-4294-a5de-5816be6cccc1
📒 Files selected for processing (3)
internal/controller/operatorconfig/controller.gointernal/controller/workspace/controller.gointernal/controller/workspace/controller_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/controller/operatorconfig/controller.go
- internal/controller/workspace/controller_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/workspace/controller.go (1)
331-342: Consider parameterizing the hardcoded UID 1000.The
chown -R 1000:1000assumes the workspace user will be UID 1000. While this works for the toolchain image's entrypoint (which creates a user with this UID), custom images might expect different ownership.For the toolchain image, the entrypoint already handles user creation. For custom images running as a different UID, these directories will have incorrect ownership.
This may be intentional given the current scope, but worth documenting or making configurable if arbitrary images need different UIDs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/workspace/controller.go` around lines 331 - 342, The chown in the initScript is hardcoded to 1000:1000; change initScript construction in controller.go to parameterize the UID/GID (e.g., use environment-configurable variables like WORKSPACE_UID and WORKSPACE_GID with sensible defaults) and substitute them into the chown command instead of 1000:1000; ensure the code that builds initScript reads those env vars (or is passed the resolved uid/gid) so custom images can override ownership, and update any related docs/comments to mention the new WORKSPACE_UID/WORKSPACE_GID variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/workspace/controller.go`:
- Around line 331-342: The chown in the initScript is hardcoded to 1000:1000;
change initScript construction in controller.go to parameterize the UID/GID
(e.g., use environment-configurable variables like WORKSPACE_UID and
WORKSPACE_GID with sensible defaults) and substitute them into the chown command
instead of 1000:1000; ensure the code that builds initScript reads those env
vars (or is passed the resolved uid/gid) so custom images can override
ownership, and update any related docs/comments to mention the new
WORKSPACE_UID/WORKSPACE_GID variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef438f4f-05fd-4ad5-a8b9-20bf3fd83dc3
📒 Files selected for processing (3)
internal/controller/operatorconfig/controller.gointernal/controller/workspace/controller.gointernal/controller/workspace/controller_test.go
2 participants
in order to make it possible for arbitrary containers to work
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests