-
Notifications
You must be signed in to change notification settings - Fork 785
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
cmd/build: add support for --push
#4677
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1268,7 +1268,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string, | |
canGenerateBuildOutput := (s.executor.buildOutput != "" && lastStage) | ||
if canGenerateBuildOutput { | ||
logrus.Debugf("Generating custom build output with options %q", s.executor.buildOutput) | ||
buildOutputOption, err = parse.GetBuildOutput(s.executor.buildOutput) | ||
buildOutputOption, err = parse.GetBuildOutput(s.executor.buildOutput, s.executor.output) | ||
if err != nil { | ||
return "", nil, false, fmt.Errorf("failed to parse build output: %w", err) | ||
} | ||
|
@@ -2363,7 +2363,17 @@ func (s *StageExecutor) commit(ctx context.Context, createdBy string, emptyLayer | |
return imgID, ref, nil | ||
} | ||
|
||
func (s *StageExecutor) generateBuildOutput(buildOutputOpts define.BuildOutputOption) error { | ||
func (s *StageExecutor) generateBuildOutput(opts define.BuildOutputOption) error { | ||
if opts.Type == define.BuildOutputImage { | ||
if opts.Push { | ||
err := internalUtil.PushImage(s.executor.store, opts, s.executor.systemContext) | ||
if err != nil { | ||
return fmt.Errorf("failed to export build output: %w", err) | ||
} | ||
} | ||
return nil | ||
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. Is this right it does no output if not opts.Push? 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. iiuc, |
||
} | ||
|
||
extractRootfsOpts := buildah.ExtractRootfsOptions{} | ||
if unshare.IsRootless() { | ||
// In order to maintain as much parity as possible | ||
|
@@ -2383,7 +2393,7 @@ func (s *StageExecutor) generateBuildOutput(buildOutputOpts define.BuildOutputOp | |
return fmt.Errorf("failed to extract rootfs from given container image: %w", err) | ||
} | ||
defer rc.Close() | ||
err = internalUtil.ExportFromReader(rc, buildOutputOpts) | ||
err = internalUtil.ExportFromReader(rc, opts) | ||
if err != nil { | ||
return fmt.Errorf("failed to export build output: %w", err) | ||
} | ||
|
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.
AFAICS
IsDir
is now never set. If so, keeping the field around is just as incompatible an API break as removing the field… and we might just as well remove it.Alternatively, if we need to keep it, it must be set in a compatible way (and I don’t know what “a compatible way” would be for
Type == …Image
.)Or, hypothetically, we could leave the
BuildOutputOption
and the existing code as is, parsing only local file outputs but keeping API compatibility, and introduce a new type and a new set of utilities that can represent image outputs.Buildah maintainers need to decide which one of the three it is; I don’t know. (My guess is the first one, at least I found no Podman user of
BuildOutputOption
, but I really lack the context.)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.
From what I can tell, and I have very little context, the idea was to minimize disruption. Outright removing it might cause more disruption than keeping it around and not using it but I understand your concern with having dead code lying around.