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

Remove dependency on deprecated github.com/pkg/errors #1887

Merged
merged 11 commits into from May 17, 2022

Conversation

zchee
Copy link
Contributor

@zchee zchee commented May 16, 2022

Summary

Remove dependency on deprecated github.com/pkg/errors.

Ticket Link

Update: #1880

Release Note

???

@zchee
Copy link
Contributor Author

zchee commented May 16, 2022

cc: @imjasonh I started cleanup pkg/errors dependency, but still draft.

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
zchee added 2 commits May 17, 2022 03:19
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
Copy link
Contributor Author

zchee commented May 16, 2022

This pull request is a huge change. Therefore, split the commit by the package one by one, as another contributor's PR may conflict with the main branch after merging this pull request.
I'll do squash last time if need squash in terms of commit messages.

@zchee
Copy link
Contributor Author

zchee commented May 16, 2022

@imjasonh Great, let's remove deprecated packages from the Go community world :P (I think it will be long-term lol

imjasonh
imjasonh previously approved these changes May 16, 2022
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
zchee added 5 commits May 17, 2022 04:13
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
Copy link
Contributor Author

zchee commented May 16, 2022

@imjasonh
I wrote this change by hand this time and it was very annoying lol
I'll write an analyzer that rewrites source code using "x/tools/go/analysis" and send a pull request to fulcio and rekor, with it. It would be useful in other projects as well.


Well, PTAL.

@zchee zchee marked this pull request as ready for review May 16, 2022 19:24
@imjasonh
Copy link
Member

I'll write an analyzer that rewrites source code using "x/tools/go/analysis" and send a pull request to fulcio and rekor, with it. It would be useful in other projects as well.

Oh please oh please oh please. If you do this I will be forever in your debt. 🙏

@imjasonh
Copy link
Member

https://github.com/sigstore/cosign/runs/6458613293?check_suite_focus=true#step:12:286 looks like a bad %w usage.

I've noticed when doing this in other repos that sometimes you'll have a errors.Wrapf("hello", someString) (without a %s), which nothing complains about. But then when you change it to fmt.Errorf things notice the missing placeholder and complain, or fail, or have bad output. It's good to find them, but it's one more annoying thing to have to look for.

@zchee
Copy link
Contributor Author

zchee commented May 16, 2022

@imjasonh Ah, mistake, will fix asap.

It's good to find them, but it's one more annoying thing to have to look for.

Yeah, it seems to pkg/errors bug(?), I'm surprised.

Oh please oh please oh please. If you do this I will be forever in your debt. 🙏

lol, will do.

@@ -433,7 +433,7 @@ func ValidatePolicySignaturesForAuthority(ctx context.Context, ref name.Referenc
// https://github.com/sigstore/cosign/issues/1652
sps, err := valid(ctx, ref, rekorClient, authority.Key.PublicKeys, remoteOpts...)
if err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("failed to validate public keys with authority %s for %s", name, ref.Name()))
return nil, fmt.Errorf(fmt.Sprintf("failed to validate public keys with authority %s for %s: %w", name, ref.Name()), err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix too.

zchee added 2 commits May 17, 2022 05:52
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
Copy link
Contributor Author

zchee commented May 16, 2022

@imjasonh

sigstore/cosign/runs/6458613293?check_suite_focus=true#step:12:286 looks like a bad %w usage.

I've noticed when doing this in other repos that sometimes you'll have a errors.Wrapf("hello", someString) (without a %s), which nothing complains about. But then when you change it to fmt.Errorf things notice the missing placeholder and complain, or fail, or have bad output. It's good to find them, but it's one more annoying thing to have to look for.

I found the root cause that GetEnvTargetRepository did not handle error until now. Fixed on 80ab7ef (#1887). PTALA.

@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #1887 (80ab7ef) into main (8b7017d) will increase coverage by 0.01%.
The diff coverage is 11.11%.

@@            Coverage Diff             @@
##             main    #1887      +/-   ##
==========================================
+ Coverage   33.35%   33.36%   +0.01%     
==========================================
  Files         146      146              
  Lines        9370     9372       +2     
==========================================
+ Hits         3125     3127       +2     
  Misses       5874     5874              
  Partials      371      371              
Impacted Files Coverage Δ
cmd/cosign/cli/attest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/commands.go 0.00% <0.00%> (ø)
cmd/cosign/cli/dockerfile/verify.go 41.66% <ø> (ø)
cmd/cosign/cli/fulcio/fulcio.go 21.42% <0.00%> (ø)
cmd/cosign/cli/fulcio/fulcioroots/fulcioroots.go 36.36% <0.00%> (ø)
cmd/cosign/cli/generate/generate_key_pair.go 9.21% <0.00%> (ø)
cmd/cosign/cli/load.go 0.00% <0.00%> (ø)
cmd/cosign/cli/manifest/verify.go 47.69% <0.00%> (ø)
cmd/cosign/cli/options/oidc.go 0.00% <0.00%> (ø)
cmd/cosign/cli/policy_init.go 1.40% <0.00%> (ø)
... and 32 more

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 8b7017d...80ab7ef. Read the comment docs.

@dlorenc dlorenc merged commit 03e66aa into sigstore:main May 17, 2022
@github-actions github-actions bot added this to the v1.9.0 milestone May 17, 2022
@zchee zchee deleted the deps/remove-pkg-errors branch May 17, 2022 12:50
@zchee
Copy link
Contributor Author

zchee commented May 17, 2022

@imjasonh @dlorenc @cpanato thanks! I'll fix rekor and fulcio too.

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.

None yet

5 participants