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

mark keyless pem files with b64 #2671

Merged
merged 1 commit into from
Mar 8, 2023
Merged

Conversation

developer-guy
Copy link
Member

Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com

Summary

This PR will help people understand that pem files are base encoded.

Fixes #2666

PTAL @znewman01 @ctron

Release Note

Documentation

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2023

Codecov Report

Merging #2671 (d34c750) into main (d6b9001) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2671      +/-   ##
==========================================
+ Coverage   29.55%   29.56%   +0.01%     
==========================================
  Files         151      151              
  Lines        9648     9642       -6     
==========================================
  Hits         2851     2851              
+ Misses       6358     6352       -6     
  Partials      439      439              
Impacted Files Coverage Δ
cmd/cosign/cli/manifest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/dockerfile.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@znewman01
Copy link
Contributor

LGTM but please wait for @cpanato review in case this might break somebody depending on these signatures

@developer-guy
Copy link
Member Author

LGTM but please wait for @cpanato review in case this might break somebody depending on these signatures

Yes absolutely 💯

@cpanato
Copy link
Member

cpanato commented Jan 28, 2023

What are the benefits of doing this?
We might break users' workflows on this, and if we want to do this, we need to communicate before.
Or just maybe add some documentation in the readme section about might be enough

@developer-guy
Copy link
Member Author

if you all agree on the change I can update the README about that for sure @cpanato.

@haydentherapper
Copy link
Contributor

+1 on just documentation and not updating goreleaser, otherwise we risk breaking users' verification flows

@znewman01
Copy link
Contributor

Could we move the files and make a symlink, along with documentation?

The current filenames are wrong, and I'd like to move in the right direction here.

Further, the next release will be a major version bump, so maybe now is the time to break things?

@developer-guy
Copy link
Member Author

Could we move the files and make a symlink?

Can you explain a bit about what needs to be done? I'm not sure that we could make it happen creating symlink in GoReleaser for these files 🤔

The current filenames are wrong, and I'd like to move in the right direction here.

Yep, I agree with you.

Further, the next release will be a major version bump, so maybe now is the time to break things?

That makes a lot of sense 🙈

@znewman01
Copy link
Contributor

Can you explain a bit about what needs to be done? I'm not sure that we could make it happen creating symlink in GoReleaser for these files 🤔

Hmm, looking into it it's actually a bit complicated.

So maybe we:

  1. Leave this as-is.
  2. Add a note to the docs (are there existing docs about how to verify releases?)
  3. When we add support for bundles, we can change the release output.

@developer-guy
Copy link
Member Author

Can you explain a bit about what needs to be done? I'm not sure that we could make it happen creating symlink in GoReleaser for these files 🤔

Hmm, looking into it it's actually a bit complicated.

So maybe we:

  1. Leave this as-is.
  2. Add a note to the docs (are there existing docs about how to verify releases?)
  3. When we add support for bundles, we can change the release output.

cool! I'm going to update this PR accordingly ASAP

@developer-guy
Copy link
Member Author

I've updated the changes accordingly and added documentation about PEM files.

PTAL @znewman01 @cpanato

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
@developer-guy
Copy link
Member Author

any updates on this? kindly ping @znewman01

@znewman01 znewman01 merged commit d31dcff into sigstore:main Mar 8, 2023
@github-actions github-actions bot added this to the v1.14.0 milestone Mar 8, 2023
dmitris pushed a commit to dmitris/cosign that referenced this pull request Mar 24, 2023
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
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.

"pem" file of the release aren't PEM files
5 participants