#1074: Add imports of previous root package in new subpackages to avoid dependency upgrade issues.#1075
Conversation
4e25fcd to
496e667
Compare
fad47d7 to
56f953f
Compare
|
Also added a fix to |
56f953f to
2d17ab9
Compare
contrib/go.mod
Outdated
| module github.com/envoyproxy/go-control-plane/contrib | ||
|
|
||
| go 1.21 | ||
| go 1.22.8 |
There was a problem hiding this comment.
nit: it would be great if the go.mod version can remain the same for this fix.
There was a problem hiding this comment.
This version is automatically updated due to the added dependency on the root-level go.mod which has been using 1.22.8 for some time.
If a blocker I can see to rollback this change but it is not, per se, part of the module split, as the new version of the main module would have also changed this
There was a problem hiding this comment.
In a separate PR we removed the dependency on the minor to now only depend on go 1.22, aligning with grpc-go required version.
envoy/go.mod
Outdated
| go 1.22.8 | ||
|
|
||
| // Used to resolve import issues related to go-control-plane package split (https://github.com/envoyproxy/go-control-plane/issues/1074) | ||
| replace github.com/envoyproxy/go-control-plane@v0.13.2 => ../ |
There was a problem hiding this comment.
Is there a reason for pinning the version here but not in the go.mod files for examples/dyplomat and ratelimit packages?
There was a problem hiding this comment.
Those packages inherit it through the envoy submodule, and therefore don't require it directly
…ndency upgrade issues. Those changes are based on https://go.dev/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository Fixes #1074 Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
…ure envoy API sync does not purge files it should not Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
2d17ab9 to
7c5afa2
Compare
Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
7c5afa2 to
2972acd
Compare
codyoss
left a comment
There was a problem hiding this comment.
left a nit otherwise LGTM. I'm a maintainer from the google-cloud-go project affected by this issue. Thanks for the fix.
| go 1.22 | ||
|
|
||
| // Used to resolve import issues related to go-control-plane package split (https://github.com/envoyproxy/go-control-plane/issues/1074) | ||
| replace github.com/envoyproxy/go-control-plane@v0.13.3 => ../ |
There was a problem hiding this comment.
nit: unless you need this for your project replace statements don't do anything for downstream users. So if you don't need this it can be removed.
There was a problem hiding this comment.
Thank you for the review. I took this from https://go.dev/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository, where they explicitly state this replace as a way to avoid the "ambiguous" import.
I do not have enough understanding of go.mod resolution logic to know if this actually helps or not
There was a problem hiding this comment.
I believe the section you are referring to is:
cd path-to/github.com/my-repo/mig
go mod edit -replace github.com/my-repo@v1.3.0=../
go test ./...
go mod edit -dropreplace github.com/my-repo@v1.3.0
If so this is meant to be a temporary test, as you can see be the drop command after. This is because it assumes the module is not yet published that you are referencing. In this case though the version where this module has been carved out already exists.
This causes no harm either way though, just thought I would share :)
There was a problem hiding this comment.
Thanks for the update. I think I get it better now, and they cut tag out of the intermediate commit with the replace. In order to keep it simpler to track I'll do this in two PRs. I'll create a new one to drop the replace
The module split seems to be breaking when the
get -ucommand is used.Using the link referred in this post, this change attempts to address the issue.
While I was able to test this change within one of our repository depending on this repository (for go-control-plane and envoy API), I do not know of a way to guarantee that this will resolve all cases.
Fixes #1074