Better shell completions - #934
Conversation
Signed-off-by: mohammed <mohammed18200118@gmail.com>
c5e0b09 to
5221519
Compare
Signed-off-by: mohammed <mohammed18200118@gmail.com>
gorkem
left a comment
gorkem
left a comment
There was a problem hiding this comment.
Each command initializes completions by reading all local repos, even if completions aren't used. This adds startup latency ( I have alot of local repos). Lazy load completions - only read local repos when tab-completion is actually triggered
| cobra.AddTemplateFunc("ensureTrailingNewline", ensureTrailingNewline) | ||
| err := installShellCompletions(cmd, opts) | ||
| if err != nil { | ||
| output.Debugf("Failed to install shell completions: %s", err) |
There was a problem hiding this comment.
Errors are only logged at debug level, but shell completion installation failures should be more visible. Silent failures could confuse users who expect completions to work.
There was a problem hiding this comment.
maybe not doing this because if the user has a problem with the setup, for example (unsupported shell), we don't want to keep showing this message on every command he run, it would be a bad experience for him, what do you think
| return nil // already present | ||
| } | ||
|
|
||
| shellRC, err := os.OpenFile(pathOfShellRC, os.O_APPEND|os.O_WRONLY, 0644) |
There was a problem hiding this comment.
There's a race condition between checking if the source command exists and writing to the file. Multiple concurrent executions could result in duplicate entries.
| } | ||
|
|
||
| func onConfigHomeError(err error) { | ||
| output.Fatalf("Failed to read base config directory") |
There was a problem hiding this comment.
output.Fatalf() should terminate execution, but subsequent output.Infof() calls can make it confusing.
There was a problem hiding this comment.
not sure actually that was already there but output.Fatalf() doesn't exit
so I manually os.exit(1)
| } | ||
| defer shellRC.Close() | ||
|
|
||
| _, err = shellRC.WriteString("\n# Kitops completions\nsource " + pathOfCompletionFile + "\n") |
There was a problem hiding this comment.
This is not safe If an attacker can control the configHome path (through the --config flag or KITOPS_HOME environment variable), they could inject malicious shell commands. Properly escape or quote the path.
|
going to fix file lock for windows later today update: ignoring shell windows completions for now, maybe will add it in a separate PR |
Signed-off-by: mohammed <mohammed18200118@gmail.com>
|
Thanks @mohammedahmed18 for the contribution. However, I think this approach to completions has a number of issues that make it unsuitable. Have a look at PR #963, which I think is a fair bit cleaner and replaces this one.
|
|
@amisevsk yeah, fair points |
3 participants
Fixes #79
This includes two main changes:
kitops-completions.online-video-cutter.com.mp4
command-completions.mp4
Let me know if you'd like to tweak anything!