Use buildah commit for podman commit by baude · Pull Request #592 · containers/podman

baude · 2018-04-03T23:47:57Z

Resolves: #586 and #520

baude · 2018-04-03T23:49:24Z

@TomSweeneyRedHat @rhatdan @mheon Again, this will eventually fix the two issues Ed has filed against podman. I know we have issues with this approach; but the problem is we have no consensus on another approach. So review and we can decide if this is a good stop gap or you want me to implement a larger, agreed upon approach.

mheon · 2018-04-03T23:54:38Z

cmd/podman/commit.go

Separate from the Buildah stuff, but I really don't like this... can we just pass in a string for the name of the new image?

We can give return a *image.Image from commit so we still get a good image to work with

mheon · 2018-04-03T23:55:10Z

cmd/podman/commit.go

This API would be a lot cleaner if we used functional options, but we can clean it up later

Yes I agree we should have a image.Config object

I think you guys are talking about two different things ... I think @mheon is talking about "WithWriter" funcs? Either would be fine by me if preferred. The previous one used a struct.

mheon · 2018-04-03T23:58:54Z

libpod/container_api.go

I'd like to figure out how much code ImportBuilder actually is - how much code from buildah do we really need to bring in for this? Is this a case where it's just a single file, like when we vendored in Kube stuff?

I'll certainly look. I was trying to avoid duplication at all costs, but if it is really small, agree that might make more sense.

mheon · 2018-04-04T00:08:34Z

I don't like the image API changes, it seems to me it would make a lot more sense to continue returning an Image and just accept the name/reference for the new image.

mheon · 2018-04-04T00:10:05Z

Otherwise... I don't like vendoring buildah here, but commit is clearly broken right now and this fixes it. When we consider basing buildah on libpod we can look into factoring the commit code into here.

rhatdan · 2018-04-04T01:21:25Z

I think a lot of this code should be added to the image object, At least have a image.Config that defines all of the the fields that can get added to an image.

mheon · 2018-04-04T02:53:33Z

I was thinking we'd make Commit accept options like NewContainer does, so we can only override what we want to - we'd have a WithEntrypoint, WithCmd, WithAuthor, etc, and pass whatever we wanted into commit itself to only modify those fields.

baude · 2018-04-04T13:08:41Z

When I think of commit, I think of container not image logically. What do you guys think ?

TomSweeneyRedHat · 2018-04-04T13:21:29Z

On 04/04/2018 09:08 AM, Brent Baude wrote: When I think of commit, I think of container not image logically. What do you guys think ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#592 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYbWBQrnOVH8_l24t1tgxVaFdb3Qv7d0ks5tlMXbgaJpZM4TF9IK>.

I personally think transmorgification. Container into an image. But that's probably due to too many hours reading Calvin and Hobbes. t

rhatdan · 2018-04-04T13:25:58Z

Those fields are image data fields. So I think the data belongs to the image even if it is temporarily stored in the container.

mheon · 2018-04-04T13:40:05Z

There's no reason we can't have a struct with the image configuration which is populated by With... functions. Can we work with that using the vendored buildah code, though? Buildah uses setter functions to modify individual bits of the config, which is part of why I like With... functions (easy to translate into the buildah api). Can we ignore the buildah functions and change the image's config after we make it with importbuilder.Commit()?

rhatdan · 2018-04-04T13:46:50Z

Sure I am not that concerned with how we hack this together to fix the current issue. But I think we need to concentrate on a long term issue of getting Buildah and Podman to blend together in a way that @mheon and @nalind can live with.

rh-atomic-bot · 2018-04-05T14:34:35Z

☔ The latest upstream changes (presumably fdcf633) made this pull request unmergeable. Please resolve the merge conflicts.

mheon · 2018-04-05T18:54:31Z

@baude Can you hit the manpages and usage text to say that image name is mandatory now?

mheon · 2018-04-05T18:55:35Z

libpod/container_commit.go

Does the container need to be mounted to do this?

I dont know. How can I tell?

Does commit on a stopped container work? If it does, we should be fine

yup it does

mheon · 2018-04-09T14:25:38Z

cmd/podman/commit.go

Move this up with io and os

mheon · 2018-04-09T14:25:46Z

cmd/podman/commit.go

Move this up with io and os

mheon · 2018-04-09T14:34:43Z

libpod/buildah/buildah.go

Did we patch this? Should we have patched this? I'm wondering if this will affect the containers buildah sees

i changed it if that is what you are asking

mheon · 2018-04-09T18:09:36Z

libpod/image/image.go

Can this and ToStorageReference go away now that we aren't using it?

Resolves: containers#586 and containers#520 Signed-off-by: baude <bbaude@redhat.com>

mheon · 2018-04-09T19:36:50Z

LGTM

rhatdan · 2018-04-04T01:20:08Z

libpod/image/image.go

Horrid Name, No idea what this function does

TomSweeneyRedHat

Changes LGTM. My questions/concerns is keeping this code up to date with anything that changes in Buildah and/or converting Buildah to use this code. Thoughts on that?

@nalind WDYT?

baude · 2018-04-10T12:59:45Z

@TomSweeneyRedHat consider this a pilot and stop gap to fix current issues filed against podman. Once this PR is merged, I am ready to start the moving of buildah code over so we no longer have a maintenance issue.

mheon · 2018-04-10T13:31:55Z

@rh-atomic-bot r+

rh-atomic-bot · 2018-04-10T13:31:56Z

📌 Commit e2b071a has been approved by mheon

rh-atomic-bot · 2018-04-10T13:32:04Z

⚡ Test exempted: merge already tested.

mheon reviewed Apr 3, 2018

View reviewed changes

baude force-pushed the commitbuildah branch from a4007c3 to f8408c0 Compare April 5, 2018 14:28

baude force-pushed the commitbuildah branch 3 times, most recently from 357e064 to a132774 Compare April 5, 2018 17:24

mheon reviewed Apr 5, 2018

View reviewed changes

baude force-pushed the commitbuildah branch from a132774 to cfd7a06 Compare April 6, 2018 17:43

baude changed the title ~~[WIP] Use buildah commit for podman commit~~ Use buildah commit for podman commit Apr 6, 2018

baude force-pushed the commitbuildah branch from cfd7a06 to 8bfa054 Compare April 6, 2018 22:29

mheon reviewed Apr 9, 2018

View reviewed changes

baude force-pushed the commitbuildah branch 2 times, most recently from fc8191c to a3e139b Compare April 9, 2018 18:03

mheon reviewed Apr 9, 2018

View reviewed changes

Use buildah commit for podman commit

e2b071a

Resolves: containers#586 and containers#520 Signed-off-by: baude <bbaude@redhat.com>

baude force-pushed the commitbuildah branch from a3e139b to e2b071a Compare April 9, 2018 19:35

rhatdan reviewed Apr 10, 2018

View reviewed changes

libpod/image/image.go Outdated

Copy link

Member

rhatdan Apr 4, 2018

Choose a reason for hiding this comment

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

Horrid Name, No idea what this function does

TomSweeneyRedHat reviewed Apr 10, 2018

View reviewed changes

rh-atomic-bot closed this in 1700f2b Apr 10, 2018

baude deleted the commitbuildah branch December 22, 2019 18:46

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Conversation

baude commented Apr 3, 2018

Uh oh!

baude commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Apr 4, 2018

Uh oh!

mheon commented Apr 4, 2018

Uh oh!

rhatdan commented Apr 4, 2018

Uh oh!

mheon commented Apr 4, 2018

Uh oh!

baude commented Apr 4, 2018

Uh oh!

TomSweeneyRedHat commented Apr 4, 2018 via email

Uh oh!

rhatdan commented Apr 4, 2018

Uh oh!

mheon commented Apr 4, 2018

Uh oh!

rhatdan commented Apr 4, 2018

Uh oh!

rh-atomic-bot commented Apr 5, 2018

Uh oh!

mheon commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Apr 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Apr 10, 2018

Uh oh!

mheon commented Apr 10, 2018

Uh oh!

rh-atomic-bot commented Apr 10, 2018

TomSweeneyRedHat left a comment •

edited

Loading