-
Notifications
You must be signed in to change notification settings - Fork 374
feat: add opt-in self-update #2993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
96ed671
feat: add opt-in self-update
dgageot 7cf5cdb
docs: document self-update
dgageot 04f84d2
feat: add interactive confirmation to self-update
dgageot a07ac50
refactor: extract answerIsYes helper for testability
dgageot 31b7de8
fix: validate backup path to prevent arbitrary file deletion
dgageot 8a43cf9
fix: detect help flags anywhere in args to skip self-update
dgageot 001a22a
fix: harden self-update download and re-exec against tampering
dgageot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package root | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/docker/cli/cli-plugins/metadata" | ||
| "github.com/spf13/cobra" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestIsManagementInvocation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| for _, args := range [][]string{ | ||
| {metadata.MetadataSubcommandName}, | ||
| {cobra.ShellCompRequestCmd}, | ||
| {cobra.ShellCompNoDescRequestCmd}, | ||
| {"completion", "bash"}, | ||
| {"version"}, | ||
| {"help"}, | ||
| {"--help"}, | ||
| {"-h"}, | ||
| {"--version"}, | ||
| {"run", "--help"}, | ||
| {"run", "agent.yaml", "-h"}, | ||
| {"share", "push", "--help"}, | ||
| } { | ||
| assert.True(t, isManagementInvocation(args), "args %v", args) | ||
| } | ||
|
|
||
| for _, args := range [][]string{ | ||
| nil, | ||
| {}, | ||
| {"run", "agent.yaml"}, | ||
| {"new"}, | ||
| } { | ||
| assert.False(t, isManagementInvocation(args), "args %v", args) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| //go:build !windows | ||
|
|
||
| package selfupdate | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "os" | ||
| "syscall" | ||
| ) | ||
|
|
||
| // swapBinary replaces dst with src on Unix. A running executable's inode can be | ||
| // replaced while it is in use, so a rename within the same filesystem is both | ||
| // safe and atomic. If src and dst live on different filesystems (the temp-dir | ||
| // fallback), os.Rename fails with EXDEV and we copy-then-replace instead. | ||
| func swapBinary(dst, src string) error { | ||
| if err := os.Rename(src, dst); err == nil { | ||
| return nil | ||
| } else if !errors.Is(err, syscall.EXDEV) { | ||
| return err | ||
| } | ||
| // Cross-filesystem fallback: copy the contents atomically. | ||
| if err := atomicWriteFromFile(dst, src); err != nil { | ||
| return err | ||
| } | ||
| _ = os.Remove(src) | ||
| return nil | ||
| } | ||
|
|
||
| // reExecProcess replaces the current process image with path using execve. | ||
| // On success it never returns: the new binary inherits our PID, file | ||
| // descriptors and terminal, so the user sees a seamless restart. | ||
| func reExecProcess(path string, args, env []string) error { | ||
| if len(args) == 0 { | ||
| args = []string{path} | ||
| } else { | ||
| // argv[0] should be the new binary's path. | ||
| args = append([]string{path}, args[1:]...) | ||
| } | ||
| if err := syscall.Exec(path, args, env); err != nil { | ||
| return fmt.Errorf("exec %s: %w", path, err) | ||
| } | ||
| return nil // unreachable on success | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| //go:build windows | ||
|
|
||
| package selfupdate | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "os/exec" | ||
| ) | ||
|
|
||
| // swapBinary replaces dst with src on Windows. | ||
| // | ||
| // Windows locks a running executable's file, so we cannot overwrite dst | ||
| // directly. Instead we rename the running binary out of the way (to a sidecar | ||
| // ".old" path, which Windows allows even while the file is mapped) and move the | ||
| // new binary into its place. The stale ".old" file is best-effort cleaned up; | ||
| // if it is still locked it lingers harmlessly until the next run. | ||
| func swapBinary(dst, src string) error { | ||
| old := dst + ".old" | ||
| _ = os.Remove(old) | ||
|
|
||
| if err := os.Rename(dst, old); err != nil { | ||
| return fmt.Errorf("moving current binary aside: %w", err) | ||
| } | ||
|
|
||
| if err := os.Rename(src, dst); err != nil { | ||
| if cpErr := atomicWriteFromFile(dst, src); cpErr != nil { | ||
| // Roll back so we never leave the install without a binary. | ||
| if rbErr := os.Rename(old, dst); rbErr != nil { | ||
| return fmt.Errorf("installing new binary: %w (copy fallback failed: %v; rollback also failed: %v)", err, cpErr, rbErr) | ||
| } | ||
| return fmt.Errorf("installing new binary: %w (copy fallback failed: %v)", err, cpErr) | ||
| } | ||
| _ = os.Remove(src) | ||
| } | ||
|
|
||
| _ = os.Remove(old) | ||
| return nil | ||
| } | ||
|
|
||
| // reExecProcess relaunches path as a child process inheriting our stdio, waits | ||
| // for it, and exits with its status. Windows has no execve, so the parent | ||
| // process must stay alive until the child finishes to preserve exit-code | ||
| // semantics for the caller (e.g. a shell or the docker CLI). | ||
| func reExecProcess(path string, args, env []string) error { | ||
| var childArgs []string | ||
| if len(args) > 1 { | ||
| childArgs = args[1:] | ||
| } | ||
|
|
||
| cmd := exec.Command(path, childArgs...) //nolint:gosec // path is our own freshly installed binary | ||
| cmd.Env = env | ||
| cmd.Stdin = os.Stdin | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| var exitErr *exec.ExitError | ||
| if ok := asExitError(err, &exitErr); ok { | ||
| os.Exit(exitErr.ExitCode()) | ||
| } | ||
| return fmt.Errorf("running updated binary: %w", err) | ||
| } | ||
|
|
||
| os.Exit(0) | ||
| return nil | ||
| } | ||
|
|
||
| // asExitError is a tiny helper kept separate so exec_unix.go does not need to | ||
| // import errors solely for this Windows branch. | ||
| func asExitError(err error, target **exec.ExitError) bool { | ||
| if e, ok := err.(*exec.ExitError); ok { //nolint:errorlint // direct type assertion is intentional here | ||
| *target = e | ||
| return true | ||
| } | ||
| return false | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LOW]
isManagementInvocationskips update for--help/-hat argv[0] but not global flags mixed with subcommandsisManagementInvocationonly inspectsargs[0](the first subcommand token). If the user runs:then
args[0]is"run", which does not match the guard list, so an update + re-exec will be triggered before the help text is shown. The user might not expect an update when asking for help on a specific subcommand. This is probably acceptable by design (the PR description says version/help queries are skipped), but the comment says "version/help queries" yet the guard only catches top-level--help/help, not per-subcommand help.This is a minor UX issue, not a correctness bug, but worth documenting in the code comment or extending the guard:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8a43cf9.
isManagementInvocationnow scans all args for--help/-h(in addition to theargs[0]switch), so per-subcommand help such asrun --helpis skipped and no longer triggers an update + re-exec. Updated the doc comment and addedrun --help/run agent.yaml -h/share push --helpcases to the test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! The fix correctly addresses the issue:
✅ isManagementInvocation now scans all args for --help/-h (lines 253-257), not just args[0]
✅ Updated doc comment accurately describes the behavior: Help and version are detected anywhere in args, not just at args[0]
✅ Test cases confirm per-subcommand help is now recognized: run --help, run agent.yaml -h, share push --help
This ensures users won't see unexpected self-update behavior when asking for help on any subcommand. Thanks for the quick fix!