-
Notifications
You must be signed in to change notification settings - Fork 519
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
#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
@@ -1,6 +1,6 @@ | |||
module github.com/envoyproxy/go-control-plane/contrib | |||
|
|||
go 1.21 | |||
go 1.22.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it would be great if the go.mod version can remain the same for this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/[email protected] => ../ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
…ure envoy API sync does not purge files it should not Signed-off-by: Valerian Roche <[email protected]>
2d17ab9
to
7c5afa2
Compare
Signed-off-by: Valerian Roche <[email protected]>
7c5afa2
to
2972acd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a nit otherwise LGTM. I'm a maintainer from the google-cloud-go project affected by this issue. Thanks for the fix.
@@ -2,8 +2,12 @@ module github.com/envoyproxy/go-control-plane/envoy | |||
|
|||
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/[email protected] => ../ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the section you are referring to is:
cd path-to/github.com/my-repo/mig
go mod edit -replace github.com/[email protected]=../
go test ./...
go mod edit -dropreplace github.com/[email protected]
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 -u
command 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