Conversation
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis PR threads workspace identity into automount provisioning, adds include/exclude annotations for per-workspace targeting, updates automount APIs and helpers to use workspace-scoped discovery, and filters/queues reconciles so only matching started DevWorkspaces receive automounted ConfigMaps, Secrets, and PVCs. ChangesWorkspace-Scoped Automount Targeting
Sequence DiagramsequenceDiagram
actor Event as Resource/Event
participant Handler as runningWorkspacesHandler
participant Matcher as automount.MatchesWorkspaceTarget
participant Reconciler as DevWorkspaceReconciler
participant Provision as automount.ProvisionAutoMountResourcesInto
participant Finder as Resource Finder<br/>(ConfigMap/Secret/PVC)
Event->>Handler: trigger
Handler->>Handler: list started DevWorkspaces
loop per workspace
Handler->>Matcher: matches workspace?
alt matches
Matcher-->>Handler: yes
Handler->>Reconciler: enqueue reconcile
else no
Matcher-->>Handler: no (skip)
end
end
Reconciler->>Provision: call with workspaceName
Provision->>Finder: list resources in workspaceNamespace
Finder->>Matcher: evaluate include/exclude vs workspaceName
alt resource matches
Matcher-->>Finder: include resource
Finder-->>Provision: return resource for mount
else does not match
Matcher-->>Finder: skip resource
end
Provision-->>Reconciler: PodAdditions with filtered mounts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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 |
tolusha
requested review from
akurinnoy,
dkwon17,
ibuziuk and
rohanKanojia
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
🧹 Nitpick comments (2)
pkg/constants/metadata.go (1)
52-72: ⚡ Quick winDocument precedence when both include and exclude annotations are present.
The testdata
testBothIncludeAndExcludeAnnotations.yamlshows that when both annotations match the workspace, the resource is not mounted (exclude wins). This precedence behavior is not captured in either docstring and is non-obvious to consumers — please call it out explicitly on at least one of the two annotation comments. Also, the bulletsuffix match*name,has a stray trailing comma in both blocks (lines 58 and 69).📝 Proposed doc tweaks
// DevWorkspaceMountIncludeAnnotation is an annotation to configure which DevWorkspaces an automount // resource should be mounted to. The value is a comma-separated list of patterns matched against // the DevWorkspace name. The resource is only mounted to workspaces whose name matches at least one pattern. // Supported patterns: // - exact match `name` // - prefix match `name*` - // - suffix match `*name`, + // - suffix match `*name` // - contains match `*name*` // - matches all workspaces `*` + // If both DevWorkspaceMountIncludeAnnotation and DevWorkspaceMountExcludeAnnotation are set and the workspace + // name matches a pattern in both, the exclude annotation takes precedence and the resource is NOT mounted. DevWorkspaceMountIncludeAnnotation = "controller.devfile.io/mount-to-devworkspace-include" // DevWorkspaceMountExcludeAnnotation is an annotation to configure which DevWorkspaces an automount // resource should NOT be mounted to. The value is a comma-separated list of patterns matched against // the DevWorkspace name. The resource is not mounted to workspaces whose name matches any pattern. // Supported patterns: // - exact match `name` // - prefix match `name*` - // - suffix match `*name`, + // - suffix match `*name` // - contains match `*name*` // - matches all workspaces `*` DevWorkspaceMountExcludeAnnotation = "controller.devfile.io/mount-to-devworkspace-exclude"🤖 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/constants/metadata.go` around lines 52 - 72, Update the comments for DevWorkspaceMountIncludeAnnotation and DevWorkspaceMountExcludeAnnotation to explicitly document precedence when both annotations match (state that exclude takes precedence and the resource will not be mounted if both match), and remove the stray trailing comma in the "suffix match `*name`" bullet in both docblocks; reference the symbols DevWorkspaceMountIncludeAnnotation and DevWorkspaceMountExcludeAnnotation when making these edits so reviewers can find the exact comment blocks to update.pkg/provision/automount/secret.go (1)
48-52: ⚡ Quick winSilent skips at V(1) may mask user-facing configuration issues.
Both skip paths (annotation-based filter at lines 48-52 and restart-required at lines 68-71) only log at verbosity 1, so a user who annotated a Secret expecting it to mount and gets nothing will have no visible feedback unless they raise the log level. Two suggestions:
- Surface at least the "requires restart" skip (line 69) as a warning/event on the DevWorkspace via the existing warning aggregation (
dwerrors.WarningError) so operators can act on it.- Log message casing is inconsistent across the two sites —
"skip Secret mount, ..."vs"Skipping Secret mount: ...". Pick one form for grep-ability.Also applies to: 68-71
🤖 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/provision/automount/secret.go` around lines 48 - 52, The Secret mount code currently silently skips in two places (the MatchesWorkspaceTarget filter and the restart-required check) with V(1) logs; change both log messages to the consistent wording "Skipping Secret mount: ..." and, for the restart-required skip, also record a DevWorkspace-level warning via the existing warning aggregation (use the dwerrors.WarningError path/utility you have in repo) including namespace, name and workspaceName so operators see the issue; locate the checks around MatchesWorkspaceTarget(&secret, workspaceName) and the restart-required check in secret.go and update the log text and add a dwerrors.WarningError entry when the restart-required branch is taken.
🤖 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 `@docs/additional-configuration.adoc`:
- Around line 255-266: Replace the incorrect example label
"controller.devfile.io/automount: true" with the same automount label/annotation
set used elsewhere in this document (the one used for ConfigMaps/Secrets) so the
example matches the actual code path; update the paragraph that references
"controller.devfile.io/automount: true" to use the canonical automount
label/annotations and ensure the text still explains that such a resource
mounted as subpath resolving to /etc/gitconfig will be used as the base git
configuration (keep the references to
controller.devfile.io/mount-to-devworkspace-include and
controller.devfile.io/mount-to-devworkspace-exclude intact).
In `@pkg/provision/automount/common.go`:
- Around line 480-512: The current matchesAnyPattern incorrectly handles
patterns with '*' in middle; replace the manual wildcard branches by iterating
the comma-separated, trimmed patterns and for each non-empty pattern call
path.Match(pattern, workspaceName), treating a true match as success; if
path.Match returns an error for a pattern, skip that pattern (or continue) and
do not panic, finally return false if no pattern matched — update the body of
matchesAnyPattern to implement this behavior (path is already imported).
---
Nitpick comments:
In `@pkg/constants/metadata.go`:
- Around line 52-72: Update the comments for DevWorkspaceMountIncludeAnnotation
and DevWorkspaceMountExcludeAnnotation to explicitly document precedence when
both annotations match (state that exclude takes precedence and the resource
will not be mounted if both match), and remove the stray trailing comma in the
"suffix match `*name`" bullet in both docblocks; reference the symbols
DevWorkspaceMountIncludeAnnotation and DevWorkspaceMountExcludeAnnotation when
making these edits so reviewers can find the exact comment blocks to update.
In `@pkg/provision/automount/secret.go`:
- Around line 48-52: The Secret mount code currently silently skips in two
places (the MatchesWorkspaceTarget filter and the restart-required check) with
V(1) logs; change both log messages to the consistent wording "Skipping Secret
mount: ..." and, for the restart-required skip, also record a DevWorkspace-level
warning via the existing warning aggregation (use the dwerrors.WarningError
path/utility you have in repo) including namespace, name and workspaceName so
operators see the issue; locate the checks around
MatchesWorkspaceTarget(&secret, workspaceName) and the restart-required check in
secret.go and update the log text and add a dwerrors.WarningError entry when the
restart-required branch is taken.
🪄 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: 546b9dcc-a7c1-45ff-9690-bca5798913b3
📒 Files selected for processing (17)
controllers/workspace/devworkspace_controller.gocontrollers/workspace/eventhandlers.godocs/additional-configuration.adocpkg/constants/metadata.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/testdata/testBothIncludeAndExcludeAnnotations.yamlpkg/provision/automount/testdata/testExcludeAnnotationDoesNotMatch.yamlpkg/provision/automount/testdata/testExcludeAnnotationMatches.yamlpkg/provision/automount/testdata/testIncludeAnnotationDoesNotMatch.yamlpkg/provision/automount/testdata/testIncludeAnnotationMatches.yaml
Contributor
Author
|
/retest |
akurinnoy
reviewed
May 6, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/provision/automount/common.go (1)
492-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWildcard matching is still incomplete for
*in middle positions.Line 492 through Line 505 only handles
*at the start/end, so patterns likeprefix*suffixora*b*care misinterpreted, which can cause incorrect workspace targeting for automounts.🤖 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/provision/automount/common.go` around lines 492 - 505, The current switch over startsWithWildcard/endsWithWildcard only handles leading/trailing '*' and misses patterns like "prefix*suffix" or multiple '*' (variables pattern and workspaceName in the shown block); replace this special-case logic with a generic wildcard matcher: split pattern by '*' into parts, ensure the first part matches the workspaceName prefix when pattern doesn't start with '*', ensure the last part matches the workspaceName suffix when pattern doesn't end with '*', and verify all intermediate parts occur in order within workspaceName (each found after the previous match). Implement this matching where matched is set (replacing the switch) so patterns such as "a*b*c" and "prefix*suffix" are handled correctly.
🤖 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.
Duplicate comments:
In `@pkg/provision/automount/common.go`:
- Around line 492-505: The current switch over
startsWithWildcard/endsWithWildcard only handles leading/trailing '*' and misses
patterns like "prefix*suffix" or multiple '*' (variables pattern and
workspaceName in the shown block); replace this special-case logic with a
generic wildcard matcher: split pattern by '*' into parts, ensure the first part
matches the workspaceName prefix when pattern doesn't start with '*', ensure the
last part matches the workspaceName suffix when pattern doesn't end with '*',
and verify all intermediate parts occur in order within workspaceName (each
found after the previous match). Implement this matching where matched is set
(replacing the switch) so patterns such as "a*b*c" and "prefix*suffix" are
handled correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 105a8981-6b5e-4f4a-9818-e98e1138545e
📒 Files selected for processing (9)
controllers/workspace/eventhandlers.godocs/additional-configuration.adocpkg/constants/metadata.gopkg/provision/automount/common.gopkg/provision/automount/common_test.gopkg/provision/automount/configmap.gopkg/provision/automount/gitconfig.gopkg/provision/automount/pvcs.gopkg/provision/automount/secret.go
dkwon17
approved these changes
May 6, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkwon17, tolusha 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 |
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
3 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?
What issues does this PR fix or reference?
eclipse-che/che#23829
Is it tested? How?
Ensure, that workspace is restarted every time.
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
New Features
Documentation