Skip to content
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

Inherit arguments between stages #5845

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

onitake
Copy link

@onitake onitake commented Nov 22, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

Docker supports argument inheritance between build stages, but this feature is missing from buildah.

How to verify it

See the linked issue below for a basic example.

Which issue(s) this PR fixes:

Fixes #5762

Special notes for your reviewer:

To do:

  • Document the changes (see the Docker documentation about variable scoping: https://docs.docker.com/build/building/variables/#scoping )
  • Add unit/functional tests for inherited arguments
  • Verify if stages with inherited arguments need to be rebuilt (args aren't persisted into container images?)
  • Other semantic changes to bring buildah in line with Docker (TBD)

Does this PR introduce a user-facing change?

ARG statements will now be inherited between build stages, like docker-build does.

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 22, 2024
Copy link
Contributor

openshift-ci bot commented Nov 22, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: onitake
Once this PR has been reviewed and has the lgtm label, please assign rhatdan for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2024
@onitake onitake marked this pull request as draft November 22, 2024 19:09
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2024
@onitake onitake force-pushed the inherit-args branch 2 times, most recently from 804c285 to 57e9ba4 Compare November 22, 2024 19:28
@onitake
Copy link
Author

onitake commented Nov 22, 2024

maps.Insert and maps.All for merging one map into another require Go 1.23.
Is there something comparable in the buildah source that I could use?

Scratch that, maps.Copy does the job and is available since Go 1.21. 🙈

Signed-off-by: Gregor Riepl <[email protected]>
@nalind
Copy link
Member

nalind commented Dec 2, 2024

Solving this here doesn't solve it for imagebuilder's CLI tool. I'm not sure what I think about that yet.

@onitake
Copy link
Author

onitake commented Dec 2, 2024

@nalind It's still incomplete and doesn't seem to work how I imagined it yet anyway.
But, what's imagebuilder? I though podman's image builder is buildah?

@nalind
Copy link
Member

nalind commented Dec 2, 2024

Ah, sorry, that reference was buried somewhere in the linked issue. When I talk about "imagebuilder", I mean https://github.com/openshift/imagebuilder.

The Dockerfile parsing logic mostly lives in github.com/openshift/imagebuilder/dockerfile. The github.com/openshift/imagebuilder.Builder object "drives" a build using an implementation of its Executor interface. imagebuilder provides a CLI tool that combines its main library with github.com/openshift/imagebuilder/dockerclient.ClientExecutor, which implements the interface using github.com/fsouza/go-dockerclient.

Buildah originally didn't handle Dockerfiles at all, and when we needed to add the ability to do so, implementing a github.com/openshift/imagebuilder.Executor using github.com/containers/buildah.Builder and using a github.com/openshift/imagebuilder.Builder to "drive" builds seemed relatively straightforward.

At times, this has meant that we had to make changes there in order to enable features here, but I could say that about other dependencies we have as well.

@onitake
Copy link
Author

onitake commented Dec 2, 2024

Ah sorry, now I get what you meant. Yes, I had originally thought I could/should place all argument handling logic there. But then I realized that arguments are only properly resolved and tracked when build stages are "executed", which all happens here in the buildah source.

Does that mean you would prefer that argument inheritance rules in Dockerfiles is handled by openshift/imagebuilder instead? I'm not sure if that is feasible at all, at least I think it would duplicate a lot of things.

@rhatdan
Copy link
Member

rhatdan commented Dec 17, 2024

@onitake any update on this?

@onitake
Copy link
Author

onitake commented Dec 18, 2024

@onitake any update on this?

I was struggling a bit to set up a proper test environment, so I haven't been able to debug why it's not working as expected yet. Will try to submit an update later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ARG inheritance works in Docker, but not Podman
4 participants