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

Replace github.com/pkg/errors dependency with native error wrapping #2549

Closed
19 tasks done
saschagrunert opened this issue Jun 8, 2022 · 12 comments · Fixed by #2555 or #2581
Closed
19 tasks done

Replace github.com/pkg/errors dependency with native error wrapping #2549

saschagrunert opened this issue Jun 8, 2022 · 12 comments · Fixed by #2555 or #2581
Assignees
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/release Categorizes an issue or PR as relevant to SIG Release.

Comments

@saschagrunert
Copy link
Member

saschagrunert commented Jun 8, 2022

The PR #2478 got reverted because it caused troubles with the error wrapping conversion.

We should still do that, because the package github.com/pkg/errors is now in maintenance mode due to the golang native error wrapping. The goal is to do that on a package-by-package basis, where everyone is welcome to contribute.

The basic conversion rules:

  • use the errors import in favor of github.com/pkg/errors
  • convert errors.Errorf to fmt.Errorf
  • convert errors.Wrap(err, "…") to fmt.Errorf("…: %w", err) and check that err != nil before
  • convert errors.Wrapf(err, "… %s: %s", foo, bar) to fmt.Errorf("… %s: %s: %w", foo, bar err) and check that err != nil before

Packages to be converted:

@saschagrunert saschagrunert added kind/bug Categorizes issue or PR as related to a bug. sig/release Categorizes an issue or PR as relevant to SIG Release. area/release-eng Issues or PRs related to the Release Engineering subproject labels Jun 8, 2022
@saschagrunert saschagrunert added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. and removed needs-priority labels Jun 8, 2022
@palnabarun
Copy link
Member

Anyone willing to take up one of the above packages, please comment here with the package that you will refactor before starting to work on it.

@Debanitrkl
Copy link
Member

I can take up the ci-reporter, krel and kubepkg

@palnabarun
Copy link
Member

/assign

(for tracking)

@Priyankasaggu11929
Copy link
Member

Priyankasaggu11929 commented Jun 8, 2022

I can take:

cmd/publish-release
cmd/schedule-builder
pkg/build
pkg/kubecross
pkg/kubepkg
pkg/release
pkg/testgrid

@ArkaSaha30
Copy link
Member

I will take up:

pkg/changelog
pkg/cve
pkg/fastforward
pkg/gcp

@puerco
Copy link
Member

puerco commented Jun 8, 2022

Folks, as @saschagrunert said in the third case above, pay attention to the following construct we sometimes have in the code. It was the one that broke krel in the reverted pr:

// This would return nil or an error, so simple search and replace will not work:
return errors.Wrap(err, "XXXXX")

// It should instead be replaced with something like this:
if err != nil {
   return fmt.Errorf("XXXXX: %w")
}
return nil

@pradeepnnv
Copy link
Contributor

I'll work on pkg/mail.

@DiptoChakrabarty
Copy link
Member

I can work on cmd/release-notes and pkg/notes

@jasonbraganza
Copy link
Member

I will take on pkg/announce and pkg/binary

@Priyankasaggu11929
Copy link
Member

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Jun 13, 2022
@k8s-ci-robot
Copy link
Contributor

@Priyankasaggu11929: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@saschagrunert
Copy link
Member Author

Thank you all! We're almost done with this, just found one missing place in #2581 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. sig/release Categorizes an issue or PR as relevant to SIG Release.
Projects
None yet
10 participants