-
Notifications
You must be signed in to change notification settings - Fork 558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/file flag completion improvements #4028
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4028 +/- ##
==========================================
- Coverage 40.10% 36.76% -3.35%
==========================================
Files 155 210 +55
Lines 10044 13386 +3342
==========================================
+ Hits 4028 4921 +893
- Misses 5530 7847 +2317
- Partials 486 618 +132 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Just one question
@@ -107,5 +108,5 @@ func (o *AttestBlobOptions) AddFlags(cmd *cobra.Command) { | |||
|
|||
cmd.Flags().StringVar(&o.RFC3161TimestampPath, "rfc3161-timestamp-bundle", "", | |||
"path to an RFC 3161 timestamp bundle FILE") | |||
_ = cmd.Flags().SetAnnotation("rfc3161-timestamp-bundle", cobra.BashCompFilenameExt, []string{}) | |||
// _ = cmd.MarkFlagFilename("rfc3161-timestamp-bundle") // no typical extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm, is there any purpose to including this? As in, will shell completion still work with rfc3161-...
for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is discussed in detail in the b6ac9b3 commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But TL;DR: yes :)
cmd/cosign/cli/options/attach.go
Outdated
@@ -80,7 +80,7 @@ func (o *AttachSBOMOptions) AddFlags(cmd *cobra.Command) { | |||
|
|||
cmd.Flags().StringVar(&o.SBOM, "sbom", "", | |||
"path to the sbom, or {-} for stdin") | |||
_ = cmd.Flags().SetAnnotation("sbom", cobra.BashCompFilenameExt, []string{}) | |||
cmd.MarkFlagFilename("sbom", sbomExts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix lint here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Be more consistent across with extensions accepted/filtered, add some. Also, mark and comment out cases where there are no known typical filename extensions for flags taking filename arguments, to make it obvious that they have not been inadvertently omitted. Marking a flag as filename without specifying extensions is a no-op, and actually considered a bug per commentary in cobra sources: https://github.com/spf13/cobra/blob/41b26ec8bb59dfba580f722201bf371c4f5703dd/completions.go#L387-L390 Closes sigstore/community#538 Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
aa5537c
to
b6ac9b3
Compare
Summary
As described in sigstore/community#538
To test, generate and install shell completions and invoke completion for flags taking filenames as arguments, observe results.
Release Note
NONE
Documentation
NONE