Skip to content

synchronize replace rules in integration/client go.mod with main go.mod#5272

Merged
fuweid merged 2 commits intocontainerd:masterfrom
thaJeztah:sync_test_replaces
Apr 6, 2021
Merged

synchronize replace rules in integration/client go.mod with main go.mod#5272
fuweid merged 2 commits intocontainerd:masterfrom
thaJeztah:sync_test_replaces

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

Add some comments explaining how the go.mod files relate to each other.

Comment thread integration/client/go.mod Outdated
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I updated the go.mod by removing all require rules, except for github.com/Microsoft/hcsshim/test, and let go mod tidy add back the versions (based on the go mod dependency tree)

Comment thread go.mod Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we need to do this for integration/client, does that mean any other random client importing containerd/containerd needs to copy over these replace rules too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes; replace rules are only respected locally, and ignored everywhere else (they were designed for local debugging only), so every consumer must copy those rules; which is why we should try to get rid of them

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Buildkit at some point had (I think deliberately) versions that didn't compile unless overridden for that reason (which was needed because go mod wasn't able to resolve the correct version and it broke several times) moby/buildkit#1425 and issues linked from/to that

Comment thread integration/client/go.mod Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like we need more effort to sync the version between two go.mod. sad :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

github.com/containerd/containerd v1.5.0-beta.3

could we possible to change it to github.com/containerd/containerd v0.0.0. In main branch, the tag doesn't make senses.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://github.com/containerd/containerd/blob/master/integration/client/benchmark_test.go#L17

and I think we need to change the package name from containerd to client.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

could we possible to change it to github.com/containerd/containerd v0.0.0. In main branch, the tag doesn't make senses.

I tried replacing, but it became a bit messy; I also tried changing it to version v1.0.0 (to better show "hey, this version is bogus"), but Microsoft/hcsshim/test depends on containerd/containerd v1.5.0-beta.4, so go mod changed it back to v1.5.0-beta.4.

and I think we need to change the package name from containerd to client.

yes, makes sense; I pushed a commit to rename it

@crosbymichael
Copy link
Copy Markdown
Member

Why are there two go mods in the first place?

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Mar 26, 2021

Why are there two go mods in the first place?

To make it a separate module with its own dependency tree; this prevented dependencies that are only used in this test suite becoming a dependency of containerd itself. It's partially fixed now (since microsoft/hcsshim#984) but before that PR, importing microsoft/hcsshim als caused whole of kubernetes (and all of its dependencies) to become a dependency of containerd (and vendored)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

asdlkjasdlkj

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the sync_test_replaces branch from fd8f9e7 to 1faca34 Compare March 30, 2021 14:24
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 30, 2021

Build succeeded.

@thaJeztah
Copy link
Copy Markdown
Member Author

@fuweid @dims PTAL

Copy link
Copy Markdown
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@fuweid fuweid merged commit ff05d49 into containerd:master Apr 6, 2021
@thaJeztah thaJeztah deleted the sync_test_replaces branch April 6, 2021 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants