-
Notifications
You must be signed in to change notification settings - Fork 349
Conversation
Hi @chenrui333. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Thanks, but I think we should begin with updating vendored pkgs with vndr |
go.mod
Outdated
github.com/containerd/typeurl v0.0.0-20190228175220-2a93cfde8c20 | ||
github.com/containernetworking/plugins v0.7.6 | ||
github.com/davecgh/go-spew v1.1.1 | ||
github.com/docker/distribution v2.7.1+incompatible |
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.
you need to vendor master
# github.com/containerd/cri/pkg/util
pkg/util/image.go:32:9: undefined: reference.ParseDockerRef
Makefile:87: recipe for target '_output/containerd' failed
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.
requested shipping v2.8.0 here: distribution/distribution#3085
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.
or v2.7.2 distribution/distribution#3002
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.
ping @dmcgowan
will circle back tonight. |
Rebased the change to the latest master branch and build with |
As I have upgraded docker/distribution to go1.14, also going to bump this PR to v1.14. |
A bit more info here containerd/containerd#4209 (comment) but in short containerd/cri can not update to modules until the dependency on k8s.io/kubernetes is removed. If you depend on k8s.io/kubernetes you are going to make everybodies life difficult if they want to use the containerd client. |
pls rebase.. we are off travis now.. and on github actions.. https://github.com/containerd/cri/blob/master/.github/workflows/ci.yml |
fyi what it would take to remove k8s.io/kubernetes dependency - #1463 |
Nice!! |
Needs rebase. Anyone interested in carrying this? As we have removed dependency on k/k (thanks @dims), this one is probably easy now. |
@AkihiroSuda we should probably concentrate on collapsing this repo with containerd/containerd and do it there? |
Repo unification might not happen until containerd v1.5 (#1479 (comment)), but migrating to go.mod (of CRI plugin, at least) seems safe to have in v1.4. |
@AkihiroSuda i read that comment as - we can do the collapse as soon as we cut a release branch for 1.4 :) which is hopefully soon. we can always experiment if someone has the time here/now! |
I am still here, doing this now. |
Based on the feedback I got, I am reverting back to |
Currently failed with
|
that's weird; the go mod specifies containerd v1.4.0-beta.0 (we should probably update to beta.1), but that version should have the RunningInUserns; https://github.com/containerd/containerd/blob/v1.4.0-beta.0/sys/userns_linux.go#L33 |
let me try a new rebase again. |
include backport fix remove k8s.io/kubernetes (per #1463) remove `github.com/docker/distribution` dependency containerd/containerd#4251 and #1484 Signed-off-by: Rui Chen <[email protected]>
Signed-off-by: Rui Chen <[email protected]>
Can I get a restart for this build,
|
I don't think I have access / a way to restart (might want to push again to trigger). That said, I do see some failures on AppVeyor that look relevant;
Do we need to set |
Hm.. actually, looks like it's not there in the vendored version as well. I see Looks like Not sure if kubernetes already switched to klog v2.2.0, because that version looks to have updated to the newer (v0.2.0) version; https://github.com/kubernetes/klog/releases/tag/v2.2.0 |
kubernetes master looks to be still on klog v2.1.0 (https://github.com/kubernetes/kubernetes/blob/a6a3bf2eb4a0b7e9f86f991ffc24b72beafce2f4/go.mod#L128) and logr v0.1.0; https://github.com/kubernetes/kubernetes/blob/a6a3bf2eb4a0b7e9f86f991ffc24b72beafce2f4/go.mod#L254 @dims what's the procedure to update that dependency? |
@thaJeztah it's happening here kubernetes/kubernetes#92554 |
Ah, perfect! I think for now we could downgrade logr to v0.1.0 (if that helps making CI pass) |
yes.. see comment ^ #1377 (comment) |
re-running jobs.. |
on the validate vendor fail.. not sure if ../project validate vendor should work or not. Might have to switch to a different validator to get passed that one. |
Signed-off-by: Rui Chen <[email protected]>
Signed-off-by: Rui Chen <[email protected]>
Signed-off-by: Rui Chen <[email protected]>
Looks like we're getting closer; I see vendor validation is failing:
And that is because this PR adds a if [ -f vendor.conf ]; then
rm -rf vendor/
vndr |& grep -v -i clone
else
go mod tidy
...
... So if it finds a |
and if he does https://github.com/containerd/cri/blob/master/hack/utils.sh#L26 and https://github.com/containerd/cri/blob/master/hack/utils.sh#L77 will likely need to be tweaked ... Note if I remember right k8s uses our vendor.conf @dims heads up. |
oh, good catch; perhaps (just for testing) rename |
Sounds good, renaming vendor.conf now |
Signed-off-by: Rui Chen <[email protected]>
oh! I think my comment was a bit ambiguous; the utils.sh script still has to be modified (to account for the rename, as it now fails), but mostly meant: "let's look at modifying it to work with
This needs a rebase now (some dependencies were updated in |
needs rebase |
missing this earlier, I will do that after my evening walk :) |
will redo the commits tomorrow. (rebase is more than a headache) |
sorry for dropping the ball a little bit, I will do something tonight about this PR. |
The master branch on this repository has been merged into containerd/containerd. This can be tried directly in the main containerd repo now. |
🆒 Thanks!! |
Relates to containerd/containerd#3031