refactor: extract shared rewriteContainerVolumeMounts logic between storage provisioners

[SAY-5]

What does this PR do?

Extracts the duplicated rewriteContainerVolumeMounts logic from CommonStorageProvisioner and PerWorkspaceStorageProvisioner into a single shared function in shared.go. The two provisioner methods were ~80 lines of nearly identical code that differed only in how the volume mount SubPath is computed, so the shared function takes a subPathFunc parameter and each provisioner passes its own strategy (workspace-id-prefixed for common, bare volume name for per-workspace).

What issues does this PR fix or reference?

Fixes #1697

Is it tested? How?

Existing unit tests for both storage provisioners (commonStorage_test.go, perWorkspaceStorage_test.go) pass unchanged: go test ./pkg/provision/storage/....

PR Checklist

• E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
  • v8-devworkspace-operator-e2e: DevWorkspace e2e test
  • v8-che-happy-path: Happy path for verification integration with Che

Release notes

Refactored the shared volume-mount logic without changing provisioning behavior.

[openshift-ci]

Hi @SAY-5. Thanks for your PR.

I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 70fcdf7a-7561-4f22-af65-9ec3ab85fcbe

📥 Commits

Reviewing files that changed from the base of the PR and between 5e8c72b and 10465bd.

📒 Files selected for processing (2)

• pkg/provision/storage/commonStorage.go
• pkg/provision/storage/perWorkspaceStorage.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The PR moves volume mount rewriting into a shared helper. Common storage supplies workspace-prefixed subpaths. Per-workspace storage supplies volume-name subpaths.

Changes

Volume mount rewrite refactor

Layer / File(s)	Summary
Shared rewrite implementation pkg/provision/storage/shared.go	The helper collects devfile, pod addition, and override volumes. It validates volume references, rewrites eligible mounts in regular and init containers, and appends the PVC-backed volume.
Provisioner integration pkg/provision/storage/commonStorage.go, pkg/provision/storage/perWorkspaceStorage.go	Both provisioners delegate to the helper. Common storage uses <workspaceID>/<volumeName> subpaths. Per-workspace storage uses <volumeName> subpaths.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to 10465

No actionable merge-blocking risk is identified; the change is mergeable after normal checks.

Architecture Summary

Architecture risk: 🔵 Low · up to 10465

The change affects 1 system.

Changed systems: pkg

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — pkg (service) was modified; 3 changed files map to changed impact.

Before / after behavior

• observed — Modified behavior in pkg/provision/storage/shared.go: The copyright year changes from 2025 to 2026.
• observed — Modified behavior in pkg/provision/storage/shared.go: Adds the overrides package import.
• observed — Modified behavior in pkg/provision/storage/shared.go: Adds rewriteContainerVolumeMounts, which indexes devfile volumes and errors on duplicate names; merges volumes from pod additions and overrides as allowed external references; and supplies an implicit projects volume when absent. For regular and init containers, non-ephemeral devfile mounts are redirected to the PVC with subpaths from subPathFunc; ephemeral mounts and recognized external mounts remain unchanged. Undefined, unrecognized mount references and override lookup failures return errors. On success, the function appends a PVC-backed volume to pod additions.
• observed — Modified behavior in pkg/provision/storage/commonStorage.go: Removed imports for constants and override-volume handling that were used by the deleted local mount-rewriting implementation.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Issue #1697 requires one shared volume-mount rewrite implementation, strategy-specific SubPath functions, preserved volume and mount handling, and passing storage provisioner tests. The PR adds shared…
Out of Scope Changes check	✅ Passed	The reported changes are limited to extracting shared storage provisioner logic, updating the two callers, adjusting imports, and updating the helper copyright year. These changes support issue #1697.…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: extracting shared rewriteContainerVolumeMounts logic between the two storage provisioners.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[tolusha]

/che-ai-assistant ok-pr-review

Task completed.

[tolusha]

Reviewed the refactor against the base branch rather than by eye, and it holds up well.

Verification

I diffed the two original function bodies at 5684f19 against each other: they differ only in the receiver type and the single SubPath line, so the premise of #1697 is accurate and no hidden drift between the copies was flattened. I then diffed the original CommonStorageProvisioner body against the new shared.go:349-428: the only differences are the added subPathFunc parameter and SubPath = subPathFunc(workspaceId, vm.Name) replacing the inline fmt.Sprintf.

So the subpath format is byte-identical for both strategies. Existing workspaces keep their data at the same paths, and no migration or coordinated rollout is needed. I also checked that AsyncStorageProvisioner does not contain a third copy of this logic, so leaving it alone is right.

Test coverage is stronger than the description suggests, which is what makes this safe: both suites compare the entire PodAdditions struct with assert.Equal plus cmp.Diff, so SubPath values are verified exactly rather than just checked for existence. Both error branches are covered on both strategies, for containers and init containers.

A few things worth calling out

• The // TODO: What should we do when a volume isn't explicitly defined? was carried over verbatim rather than quietly resolved. Answering open questions inside a mechanical refactor is a common way to bury a semantic change, so this is the disciplined choice.
• Every original comment survived byte-identical and in position, and no new commentary was added to the moved code. The only new prose is the doc comment on the new function, and it explains why the callback exists rather than restating the signature.
• The now-unused constants / devfileConstants imports were actually removed from both provisioner files.

Two non-code items before merge

1. No compiling check has run on this branch. CodeRabbit, DCO, WIP and the two Snyk checks are all non-compiling (Snyk reports "No manifest changes detected"), and the needs-ok-to-test label means Prow has not run go build or go test. That matters here because the PR removes imports, which is a compile-time failure in Go. I traced the remaining usages by hand and they look correct, but could a devfile member drop an /ok-to-test so the compiler confirms it on the merge commit?
2. The CodeRabbit-generated release notes in the description file this under Bug Fixes ("Ensured non-ephemeral volume mounts consistently map to the appropriate PVC subpaths"). This is a pure refactor with no behaviour change. Would you mind correcting that section so it does not reach a changelog as a fix?

Two optional nits

• Both rewriteContainerVolumeMounts methods are now one-line delegations, are not part of the Provisioner interface, and have one caller each. They still earn their keep through the per-strategy doc comments, so keeping them is fine - just worth being a deliberate choice rather than a leftover of the extraction.
• shared.go still reads Copyright (c) 2019-2025 while the two files it absorbed code from read 2019-2026. addlicense -check only verifies presence, so CI will not catch it.

Nice, tightly scoped first contribution. Nothing here blocks merge.

[tolusha]

Not a change request, just noting it while the context is fresh.

This branch has no test coverage. I checked every fixture under testdata/common-storage/ and testdata/perWorkspace-storage/ and none of them define pod overrides, so GetVolumesFromOverrides returns an empty map in every test and the additionalVolumes[vm.Name] escape below is never reached via the overrides path.

The gap is pre-existing and should not block this PR. Worth mentioning because the extraction makes it cheaper to close: it used to need two fixtures kept in sync, now a single one covers both strategies. Does it make sense to add one as a follow-up?

[SAY-5]

Yeah, a single pod-overrides fixture should cover both strategies now. I can do that as a follow-up.

[SAY-5]

Also bumped the copyright year in shared.go and moved the release notes out of Bug Fixes since it's a pure refactor.

[SAY-5]

Rebased onto main. #1693 moved those call sites to restrictions.GetRestrictedPodFields, which left the overrides imports unused and broke the build. Fixed, go build ./... and the storage tests pass locally.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rohanKanojia, SAY-5, tolusha
Once this PR has been reviewed and has the lgtm label, please assign dkwon17 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tolusha]

/retest

[tolusha]

/retest