fix: plugin replace handling #32
Conversation
Fix getBuildInfo to use dependency replacements if set to ensure that check plugin doesn't incorrectly report issues for matching replace directives. Fixes krakend#33
stevenh
force-pushed
the
fix/mod-replace
branch
from
d91e9ad
to
6c32ba4
Compare
| "runtime/debug" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" |
There was a problem hiding this comment.
please, do not add extra deps just for testing
There was a problem hiding this comment.
Sorry that's just habit as it makes writing tests so much easier and is so common. I'll get that's swapped out when I'm back online, thanks for the review!
There was a problem hiding this comment.
Back at a PC now and looking at this, it isn't an extra dependency it's already used here, does that mean it's OK to keep as is?
There was a problem hiding this comment.
you're right, I didn't notice the inclusion of that dependency on #28. I'd like to remove it and just write plain tests (we already have a ton of deps and managing them is already cumbersome).
We already closed v2.7.1-CE but we can complete this and include it into v2.7.2-CE
There was a problem hiding this comment.
As its a test only dependency, so never has knock on effect with upstreams or downstreams using conflicting versions, would you consider keeping as it really does make for a much better developer experience?
There was a problem hiding this comment.
I'm sorry but we can't afford it. The CE has more than 260 deps (direct and indirect) and we already had several issues with packages using incompatible deps, breaking contracts, renaming projects, even removing the package itself! Using a testing package just for avoiding having to write some more lines is not worth the risk.
There was a problem hiding this comment.
Ack, I'll get that removed for you.
There was a problem hiding this comment.
I was about to push the change when I noticed that your dependencies already require github.com/stretchr/testify, it's just currently indirect, so removing the direct dependency is not actually going to change the required dependencies.
So I wanted to double check, do you still want me to remove it?
diff --git a/go.mod b/go.mod
index 685748f..8ff8730 100644
--- a/go.mod
+++ b/go.mod
@@ -10,7 +10,6 @@ require (
github.com/mattn/go-isatty v0.0.20
github.com/santhosh-tekuri/jsonschema/v5 v5.0.0
github.com/spf13/cobra v1.1.3
- github.com/stretchr/testify v1.9.0
golang.org/x/mod v0.19.0
)
@@ -47,7 +46,6 @@ require (
github.com/chenzhuoyu/base64x v0.0.0-20230717121745-296ad89f973d // indirect
github.com/chenzhuoyu/iasm v0.9.1 // indirect
github.com/clbanning/mxj v1.8.4 // indirect
- github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
github.com/fsnotify/fsnotify v1.6.0 // indirect
github.com/gabriel-vasile/mimetype v1.4.3 // indirect
github.com/gin-contrib/sse v0.1.0 // indirect
@@ -109,7 +107,6 @@ require (
github.com/pelletier/go-toml v1.9.5 // indirect
github.com/pelletier/go-toml/v2 v2.1.0 // indirect
github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 // indirect
- github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475 // indirect
github.com/rogpeppe/go-internal v1.11.0 // indirect
github.com/ryanuber/go-glob v1.0.0 // indirect
@@ -119,6 +116,7 @@ require (
github.com/spf13/jwalterweatherman v1.1.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
github.com/spf13/viper v1.12.0 // indirect
+ github.com/stretchr/testify v1.9.0 // indirect
github.com/subosito/gotenv v1.4.1 // indirect
github.com/tmthrgd/atomics v0.0.0-20190904060638-dc7a5fcc7e0d // indirect
github.com/tmthrgd/go-bitset v0.0.0-20190904054048-394d9a556c05 // indirect
Fix getBuildInfo to use dependency replacements if set to ensure that check plugin doesn't incorrectly report issues for matching replace directives.
Fixes #33