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 github.com/pkg/errors #113627

Closed
alculquicondor opened this issue Nov 4, 2022 · 39 comments · Fixed by #113672
Closed

Remove dependency on github.com/pkg/errors #113627

alculquicondor opened this issue Nov 4, 2022 · 39 comments · Fixed by #113672
Labels
area/code-organization Issues or PRs related to kubernetes code organization good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture.

Comments

@alculquicondor
Copy link
Member

alculquicondor commented Nov 4, 2022

Why is this needed?

The functionality that the package provides is already present in golang's builtin errors (using %w in fmt.Errof) and the github repo is now read-only.

/kind cleanup
/priority backlog
/good-first-issue

Notes to people that want to collaborate

Avoid creating a single PR for the entire repo. Create multiple PRs, each targeting a specific SIG.

Do not submit PRs for cmd/kubeadm

@dims
Copy link
Member

dims commented Nov 4, 2022

/area code-organization

@k8s-ci-robot k8s-ci-robot added the area/code-organization Issues or PRs related to kubernetes code organization label Nov 4, 2022
@dims dims added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Nov 4, 2022
@alculquicondor
Copy link
Member Author

/kind cleanup

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 4, 2022
@BenTheElder
Copy link
Member

The functionality that the package provides is already present in golang's builtin errors (using %w in fmt.Errof) and the github repo is now read-only.

this is only partially true, e.g. %w does not provide stack traces

@falser101
Copy link
Contributor

/assign

@alculquicondor
Copy link
Member Author

@BenTheElder I see. Should we close this issue then?

@BenTheElder
Copy link
Member

BenTheElder commented Nov 7, 2022

I think it depends on how it's being used. If the traces are not the goal then we can do a simple substitution. It's probably best to drop read only dependencies ASAP but it's worth noting that if we depended on the full functionality in any way we'll need a more complex replacement (KIND did/does, not sure about k/k)

@mythi
Copy link
Contributor

mythi commented Nov 8, 2022

I think it depends on how it's being used. If the traces are not the goal then we can do a simple substitution. It's probably best to drop read only dependencies ASAP

k/k git log suggests that some cleanup has already been done in the past (maybe in other commits too):

$ git log --format="%ai %h %s" |grep pkg/\errors|head
2022-07-26 16:44:59 +0800 6cab9800a71 kubeadm: prefer to use pkg/errors package and cleanup fmt.Errorf
2021-09-13 13:02:59 +0530 c6b82bddd6e removing usage of github.com/pkg/errors from test/conformance/image/go-runner/ directory
2021-06-23 22:14:18 +0800 e2e1c94f063 use native error instead of github.com/pkg/errors
2021-06-23 10:14:19 +0800 ceb42d09389 Update github.com/pkg/errors with go native errors pkg
2021-06-23 17:06:43 +0800 674802147c8 update to remove github.com/pkg/errors
2021-06-23 18:13:44 +0800 e7a240395e9 uses native errors instead of github.com/pkg/errors
2021-06-23 17:46:23 +0800 12dcd2f84d4 Remove usage of github.com/pkg/errors
2021-06-22 11:54:56 +0800 4b140218747 update github.com/pkg/errors to go native errors pkg in staging
2021-06-23 08:07:05 +0530 992993257d8 Removed usage of github.com/pkg/errors
2021-06-22 10:43:30 +0530 ec93b3b0be4 Stop using github.com/pkg/errors

This issue triggered 3 PRs and 2 of them are merged.

but it's worth noting that if we depended on the full functionality in any way we'll need a more complex replacement (KIND did/does, not sure about k/k)

there does not seem to be calls to errors.WithStack() so most cases it looks it's just wrapping errors.

@neolit123
Copy link
Member

for cmd/kubeadm in kubernetes/kubernetes we decided to not drop usage of pkg/errors even if it's a deprecated project.

prior ticket and comments are here
#103043 (comment)

1 similar comment
@neolit123
Copy link
Member

for cmd/kubeadm in kubernetes/kubernetes we decided to not drop usage of pkg/errors even if it's a deprecated project.

prior ticket and comments are here
#103043 (comment)

@alculquicondor
Copy link
Member Author

I see. Added a note.

