fix (storage) : use operator podSecurityContext for PVC cleanup job on OpenShift#1638
fix (storage) : use operator podSecurityContext for PVC cleanup job on OpenShift#1638rohanKanojia wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes OpenShift-specific conditional logic from the cleanup job pod security context setup and sets the Job pod spec SecurityContext directly from the workspace configuration. A new unit test verifies the cleanup job uses the configured PodSecurityContext. ChangesPod Security Context Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
rohanKanojia
requested review from
akurinnoy,
btjd,
dkwon17 and
ibuziuk
as code owners
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/provision/storage/cleanup_test.go`:
- Line 2: Update the copyright header string in this Go source to reflect the
current year: change the header text that reads "// Copyright (c) 2019-2025 Red
Hat, Inc." to "// Copyright (c) 2019-2026 Red Hat, Inc." so it matches the
required pattern for Go files (the header followed by the Apache License 2.0
text); ensure the exact header line in the file is replaced accordingly.
- Around line 18-33: Reorder the import block in cleanup_test.go into three
groups separated by single blank lines: first put standard library imports
(context, testing), then third-party + Kubernetes imports
(github.com/stretchr/testify/assert, sigs.k8s.io/controller-runtime/pkg/log/zap,
sigs.k8s.io/controller-runtime/pkg/client/fake, k8s.io/api/core/v1,
k8s.io/apimachinery/pkg/apis/meta/v1, and any other non-local packages), and
finally project-local imports (github.com/devfile/api/v2/...,
github.com/devfile/devworkspace-operator/...,
github.com/devfile/devworkspace-operator/pkg/..., etc.); run make fmt or
goimports to enforce formatting. Ensure the import names shown in the diff
(context, testing, dw, assert, corev1, metav1, fake, zap and project packages
like common, constants, infrastructure, sync, v1alpha1) are placed into the
correct groups.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f316ac07-7b25-41eb-9d5b-7755d159173f
📒 Files selected for processing (2)
pkg/provision/storage/cleanup.gopkg/provision/storage/cleanup_test.go
…n OpenShift Apply workspace.Config.Workspace.PodSecurityContext to the cleanup Job pod spec, matching the workspace deployment behavior. Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/cleanup-job-podsecuritycontext
branch
from
0199538 to
2dd5da1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1638 +/- ##
==========================================
+ Coverage 36.92% 37.79% +0.87%
==========================================
Files 168 168
Lines 14744 14755 +11
==========================================
+ Hits 5444 5577 +133
+ Misses 8949 8814 -135
- Partials 351 364 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Comprehensive PR Review Summary
I've completed a thorough multi-phase review of this PR (standard code review, deep design analysis, and system-level impact assessment). Overall verdict: Ready to merge ✅
Key Strengths
✅ Design quality: Eliminates platform-specific branching that violated the operator's configuration model
✅ Test coverage: Good regression test for the primary bug scenario with realistic OpenShift fields
✅ Consistency: Cleanup jobs now follow the same contract as workspace deployments
✅ Security: No privilege escalation - applies exactly what the cluster admin configured
✅ Impact: Minimal system-level impact, fixes a bug where config was silently ignored
Optional Suggestions for Future Enhancement
1. Consider adding nil-config test case
The current test validates that a custom PodSecurityContext is correctly applied. Consider adding a second test case where PodSecurityContext is nil to verify the default-config path:
Suggested test case
```go
func TestGetSpecCommonPVCCleanupJobWithNilPodSecurityContext(t *testing.T) {
infrastructure.InitializeForTesting(infrastructure.Kubernetes)
namespace := "test-ns"
pvcName := "claim-devworkspace"
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(
&corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}},
).Build()
workspace := &common.DevWorkspaceWithConfig{
DevWorkspace: &dw.DevWorkspace{
ObjectMeta: metav1.ObjectMeta{
Name: "test-workspace",
Namespace: namespace,
Labels: map[string]string{
constants.DevWorkspaceCreatorLabel: "test-creator",
},
},
Status: dw.DevWorkspaceStatus{
DevWorkspaceId: "test-workspace-id",
},
},
Config: &v1alpha1.OperatorConfiguration{
Workspace: &v1alpha1.WorkspaceConfig{
PVCName: pvcName,
PodSecurityContext: nil, // No custom security context
},
},
}
clusterAPI := sync.ClusterAPI{
Client: fakeClient,
Scheme: scheme,
Logger: zap.New(zap.UseDevMode(true)),
Ctx: context.Background(),
}
job, err := getSpecCommonPVCCleanupJob(workspace, clusterAPI)
assert.NoError(t, err)
assert.Nil(t, job.Spec.Template.Spec.SecurityContext)
}
```
This would:
- Complete the test coverage matrix (nil vs. non-nil security context)
- Guard against future regressions where someone might re-introduce platform-specific logic
- Validate backward compatibility for deployments without custom security settings
2. Optional: Document the design decision
Consider adding a brief comment in cleanup.go at the SecurityContext assignment explaining why there's no platform-specific branching, since the old code clearly thought it was needed. This helps future developers understand the design decision.
Review Details
Verdicts across all review phases:
- Standard review: ✅ Approve - Clean, minimal fix with good test coverage
- Deep review: ✅ Design is Sound - Aligns with operator's configuration model
- Impact review: ✅ No concerns - Minimal system-level impact
The suggestions above are optional improvements. The PR is production-ready as-is.
Review conducted by Claude Code PR Review (standard + deep + impact analysis)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia, tolusha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 participants
What does this PR do?
Apply
workspace.Config.Workspace.PodSecurityContextto the cleanup Job pod spec, matching the workspace deployment behavior.What issues does this PR fix or reference?
https://redhat.atlassian.net/browse/CRW-10864
Is it tested? How?
Deploy DevWorkspaceOperator and follow these steps:
Expected:
Behavior on main:
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
Tests
Refactor