-
Notifications
You must be signed in to change notification settings - Fork 84
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
go get explicit versions to avoid ambiguous imports #305
go get explicit versions to avoid ambiguous imports #305
Conversation
9475fb8
to
2907ef3
Compare
cc @skitt -- since you created googleapis/google-cloud-go#7209 |
2907ef3
to
b2e6451
Compare
b2e6451
to
a329b53
Compare
Verified for just publishing the |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, nikhita The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
# ref: https://github.com/kubernetes/publishing-bot/issues/304 | ||
# TODO(nikhita): remove this when k/k drops or bumps | ||
# cloud.google.com/go to a version > v0.105.0 | ||
go get cloud.google.com/[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.
Hmm, I'm not sure we should do this… doesn't this change the versions the module is requiring?
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.
doesn't this change the versions the module is requiring
Yes, but it ends up being a no-op for repos (e.g kube-aggregator) that don't need to require
cloud.google.com/go
because go mod tidy
will remove it from go.mod.
For repos that already require
it (e.g. apiserver), they are already at v0.97.0 so it ends up being a no-op again.
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 still seems like something that shouldn't happen at this level, and will get stale or force the wrong version eventually on some branch
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.
At least it could check that the current go.mod
pulls in cloud.google.com/go, in a version older than v0.105.0 (if any); something like
if [ "$((go list -m -json cloud.google.com/go | jq -r '.Version'; echo v0.105.0) | sort -V | head -n 1)" != v0.105.0 ]; then
Or just re-get the currently-required version:
if [ -n "$(go list -m cloud.google.com/go)" ]; then go get "cloud.google.com/go@$(go list -m -json cloud.google.com/go | jq -r '.Version')"; fi
Fixes #304
Ref: kubernetes/kubernetes#113366, kubernetes/kubernetes#114829
I haven't had the chance to do a test run of the bot to verify this but can reproduce this locally using:
go mod edit -require=golang.org/x/[email protected]
. It's not an exact representation of what the bot does since it doesn't update k8s deps but it's enough to reproduce the error.go mod tidy
. We'll see a failure:go get cloud.google.com/[email protected]
and then rungo mod tidy
again. It passes without an error.Notes:
go get cloud.google.com/[email protected]
forgo mod tidy
to succeed (due to cmd/go: ensure that 'go mod tidy' andgo get -u
do not introduce ambiguous imports golang/go#27899).go get cloud.google.com/go/compute/[email protected]
will also technically allowgo mod tidy
for kube-aggregator but it'll fail for apiserver becaue it has an explict require directive forcloud.google.com/[email protected]
.This will be necessary until we bump
cloud.google.com/go
or remove dependency on it as mentioned in #304./hold
for review
/cc @dims @liggitt