Attempt to clean up pkg/cosign #2018
Conversation
- remove pkg/cosign.SetSkipConfirmation, move it from a root option to a per-method option when it's actually used, remove the global bool. - add TODOs to move more methods to internal packages, or to the only places they're used. - unexport some stuff that can be unexported easily. Signed-off-by: Jason Hall <jason@chainguard.dev>
Codecov Report
@@ Coverage Diff @@
## main #2018 +/- ##
==========================================
- Coverage 26.15% 26.13% -0.03%
==========================================
Files 127 127
Lines 7425 7432 +7
==========================================
Hits 1942 1942
- Misses 5228 5235 +7
Partials 255 255
Continue to review full report at Codecov.
|
Signed-off-by: Jason Hall <jason@chainguard.dev>
I wonder if there's a good way to put this in rekor over sigstore/sigstore. It feels more appropriate there, since it's very specific to rekor's API |
|
Sorry, just saw this PR - @imjasonh , why did we remove the global skip flag? This was intentional so we wouldn't have to worry about duplication. |
Sorry about that. I removed it since it wasn't actually used in most CLI surfaces, and could be confusing/misleading (e.g., I'm fine putting it back if that's a problem, but I think it's kind of a smell. If it exists I think I'd want to make sure it ends up in an |
|
Agreed, don't want to let users use this flag. If not in internal, another option would be under |
That's what's potentially confusing though, many commands don't have a confirmation to skip, so it's misleading (I thought) to have the option. I don't feel that strongly though about having a global |
|
I don't feel super strongly. I'd like there to not be duplication for the prompt functions though for uniformity, so if they were centralized in one location, that'd be great. |
| @@ -49,9 +50,21 @@ func Clean() *cobra.Command { | |||
| return cmd | |||
| } | |||
|
|
|||
| // confirmPromptDestructive prompts the user for confirmation for an action. Ignores | |||
| // skipConfirmation. | |||
| func confirmPromptDestructive(msg string) (bool, error) { | |||
There was a problem hiding this comment.
@haydentherapper do you mean replacing this with a cosign.ConfirmPrompt(msg, false)? If so that sounds like an easy fix I can send out shortly.
There was a problem hiding this comment.
Yea, just having one destructive prompt function, one normal confirm prompt function.
Summary
pkg/cosignis a bit of a kitchen sink at the moment, with 58 total imports, and lots of exported methods that only make sense to be used by the CLI and shouldn't be used by external packages. I started doing more and wanted to stop before making this too big. I'll send another PR to move stuff to internal packages.per-method option when it's actually used, remove the global bool.
places they're used.
Things like
pkg/cosign.FindTLogEntryseem generally useful, and probably belong in sigstore/sigstore, if they're not already.Signed-off-by: Jason Hall jason@chainguard.dev
Release Note