opa icon indicating copy to clipboard operation
opa copied to clipboard

Consider dropping use of github.com/pkg/errors package

Open koponen-styra opened this issue 5 years ago • 18 comments

Golang 1.13 introduced new features to the errors package that largely match the capabilities provided by the github.com/pkg/errors package. It would be good to remove this unnecessary dependency to a thirdparty library.

koponen-styra avatar Feb 26 '20 23:02 koponen-styra

There was some discussion around requiring go 1.13 in OPA over in https://github.com/open-policy-agent/opa/pull/1969#discussion_r382850242

As-is 1.13 crept in as a requirement to cmd and tester, if we make changes to use the newer functionality it would become much more widely spread (probably everything).

patrick-east avatar Feb 26 '20 23:02 patrick-east

Hey, @patrick-east I can take this up. Do you have any suggestions? I am thinking to replace github.com/pkg/errors package with errors standard package from go

VineethReddy02 avatar May 07 '20 05:05 VineethReddy02

Most of our use cases with github.com/pkg/errors are around errors.wrap() which isn't offered by standard errors package from go. But the official go docs say to use fmt.Errorf() instead of wrap(). which does a similar job.

VineethReddy02 avatar May 07 '20 06:05 VineethReddy02

@patrick-east if we are okay with dropping github.com/pkg/errors I can take this up.

VineethReddy02 avatar May 08 '20 21:05 VineethReddy02

golang 1.13 is only 8 months old, though. Not sure if we should give it a bit more time before we require 1.13+ for OPA.

koponen-styra avatar May 08 '20 21:05 koponen-styra

Lets bring it up as a topic for the next OPA community meeting, for now lets hold off on making any changes.

patrick-east avatar May 08 '20 21:05 patrick-east

👍 I agree with @patrick-east here. We need to figure out what packages are impacted and then make the call. If it impacts the rego package (or any of it's dependencies), we'll probably want to hold for a while. EDIT: Of course we can also look at usages and see if they'd be easy to port to just use fmt.Errorf or similar without breaking compatibility in a significant way.

tsandall avatar May 08 '20 21:05 tsandall

Of all the vendored dependencies, it looks like at the moment github.com/yashtewari/glob-intersection is the only one with a dependency to pkg/errors. I prepared a PR to fix that: https://github.com/yashtewari/glob-intersection/pull/2 After that, it's only a few internal uses of the errors package we have left. They can be removed as we already have parts of the codebase dependning on the 1.13 "%w" formatting.

koponen-styra avatar Aug 10 '21 22:08 koponen-styra

ℹ️ This also blocks using TinyGo with the code base, due to its use of the runtime stdlib package.

srenatus avatar Oct 07 '21 08:10 srenatus

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

stale[bot] avatar Nov 22 '21 18:11 stale[bot]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

stale[bot] avatar Jan 02 '22 03:01 stale[bot]

github.com/yashtewari/glob-intersection has removed its dependency to pkg/errors.

koponen-styra avatar Mar 25 '22 05:03 koponen-styra

thanks for the heads-up. I'll see if dependabot picks it up or if it's time for a manual update.

srenatus avatar Mar 25 '22 08:03 srenatus

I'm afraid with github.com/yashtewari/glob-intersection gone, we've still got more third party stuff using github.com/pkg/errors since this issue was opened: github.com/dgraph-io/{badger,ristretto}

But I think getting rid of this is still worthwhile; if we'd desire to make parts of OPA (notably rego|ast|topdown) buildable with tinygo (compiling to wasm), we wouldn't want to build the storage/disk pieces anyways.

srenatus avatar Mar 25 '22 08:03 srenatus

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

stale[bot] avatar Apr 24 '22 11:04 stale[bot]

Even ignoring vendored dependencies, there are quite a lot of direct dependencies on pkg/errors in the codebase:

$ git grep -l github.com/pkg/errors | grep \.go$ | grep -v vendor
ast/location/location.go
ast/parser.go
ast/parser_ext.go
ast/term.go
bundle/bundle.go
bundle/bundle_test.go
bundle/file.go
bundle/verify.go
cmd/deps.go
cmd/sign.go
compile/compile.go
download/download.go
internal/compiler/wasm/wasm.go
internal/jwx/buffer/buffer.go
internal/jwx/jwa/key_type.go
internal/jwx/jwa/signature.go
internal/jwx/jwk/ecdsa.go
internal/jwx/jwk/headers.go
internal/jwx/jwk/jwk.go
internal/jwx/jwk/key_ops.go
internal/jwx/jwk/rsa.go
internal/jwx/jwk/symmetric.go
internal/jwx/jws/headers.go
internal/jwx/jws/jws.go
internal/jwx/jws/sign/ecdsa.go
internal/jwx/jws/sign/hmac.go
internal/jwx/jws/sign/rsa.go
internal/jwx/jws/sign/sign.go
internal/jwx/jws/verify/ecdsa.go
internal/jwx/jws/verify/hmac.go
internal/jwx/jws/verify/rsa.go
internal/jwx/jws/verify/verify.go
internal/runtime/init/init.go
internal/wasm/encoding/reader.go
plugins/bundle/status.go
plugins/status/plugin.go
server/server.go
server/server_test.go
test/wasm/cmd/wasm-rego-testgen/main.go
topdown/tokens.go

If there's interest in cleaning these up I can send a PR.

imjasonh avatar May 16 '22 16:05 imjasonh

@imjasonh Thanks a bunch! I've got the (not so) secret mission of running the compiler and maybe even the evaluator in Wasm via tinygo; removing this package from our code will go a long way towards that. The compiler should have little external dependencies; the evaluator might have a few (due to builtin functions... of which we could also not support a few when running under Wasm)

srenatus avatar May 16 '22 18:05 srenatus

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

stale[bot] avatar Jun 15 '22 18:06 stale[bot]