Conversation
3c5f8cc to
e54c2ff
Compare
2049cf3 to
78cbc04
Compare
eb5108d to
3435fa3
Compare
f3cda46 to
20c5213
Compare
Makefile
Outdated
There was a problem hiding this comment.
binary alone should be enough
Makefile
Outdated
There was a problem hiding this comment.
You are testing the docker daemon that's provided by Jenkins, not the docker daemon in the codebase (ok you bind mount the binary but that's not enough). If you look at how test-docker-py works as an example, that runs inside the container: $(DOCKER_RUN_DOCKER) hack/make.sh dynbinary test-docker-py and buildkit should do the same. So you do need a hack/make/test-buildkit in moby that will call into buildkit's /hack/test integration from within the container.
There was a problem hiding this comment.
The reason this ended up being changed was because the buildkit integration tests also try to launch containers and the docker-in-docker stuff was likely to be a source of problems and confusion, although I don't remember the specific issues I was running into. I'll go reread the conversation with @thaJeztah and play around with reverting it back; thanks!
|
@SamWhited i added b43076e to always generate a bundle tgz for the buildkit step. see b43076e#diff-58231b16fdee45a03a4ee3cf94a9f2c3R113 for how it collects files to put into the tgz. |
|
💚 tests! Going to work on getting a more recent commit of Docker vendored in buildkit, and then update the buildkit commit used in these tests and re-enable all the other tests now. |
|
Roadmap:
|
|
@tibor can you mark this as ready for review? I don't have permissions. Thanks! |
|
moved it out of draft |
|
What's current status? |
|
Just needs review and merge, IIRC, all requested changes should have been made or explained. |
|
@SamWhited could you rebase the PR to force CI to run again? |
Dockerfile
Outdated
There was a problem hiding this comment.
this looks to be v2.7.0-rc.0 distribution/distribution@17b3ff1
Could you use v2.7.1 here (pin by commit: 2461543d988979529609e8cb6fca9ca190dc48da, but add a comment what version it is)
(Also curious; was this needed to make the tests pass?)
There was a problem hiding this comment.
Fixed. I don't remember why this was needed now, sadly.
|
As soon as #40814 is merged this can be rebased, pointed at vendor, and then it will also be ready for merge. /cc @tonistiigi |
hack/make/test-buildkit
Outdated
There was a problem hiding this comment.
buildkitd should not be needed
Signed-off-by: Sam Whited <sam@samwhited.com>
Signed-off-by: Andrew Hsu <andrewhsu@docker.com> Signed-off-by: Sam Whited <sam@samwhited.com>
Signed-off-by: Sam Whited <sam@samwhited.com>
Signed-off-by: Sam Whited sam@samwhited.com