Fix shell completion by adding missing usage message required by spf13/cobra #1688

DanHam · 2023-03-22T13:35:00Z

Attempts a fix for #962.

All credit to @cschug - see this comment

As pointed out in that comment, the spf13/cobra library writes out the shell completion script using a fmt.Sprintf here.

The %[1]s in that string seem to be replaced with the first command in the Use: field of the cobra.Command struct. Currently this field is not set so the %[1]s expand to an empty string resulting in a broken shell completion script.

Adding the field (following the same method used by grype - here) gives the spf13/cobra library the info it needs to correctly generate the shell completion script.

Testing locally seems to show everything is working OK. The shell completion now works as expected after running source <(syft completion bash)

Fixes: #962

kzantow · 2023-03-22T14:35:32Z

I'm having a bit of a hard time getting this working. If I try this on my mac with zsh, it simply seems to do nothing. If I try this in a bash container (golang:latest), when I type syft p<tab>, I get this:

bash: _get_comp_words_by_ref: command not found

Is there a particular container I could use to see this work properly?

Also: you will need to sign-off your commits.

kzantow · 2023-03-22T14:43:21Z

cmd/syft/cli/commands.go

@@ -55,6 +55,7 @@ func New() (*cobra.Command, error) {

 	// rootCmd is currently an alias for the packages command
 	rootCmd := &cobra.Command{
+		Use:           fmt.Sprintf("%s package [SOURCE]", internal.ApplicationName),


package is not a valid command, and this is the root command, so this should probably be:

fmt.Sprintf("%s [SOURCE]", internal.ApplicationName)

That should have been fmt.Sprintf("%s packages [SOURCE]", ...

Even so this is the root command, not the packages command (even though they are duplicates), so it should display usage text as the root command.

Using fmt.Sprintf("%s packages [SOURCE]",... causes the following to be output for the 'Usage'

$ syft Generate a packaged-based Software Bill Of Materials (SBOM) from container images and filesystems Usage: syft packages [SOURCE] [flags] syft [command] Examples: syft packages alpin...

Happy to change that to what ever you prefer. The important bit (for our fix) is that we have syft at the start of the command. That is what the spf13/cobra library needs to generate a working shell completion script (for bash at least!!)

Right, but since a user can run syft without the packages command, I think we should leave this as "%s [SOURCE]", this leaves syft as the first part of the usage, which is what you need.

Hmmm... So the [SOURCE] bit of that usage command is only really relevant when running syft packages ...

$ syft packages Generate a packaged-based Software Bill Of Materials (SBOM) from container images and filesystems Usage: syft packages [SOURCE] [flags]

Perhaps we should just have fmt.Sprintf("%s [command] [flags]", ... as the usage for the main syft command?

This would just give:

$ syft Generate a packaged-based Software Bill Of Materials (SBOM) from container images and filesystems Usage: syft [command] [flags] syft [command] Examples: syft packages alpine:latest a summary of discovered packages syft packages alpine:la...

...which seems to make a bit more sense. Are you OK with that?

There's a bit of a mismatch between the help text and the command itself (this should be fixed, but I would not ask you to change this as part of this PR).

You can run Syft like:

syft alpine:latest

without the packages command keyword, because the root command duplicates the functionality of the packages command.

For this reason, I would prefer that the root help text indicates how to run the root command, rather than indicating packages or some sub-command is necessary. It's a very small detail.

Ahh... OK. That makes sense now. The penny finally dropped!! 😄

DanHam · 2023-03-22T15:30:19Z

Hi Keith

I'm having a bit of a hard time getting this working. If I try this on my mac with zsh...

The original issue (#962) was with shell completion for bash. I don't use zsh so I've never tried the completions that are generated for that particular shell...

EDIT: FWIW I've just tried to get the completions working for zsh and haven't had much luck either (note that I'm a complete noob with zsh). Perhaps the zsh completions need fixing as well?

Is there a particular container I could use to see this work properly?

I can see this working using the golang:latest image. Note: my arch is amd64:

Start: docker run -it -v ${PWD}:/usr/src/syft -w /usr/src/syft --name syft-build golang:latest /bin/bash
Fix dubious ownership warning: git config --global --add safe.directory /usr/src/syft
Install build deps: make bootstrap
Build: make build
Install syft for my os/arch: install snapshot/linux-build_linux_amd64_v1/syft /usr/local/bin
Verify: syft --version # Outputs: syft 0.0.0-SNAPSHOT-4867698
The golang:latest image does not have the bash-completion package installed so lets add that: apt update && apt install bash-completion
Source the main bash-completion script: source /usr/share/bash-completion/bash_completion
Typing ls --<tab><tab> should now show completions e.g. --all etc
Source the syft completions script: source <(syft completion bash)
Typing syft <tab><tab> should now show completions

DanHam · 2023-03-22T16:06:44Z

Also: you will need to sign-off your commits.

Shall I fix that typo (package -> packages) and force push with a signed commit?

kzantow · 2023-03-22T16:12:14Z

Shall I fix that typo (package -> packages) and force push with a signed commit?

Again, I think this should be removed. And force-push a signed-off commit, yes 👍

kzantow · 2023-03-22T16:29:15Z

Ahh, this was the magic sauce:

apt update && apt install bash-completion
source /usr/share/bash-completion/bash_completion
install snapshot/linux-build_linux_amd64_v1/syft /usr/local/bin

I think we can move forward after the aforementioned changes are done, thanks @DanHam

DanHam · 2023-03-22T17:04:43Z

Thanks Keith,

I've now pushed up a signed commit with changes.

I've set the usage message to fmt.Sprintf('%s [command] [flags]",...) as this seems to make the most sense for the main syft command (see my comment above - #1688 (comment))

Please let me know if you require any additional changes.

kzantow · 2023-03-22T17:12:26Z

@DanHam I hope this explanation makes sense why I still feel this should be "%s [SOURCE]": #1688 (comment)

…3/cobra The spf13/cobra library requires and uses the first command given in the usage to correctly generate the shell completion script. Signed-off-by: DanHam <DanHam@users.noreply.github.com>

kzantow

👍 thanks for sticking through this, @DanHam !

DanHam · 2023-03-22T17:40:03Z

I hope this explanation makes sense...

It does! Thank you for your patience!!!

Hopefully, that last push should be OK.

kzantow · 2023-03-22T17:44:47Z

FYI -- I tested this out with bash based on the instructions @DanHam provided:

$ syft <tab>
attest      (Generate an SBOM as an attestation for the given [SOURCE] container image)
completion  (Generate the autocompletion script for the specified shell)
convert     (Convert between SBOM formats)
help        (Help about any command)
login       (Log in to a registry)
packages    (Generate a package SBOM)
version     (show the version)

$ syft completion <tab>
bash        (Generate the autocompletion script for bash)
fish        (Generate the autocompletion script for fish)
powershell  (Generate the autocompletion script for powershell)
zsh         (Generate the autocompletion script for zsh)

…13/cobra (anchore#1688) Signed-off-by: DanHam <DanHam@users.noreply.github.com>

kzantow reviewed Mar 22, 2023

View reviewed changes

Fix shell completion by adding missing usage message required by spf1…

9d25d3f

…3/cobra The spf13/cobra library requires and uses the first command given in the usage to correctly generate the shell completion script. Signed-off-by: DanHam <DanHam@users.noreply.github.com>

kzantow approved these changes Mar 22, 2023

View reviewed changes

kzantow merged commit d02c56a into anchore:main Mar 22, 2023

DanHam deleted the shell-completion branch March 22, 2023 17:46

This was referenced Apr 10, 2023

Bump github.com/anchore/syft from 0.71.0 to 0.76.1 valllabh/security-cli#22

Merged

build(deps): bump github.com/anchore/syft from 0.72.0 to 0.77.0 docker/buildkit-syft-scanner#46

Merged

GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024

fix: shell completion by adding missing usage message required by spf…

6ce47bc

…13/cobra (anchore#1688) Signed-off-by: DanHam <DanHam@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shell completion by adding missing usage message required by spf13/cobra #1688

Fix shell completion by adding missing usage message required by spf13/cobra #1688

DanHam commented Mar 22, 2023

kzantow commented Mar 22, 2023

kzantow Mar 22, 2023

DanHam Mar 22, 2023

kzantow Mar 22, 2023

DanHam Mar 22, 2023 •

edited

Loading

kzantow Mar 22, 2023 •

edited

Loading

DanHam Mar 22, 2023

kzantow Mar 22, 2023

DanHam Mar 22, 2023

DanHam commented Mar 22, 2023 •

edited

Loading

DanHam commented Mar 22, 2023

kzantow commented Mar 22, 2023

kzantow commented Mar 22, 2023

DanHam commented Mar 22, 2023

kzantow commented Mar 22, 2023 •

edited

Loading

kzantow left a comment

DanHam commented Mar 22, 2023

kzantow commented Mar 22, 2023

Fix shell completion by adding missing usage message required by spf13/cobra #1688

Fix shell completion by adding missing usage message required by spf13/cobra #1688

Conversation

DanHam commented Mar 22, 2023

kzantow commented Mar 22, 2023

kzantow Mar 22, 2023

Choose a reason for hiding this comment

DanHam Mar 22, 2023

Choose a reason for hiding this comment

kzantow Mar 22, 2023

Choose a reason for hiding this comment

DanHam Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

kzantow Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

DanHam Mar 22, 2023

Choose a reason for hiding this comment

kzantow Mar 22, 2023

Choose a reason for hiding this comment

DanHam Mar 22, 2023

Choose a reason for hiding this comment

DanHam commented Mar 22, 2023 • edited Loading

DanHam commented Mar 22, 2023

kzantow commented Mar 22, 2023

kzantow commented Mar 22, 2023

DanHam commented Mar 22, 2023

kzantow commented Mar 22, 2023 • edited Loading

kzantow left a comment

Choose a reason for hiding this comment

DanHam commented Mar 22, 2023

kzantow commented Mar 22, 2023

DanHam Mar 22, 2023 •

edited

Loading

kzantow Mar 22, 2023 •

edited

Loading

DanHam commented Mar 22, 2023 •

edited

Loading

kzantow commented Mar 22, 2023 •

edited

Loading