Fix ctrl-c in docker agent serve api and fix docker agent defaulting to docker agent run#1782
Conversation
…r agent run` Signed-off-by: David Gageot <david.gageot@docker.com>
There was a problem hiding this comment.
Review Summary
Found 2 potential issues in the refactored plugin mode execution logic:
- Missing error handling for
plugin.Run()- errors could be silently swallowed - Asymmetric argument handling between
rootCmd.SetArgs()andos.Argsmanipulation
Both issues are in newly added code. Please review the inline comments.
| rootCmd.SetArgs(append(args[0:1], defaultToRun(rootCmd, args[1:])...)) | ||
| os.Args = append(os.Args[0:2], defaultToRun(rootCmd, os.Args[2:])...) | ||
|
|
||
| plugin.Run(func(dockerCli command.Cli) *cobra.Command { |
There was a problem hiding this comment.
Missing Error Handling
The plugin.Run() function returns an error (signature: func Run(fn func(Cli) *cobra.Command, meta Metadata) error), but this return value is not being captured or handled. This means any errors that occur during plugin execution will be silently swallowed, and the function will unconditionally return nil on line 189.
In contrast, the standalone mode path (lines 155-159) properly checks and handles errors:
if err := rootCmd.Execute(); err != nil {
return processErr(ctx, err, stderr, rootCmd)
}Consider wrapping the plugin.Run() call:
return plugin.Run(func(dockerCli command.Cli) *cobra.Command {
// ... existing callback code ...
}, metadata.Metadata{
// ... existing metadata ...
})This ensures error conditions are properly propagated to the caller.
There was a problem hiding this comment.
plugin.Run() does not return an error, it directly os.exit(x) after displaying errors on stderr.
| } | ||
|
|
||
| // When no subcommand is given, default to "run". | ||
| rootCmd.SetArgs(append(args[0:1], defaultToRun(rootCmd, args[1:])...)) |
There was a problem hiding this comment.
Asymmetric Argument Handling
There's an asymmetry in how arguments are processed for rootCmd.SetArgs() vs os.Args:
- Line 166: Preserves
args[0:1](1 element) and processesargs[1:] - Line 167: Preserves
os.Args[0:2](2 elements) and processesos.Args[2:]
This creates different slicing offsets. For example:
- If
args = ["docker-agent", "myfile.yaml"]andos.Args = ["docker", "agent", "myfile.yaml"] - After processing:
rootCmdgets:["docker-agent", "run", "myfile.yaml"]os.Argsbecomes:["docker", "agent", "run", "myfile.yaml"]
This divergence could cause issues if the Docker plugin framework or other code reads os.Args directly after this point. Could you confirm this asymmetry is intentional? If so, consider adding a comment explaining why args and os.Args need different handling.
No description provided.