Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines the once CLI by separating “self update” from “application update”, adding a shared flag set for application settings, improving list/deploy/update output, and switching several commands to use hostname as the primary identifier.
Changes:
- Added
once self-update(and repurposedonce updateto update an existing app’s settings by host). - Introduced shared
settingsFlagsto support flags across deploy/update (env vars, resources, SMTP, backups, etc.). - Updated multiple commands (
start/stop/remove/backup/list) to operate on hostnames and improved output formatting/progress UX.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/docker/namespace.go | Adds ApplicationByHost lookup and reuses it for hostname checks. |
| internal/docker/namespace_test.go | Adds coverage for hostname-based lookup. |
| internal/command/settings_flags.go | New shared CLI flag parsing and settings mutation logic. |
| internal/command/deploy.go | Uses settingsFlags and new progress UI for deploy. |
| internal/command/update.go | Repurposes update to modify an existing app by host + optional image change. |
| internal/command/self_update.go | Adds dedicated self-update command for updating the once binary. |
| internal/command/cli_progress.go | New Bubble Tea-based progress display for deploy/update. |
| internal/command/list.go | Improves list output with status + hyperlink styling. |
| internal/command/root.go | Wires in self-update and switches withApplication to host-based lookup. |
| internal/command/root_test.go | Updates tests for host-based withApplication. |
| internal/command/start.go | Switches identifier from app name to host. |
| internal/command/stop.go | Switches identifier from app name to host. |
| internal/command/remove.go | Switches identifier from app name to host. |
| internal/command/backup.go | Switches identifier to host and resolves filename to absolute path. |
| internal/command/deploy_test.go | Adds unit tests around env var parsing used by settings flags. |
| internal/command/update_test.go | Adds unit tests for settings flag application. |
| integration/docker_test.go | Adds integration coverage for deploy/update settings persistence and host changes. |
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AutoUpdate: f.autoUpdate, | ||
| Backup: docker.BackupSettings{ | ||
| Path: f.backupPath, | ||
| AutoBackup: f.autoBackup, | ||
| }, |
There was a problem hiding this comment.
--auto-backup can be enabled without providing a backup path, which will later cause backups (and auto-backups in the background runner) to fail with "backup location is required". Consider validating that when auto-backup is true, backupPath is non-empty (and absolute) in buildSettings, returning a clear CLI error early.
| } | ||
| s.Backup.Path = f.backupPath | ||
| } | ||
| if cmd.Flags().Changed("auto-backup") { |
There was a problem hiding this comment.
applyChanges allows toggling --auto-backup to true even if Backup.Path is empty (either because it was never set or because --backup-path wasn't included in this update). This can leave the app in a configuration where background backups will repeatedly error. Recommend adding validation when auto-backup is changed to true to require a non-empty, absolute backup path (either existing or provided via --backup-path).
| if cmd.Flags().Changed("auto-backup") { | |
| if cmd.Flags().Changed("auto-backup") { | |
| if f.autoBackup { | |
| effectivePath := s.Backup.Path | |
| if cmd.Flags().Changed("backup-path") { | |
| effectivePath = f.backupPath | |
| } | |
| if effectivePath == "" { | |
| return s, fmt.Errorf("backup path must be set when enabling auto-backup") | |
| } | |
| if !filepath.IsAbs(effectivePath) { | |
| return s, docker.ErrBackupPathRelative | |
| } | |
| } |
| m.progress = m.progress.SetPercent(msg.p.Percentage) | ||
| case docker.DeployStageStarting: | ||
| m.stage = "starting" | ||
| m.progress = m.progress.SetPercent(-1) |
There was a problem hiding this comment.
cliProgress.Update doesn't handle docker.DeployStageFinished, so the UI stage/progress can remain stuck on "starting" (with indeterminate progress) even after deploy has completed, until the program quits. Consider adding a DeployStageFinished case to set a sensible final stage (or transition to a new stage like "verifying") and/or set percent to 100.
| m.progress = m.progress.SetPercent(-1) | |
| m.progress = m.progress.SetPercent(-1) | |
| case docker.DeployStageFinished: | |
| m.stage = "finished" | |
| m.progress = m.progress.SetPercent(1) |
There was a problem hiding this comment.
No, the TUI exits at that stage, so it shouldn't be trying to displaying anything.
| oldSettings := app.Settings | ||
| app.Settings = settings | ||
|
|
||
| p := newCLIProgress("Updating "+currentHost, func(progress docker.DeployProgressCallback) error { |
There was a problem hiding this comment.
The progress label uses currentHost (the old host) even if --host changes the application host. This can be confusing when the update is primarily a host rename; consider using the new host in the label once changes are applied (or including both old and new).
| p := newCLIProgress("Updating "+currentHost, func(progress docker.DeployProgressCallback) error { | |
| labelHost := currentHost | |
| if settings.Host != "" && settings.Host != currentHost { | |
| labelHost = fmt.Sprintf("%s → %s", currentHost, settings.Host) | |
| } | |
| p := newCLIProgress("Updating "+labelHost, func(progress docker.DeployProgressCallback) error { |
There was a problem hiding this comment.
No, this is intentional. It displays the host that is being updated.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := app.Deploy(ctx, progress); err != nil { | ||
| app.Settings = oldSettings | ||
| return fmt.Errorf("%w: %w", docker.ErrDeployFailed, err) | ||
| } |
There was a problem hiding this comment.
update performs a deploy but never runs the HTTP health verification step used by deploy (via VerifyHTTPOrRemove). That means an update can report success even if the app starts but returns non-2xx on /health, leaving the user with a broken deployment. Consider verifying after app.Deploy and, on verification failure, rolling back to the previous settings (and redeploying) or otherwise surfacing a clear failure state rather than printing done.
| } | |
| } | |
| if err := ns.VerifyHTTPOrRemove(ctx, app, progress); err != nil { | |
| app.Settings = oldSettings | |
| return err | |
| } |
There was a problem hiding this comment.
No, that's by design, and matches the main TUI behaviour. THe external check only applies to new apps. Updates already verify the healthcheck as part of the proxy deploy, and will rollback to the previous container if it fails.
| func (s ApplicationSettings) Validate() error { | ||
| if s.Image == "" { | ||
| return ErrImageRequired | ||
| } | ||
| if s.Backup.AutoBackup && s.Backup.Path == "" { | ||
| return ErrAutoBackupWithoutPath | ||
| } | ||
| return nil |
There was a problem hiding this comment.
ApplicationSettings.Validate currently doesn’t validate resource limits. Since cpus/memory come from CLI flags, negative values can pass validation and then be multiplied into Docker resource settings (e.g., negative NanoCPUs/Memory), which will fail at deploy time with a less actionable error. Consider validating that Resources.CPUs and Resources.MemoryMB are >= 0 (and possibly > 0 when explicitly set) and returning a dedicated error early.
Improve the CLI interactions:
deployandupdatenow support flags for all settings: env vars, CPU/memory quota, backup options, etcdeploy,updateandlistlook nicer