Conversation
tolusha
requested review from
akurinnoy,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
rohanKanojia
reviewed
Nov 3, 2025
tolusha
changed the title
Contributor
Author
|
The main problem here is that even if we mount the resources (marked with special attributes) only when the workspace starts—and not while it’s running—we still need to decide how to handle changes to those resources while the workspace is active. |
Collaborator
|
I haven't tried, but is it possible to ignore configmaps/secrets that have the |
Contributor
Author
|
It slightly mitigates the problem, but it doesn’t address it completely. For example:
|
Contributor
Author
|
There is a solution that I don’t really like, but it could work. |
|
PR needs rebase. 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. |
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
…tart Signed-off-by: Anatolii Bazko <abazko@redhat.com>
📝 WalkthroughWalkthroughUpdates workspace automount provisioning to conditionally gate resource mounting based on a new Changes
Sequence DiagramsequenceDiagram
participant Controller
participant Automount as Automount<br/>Provisioner
participant DeploymentFetcher
participant ResourceHandlers as ConfigMap/<br/>Secret/PVC<br/>Handlers
participant FilterLogic as Filter &<br/>Presence Check
Controller->>DeploymentFetcher: GetClusterDeployment(workspace)
DeploymentFetcher-->>Controller: workspaceDeployment?
Controller->>Automount: ProvisionAutoMountResourcesInto(...<br/>isWorkspaceRunning, workspaceDeployment)
Automount->>ResourceHandlers: getDevWorkspaceConfigmaps<br/>(... isWorkspaceRunning, deployment)
Automount->>ResourceHandlers: getDevWorkspaceSecrets<br/>(... isWorkspaceRunning, deployment)
Automount->>ResourceHandlers: getAutoMountPVCs<br/>(... isWorkspaceRunning, deployment)
loop For each resource
ResourceHandlers->>FilterLogic: isAllowedToMount(resource<br/>isMountOnStart?, hasDeployment?, isRunning?)
alt Mount-on-start & Running
FilterLogic->>FilterLogic: Check if volume/env<br/>exists in deployment
FilterLogic-->>ResourceHandlers: Include if<br/>present in deployment
else Mount-on-start & Not Running
FilterLogic-->>ResourceHandlers: Include (will be<br/>mounted on start)
else Not Mount-on-start
FilterLogic-->>ResourceHandlers: Always include
end
ResourceHandlers-->>Automount: Filtered Resources
end
Automount-->>Controller: Provisioned volumes &<br/>volumeMounts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/provision/automount/gitconfig.go (1)
53-76:⚠️ Potential issue | 🟠 MajorDo not mount empty synthetic git resources after filtering all inputs.
When a first git credential/TLS resource is added with
mount-on-startwhile the workspace is running, the helpers can filter out every input, but lines 63-74 still sync and return the merged Secret/ConfigMap mounts. That changes the Deployment with empty synthetic git config and can restart the workspace. Have the helpers report whether any eligible input was included, and skip returning these synthetic mounts unless they are already present inworkspaceDeploymentor contain eligible/base config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/gitconfig.go` around lines 53 - 76, The code currently always syncs and mounts synthetic git Secret/ConfigMap even when helpers filtered out all inputs; update mergeGitCredentials and constructGitConfig (or their callers) to return a boolean indicating whether any eligible inputs or base config/tls were included, then in this block only call sync.SyncObjectWithCluster and include the results from getAutomountSecret/getAutomountConfigmap in flattenAutomountResources if that boolean is true OR the corresponding resource already exists in workspaceDeployment or contains non-empty/base config; otherwise skip syncing and do not return the synthetic mounts. Ensure you reference mergeGitCredentials, constructGitConfig, sync.SyncObjectWithCluster, getAutomountSecret, getAutomountConfigmap and workspaceDeployment when implementing the conditional logic.
🧹 Nitpick comments (3)
pkg/provision/automount/common.go (1)
412-422: Nit: loop variables are pluralized but represent single items.
automountVolumes/deploymentVolumesare singular elements produced by ranging over slices. Renaming improves readability.Proposed rename
-func isVolumeMountExistsInDeployment(automountResource Resources, workspaceDeployment *appsv1.Deployment) bool { - for _, automountVolumes := range automountResource.Volumes { - for _, deploymentVolumes := range workspaceDeployment.Spec.Template.Spec.Volumes { - if automountVolumes.Name == deploymentVolumes.Name { +func isVolumeMountExistsInDeployment(automountResource Resources, workspaceDeployment *appsv1.Deployment) bool { + for _, automountVolume := range automountResource.Volumes { + for _, deploymentVolume := range workspaceDeployment.Spec.Template.Spec.Volumes { + if automountVolume.Name == deploymentVolume.Name { return true } } } return false }Also, function names like
isVolumeMountExistsInDeployment/isEnvFromSourceExistsInDeploymentread awkwardly — considervolumeExistsInDeployment/envFromSourceExistsInDeployment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/common.go` around lines 412 - 422, Rename the pluralized loop variables to singulars and consider clearer function names: in isVolumeMountExistsInDeployment change automountVolumes -> automountVolume and deploymentVolumes -> deploymentVolume; similarly rename variables in isEnvFromSourceExistsInDeployment if present. Also consider renaming the functions to volumeExistsInDeployment and envFromSourceExistsInDeployment (update all call sites) to improve readability while preserving behavior.pkg/provision/automount/templates.go (1)
96-98: Vacuous-truth and nesting readability — small simplification possible.
!slices.ContainsFunc(xs, func(x) bool { return !p(x) })reads as a double negation of "all satisfy p". It also returnstruefor an empty slice, which is fine here only because the loop below then doesn't iterate. Consider using a helper or an explicit all-loop for clarity, especially since the same pattern is repeated inmergeGitCredentials(Line 159-161):Optional readability tweak
allConfigMapsMountOnStart := len(certificatesConfigMaps) > 0 for i := range certificatesConfigMaps { if !isMountOnStart(&certificatesConfigMaps[i]) { allConfigMapsMountOnStart = false break } }or, if you prefer keeping
slices:allConfigMapsMountOnStart := len(certificatesConfigMaps) > 0 && !slices.ContainsFunc(certificatesConfigMaps, func(cm corev1.ConfigMap) bool { return !isMountOnStart(&cm) })Not blocking — the behavior is correct today because empty slices short-circuit via the surrounding loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/templates.go` around lines 96 - 98, Replace the double-negation slices.ContainsFunc pattern used to compute allConfigMapsMountOnStart with a clearer "all" check: iterate certificatesConfigMaps (or use len(certificatesConfigMaps) > 0 && ...) and set allConfigMapsMountOnStart to false on the first element for which isMountOnStart(&cm) is false; apply the same clearer pattern in the mergeGitCredentials block where the identical slices.ContainsFunc(...) negation is used so the logic and empty-slice semantics remain explicit and readable.pkg/provision/automount/common_test.go (1)
413-710: Duplicate test coverage — three pairs assert the exact same scenario.These pairs pass identical arguments (
false, true, emptyDeployment()) against the same fake object and assert identical outcomes:
TestShouldNotMountSecretWithMountOnStarIfWorkspaceStarted(Line 413) ↔TestShouldNotMountSecretWithMountOnStartWhenRunningAndNotInDeployment(Line 612)TestShouldNotMountConfigMapWithMountOnStartIfWorkspaceStarted(Line 452) ↔TestShouldNotMountConfigMapWithMountOnStartWhenRunningAndNotInDeployment(Line 530)TestShouldNotMountPVCWithMountOnStartIfWorkspaceStarted(Line 491) ↔TestShouldNotMountPVCWithMountOnStartWhenRunningAndNotInDeployment(Line 694)Recommend collapsing each pair into a single test (or making one of them genuinely test a different state, e.g.
isWorkspaceRunning=truewithworkspaceDeployment=nil, which currently has no coverage and is the ambiguous case discussed incommon.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provision/automount/common_test.go` around lines 413 - 710, Duplicate tests cover the same inputs and assertions: collapse each pair into one test (e.g., remove either TestShouldNotMountSecretWithMountOnStarIfWorkspaceStarted or TestShouldNotMountSecretWithMountOnStartWhenRunningAndNotInDeployment, same for the ConfigMap and PVC pairs) so you only call ProvisionAutoMountResourcesInto once per scenario; alternatively change one test of each pair to exercise the ambiguous case described in common.go by calling ProvisionAutoMountResourcesInto(testPodAdditions, testAPI, testNamespace, false, true, nil) (i.e., isWorkspaceStarted/isWorkspaceRunning=true with workspaceDeployment=nil) and adjust assertions accordingly; update or remove redundant test helper usage (mountOnStartSecretAsFile, mountOnStartConfigMapAsFile, mountOnStartPVC) so each remaining test uniquely covers either the "workspace started" or the "running and not in deployment" scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/workspace/devworkspace_controller.go`:
- Around line 459-467: The current branch around isWorkspaceRunning is inverted:
it only calls wsprovision.GetClusterDeployment when the workspace is NOT
running, leaving workspaceDeployment nil for running workspaces and causing
automount/isAllowedToMount to allow unsafe mounts. Change the condition to fetch
the existing deployment when isWorkspaceRunning is true by calling
wsprovision.GetClusterDeployment(workspace, clusterAPI) and assigning
workspaceDeployment (and returning errors as before) so
automount/isAllowedToMount receives the actual deployment for running
workspaces.
In `@pkg/constants/attributes.go`:
- Around line 175-179: The exported annotation constant MountOnStartAttribute
currently uses "controller.devfile.io/mount-on-start" but the PR and docs expect
"controller.devfile.io/mount-on-start-only"; update the value of
MountOnStartAttribute to "controller.devfile.io/mount-on-start-only" and then
search/replace all usages (tests, docs, and any code references) to use
MountOnStartAttribute so the public API key is consistent across code, tests,
and documentation.
In `@pkg/provision/automount/common_test.go`:
- Line 413: Rename the test function
TestShouldNotMountSecretWithMountOnStarIfWorkspaceStarted to correct the typo
(MountOnStart) so the function name reads
TestShouldNotMountSecretWithMountOnStartIfWorkspaceStarted; update any
references or subtests that call or document this function name to match the
corrected identifier (e.g., in package tests, test runners, or comments) to
ensure compilation and clarity.
- Around line 712-739: The test constructs a Deployment volume using the raw
string "test-pvc" but the production helper common.AutoMountPVCVolumeName(...)
is used to name volumes; update the test to call common.AutoMountPVCVolumeName
on the PVC name when creating the deployment volume and in the final assertion
so the test uses the same naming logic as ProvisionAutoMountResourcesInto and
getAutoMountPVCs; locate the volume creation in
TestMountOnStartPVCAllowedWhenVolumeExistsInDeployment and replace the literal
"test-pvc" with common.AutoMountPVCVolumeName("test-pvc") and also assert
equality against common.AutoMountPVCVolumeName("test-pvc") in the assert.Equal
call.
In `@pkg/provision/automount/common.go`:
- Around line 408-422: The current name-only check in
isVolumeMountExistsInDeployment can miss "shape changes" (e.g., ConfigMap
switched from file volume to envFrom) causing the new representation to be
ignored; update the reconciliation checks so that when a resource name matches
an existing deployment volume but the new automount Resources describe it as an
EnvFrom (or vice versa) we detect the mismatch and treat it as an
existing-but-changed resource that requires a restart/reconcile. Concretely,
augment the logic around isVolumeMountExistsInDeployment and
isEnvFromSourceExistsInDeployment (or the higher-level existsInDeployment /
isAllowedToMount flow) to: 1) detect same underlying resource name present in
deployment volumes but absent as a volume in automount.Resources (and check
container.EnvFrom entries for the same name), 2) return a sentinel
"shape-changed" result or treat it as exists so the controller triggers the
restart/reconcile, and 3) add a unit test covering the transition from
mount-as:file → mount-as:env (and vice versa) to ensure the change is not
silently ignored.
- Around line 378-401: The gating logic in isAllowedToMount is inverted: when
the workspace is running we must check the existing deployment, but the code
currently does the opposite; update the conditional so that the branch that
calls existsInDeployment(automountResource, workspaceDeployment) runs only when
isWorkspaceRunning is true (i.e., change the check around isWorkspaceRunning),
ensuring isMountOnStart is respected for running workspaces and
workspaceDeployment is used to gate mounts.
In `@pkg/provision/automount/configmap.go`:
- Around line 62-67: When a running workspace has an existing mount-on-start
ConfigMap, the current logic builds a new shape via getAutomountConfigmap(...)
and then drops it if isAllowedToMount(...) can't find an exact match, causing
the old mount to be removed and triggering a rollout; instead, detect that
workspaceDeployment already contains the mounted resource and preserve its
existing volume/mount/envFrom into the desired spec: if isAllowedToMount(...)
fails for a mount-on-start resource while isWorkspaceRunning is true, copy the
cluster-side resource (volume, volumeMount, and envFrom entries) from
workspaceDeployment into the automountCM entry added to allAutoMountResources
(or change getAutomountConfigmap to return the preserved cluster-side resource),
and apply the same preservation pattern for Secret and PVC automount paths so
in-place shape changes don’t drop existing mounts.
In `@pkg/provision/automount/gitconfig_test.go`:
- Around line 407-434: The test
TestMountGitCredentialWhenMixedMountOnStartSecrets asserts behavior that
contradicts the comment on shouldIncludeGitObject; either document the contract
or change logic—here, add a clarifying comment inside
TestMountGitCredentialWhenMixedMountOnStartSecrets stating that
ProvisionGitConfiguration will allow mount-on-start secrets through when any
sibling secret lacks the mount-on-start annotation (per shouldIncludeGitObject
in templates.go), and that this is an intentional tradeoff (only
fully-mount-on-start sets are gated), so the test expects 2 volumes/2 mounts for
the mixed case; reference shouldIncludeGitObject and ProvisionGitConfiguration
in the comment for future readers.
- Around line 224-251: The test initializes sync.ClusterAPI without setting
ClusterAPI.Ctx, leaving it nil and diverging from production; set ClusterAPI.Ctx
to a real context (e.g., context.Background() or context.TODO()) in the test
setup where ClusterAPI is constructed so getGitResources() and
sync.SyncObjectWithCluster() receive a non-nil context; update the ClusterAPI
construction in this test (and the other tests noted) so clusterAPI.Ctx is
assigned before calling ProvisionGitConfiguration or any API client operations.
In `@pkg/provision/automount/templates.go`:
- Around line 183-201: The current shouldIncludeGitObject logic short-circuits
on !allGitObjectsMountOnStart causing mount-on-start objects to be included
whenever any sibling is non-mount-on-start; fix by making that branch also
require the merged volume to already be present (i.e., require
workspaceDeployment != nil and/or isVolumeMountExistsInDeployment(...) to be
true) so a global "some sibling is non-mount-on-start" does not alone permit
inclusion, or alternatively update the comment to state the true behaviour;
change the condition involving allGitObjectsMountOnStart so it reads like
"!allGitObjectsMountOnStart && (workspaceDeployment != nil ||
isVolumeMountExistsInDeployment(automountMergedResource, workspaceDeployment))"
(or equivalent) and keep isMountOnStart, allGitObjectsMountOnStart,
workspaceDeployment, and isVolumeMountExistsInDeployment as the referenced
symbols.
---
Outside diff comments:
In `@pkg/provision/automount/gitconfig.go`:
- Around line 53-76: The code currently always syncs and mounts synthetic git
Secret/ConfigMap even when helpers filtered out all inputs; update
mergeGitCredentials and constructGitConfig (or their callers) to return a
boolean indicating whether any eligible inputs or base config/tls were included,
then in this block only call sync.SyncObjectWithCluster and include the results
from getAutomountSecret/getAutomountConfigmap in flattenAutomountResources if
that boolean is true OR the corresponding resource already exists in
workspaceDeployment or contains non-empty/base config; otherwise skip syncing
and do not return the synthetic mounts. Ensure you reference
mergeGitCredentials, constructGitConfig, sync.SyncObjectWithCluster,
getAutomountSecret, getAutomountConfigmap and workspaceDeployment when
implementing the conditional logic.
---
Nitpick comments:
In `@pkg/provision/automount/common_test.go`:
- Around line 413-710: Duplicate tests cover the same inputs and assertions:
collapse each pair into one test (e.g., remove either
TestShouldNotMountSecretWithMountOnStarIfWorkspaceStarted or
TestShouldNotMountSecretWithMountOnStartWhenRunningAndNotInDeployment, same for
the ConfigMap and PVC pairs) so you only call ProvisionAutoMountResourcesInto
once per scenario; alternatively change one test of each pair to exercise the
ambiguous case described in common.go by calling
ProvisionAutoMountResourcesInto(testPodAdditions, testAPI, testNamespace, false,
true, nil) (i.e., isWorkspaceStarted/isWorkspaceRunning=true with
workspaceDeployment=nil) and adjust assertions accordingly; update or remove
redundant test helper usage (mountOnStartSecretAsFile,
mountOnStartConfigMapAsFile, mountOnStartPVC) so each remaining test uniquely
covers either the "workspace started" or the "running and not in deployment"
scenario.
In `@pkg/provision/automount/common.go`:
- Around line 412-422: Rename the pluralized loop variables to singulars and
consider clearer function names: in isVolumeMountExistsInDeployment change
automountVolumes -> automountVolume and deploymentVolumes -> deploymentVolume;
similarly rename variables in isEnvFromSourceExistsInDeployment if present. Also
consider renaming the functions to volumeExistsInDeployment and
envFromSourceExistsInDeployment (update all call sites) to improve readability
while preserving behavior.
In `@pkg/provision/automount/templates.go`:
- Around line 96-98: Replace the double-negation slices.ContainsFunc pattern
used to compute allConfigMapsMountOnStart with a clearer "all" check: iterate
certificatesConfigMaps (or use len(certificatesConfigMaps) > 0 && ...) and set
allConfigMapsMountOnStart to false on the first element for which
isMountOnStart(&cm) is false; apply the same clearer pattern in the
mergeGitCredentials block where the identical slices.ContainsFunc(...) negation
is used so the logic and empty-slice semantics remain explicit and readable.
🪄 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: bb85f151-8a4c-494e-833d-a35807facca0
📒 Files selected for processing (12)
controllers/workspace/devworkspace_controller.gopkg/constants/attributes.gopkg/provision/automount/common.gopkg/provision/automount/common_persistenthome_test.gopkg/provision/automount/common_test.gopkg/provision/automount/configmap.gopkg/provision/automount/gitconfig.gopkg/provision/automount/gitconfig_test.gopkg/provision/automount/pvcs.gopkg/provision/automount/secret.gopkg/provision/automount/templates.gopkg/provision/workspace/deployment.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1533 +/- ##
==========================================
+ Coverage 35.48% 36.59% +1.11%
==========================================
Files 168 168
Lines 14484 14671 +187
==========================================
+ Hits 5139 5369 +230
+ Misses 9006 8953 -53
- Partials 339 349 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR prevents automatic workspace restarts when mounting resources (ConfigMaps, Secrets, PVCs) by introducing a new
controller.devfile.io/mount-on-startattribute.What issues does this PR fix or reference?
eclipse-che/che#23553
Is it tested? How?
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