-
Notifications
You must be signed in to change notification settings - Fork 219
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
Add support for creating toolboxes from Containerfile #1401
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ import ( | |
"errors" | ||
"fmt" | ||
"os" | ||
"path" | ||
"path/filepath" | ||
"strings" | ||
"time" | ||
|
@@ -47,6 +48,7 @@ var ( | |
authFile string | ||
container string | ||
distro string | ||
file string | ||
image string | ||
release string | ||
} | ||
|
@@ -87,6 +89,12 @@ func init() { | |
"", | ||
"Create a toolbox container for a different operating system distribution than the host") | ||
|
||
flags.StringVarP(&createFlags.file, | ||
"file", | ||
"f", | ||
"", | ||
"Create a toolbox container from the given Containerfile") | ||
|
||
flags.StringVarP(&createFlags.image, | ||
"image", | ||
"i", | ||
|
@@ -168,10 +176,43 @@ func create(cmd *cobra.Command, args []string) error { | |
containerArg = "--container" | ||
} | ||
|
||
image := createFlags.image | ||
|
||
if cmd.Flag("file").Changed { | ||
if !utils.PathExists(createFlags.file) { | ||
var builder strings.Builder | ||
fmt.Fprintf(&builder, "file %s not found\n", createFlags.authFile) | ||
fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase) | ||
|
||
errMsg := builder.String() | ||
return errors.New(errMsg) | ||
} | ||
|
||
var err error | ||
if container == "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
container, err = dirname() | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... and this one. |
||
|
||
if image == "" { | ||
image, err = dirname() | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
err = buildContainerImage(image, createFlags.file) | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
container, image, release, err := resolveContainerAndImageNames(container, | ||
containerArg, | ||
createFlags.distro, | ||
createFlags.image, | ||
image, | ||
createFlags.release) | ||
|
||
if err != nil { | ||
|
@@ -185,6 +226,20 @@ func create(cmd *cobra.Command, args []string) error { | |
return nil | ||
} | ||
|
||
func buildContainerImage(image, containerFile string) error { | ||
buildArgs := []string{ | ||
"build", | ||
"--file", | ||
containerFile, | ||
"--tag", | ||
image, | ||
} | ||
if err := shell.Run("podman", nil, nil, nil, buildArgs...); err != nil { | ||
return fmt.Errorf("failed to build image %s", image) | ||
} | ||
return nil | ||
} | ||
|
||
func createContainer(container, image, release, authFile string, showCommandToEnter bool) error { | ||
if container == "" { | ||
panic("container not specified") | ||
|
@@ -781,3 +836,11 @@ func systemdPathBusEscape(path string) string { | |
} | ||
return string(n) | ||
} | ||
|
||
func dirname() (string, error) { | ||
cwd, err := os.Getwd() | ||
if err != nil { | ||
return "", err | ||
} | ||
return path.Base(cwd), nil | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I fiddled with
podman build
a bit more today. This works: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(...)
asstdin
when invokingpodman 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 bycpp(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 whatpodman build
offers. It will help us avoid fronting the entirepodman build
CLI intoolbox 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a consequence of that, I think we should change
--file
to--image-build-context
.