-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Use buildah commit and bud in podman #681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
LGTM |
libpod/container_commit.go
Outdated
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.
When we "docker commit" a container there, which of these is/are set in the image's configuration when you then "docker inspect" the result?
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.
The both appear to make it
|
Can you grab the tests from buildah.bud to make sure they work. |
|
you want to add bats tests back into libpod? |
|
@baude or convert them to the new format. |
TomSweeneyRedHat
left a comment
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.
Good first pass. what a mess load of vendored files. Hope it doesn't make things too huge!
cmd/podman/build.go
Outdated
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.
Can we keep this code in Buildah, vendor it in and then call that code from here? Otherwise we'll have two places to make changes in the future.
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.
i didnt see any obvious ways
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.
Lets move forward on this and revisit it in the future. @baude Could you add a FIXME or something suggesting that we look into a way to vendor or share the code between podman and buildah.
cmd/podman/build.go
Outdated
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.
will these still work? Or are all the flags defined in buildachl? I thought a few from here weren't there. I'd to go through some hoops to get the global flags working. There's a difference between Buildah's and Podman's global flags. Are they all hip?
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.
if they are local variables yes.
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.
This is an awful lot of seemingly unrelated to the build changes stuff. I'd prefer it in it's own PR. Easier parsing the history months from now if there's an issue in either.
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.
that is a dynamically generated file. nothing i can do about that
|
Travis tests are failing with a seccomp error that I don't recall seeing before.... |
|
☔ The latest upstream changes (presumably 39a7a77) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Some failing commit tests |
c866983 to
00adca1
Compare
|
LGTM once tests are green |
pkg/inspect/inspect.go
Outdated
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.
Need "" marks around ManifestType
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.
fixed
bbc6a8e to
53cb271
Compare
|
Need to manually build + install libseccomp >= 2.2.0 for travis |
9cb98f9 to
a3f7e64
Compare
.travis.yml
Outdated
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.
The Travis tests are all containerized, so these both do nothing
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.
Need to add install.libseccomp.sudo to the Dockerfile and remove libseccomp from the things apt installs in the dockerfile
Makefile
Outdated
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.
Move this to global .PHONY section
Makefile
Outdated
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.
Extra newline
Makefile
Outdated
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.
Need to define $LIBSECCOMP_COMMIT somewhere - I suggest whatever's the most recent stable tag?
536bec4 to
93dd25a
Compare
Vendor in buildah and use as much of commit and bug as possible for podman build and commit. Resolves containers#586 Signed-off-by: baude <[email protected]>
|
📌 Commit 52fdca6 has been approved by |
|
☀️ Test successful - status-papr |
This directory just had Markdown and vendor.conf. I'm not sure why we have it in our version control, maybe old versions of vndr kept it? Or maybe folk dropped it into vendor/ by hand without using vndr? The history of that vendored directory is: * 619637a (Handle Linux Capabilities from command line, 2017-11-03, containers#17) added the three files to our version control. * c344fe6 (Update vendoring, 2017-11-22, containers#60) bumped hack/README.md. * af64e10 (Vendor in lots of kubernetes stuff to shrink image size, 2018-03-26, containers#554) bumped hack/README.md. * 27107fd (Vendor in latest containers/image and contaners/storage, 2018-04-18, containers#509) removed the files. * a824186 (Use buildah commit and bud in podman, 2018-04-25, containers#681) added the files back. * I'm removing them again in this commit. With this commit, $ vndr github.com/docker/docker becomes a no-op. Signed-off-by: W. Trevor King <[email protected]>
This directory just had Markdown and vendor.conf. I'm not sure why we have it in our version control, maybe old versions of vndr kept it? Or maybe folk dropped it into vendor/ by hand without using vndr? The history of that vendored directory is: * 619637a (Handle Linux Capabilities from command line, 2017-11-03, #17) added the three files to our version control. * c344fe6 (Update vendoring, 2017-11-22, #60) bumped hack/README.md. * af64e10 (Vendor in lots of kubernetes stuff to shrink image size, 2018-03-26, #554) bumped hack/README.md. * 27107fd (Vendor in latest containers/image and contaners/storage, 2018-04-18, #509) removed the files. * a824186 (Use buildah commit and bud in podman, 2018-04-25, #681) added the files back. * I'm removing them again in this commit. With this commit, $ vndr github.com/docker/docker becomes a no-op. Signed-off-by: W. Trevor King <[email protected]> Closes: #752 Approved by: baude
Vendor in buildah and use as much of commit and bud as possible for podman
build and commit.
Resolves #586
Signed-off-by: baude [email protected]