Skip to content

Refactor: extract shared rewriteContainerVolumeMounts logic between storage provisioners #1697

Description

@rohanKanojia

Summary

CommonStorageProvisioner.rewriteContainerVolumeMounts and PerWorkspaceStorageProvisioner.rewriteContainerVolumeMounts contain ~80 lines of nearly identical code. The only difference is a single expression in the SubPath computation.

Files

  • pkg/provision/storage/commonStorage.go lines 136–214
  • pkg/provision/storage/perWorkspaceStorage.go lines 101–179

What's duplicated

Both methods perform the same steps:

  1. Build a devfileVolumes map from workspace components
  2. Build an additionalVolumes map from podAdditions.Volumes
  3. Build an overridesVolumes map via overrides.GetVolumesFromOverrides
  4. Add the implicit projects volume
  5. Define and call a rewriteVolumeMounts closure for both Containers and InitContainers
  6. Append the PVC volume to podAdditions.Volumes

The only difference is the SubPath value:

// CommonStorage (line 190):containers[cIdx].VolumeMounts[vmIdx].SubPath=fmt.Sprintf("%s/%s", workspaceId, vm.Name)
// PerWorkspaceStorage (line 155):containers[cIdx].VolumeMounts[vmIdx].SubPath=vm.Name

Suggested fix

Extract a shared function (e.g., in pkg/provision/storage/shared.go) that accepts a subPathFunc parameter:

funcrewriteContainerVolumeMounts(
workspaceId, pvcNamestring,
podAdditions*v1alpha1.PodAdditions,
workspace*dw.DevWorkspaceTemplateSpec,
restrictedFields []string,
subPathFuncfunc(workspaceId, volumeNamestring) string,
) error {
// ... shared logic ...containers[cIdx].VolumeMounts[vmIdx].SubPath=subPathFunc(workspaceId, vm.Name)
// ...
}

Then each provisioner calls it with the appropriate SubPath transformation:

// CommonStorageProvisioner:subPathFunc:=func(wid, namestring) string { returnwid+"/"+name }
// PerWorkspaceStorageProvisioner:subPathFunc:=func(_, namestring) string { returnname }

Why this matters

When someone fixes a bug or adds a feature to the volume mount rewriting logic, they must update both methods identically. The identical // TODO: comment in both methods confirms they were copy-pasted and have already diverged in intent.

Verification

Run make test — existing tests for both storage provisioners should continue to pass with no changes.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions