fix: shell completion improvements #831
Conversation
cli/slsa-verifier/verify/options.go
Outdated
|
|
||
| cmd.Flags().StringVar(&o.AttestationPath, "attestation-path", "", | ||
| "path to a file containing the attestation") | ||
| cmd.MarkFlagFilename("attestation-path", "intoto.jsonl") |
There was a problem hiding this comment.
The filename can actually have any extension.
There was a problem hiding this comment.
Ok, removed this filter.
There was a problem hiding this comment.
Thanks, also remove it for the other subcommands.
There was a problem hiding this comment.
I see we can actually specify multiple extensions, so how about using the common ones: .sigstore, .intoto, .intoto.jsonl, .json.
There was a problem hiding this comment.
Done for both related flags. Thanks for the guidance, I'm familiar with cobra but not that much with all things SLSA.
I suppose it should be noted that at least with bash (well readline more accurately), the user can invoke completion of all filenames by hitting Meta-/ (typically Esc-/) instead of Tab. Even though that's a bit inconvenient and not too well known combo, at least there is an escape hatch if we get some of the file extension filters wrong.
| @@ -145,27 +162,34 @@ var _ Interface = (*VerifyVSAOptions)(nil) | |||
| func (o *VerifyVSAOptions) AddFlags(cmd *cobra.Command) { | |||
| cmd.Flags().StringArrayVar(&o.SubjectDigests, "subject-digest", []string{}, | |||
| "the digests to be verified. Pass multiple digests by repeating the flag. e.g. --subject-digest <digest type>:<digest value> --subject-digest <digest type>:<digest value>") | |||
| cmd.RegisterFlagCompletionFunc("subject-digest", cobra.NoFileCompletions) | |||
There was a problem hiding this comment.
If we don't add this line, would the behavior be the same?
There was a problem hiding this comment.
No; without it, all filenames in the current dir will be suggested as arguments for the flag.
scop
force-pushed
the
fix/shell-completions
branch
2 times, most recently
from
589b2f3 to
4134a9b
Compare
| if len(args) != 0 { | ||
| return nil, cobra.ShellCompDirectiveNoFileComp | ||
| } |
There was a problem hiding this comment.
This works like a shortcut to say that all of options shouldn't offer completion?
There was a problem hiding this comment.
It means that if we already have one or more positional argument, do not offer any more as completions, as verify-npm-package takes only one positional argument.
scop
force-pushed
the
fix/shell-completions
branch
from
4134a9b to
7aedf88
Compare
cli/slsa-verifier/verify/options.go
Outdated
| @@ -45,31 +45,41 @@ type VerifyOptions struct { | |||
|
|
|||
| var _ Interface = (*VerifyOptions)(nil) | |||
|
|
|||
| var CommonFilenameExtensions = []string{"sigstore", "intoto", "intoto.jsonl", "json"} | |||
There was a problem hiding this comment.
| var CommonFilenameExtensions = []string{"sigstore", "intoto", "intoto.jsonl", "json"} | |
| var CommonAttestationFilenameExtensions = []string{"sigstore", "intoto", "intoto.jsonl", "json"} |
There was a problem hiding this comment.
This is also being used for provenance filenames per earlier comments. Do we still want to rename it as suggested?
There was a problem hiding this comment.
Yes, provenance and attestation are mostly interchangeable terms, but attestation is more generic.
scop
force-pushed
the
fix/shell-completions
branch
from
7aedf88 to
8c2605e
Compare
Done. This would be useful info to have in docs/CONTRIBUTING.md |
Subject digests are not given as positional arguments, but with --subject-digest flags. Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
scop
force-pushed
the
fix/shell-completions
branch
from
8c2605e to
d7880e2
Compare
See individual commits and their messages for details.