fix: default init container imagePullPolicy to IfNotPresent - #1692
fix: default init container imagePullPolicy to IfNotPresent#1692dkwon17 wants to merge 1 commit into
Conversation
Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: David Kwon <dakwon@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkwon17 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe change sets the project-clone default image pull policy to ChangesInit container image pull policy
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/config/defaults.go`:
- Line 60: Update the public API documentation for
workspace.projectClone.imagePullPolicy in the relevant type definition and
regenerated CRD output to state that an unset value defaults to IfNotPresent,
rather than inheriting workspace.imagePullPolicy. Ensure the generated
documentation matches the API source and explicitly avoids implying that
workspace.imagePullPolicy: Always affects project-clone.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16db8c69-d1eb-4471-974d-0527c92b28e1
📒 Files selected for processing (4)
controllers/workspace/devworkspace_controller.gocontrollers/workspace/devworkspace_controller_test.gopkg/config/defaults.gopkg/config/sync_test.go
| ContainerSecurityContext: nil, // Set per-platform in setDefaultContainerSecurityContext() | ||
| DefaultTemplate: nil, | ||
| ProjectCloneConfig: &v1alpha1.ProjectCloneConfig{ | ||
| ImagePullPolicy: corev1.PullIfNotPresent, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the public fallback documentation.
Line 60 makes an unset workspace.projectClone.imagePullPolicy resolve to IfNotPresent. apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go:395-407 still states that an unset value uses .config.workspace.imagePullPolicy. Update the API and generated CRD documentation so users do not expect workspace.imagePullPolicy: Always to affect project-clone.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/config/defaults.go` at line 60, Update the public API documentation for
workspace.projectClone.imagePullPolicy in the relevant type definition and
regenerated CRD output to state that an unset value defaults to IfNotPresent,
rather than inheriting workspace.imagePullPolicy. Ensure the generated
documentation matches the API source and explicitly avoids implying that
workspace.imagePullPolicy: Always affects project-clone.
There was a problem hiding this comment.
devworkspace_controller.go:396 : this else is unreachable after the defaults.go change. workspace.Config comes from ResolveConfigForWorkspace → mergeConfig, which only overwrites
non-empty values, so ProjectCloneConfig.ImagePullPolicy is never "".
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline controllers/workspace/devworkspace_controller.go --items all --match 'ResolveConfigForWorkspace|ImagePullPolicy|InitContainer' || true
ast-grep outline pkg/config --items all --match 'ResolveConfigForWorkspace|mergeConfig' || true
printf '\n--- controller policy references ---\n'
rg -n -C 8 'ProjectCloneConfig\.ImagePullPolicy|ImagePullPolicy' controllers/workspace/devworkspace_controller.go
printf '\n--- configuration resolution and merge references ---\n'
rg -n -C 8 'func\s+(ResolveConfigForWorkspace|mergeConfig)\s*\(' pkg/config
printf '\n--- configuration assignment references ---\n'
rg -n -C 5 'workspace\.Config|\.Config\s*=' controllers pkg --glob '*.go'
printf '\n--- relevant tests ---\n'
rg -n -C 5 'ProjectCloneConfig.*ImagePullPolicy|ImagePullPolicy.*ProjectCloneConfig|PullIfNotPresent' controllers/workspace pkg/config --glob '*_test.go'Length of output: 67843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workspace configuration resolution ---'
sed -n '110,150p' controllers/workspace/devworkspace_controller.go
printf '%s\n' '--- ResolveConfigForWorkspace ---'
sed -n '59,115p' pkg/config/sync.go
printf '%s\n' '--- global configuration initialization and default merge ---'
rg -n -C 10 'defaultConfig|internalConfig|SetGlobalConfig|mergeConfig\(' pkg/config --glob '*.go'
printf '%s\n' '--- mergeConfig ProjectCloneConfig behavior ---'
sed -n '250,430p' pkg/config/sync.go
printf '%s\n' '--- configuration-related tests that establish default merging ---'
rg -n -C 6 'defaultConfig|ProjectCloneConfig|ResolveConfigForWorkspace|mergeConfig' pkg/config --glob '*_test.go'Length of output: 91908
@rohanKanojia, confirmed.
internalConfig starts from defaultConfig, and mergeConfig does not replace ProjectCloneConfig.ImagePullPolicy with an empty value. ResolveConfigForWorkspace also merges an external configuration over internalConfig.
Therefore, workspace.Config.Workspace.ProjectCloneConfig.ImagePullPolicy is always non-empty. The fallback at devworkspace_controller.go:396-398 is unreachable.
This also confirms the documentation finding. The API documentation must not state that project-clone inherits workspace.imagePullPolicy.
🧬 Code Graph Analysis Results
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go:395-407
ProjectCloneConfig struct {
// Image is the container image to use for cloning projects
Image string `json:"image,omitempty"`
// ImagePullPolicy configures the imagePullPolicy for the project clone container.
// If undefined, the general setting .config.workspace.imagePullPolicy is used instead.
ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"`
// Resources defines the resource (cpu, memory) limits and requests for the project
// clone container. To explicitly not specify a limit or request, define the resource
// quantity as zero ('0')
Resources *corev1.ResourceRequirements `json:"resources,omitempty"`
// Env allows defining additional environment variables for the project clone container.
Env []corev1.EnvVar `json:"env,omitempty"`
}apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go:167-174
WorkspaceConfig struct {
// ProjectCloneConfig defines configuration related to the project clone init container
// that is used to clone git projects into the DevWorkspace.
ProjectCloneConfig *ProjectCloneConfig `json:"projectClone,omitempty"`
// RestoreConfig defines configuration related to the workspace restore init container
// that is used to restore workspace data from a backup image.
RestoreConfig *RestoreConfig `json:"restore,omitempty"`apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go:175-181
// ImagePullPolicy defines the imagePullPolicy used for containers in a DevWorkspace
// For additional information, see Kubernetes documentation for imagePullPolicy. If
// not specified, the default value of "Always" is used.
// +kubebuilder:validation:Enum=IfNotPresent;Always;Never
ImagePullPolicy string `json:"imagePullPolicy,omitempty"`✏️ Learnings added
Learnt from: rohanKanojia
Repo: devfile/devworkspace-operator PR: 1692
File: pkg/config/defaults.go:60-60
Timestamp: 2026-08-13T07:01:37.222Z
Learning: In `pkg/config/sync.go`, `internalConfig` is initialized from `defaultConfig`, and `mergeConfig` only overwrites `WorkspaceConfig.ProjectCloneConfig.ImagePullPolicy` when the incoming value is non-empty. `ResolveConfigForWorkspace` merges external DevWorkspaceOperatorConfig values over `internalConfig`. Therefore, after `pkg/config/defaults.go` sets `ProjectCloneConfig.ImagePullPolicy` to `corev1.PullIfNotPresent`, the fallback branch in `controllers/workspace/devworkspace_controller.go` that handles an empty project-clone image pull policy is unreachable.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
@dkwon17: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
I tested all scenarios in test plan and can confirm they work as expected ✔️ Claude pointed out this scenario, do you think it's valid? DWOC init container default overrides admin-configured imagePullPolicy during strategic mergeThe new default at line 427 sets Steps to reproduceStep 1. Set oc patch devworkspaceoperatorconfig devworkspace-operator-config -n "$DWO_NS" \
--type=merge -p '{
"config":{
"workspace":{
"projectClone":{"imagePullPolicy":"Always"},
"initContainers":[
{"name":"project-clone","env":[{"name":"EXTRA_VAR","value":"injected"}]}
]
}
}
}'Step 2. Create a DevWorkspace with a project: cat <<'EOF' | oc apply -n "$TEST_NS" -f -
kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
name: test-merge-conflict
spec:
started: true
template:
projects:
- name: sample
git:
remotes:
origin: "https://github.com/che-samples/web-nodejs-sample.git"
components:
- name: dev
container:
image: quay.io/devfile/universal-developer-image:latest
memoryLimit: 512Mi
EOFStep 3. Wait for the deployment to be created: while ! oc get deployment -l controller.devfile.io/devworkspace_name=test-merge-conflict \
-n "$TEST_NS" 2>/dev/null | grep -q .; do sleep 5; doneStep 4. Check the project-clone init container's imagePullPolicy: oc get deployment -l controller.devfile.io/devworkspace_name=test-merge-conflict -n "$TEST_NS" \
-o jsonpath='{.items[0].spec.template.spec.initContainers[?(@.name=="project-clone")].imagePullPolicy}'
|
2 participants
Assisted-by: Claude Opus 4.6
What does this PR do?
Changes the default
imagePullPolicyfor DWO-managed init containers (project-clone, and DWOC-defined init containers) fromAlwaystoIfNotPresent. The globalworkspace.imagePullPolicyfor user/devfile containers remains Always.Admins can still override via DWOC (
projectClone.imagePullPolicy, or per init container).What issues does this PR fix or reference?
#1674
Is it tested? How?
Tested with this test plan: https://gist.github.com/dkwon17/9f21c98fae6e9eeecf985cf7c818eaf4
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Bug Fixes
IfNotPresentimage pull policy when none is specified.Alwayspolicy.Tests