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

deps(*) move from github.com/pkg/errors to 'errors' and 'fmt' #4486

Closed
wants to merge 1 commit into from
Closed

deps(*) move from github.com/pkg/errors to 'errors' and 'fmt' #4486

wants to merge 1 commit into from

Conversation

mmorel-35
Copy link
Contributor

Summary

The package https://github.com/pkg/errors is not maintained anymore.
This PR replaces https://github.com/pkg/errors with official errors and fmt golang packages

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Update UPGRADE.md with any steps users will need to take when upgrading.
  • Add backport-to-stable label if the code follows our backporting policy

Signed-off-by: Matthieu MOREL [email protected]

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2022

Codecov Report

Merging #4486 (0a5bd97) into master (a066dca) will decrease coverage by 0.04%.
The diff coverage is 19.65%.

@@            Coverage Diff             @@
##           master    #4486      +/-   ##
==========================================
- Coverage   55.43%   55.39%   -0.05%     
==========================================
  Files         947      946       -1     
  Lines       58052    58062      +10     
==========================================
- Hits        32184    32164      -20     
- Misses      23320    23344      +24     
- Partials     2548     2554       +6     
Impacted Files Coverage Δ
app/kuma-cp/cmd/migrate.go 52.94% <0.00%> (ø)
app/kuma-dp/cmd/dataplane.go 52.17% <0.00%> (ø)
app/kuma-dp/pkg/dataplane/accesslogs/server.go 61.11% <0.00%> (ø)
app/kuma-dp/pkg/dataplane/accesslogs/v3/sender.go 0.00% <0.00%> (ø)
app/kuma-dp/pkg/dataplane/accesslogs/v3/server.go 13.33% <0.00%> (ø)
app/kuma-dp/pkg/dataplane/dnsserver/config_file.go 55.55% <0.00%> (ø)
app/kuma-dp/pkg/dataplane/dnsserver/dnsserver.go 72.97% <0.00%> (ø)
app/kuma-dp/pkg/dataplane/envoy/bootstrap_file.go 55.55% <0.00%> (ø)
app/kuma-dp/pkg/dataplane/envoy/compatibility.go 50.00% <0.00%> (ø)
app/kuma-dp/pkg/dataplane/envoy/envoy.go 79.34% <0.00%> (ø)
... and 333 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 a066dca...0a5bd97. Read the comment docs.

@mmorel-35 mmorel-35 marked this pull request as ready for review June 18, 2022 15:47
@mmorel-35 mmorel-35 requested a review from a team as a code owner June 18, 2022 15:47
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

I have the same reservations outlined here

@lahabana
Copy link
Contributor

OK so should we just keep it as is until there's potentially a better API in the lib? @jakubdyszkiewicz what's your opinion here?

@slonka
Copy link
Contributor

slonka commented Jun 29, 2022

Is there a risk of a security vuln not getting patched if we stay on this?

@jakubdyszkiewicz
Copy link
Contributor

Is there a risk of a security vuln not getting patched if we stay on this?

I'd say it's very low. It's not a big package and it's used in many projects.

OK so should we just keep it as is until there's potentially a better API in the lib? @jakubdyszkiewicz what's your opinion here?

Honestly, I don't have a strong opinion here.

I would be fine with fmt+error. An advantage of this approach is that if that's the way how the majority of projects handle errors then it's a bit easier to grasp the code by new contributors.
Looking at linked issues here pkg/errors#245 it seems that the majority of projects are going in this direction.

I'd be ok with a wrapper, but it has to be consistent across all Kuma-related projects (for example: kuma-net etc.). Right now I think that all Kuma related projects depend on https://github.com/kumahq/kuma so it's fine. If we were to introduce more independent modules (like thin Kuma client etc.) then this would need to be extracted to a separate project.

@lahabana
Copy link
Contributor

We've decided that this change wasn't worth it for the moment. Thanks!

@lahabana lahabana closed this Jul 12, 2022
@mmorel-35 mmorel-35 deleted the pkg/errors branch July 12, 2022 10:29
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.

6 participants