Skip to content

go.mod: add warnings about overridden versions#5208

Closed
thaJeztah wants to merge 1 commit intocontainerd:mainfrom
thaJeztah:add_replace_warnings
Closed

go.mod: add warnings about overridden versions#5208
thaJeztah wants to merge 1 commit intocontainerd:mainfrom
thaJeztah:add_replace_warnings

Conversation

@thaJeztah
Copy link
Member

Adding warning comments to go.mod to guide users to the actual version that is used (and tested against).

In the "replace" section, adding comments that explain the reason for pinning some of the dependencies to specific versions.

Adding warning comments to go.mod to guide users to the actual version
that is used (and tested against).

In the "replace" section, adding comments that explain the reason for
pinning some of the dependencies to specific versions.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Comment on lines +73 to 74
// FIXME gogo/googleapis must be v1.3.2 <BECAUSE>
github.com/gogo/googleapis => github.com/gogo/googleapis v1.3.2
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This replace was added in 6a2df5f (#4760). Most likely, this was only done to keep the vendored versions the same as before switching to go.mod, but we should add an explanation (and check if this is still required)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

googleapis should be safe, it was kept because bumping will cause some pb.go changed, again IIRC 😂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking! I'll wait for others to comment if they have more details, and will update the PR based on that.

Comment on lines +75 to 82
// FIXME golang/protobuf must be v1.3.5 <BECAUSE>
github.com/golang/protobuf => github.com/golang/protobuf v1.3.5
// urfave/cli must be <= v1.22.1 due to a regression: https://github.com/urfave/cli/issues/1092
github.com/urfave/cli => github.com/urfave/cli v1.22.1
// FIXME genproto must be v0.0.0-20200224152610-e50cd9704f63 <BECAUSE>
google.golang.org/genproto => google.golang.org/genproto v0.0.0-20200224152610-e50cd9704f63
// FIXME olang.org/grpc be v1.27.1 <BECAUSE>
google.golang.org/grpc => google.golang.org/grpc v1.27.1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three replace rules (protobuf , genproto, grape) were added in de059b1 (#4760). Most likely, this was only done to keep the vendored versions the same as before switching to go.mod, but we should add an explanation (and check if this is still required)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, bumping grpc will cause protobuf to be bumped, thus containerd/ttrpc#62

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Yes, I recalled there were some specific issues with grpc, but forgot which ones that were (but the motivation for me to open this PR, because it's very easy to miss for users, and (IIRC), some of the incompatibilities may not "fail hard", but only occur "sometimes", so better to have clear warnings

@thaJeztah
Copy link
Member Author

@AkihiroSuda @dmcgowan I could use some help on this one; for some of the replace rules, there's no (documented) motivation; we should check if they're still relevant (and if so, add the description)

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 16, 2021

Build succeeded.

@k8s-ci-robot
Copy link

@thaJeztah: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-containerd-node-e2e 9e85acd link /test pull-containerd-node-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@kzys
Copy link
Member

kzys commented Apr 25, 2022

We should be able to remove these replace's after merging #6841 😎

@kzys
Copy link
Member

kzys commented Jun 6, 2022

Closing. We don't have replace directives anymore.

@kzys kzys closed this Jun 6, 2022
@thaJeztah thaJeztah deleted the add_replace_warnings branch June 6, 2022 18:35
@thaJeztah
Copy link
Member Author

Ah, yup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/needs-update Awaiting contributor update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants