-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Vendor in latest Buildah #2931
Vendor in latest Buildah #2931
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomSweeneyRedHat The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes LGTM. While parsing through the diff, I noticed y'all are using libnetwork's resolvconf package in Buildah. We ended up copying it into Libpod because trying to vendor it was bringing in somewhere around 50k lines of code; if you've managed to import it for Buildah without doing that, we should delete our local copy and swap to the vendored one once this merges. |
@@ -112,3 +112,4 @@ gopkg.in/tomb.v1 v1 | |||
github.com/spf13/cobra v0.0.3 | |||
github.com/inconshreveable/mousetrap v1.0.0 | |||
gopkg.in/fsnotify.v1 v1.4.7 | |||
github.com/ishidawataru/sctp 07191f837fedd2f13d1ec7b5f885f0f3ec54b1cb |
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'd to add this line and bump libnetwork due to the changes @QiWang19 made for containers/buildah#1491
Maybe I'm not remembering right, but thought the Buildah vendor would pull that in automagically. Any who when I tried building libpod it failed until I added them.
Let's go back and copy it into buildah and then revendor it into libpod, removing that vendor. |
No, it's worth using the vendored one if I aren't including massive amounts
of unnecessary code, and that seems to be the case. I'd just as soon merge
this and delete the local implementation we have.
…On Mon, Apr 15, 2019, 05:34 Daniel J Walsh ***@***.***> wrote:
Let's go back and copy it into buildah and then revendor it into libpod,
removing that vendor.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#2931 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHYHCDlz0Fr_R5R9BHg10sWOHBKPunJWks5vhEeggaJpZM4cuwu_>
.
|
bot, retest this please |
Signed-off-by: TomSweeneyRedHat <[email protected]>
Had to change a few tests due to the way layers now works.
New:
Note the image ids are now the same. @nalind, please verify this is expected, I believe it is. |
@TomSweeneyRedHat yes, when there are no instructions we don't create a new layer. This is going to require some changes to code that assumes that every image has more layers than an image that it's based on, which has not always been the case for images built using |
Happy Green Test buttons |
/lgtm |
Signed-off-by: TomSweeneyRedHat [email protected]
Title says it all.