open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.3k stars 1.29k forks source link

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

Open koponen-styra opened 4 years ago

koponen-styra commented 4 years ago

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.

patrick-east commented 4 years ago

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).

VineethReddy02 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

koponen-styra commented 4 years ago

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.

patrick-east commented 4 years ago

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

tsandall commented 4 years ago

👍 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.

koponen-styra commented 2 years ago

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.

srenatus commented 2 years ago

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

stale[bot] commented 2 years ago

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

stale[bot] commented 2 years ago

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

koponen-styra commented 2 years ago

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

srenatus commented 2 years ago

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

srenatus commented 2 years ago

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.

stale[bot] commented 2 years ago

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

imjasonh commented 2 years ago

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.

srenatus commented 2 years ago

@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)

stale[bot] commented 2 years ago

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