Remove use of pkg/errors - it has been deprecated and archived

[stmcginnis]

Feature Request

Found out today the pkg/errors project had been looking for maintainers for some time and had finally decided to archive the project. We should move away from using it.

pkg/errors#245

Describe alternatives you've considered

A few alternatives:

• Continue using until things break
• Wait to see if a new fork becomes maintained and switch references to that
• Use native things like fmt.Error instead

[harshanarayana]

/assign

Mind if I take a look at this ?

[harshanarayana]

With new native errors library most of what is provided by the pkg/errors is nearly available. I think it will be a safe bet to use that in place of this. However, while looking around the code base, I also found that we have a few different ways of handling the errors across the board in the TCE repo. Should we take this opportunity and create a standard errors package (github.com/vmware-tanzu/community-edition/errors) that can be re-used across the board in the TCE ? xref: https://github.com/kubernetes/apimachinery/tree/master/pkg/api/errors

[stmcginnis]

That's a good idea to take the time to get something consistent across the board Harsha.

It would be more work, but I wonder if we should get that common error handling into tanzu-framework, then pick it up in community-edition. Though we're pinned to an older version of t-f for awhile yet, so maybe that's a good long term plan that isn't actually feasible at this point. So maybe both?

Either way, I like the idea of taking the opportunity to review our error handling and seeing if we can do something more consistent and better overall.

[harshanarayana]

So maybe both?

I think that should be doable. If we create the error package on the TCE repo here in a generic way, we can port the same to the tanzu-framework and re-use it here down the line once we are ready to uptick the version of the T-f here.

Let me scan through the entire repo once and get a list of all possible ways the error packages are being consumed and see how we can replace them with a custom error package tailed made for the tanzu use-cases.

[harshanarayana]

❯ rg pkg/errors -g "*.go"
extensions/docker-desktop/apps/clustermgr/pkg/kubeconfig/internal/kubeconfig/helpers.go
7:	"sigs.k8s.io/kind/pkg/errors"

extensions/docker-desktop/apps/clustermgr/pkg/kubeconfig/internal/kubeconfig/write.go
10:	"sigs.k8s.io/kind/pkg/errors"

extensions/docker-desktop/apps/clustermgr/pkg/kubeconfig/internal/kubeconfig/read.go
11:	"sigs.k8s.io/kind/pkg/errors"

extensions/docker-desktop/apps/clustermgr/pkg/kubeconfig/internal/kubeconfig/encode.go
10:	"sigs.k8s.io/kind/pkg/errors"

extensions/docker-desktop/apps/clustermgr/pkg/kubeconfig/internal/kubeconfig/merge.go
9:	"sigs.k8s.io/kind/pkg/errors"

extensions/docker-desktop/apps/clustermgr/pkg/utils/lock.go
12:	"github.com/pkg/errors"

extensions/docker-desktop/apps/clustermgr/pkg/kubeconfig/internal/kubeconfig/remove.go
9:	"sigs.k8s.io/kind/pkg/errors"

This is not too many. I see we are using a few sigs.k8s.io/kind/pkg/errors which internally wraps the pkg/errors.

Should be pretty straight forward to get us going. I will open a DRAFT PR with proposed changes and we can get it reviewed.

[harshanarayana]

package errors

import "fmt"

type Reason string

const (
	ReasonUnknown     Reason = "Unknown"
	ReasonLockFailed  Reason = "LockFailed"
	ReasonMergeFailed Reason = "MergeFailed"
)

type TanzuError struct {
	ErrorDetails ErrorDetails `json:"errorDetails"`
}

type ErrorDetails struct {
	Message string `json:"message,omitempty"`
	Reason  Reason `json:"reason,omitempty"`
	Err     error  `json:"err"`
}

func (t *TanzuError) Error() string {
	return fmt.Sprintf("%s: %s (%s)", t.ErrorDetails.Reason, t.ErrorDetails.Message, t.ErrorDetails.Err)
}

func NewLockFailed(err error, configPath string) *TanzuError {
	return &TanzuError{
		ErrorDetails: ErrorDetails{
			Reason:  ReasonLockFailed,
			Message: fmt.Sprintf("failed to lock config file %s", configPath),
			Err:     err,
		},
	}
}

func NewMergeFailed(err error, configPath string) *TanzuError {
	return &TanzuError{
		ErrorDetails: ErrorDetails{
			Reason:  ReasonMergeFailed,
			Message: fmt.Sprintf("failed to get kubeconfig (%s) to merge", configPath),
			Err:     err,
		},
	}
}

@stmcginnis @jpmcb Here is an example of the error package I am thinking. Is this inline with what you have in mind or do I need to tune some bits of it ?

[stmcginnis]

Looks great Harsha!

[harshanarayana]

great. Let me open a PR as early as tomorrow with this change and migrate all of the items mentioned above to use this common errors packages. Thanks for your time in reviewing this.

[harshanarayana]

❯ go mod why github.com/pkg/errors
# github.com/pkg/errors
github.com/docker/docker/client
github.com/pkg/errors

I was cleaning up things and noticed that we will still be left with some dependency on the github.com/pkg/errors due to transitive requirements from things like above. This is just an FYI

[harshanarayana]

@stmcginnis @jpmcb I came across https://github.com/zchee/go-analyzer/tree/main/pkgerrors in the k8s Kind slack group.

[jpmcb]

Here is an example of the error package I am thinking

I'm actually not a big fan of this.

Go 2 isn't too far away and will include in it's standard library a complete overhaul of error handling.

I'd prefer we approach this using current paradigms in Go (probably through fmt.Errorf) instead of having to maintain another open source Go package (which people out of band of TCE may import causing a whole cycle of maintenance in itself) or import one of the many different error handling packages in the K8s and open source community (which will also come with it's own set of dependencies, CVEs, etc.)

My proposal: close #4885 in favor of removing github.com/pkg/errors and using Go 1 standard library ways of handling errors. Than, in the future, we adopt Go 2's new methods of error handling without worrying about breaking others who import something.

[harshanarayana]

Works for me. I will re-open the PR with the suggested changes.

[harshanarayana]

@jpmcb @stmcginnis PTAL at the #4914 I have opened a new one based on comment from @jpmcb