Add docker migrate-storage-driver command#613
Conversation
Implement CLI for home-assistant/supervisor#6361. Without any extra argument, the command migrates to the overlayfs Containerd snapshotter, but the driver can be specified and will be validated by the Supervisor API - no need for another validation layer here. The list of driver is only used for shell completion.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new Cobra CLI command Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds a new CLI command docker migrate-storage-driver to schedule Docker storage driver migration on the next system reboot. The command defaults to migrating to overlayfs (Containerd snapshotter) but allows specifying alternative drivers, with validation handled by the Supervisor API.
- Implements interactive confirmation prompt for destructive operation
- Provides shell completion for supported storage drivers (overlayfs, overlay2)
- Uses standard GenericJSONPost helper for API communication
| This will schedule a Docker storage driver migration to "`+storageDriver+`". | ||
| The migration will be applied on the next system reboot. It is strongly | ||
| recommended to back up your data before and have at least 50% of free storage. | ||
| Are you sure you want to proceed?`, 0) |
There was a problem hiding this comment.
I would also add that Internet connectivity is required to re-download all the container image.
Maybe writing the message separately before calling the AskForConfirmation is better?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/docker_migrate_storage_driver.go (1)
42-56: Confirmation prompt looks good with the Internet connectivity note.The prompt now includes the important details: backup recommendation, internet connectivity requirement, and storage space recommendation.
One minor observation:
tries: 0relies on the helper's internal default of 2. Consider using an explicit value for clarity, though the current behavior is correct.- `, 0) + `, 2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
cmd/docker_migrate_storage_driver.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
*/**(html|markdown|md)
⚙️ CodeRabbit configuration file
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
*/**(html|markdown|md): - Use bold to mark UI strings.
If "" are used to mark UI strings, replace them by bold.
Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Use sentence-style capitalization also in headings.
do not comment on HTML used for icons
Avoid flagging inline HTML for embedding videos in future reviews for this repository.
Files:
cmd/docker_migrate_storage_driver.go
🧬 Code graph analysis (1)
cmd/docker_migrate_storage_driver.go (3)
client/helper.go (3)
AskForConfirmation(203-230)PrintError(314-316)ShowJSONResponse(131-178)cmd/root.go (1)
ExitWithError(27-27)client/client.go (1)
GenericJSONPost(79-81)
🔇 Additional comments (4)
cmd/docker_migrate_storage_driver.go (4)
9-22: LGTM!The command definition is well-structured with clear documentation. The
Use,Short,Long, andExamplefields provide comprehensive guidance for users.
23-29: LGTM!The shell completion function correctly limits suggestions to the first argument and returns the intended driver options for completion purposes.
76-78: LGTM!Standard Cobra pattern for registering the subcommand under the parent
dockerCmd.
69-72: Verify: SettingExitWithErroron deliberate user abort.When the user explicitly declines the confirmation (enters "no"),
ExitWithErroris set totrue, resulting in a non-zero exit code. A deliberate abort is typically not considered an error condition, and scripts checking exit codes might misinterpret this as a failure.Verification of the codebase convention could not be completed due to repository access issues. Manual verification is needed to:
- Check how other commands in the codebase handle user abort scenarios
- Confirm whether setting
ExitWithErroron deliberate user abort is consistent with established patterns- Determine if this behavior aligns with project conventions for exit codes
Implement CLI for home-assistant/supervisor#6361. Without any extra argument, the command migrates to the overlayfs Containerd snapshotter, but the driver can be specified and will be validated by the Supervisor API - no need for another validation layer here. The list of driver is only used for shell completion.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.