@liggitt liggitt added this to Dependencies in code-organization subproject Nov 10, 2022
@devansh-m12
Copy link

is it still open ?

@tillknuesting
Copy link

pkg/errors should be replaced with the standard library for the following reasons:

  • archived library with no maintenance or security updates
  • the go community used pkg/errors to introduce eventually into the standard library hence the approach that was selected is more idiomatic
  • stack traces are hardly used in error wrapping and are not a reason not to refactor

@pacoxu
Copy link
Member

pacoxu commented Jan 9, 2023

On place left in

Except the k/k repo, as github.com/pkg/errors is archived, some other repos may need an update.

"github.com/pkg/errors": [
"github.com/Microsoft/go-winio",
"github.com/Microsoft/hcsshim",
"github.com/aws/aws-sdk-go",
"github.com/cockroachdb/datadriven",
"github.com/containerd/cgroups",
"github.com/containerd/continuity",
"github.com/containerd/fifo",
"github.com/containerd/go-runc",
"github.com/containerd/typeurl",
"github.com/go-logr/zapr",
"github.com/google/cadvisor",
"github.com/grpc-ecosystem/go-grpc-middleware",
"github.com/moby/ipvs",
"github.com/moby/term",
"go.etcd.io/etcd/raft/v3",
"go.uber.org/zap",
"gotest.tools/v3",
"k8s.io/system-validators",
"sigs.k8s.io/kustomize/api",
"sigs.k8s.io/kustomize/kustomize/v4",
"sigs.k8s.io/kustomize/kyaml"

@pacoxu
Copy link
Member

pacoxu commented Jan 9, 2023

/reopen

@k8s-ci-robot
Copy link
Contributor

@pacoxu: 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.

@k8s-ci-robot k8s-ci-robot reopened this Jan 9, 2023
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 9, 2023
@k8s-ci-robot
Copy link
Contributor

@alculquicondor: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@0xchabbi
Copy link

0xchabbi commented Feb 7, 2023

to remove dependency you can use the standard Go error package or creating your own handling package, this could contain custom functions for generating and handling errors.

@ashutosh887
Copy link

let me work on this issue now please
/assign

@alculquicondor
Copy link
Member Author

We are in code freeze. I suggest waiting for a couple of weeks.

@sherifhmdy
Copy link

Hello, is this still up?

@tillknuesting
Copy link

Let's get this going

@jbiers
Copy link

jbiers commented Apr 23, 2023

/assign

@jbiers
Copy link

jbiers commented Apr 25, 2023

@tillknuesting @pacoxu Just opened a PR for this issue
#117572

@juniorsaldanha
Copy link

/assign

@pacoxu
Copy link
Member

pacoxu commented Sep 7, 2023

"github.com/pkg/errors": [
"github.com/Microsoft/hcsshim",
"github.com/aws/aws-sdk-go",
"github.com/containerd/continuity",
"github.com/containerd/fifo",
"github.com/containerd/go-runc",
"github.com/containerd/typeurl",
"github.com/go-logr/zapr",
"github.com/google/cadvisor",
"github.com/grpc-ecosystem/go-grpc-middleware",
"go.uber.org/zap",
"gotest.tools/v3",
"k8s.io/kubectl",
"k8s.io/kubernetes",
"k8s.io/system-validators",
"sigs.k8s.io/kustomize/api",
"sigs.k8s.io/kustomize/kustomize/v5"
],

Update the remaining todo items.

@neolit123
Copy link
Member

/sig architecture
umbrella / code org

@k8s-ci-robot k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 8, 2023
@ArjunMenon-bit
Copy link

/assign

@moshevayner
Copy link
Member

Hey @ArjunMenon-bit!
Are you still working on this one? If not- would you mind if I take a stab at it?
Thanks!

@Git-Jiro
Copy link
Contributor

Git-Jiro commented Nov 9, 2023

Is this issue really still open?
When I do
grep -lr "github.com/pkg/errors" api cluster cmd pkg plugin
I only get hits for cmd/kubeadm.

Am I missing something?

@alculquicondor
Copy link
Member Author

/close

@k8s-ci-robot
Copy link
Contributor

@alculquicondor: Closing this issue.

In response to this:

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment