Add support for creating toolboxes from Containerfile #1401
Conversation
Signed-off-by: William Brawner <[email protected]>
|
Build failed. ❌ unit-test FAILURE in 8m 55s |
There was a problem hiding this comment.
Thanks for working on this! I like the idea of re-using the --image flag to name the image that was just built.
I left some comments on #1397 about some high level questions.
| image := createFlags.image | ||
|
|
||
| if cmd.Flag("file").Changed { | ||
| if !utils.PathExists(createFlags.file) { |
There was a problem hiding this comment.
I wish podman build would have a separate exit code for this. Even if the file exists here, there's no guarantee that it wouldn't disappear underneath us before the build starts. Filesystems are inherently racy.
There was a problem hiding this comment.
I see a potential solution. Copy the contents of the file to a temporary location or into a buffer and pass that to podman build instead.
There was a problem hiding this comment.
Yes. I fiddled with podman build a bit more today. This works:
$ cat /path/to/build/context/Containerfile | podman build --file - /path/to/build/context
We could try to spot the Containerfile in the build context, then open it to get a io.Reader and feed it to shell.Run(...) as stdin when invoking podman build ....
It's worth bearing in mind that podman build supports specifying the build context directory as a HTTP URL of an archive, Git repository or a Containerfile, Containerfile.in that's supposed to be pre-processed by cpp(1), multiple Containerfiles and build contexts through a mix of --file, --build-context options and a [context] argument, etc..
Reading the code of the BuildDockerfiles() function in github.com/containers/buildah/imagebuildah, TempDirForURL() function in github.com/containers/buildah/define, and the podman build code leading up to them, I think we should only support a simple subset of what podman build offers. It will help us avoid fronting the entire podman build CLI in toolbox create, keep our error handling clean, and avoid race conditions that could lead to future (including security) bugs.
So, I propose supporting only a single build context that should contain a Container/Dockerfile. Supporting HTTP URLs for the context looks simple enough, but we should avoid the rest.
There was a problem hiding this comment.
As a consequence of that, I think we should change --file to --image-build-context.
| } | ||
|
|
||
| var err error | ||
| if container == "" { |
There was a problem hiding this comment.
go fmt is unhappy about this line ...
| if err != nil { | ||
| return err | ||
| } | ||
| } |
|
I'm afraid I don't really have the time or inclination to work on this anymore, so I'll close this PR. Thank you! |
This adds a new flag to
toolbox create,-f, --file, which allows the user to create a toolbox from the given Containerfile. If--imageis provided, then that is the name that is used to tag the container. If--containeris provided, then that is used as the name of the newly created container. If either of those parameters are absent, then the name of the current directory is used in its place. I used the followingContainerfileto test this, which installs the dependencies needed to buildtoolbox:Containerfile
Of note with this is I found that using
registry.fedoraproject.org/fedorainstead ofregistry.fedoraproject.org/fedora-toolboxled to errors in toolbox initialization. Should we attempt to detect this and print a warning to the user to ensure they have the necessary packages installed in order for toolbox to work?This closes #1397.