Conversation
3a2a790 to
e52c086
Compare
…ess start/stop/restart Add shell completion to process-name-taking commands such as `process-compose run <TAB>`. Uses cobra's `ValidArgsFunction` functionality. Should work in every supported shell. `run` and `up` load names from the config file, which also allows them to pass the first line of the process descriptions (if defined in the config) to `cobra` together with the names. The `process start/stop/restart` commands query the running server. The server doesn't currently return the process descriptions (this could be added relatively easily), so the completions will come without descriptions. On any error (missing config or unreachable server) the helpers return ShellCompDirectiveNoFileComp with no candidates. The config path sets IsInternalLoader so a malformed or half-edited config returns an error instead of aborting completion via log.Fatal.
One of `process-compose` CLI rules is that the `--unix-socket blah` flag implies `-U`. Because of the way this logic is implemented inside `PersistentPreRun`, it doesn't work in the completion execution flow for `completeProcessNamesFromServer`. Before this commit, tab completion would work correctly for either `process-compose -U --unix-socket blah process start <TAB>` or `PC_SOCKET_PATH=blah process-compose ...`, but it would surprisingly not work for just `process-compose --unix-socket blah ...`.
e52c086 to
9a10107
Compare
|
F1bonacc1
left a comment
F1bonacc1
left a comment
There was a problem hiding this comment.
Thanks for this solid and well tested PR!
There are a few minor nits and one change request.
| // `--unix-socket`, they would be really confused if we didn't implement such a | ||
| // workaround, since they'd be used to `--unix-socket` implying `-U` in all | ||
| // other situations. | ||
| func TestProcessStopCompletionOverUnixSocketFlag(t *testing.T) { |
There was a problem hiding this comment.
This test will fail on the windows-latest CI leg: the --unix-socket flag is only registered when runtime.GOOS != "windows" (root.go), and cobra's getCompletions errors out on unknown flags during __complete (completions.go:373 in cobra v1.10.2), so the command yields no candidates and the strings.Contains assertions fail.
Please guard at the top:
if runtime.GOOS == "windows" {
t.Skip("--unix-socket flag is not registered on Windows")
}(FWIW I verified the test is meaningful on Linux — with the workaround in completeProcessNamesFromServer removed, it fails with output ":4\n", so it does guard the fix.)
| // offering candidates once a positional arg is already present. | ||
| func completeProcessNamesFromConfig(single bool) func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective) { | ||
| return func(_ *cobra.Command, args []string, _ string) ([]string, cobra.ShellCompDirective) { | ||
| if single && len(args) != 0 { |
There was a problem hiding this comment.
Minor UX regression for run: once the process name is present, this returns NoFileComp, which also suppresses file completion for the passthrough args after -- (process-compose run web -- ./scr<TAB> no longer completes files; previously the shell fell back to file completion).
Since flags are already parsed by the time ValidArgsFunction runs, this branch could return cobra.ShellCompDirectiveDefault when cmd.Flags().ArgsLenAtDash() >= 0 (needs the cmd param that's currently discarded) and keep NoFileComp otherwise. Fine as a follow-up too.
| // also surfaces dynamically scaled replica names. | ||
| func completeProcessNamesFromServer(single bool) func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective) { | ||
| return func(cmd *cobra.Command, args []string, _ string) ([]string, cobra.ShellCompDirective) { | ||
| if single && len(args) != 0 { |
There was a problem hiding this comment.
Tiny nit, applies to both helpers when single is false: already-typed names are offered again (process stop web <TAB> suggests web a second time). Filtering out names already present in args would be a one-liner. Not blocking.
| } | ||
| // Names only, no descriptions. TODO: consider adding process | ||
| // descriptions to the server's `/processes` response. | ||
| names, err := getClient().GetLexicographicProcessNames() |
There was a problem hiding this comment.
Heads-up rather than a request: getClient() builds an http.Client with no timeout, so completion against a configured-but-unresponsive server (e.g. a firewalled address that drops packets) will hang the user's shell until the kernel TCP timeout. This is pre-existing for all client commands, but completion is more latency-sensitive. A short-timeout client for the completion path would be a nice follow-up; doesn't need to happen in this PR.
This branch has not been deployed
2 participants
Add shell completion to process-name-taking commands such as
process-compose run <TAB>. Uses cobra'sValidArgsFunctionfunctionality. Should work in every supported shell.
runandupload names from the config file, which also allows themto pass the first line of the process descriptions (if defined in the
config) to
cobratogether with the names.The
process start/stop/restartcommands query the running server.The server doesn't currently return the process descriptions (this could
be added relatively easily), so the completions will come without
descriptions.
On any error (missing config or unreachable server) the helpers return
ShellCompDirectiveNoFileComp with no candidates. The config path sets
IsInternalLoader so a malformed or half-edited config returns an error
instead of aborting completion via log.Fatal.
About the third commit,
honor --unix-socket ...That is a preventive fix to a somewhat obscure but really confusing problem
discovered by AI. See the description and the code for details. It comes with
a somewhat heavy-weight AI-generated test to prove that the bug actually exists.
I'm happy to remove that commit if you prefer a different solution, or to
keep the fix and remove the heavy-weight (but IMO easy-to-follow) test.
One potentially better solution IMO would be to remove the
-Uflag entirely, I'm notsure it's useful when
--unix-socketimplies-Ubut a bare-Ucreatesa socket at a hard-to-guess path.