feat(vhdx): support mount source code disk into wsl#114
feat(vhdx): support mount source code disk into wsl#114BlackHole1 merged 4 commits intooomol-lab:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis changeset introduces source code disk initialization and management capabilities. The changes add a new setup step in the CLI that extracts a sourcecode.vhdx file from an embedded ZIP archive, integrate this disk into the update handler's unmount sequence, modify WSL distro mounting and unmounting logic to accept and handle multiple VHDX disk paths via variadic parameters, and update the Launch method to mount both data and source code disks with consolidated error handling. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
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 |
![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 (1)
pkg/winapi/vhdx/vhdx.go (1)
50-62: Consider renaming parameter for clarity.The parameter
pathis used as a directory path throughout the function (passed asdestDirtoextractFile). Consider renaming it todestDirortargetDirto make the API clearer.🔎 Proposed refactor
-func ExtractSourceCode(path string) error { - reader, err := zip.NewReader(bytes.NewReader(sourceCodeZip), int64(len(sourceCodeZip))) - if err != nil { - return fmt.Errorf("open embedded zip: %w", err) - } - - for _, f := range reader.File { - if err := extractFile(f, path); err != nil { - return fmt.Errorf("extract %s: %w", f.Name, err) - } - } - return nil -} +func ExtractSourceCode(destDir string) error { + reader, err := zip.NewReader(bytes.NewReader(sourceCodeZip), int64(len(sourceCodeZip))) + if err != nil { + return fmt.Errorf("open embedded zip: %w", err) + } + + for _, f := range reader.File { + if err := extractFile(f, destDir); err != nil { + return fmt.Errorf("extract %s: %w", f.Name, err) + } + } + return nil +}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/winapi/vhdx/sourcecode.vhdx.zipis excluded by!**/*.zip
📒 Files selected for processing (5)
.gitignorepkg/cli/run.gopkg/update/handler.gopkg/winapi/vhdx/vhdx.gopkg/wsl/distro.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/cli/run.go (1)
pkg/winapi/vhdx/vhdx.go (1)
ExtractSourceCode(50-62)
pkg/update/handler.go (1)
pkg/wsl/distro.go (1)
UmountVHDX(121-137)
pkg/wsl/distro.go (3)
pkg/update/update.go (1)
Context(19-24)pkg/logger/logger.go (1)
Context(101-106)pkg/util/file.go (1)
Exists(8-11)
🔇 Additional comments (9)
.gitignore (1)
12-12: LGTM!Standard addition to exclude a documentation or assistant file from version control.
pkg/winapi/vhdx/vhdx.go (2)
7-14: LGTM!All new imports are appropriately used for the embedded ZIP extraction functionality.
47-48: The embedded filesourcecode.vhdx.zipexists at the correct path and is properly positioned for the build.pkg/update/handler.go (1)
64-67: LGTM!The update handler correctly integrates the new source code disk into the unmount workflow, aligning with the multi-disk support introduced in
wsl.UmountVHDX.pkg/cli/run.go (2)
19-19: LGTM!Import added to support the new source code disk extraction functionality.
61-63: LGTM!The integration of
SetupSourceCodeDiskinto the setup flow is clean with appropriate error handling.pkg/wsl/distro.go (3)
107-119: LGTM!The variadic
MountVHDXimplementation correctly handles multiple disk paths with appropriate error handling—continuing when disks are already mounted and failing fast on genuine errors.
121-137: LGTM!The best-effort unmount strategy is appropriate for cleanup operations. The function attempts to unmount all disks, logs warnings on failures, but always succeeds to allow cleanup to proceed even if individual unmounts fail.
182-184: LGTM with dependency on setup fix.The integration correctly mounts both data and source code disks during launch. Note that this depends on
SetupSourceCodeDiskinpkg/cli/run.goextracting the source code disk to the correct location (see critical issue flagged in that file).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/wsl/distro.go (1)
121-137: LGTM: Robust unmount with appropriate error handling.The variadic signature correctly handles unmounting multiple disks. The dual existence checks (filesystem via
os.Statand WSL mount state viaERROR_FILE_NOT_FOUND) provide resilient cleanup behavior.Optional: Add context to error at line 132
For consistency with line 114, consider wrapping the error with context:
- return err + return fmt.Errorf("wsl unmount %s failed: %w", path, err)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/cli/run.gopkg/winapi/vhdx/vhdx.gopkg/wsl/distro.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/winapi/vhdx/vhdx.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/cli/run.go (1)
pkg/winapi/vhdx/vhdx.go (1)
ExtractSourceCode(49-92)
pkg/wsl/distro.go (3)
pkg/update/update.go (1)
Context(19-24)pkg/logger/logger.go (1)
Context(101-106)pkg/util/file.go (1)
Exists(8-11)
🔇 Additional comments (1)
pkg/wsl/distro.go (1)
107-119: LGTM: Variadic mount with proper idempotency.The variadic signature and loop correctly handle mounting multiple disks. The special handling for
WSL_E_USER_VHD_ALREADY_ATTACHEDmakes the operation idempotent, which is the right behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/wsl/distro.go (1)
182-184: Update error message to reflect multiple disks.Good news: The critical filename issue from the previous review has been resolved—line 182 now correctly uses
"sourcecode.vhdx"(no underscore).However, the error message at line 184 still says "disk" (singular) when the code now mounts multiple disks.
🔎 Proposed fix
if err := MountVHDX(log, dataPath, sourceCodeDiskPath); err != nil { - return fmt.Errorf("failed to mount vhdx disk: %w", err) + return fmt.Errorf("failed to mount vhdx disks: %w", err) }
🧹 Nitpick comments (1)
pkg/wsl/distro.go (1)
121-137: Wrap unmount errors with path context for consistency and debuggability.The implementation is solid, but at line 132, the error is returned without including which path failed. This is inconsistent with
MountVHDX(line 114), which wraps errors with the failing path. When unmounting multiple disks, this context is valuable for debugging.🔎 Proposed fix
if _, err := wslExec(log, "--unmount", path); err != nil { if strings.Contains(err.Error(), "ERROR_FILE_NOT_FOUND") { log.Infof("VHDX already unmounted: %s", path) continue } - return err + return fmt.Errorf("wsl unmount %s failed: %w", path, err) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/cli/migrate.gopkg/cli/run.gopkg/update/handler.gopkg/wsl/distro.go
✅ Files skipped from review due to trivial changes (1)
- pkg/cli/migrate.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/update/handler.go
- pkg/cli/run.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/wsl/distro.go (3)
pkg/update/update.go (1)
Context(19-24)pkg/logger/logger.go (1)
Context(101-106)pkg/util/file.go (1)
Exists(8-11)
🔇 Additional comments (1)
pkg/wsl/distro.go (1)
107-119: LGTM! Well-designed multi-disk mount implementation.The variadic refactor is clean and the error handling is appropriate—already-mounted disks log and continue rather than failing the entire operation. Including the failing path in error messages aids debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/wsl/distro.go (1)
182-184: Update error message to reflect multiple disks.The error message on line 184 should use "disks" (plural) since the code now mounts multiple VHDX files.
🔎 Proposed fix
dataPath := filepath.Join(opt.ImageDir, "data.vhdx") sourceCodeDiskPath := filepath.Join(opt.ImageDir, "sourcecode.vhdx") if err := MountVHDX(log, dataPath, sourceCodeDiskPath); err != nil { - return fmt.Errorf("failed to mount vhdx disk: %w", err) + return fmt.Errorf("failed to mount vhdx disks: %w", err) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/cli/run.gopkg/update/handler.gopkg/wsl/distro.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/update/handler.go
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/wsl/distro.go (1)
pkg/util/file.go (1)
Exists(8-11)
pkg/cli/run.go (1)
pkg/winapi/vhdx/vhdx.go (1)
ExtractSourceCode(49-92)
🔇 Additional comments (3)
pkg/cli/run.go (2)
19-19: LGTM!The import is necessary for the new source code disk setup functionality.
61-63: LGTM!The source code disk setup is well-implemented:
- Proper idempotency check prevents re-extraction
- Correct path handling (passes directory to
ExtractSourceCode)- Filename
"sourcecode.vhdx"matches the embedded asset- Error handling and integration into
Setup()are appropriateAlso applies to: 163-171
pkg/wsl/distro.go (1)
107-119: LGTM!The variadic implementations for
MountVHDXandUmountVHDXcorrectly handle multiple disk paths with appropriate error handling:
- Already-attached/unmounted disks are logged and skipped
- Path existence is checked before unmounting
- Errors include the specific path for easier debugging
Also applies to: 121-137
2 participants
No description provided.