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.

Sign up for GitHub

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

Jump to bottom

all: remove dependency on deprecated github.com/pkg/errors #834

Merged
merged 5 commits into from
May 20, 2022
Merged

all: remove dependency on deprecated github.com/pkg/errors #834

merged 5 commits into from
May 20, 2022

Conversation

zchee
Copy link
Contributor

@zchee zchee commented May 19, 2022

Summary

all: remove dependency on deprecated github.com/pkg/errors

$ go install github.com/zchee/go-analyzer/pkgerrors/cmd/pkgerrors@main
$ pkgerrors -fix ./...
$ goimports -w .

Ticket Link

Update: sigstore/cosign#1880

@zchee
all: remove dependency on deprecated github.com/pkg/errors
0aa7731 
$ pkgerrors -fix ./...
$ goimports -w .

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee zchee requested review from bobcallaway and a team as code owners May 19, 2022 20:00
@zchee
Copy link
Contributor Author

zchee commented May 19, 2022

cc: @imjasonh

@zchee
Copy link
Contributor Author

zchee commented May 19, 2022

ah, still there are pkg/errors.

@imjasonh
Copy link
Member

It looks like github.com/pkg/errors stays around in imports sometimes because goimports thinks errors.New should use pkg/errors' method instead of the stdlib errors package.

I think if you delete all those pkg/errors imports and goimports -w again it should pick the right one. If it doesn't, you could always replace them yourself using sed. Maybe the x/go/tools/analysis analyzer can do this for us too, I'm not sure.

@zchee zchee marked this pull request as draft May 19, 2022 20:05
@zchee
Copy link
Contributor Author

zchee commented May 19, 2022

@imjasonh done

@zchee zchee force-pushed the pkg-errors branch 2 times, most recently from de0f2c1 to c22b929 Compare May 19, 2022 20:10
@zchee
all: fix missing convert
44a56b6 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee zchee marked this pull request as ready for review May 19, 2022 20:11
@zchee
Copy link
Contributor Author

zchee commented May 19, 2022

Maybe the x/go/tools/analysis analyzer can do this for us too, I'm not sure.

Yes can, but implements next time :D

imjasonh
imjasonh previously approved these changes May 19, 2022
View reviewed changes
@zchee
Copy link
Contributor Author

zchee commented May 19, 2022

😇

https://github.com/sigstore/rekor/runs/6514419608?check_suite_focus=true

@imjasonh
Copy link
Member

We should also be able to go mod tidy to remove the pkg/errors dependency, or at least make it indirect instead of direct.

zchee added 2 commits May 20, 2022 05:26
@zchee
pkg/types/hashedrekord: fix lint sugggestion
a6e279b 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
go.mod: go mod tidy
6015efc 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee zchee requested a review from asraa as a code owner May 19, 2022 20:27
dlorenc
dlorenc previously approved these changes May 19, 2022
View reviewed changes
cpanato
cpanato reviewed May 19, 2022
View reviewed changes
pkg/types/hashedrekord/hashedrekord.go Outdated
@@ -52,7 +52,7 @@ func (rt BaseRekordType) UnmarshalEntry(pe models.ProposedEntry) (types.EntryImp

rekord, ok := pe.(*models.Hashedrekord)
if !ok {
return nil, errors.New(fmt.Sprintf("%s, %s", "cannot unmarshal non-hashed Rekord types", pe.Kind()))
return nil, fmt.Errorf("%s, %s", "cannot unmarshal non-hashed Rekord types", pe.Kind())
Copy link
Member

Choose a reason for hiding this comment

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

why not

Suggested change
return nil, fmt.Errorf("%s, %s", "cannot unmarshal non-hashed Rekord types", pe.Kind())
return nil, fmt.Errorf("cannot unmarshal non-hashed Rekord types: %s", pe.Kind())

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, my linter still develop phase(I wrote it for sigstore), will fix by hand.

@zchee
pkg/types/hashedrekord: fix unconverted by pkgerrors
8e76861 
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
Copy link
Contributor Author

zchee commented May 19, 2022

@cpanato PTAL

cpanato
cpanato reviewed May 20, 2022
View reviewed changes
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

thanks!
LGTM

@cpanato cpanato requested a review from dlorenc May 20, 2022 09:00
@codecov-commenter
Copy link

Codecov Report

Merging #834 (8e76861) into main (ed585a3) will decrease coverage by 0.03%.
The diff coverage is 7.50%.

@@            Coverage Diff             @@
##             main     #834      +/-   ##
==========================================
- Coverage   46.66%   46.62%   -0.04%     
==========================================
  Files          60       60              
  Lines        5094     5094              
==========================================
- Hits         2377     2375       -2     
- Misses       2443     2445       +2     
  Partials      274      274
Impacted Files Coverage Δ
cmd/rekor-cli/app/log_info.go 2.09% <0.00%> (ø)
cmd/rekor-cli/app/pflags.go 84.50% <0.00%> (ø)
cmd/rekor-cli/app/upload.go 2.54% <0.00%> (ø)
pkg/sharding/ranges.go 47.27% <0.00%> (ø)
pkg/types/alpine/alpine.go 37.03% <0.00%> (ø)
pkg/types/alpine/apk.go 59.57% <0.00%> (ø)
pkg/types/hashedrekord/hashedrekord.go 37.03% <0.00%> (ø)
pkg/types/helm/helm.go 37.03% <0.00%> (ø)
pkg/types/helm/provenance.go 41.30% <0.00%> (ø)
pkg/types/helm/v0.0.1/entry.go 48.49% <ø> (ø)
... and 13 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 ed585a3...8e76861. Read the comment docs.

@cpanato
Copy link
Member

cpanato commented May 20, 2022

@bobcallaway can approve/merge when having some free cycles? thanks

@bobcallaway
Copy link
Member

@cpanato can't you see im busy lounging? 🤣

bobcallaway
bobcallaway approved these changes May 20, 2022
View reviewed changes
Copy link
Member

@bobcallaway bobcallaway left a comment

Choose a reason for hiding this comment

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

lgtm

@bobcallaway bobcallaway merged commit 547eb3c into sigstore:main May 20, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone May 20, 2022
@zchee zchee deleted the pkg-errors branch May 20, 2022 15:01
@mtrmac mtrmac mentioned this pull request Jun 30, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpanato cpanato cpanato left review comments

@bobcallaway bobcallaway bobcallaway approved these changes

@imjasonh imjasonh imjasonh left review comments

@asraa asraa Awaiting requested review from asraa

@dlorenc dlorenc Awaiting requested review from dlorenc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@zchee @imjasonh @codecov-commenter @cpanato @bobcallaway @dlorenc