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 use of deprecated package pkg/errors #5269

Open
1 task done
lukemassa opened this issue Jan 24, 2025 · 1 comment · Fixed by #5294
Open
1 task done

Remove use of deprecated package pkg/errors #5269

lukemassa opened this issue Jan 24, 2025 · 1 comment · Fixed by #5294
Labels
feature New functionality/enhancement

Comments

@lukemassa
Copy link
Contributor

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment.

Describe the user story

Per, pkg/errors#245 github.com/pkg/errors is deprecated and read-only. Golang's standard library now has support for wrapping errors, so we can switch to that.

Another benefit if right now there's code that looks like:

		if !os.IsNotExist(err) {
			return valid.RepoCfg{}, errors.Wrapf(err, "unable to read %s file", repoConfigFile)
		}
		// Don't wrap os.IsNotExist errors because we want our callers to be
		// able to detect if it's a NotExist err.
		return valid.RepoCfg{}, err

Because later code calls os.IsNotExist on the error. The documentation for os.IsNotExist recommends using errors.Is(err, ErrNotExists, which can walk "up the chain" of a wrapped error, thus we can get rid of that entire block above.

Describe the solution you'd like
Eliminate pkg/errors as a dependency.

Describe the drawbacks of your solution
There are a lot of calls to errors.Wrapf() that will need to be changed, but I imagine it'll be pretty mechanical.
Another concern is a lot of merge conflicts because this code is all over the code base, hopefully we can find ways to chunk it up and/or do it quickly

Describe alternatives you've considered
There are other error wrapping packages we could use, but it seems like no reason not to just the one in the stdlib.

@lukemassa
Copy link
Contributor Author

lukemassa commented Feb 1, 2025

UPDATE: I noticed this line in our CONTRIBUTING.md doc:

- **ALWAYS** use `errors.Wrap(err, "additional context...")"` instead of `fmt.Errorf("additional context: %s", err)`
because it is less likely to result in mistakes and gives us the ability to trace call stacks

This was likely written before go 1.13, since we'd be able to use %w instead of %s to effectively "wrap" the error. However it does point to an important difference between stdlib wrapped errors vs pkg/error: the stdlib implementation does not include stack traces (though interestingly there's a proposal to add them with a debug flag: golang/go#63358).

I think this leaves us with three choices for this issue:

  1. Leave pkg/errors in, despite being unmaintained
  2. Full steam ahead, remove references to pkg/errors, live without the stack traces
  3. Convert to a different third party error package like https://github.com/go-errors/errors

My personal vote is 2, but I wanted to post this here (as well as slack) to make sure folks had a chance to weigh in. My thought is that, having debugged Atlantis, I haven't felt a big benefit from stack traces, and I think the benefit is outweighed by getting ourselves onto the standard library, given that it does support the wrapping we do need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant