Skip to content
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

Normalize certificate flag names #1868

Merged
merged 2 commits into from
May 12, 2022

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented May 11, 2022

This changes the flag names to use certificate instead of
the abbreviated cert. To avoid breaking clients, we add
a global alias that translates between the two.

Signed-off-by: Hayden Blauzvern [email protected]

Summary

Ticket Link

Fixes #1847

Release Note

Changed flag names to use certificate instead of abbreviated cert. Not a breaking change, as the previous flag names are aliased to the new ones.

@haydentherapper
Copy link
Contributor Author

cc @znewman01

This changes the flag names to use certificate instead of
the abbreviated cert. To avoid breaking clients, we add
a global alias that translates between the two.

Signed-off-by: Hayden Blauzvern <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #1868 (a7be2af) into main (7a3c04f) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1868      +/-   ##
==========================================
- Coverage   33.44%   33.37%   -0.08%     
==========================================
  Files         146      146              
  Lines        9340     9360      +20     
==========================================
  Hits         3124     3124              
- Misses       5843     5863      +20     
  Partials      373      373              
Impacted Files Coverage Δ
cmd/cosign/cli/commands.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/attest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/certificate.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/attach.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a3c04f...a7be2af. Read the comment docs.

Signed-off-by: Hayden Blauzvern <[email protected]>
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to do it as a normalization function or as an alias? I think viper supports RegisterAlias which we use elsewhere in this repo, which to me feels like a better match for what we're actually doing here.

@@ -41,8 +41,8 @@ cosign attest [flags]
```
--allow-insecure-registry whether to allow insecure connections to registries. Don't use this for anything but testing
--attachment-tag-prefix [AttachmentTagPrefix]sha256-[TargetImageDigest].[AttachmentName] optional custom prefix to use for attached image tags. Attachment images are tagged as: [AttachmentTagPrefix]sha256-[TargetImageDigest].[AttachmentName]
--cert string path to the X.509 certificate in PEM format to include in the OCI Signature
--cert-chain string path to a list of CA X.509 certificates in PEM format which will be needed when building the certificate chain for the signing certificate. Must start with the parent intermediate CA certificate of the signing certificate and end with the root certificate. Included in the OCI Signature
--certificate string path to the X.509 certificate in PEM format to include in the OCI Signature
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the help text reflect that these flags are aliased?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using RegisterAlias for cosign sign-blob to alias output and output-signature. One downside is the help text shows each of these flags on separate lines, so it looks like they're two separate flags.

Besides that, aliasing and normalizing seem pretty similar. I prefer normalize because I ideally don't want to continue to support the old flag names - Maybe we can drop the normalize function in a 2.0 release?

Another benefit of normalize over aliasing is I don't have to alias the flag in each command, there's no GlobalRegisterAlias like with normalize.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One downside is the help text shows each of these flags on separate lines, so it looks like they're two separate flags.

Ugh 😭

I prefer normalize because I ideally don't want to continue to support the old flag names

That's a sufficient argument for me.

@dlorenc dlorenc merged commit 9fe2654 into sigstore:main May 12, 2022
@github-actions github-actions bot added this to the v1.9.0 milestone May 12, 2022
@haydentherapper haydentherapper deleted the flag-cert-normalize branch January 10, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--certificate or --cert?
4 participants