Skip to content

Conversation

@dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Oct 27, 2023

The first 3 commits are preparing for the module and the last commit is generated with a script to make rebasing easier.

Commit 1: Temporarily remove the integration/client submodule. I could not figure out a clean way to move this module to use the new v2 module name since go mod will always fail lookup since there is no tag or commit with the new module name. I'm not sure how this was ever supposed to work, but replace doesn't seem to do the trick for me.

Commit 2: Temporarily removing imgcrypt since it uses v1 datatype via import where v2 are not expected. We can possibly try to clean this up so that we don't have a circular import. I was happy though that this was the only real circular dependency we had.

Commit 3: Does a move of the protoc generated files from a "v2" directory to the expected location. I'm not sure if there is a fix somewhere in the proto go world to handle this, the mv seems to work fine for now though. This seems similar to the other issues we have had with how inflexible protoc output is. This is done now just before our other fix script of the output.

Commit 4: Fixes a bunch of linter issues with integration/client which weren't checked before.

Commit 5: This commit is generated by the following script. It might be easier to look at the script before looking at the commit content.

#!/bin/sh
set -e

# Update Go mod version in go.mod
go mod edit -module github.com/containerd/containerd/v2

# Update version/version.go
perl -pi -e 's/Package = "github.com\/containerd\/containerd"/Package = "github.com\/containerd\/containerd\/v2"/g' version/version.go

# Update Makefile
perl -pi -e 's/Package="github.com\/containerd\/containerd"/Package="github.com\/containerd\/containerd\/v2"/g' Makefile

# Update oss fuzz build
perl -pi -e 's/"github.com\/containerd\/containerd\/\$pkg"/"github.com\/containerd\/containerd\/v2\/\$pkg"/g' contrib/fuzz/oss_fuzz_build.sh

# Update all import lines
for GOFILE in $(find . -name "*.go" | grep -v "./vendor/" ); do
  perl -pi -e 's/([\t]|[ ]{2,8}|import )([_\.a-zA-Z0-9]+ )?"github\.com\/containerd\/containerd(?!\/v2)(\/\S+)?"/$1$2"github.com\/containerd\/containerd\/v2$3"/g' $GOFILE
done

# Update go package in protofiles
for PROTOFILE in $(find . -name "*.proto" | grep -v "./vendor/" ); do
  perl -pi -e 's/option go_package = "github\.com\/containerd\/containerd(?!\/v2)(\/\S+)?";/option go_package = "github.com\/containerd\/containerd\/v2$1";/g' $PROTOFILE
done

set -x

# Get latest containerd for remaining circular dependencies (until fixed)
go get github.com/containerd/[email protected]

make vendor protos test binaries

git add -u && git add vendor && git commit -s -m "Update go module to github.com/containerd/containerd/v2"

Protobuf will automatically put the files generated for a v2 module into
a v2 directory. Move them to their correct location after running the
protobuild.

Signed-off-by: Derek McGowan <[email protected]>
@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@dmcgowan dmcgowan force-pushed the containerd-v2-module branch 5 times, most recently from 2c15c58 to 58cedff Compare October 28, 2023 05:32
Signed-off-by: Derek McGowan <[email protected]>
@dmcgowan dmcgowan force-pushed the containerd-v2-module branch from 58cedff to efc0253 Compare October 28, 2023 05:40
@dmcgowan dmcgowan force-pushed the containerd-v2-module branch from e8fd1b2 to 5fdf55e Compare October 30, 2023 04:03
@dmcgowan dmcgowan marked this pull request as ready for review October 30, 2023 04:58
github.com/AdamKorcz/go-118-fuzz-build v0.0.0-20230306123547-8075edf89bb0
github.com/Microsoft/go-winio v0.6.1
github.com/Microsoft/hcsshim v0.12.0-rc.0
github.com/Microsoft/hcsshim/test v0.0.0-20210227013316-43a75bb4edd3
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have import of this module. Why didnt we have this earlier? Because earlier also, it was being used in windows tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Commit 1: Temporarily remove the integration/client submodule. I could not figure out a clean way to move this module to use the new v2 module name since go mod will always fail lookup since there is no tag or commit with the new module name. I'm not sure how this was ever supposed to work, but replace doesn't seem to do the trick for me.

I believe this was the only reason this submodule was created. We will add it back if still necessary, something was complaining about the checked in rsrc_amd64.syso, but no issues with CI now.

github.com/cilium/ebpf v0.9.1 // indirect
github.com/containerd/typeurl v1.0.2 // indirect
github.com/containers/ocicrypt v1.1.6 // indirect
github.com/containerd/containerd v1.7.8 // indirect
Copy link
Member

Choose a reason for hiding this comment

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

Is this a concern (even it's an indirect dependency)? It seems like this was because of Microsoft/hcsshim...

Copy link
Member Author

@dmcgowan dmcgowan Oct 31, 2023

Choose a reason for hiding this comment

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

Not a concern in the short term, we should aim to get rid of it before the final release though. I think we would only need it if we wanted to do auto migration or interface translation for plugins. Once we get this in and tagged, then we can get plugins updated.

Copy link
Member

Choose a reason for hiding this comment

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

Can this be revisited to remove indirect dependency on an older containerd version since, the PR is merged now.

@AkihiroSuda AkihiroSuda added this pull request to the merge queue Nov 1, 2023
Merged via the queue into containerd:main with commit 19ff94b Nov 1, 2023
@thaJeztah
Copy link
Member

Completely missed that it was merged 🎉

Happy to see this done; thanks @dmcgowan !

@japottatwee

This comment was marked as spam.

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.

8 participants