cmd/build: add support for `--push` #4677

danishprakash · 2023-03-21T13:26:01Z

What type of PR is this?

/kind feature

Which issue(s) this PR fixes:

Special notes for your reviewer:

Extremely WIP draft.

Does this PR introduce a user-facing change?

- add support for `--push` flag to `bud` push image immediately after building

rhatdan · 2023-03-21T19:46:13Z

I was thinking this would just be added to the client and not the service side.

buildah build --push

Would internally do

buildah build followed by buildah push.

rhatdan · 2023-03-21T19:46:31Z

What do you think @nalind @flouthoc @vrothberg

nalind · 2023-03-21T20:18:24Z

I was thinking this would just be added to the client and not the service side.

buildah build --push

Would internally do

buildah build followed by buildah push.

That would be simpler; API callers already have a buildah.Push() that they can call directly, and it's less API churn if we add such a call to the tail end of cmd/buildah/buildCmd() if --push.

For the "output=registry" case, setting IsDir in a BuildOutputOption while also adding a Type field that overlaps its meaning is confusing. Making that a special case after we've extracted the output image's rootfs as an archive (an archive which doesn't get used if we're just going to push the image) looks wrong; pushing to a registry probably needs to be an entirely separate branch in generateBuildOutput().

define/build.go

TomSweeneyRedHat · 2023-03-21T22:10:23Z

And just in case, we'll need updates to the man page and additional test added for the final PR. From a quick flyby review, it LGTM other than other comments.

vrothberg · 2023-03-22T08:39:49Z

What do you think @nalind @flouthoc @vrothberg

I am no fan of mixing multiple commands as it breaks separation of concerns. buildah push has over a dozen command-line options. How would they be exposed in buildah build?

But I leave the decision up to the core maintainers of Buildah.

rhatdan · 2023-03-22T15:07:58Z

I see this for compatibility with buildx
https://docs.docker.com/engine/reference/commandline/buildx_build/
--push | | Shorthand for --output=type=registry

I don't think we have to add other options for push, and potentially could have just done this in podman build, but the --output=type calls are handled within buildah.

flouthoc · 2023-03-22T15:31:20Z

I concur should we just do this at podman, and only add --output=type=registry to buildah ?

danishprakash · 2023-03-23T11:29:13Z

I was thinking this would just be added to the client and not the service side.

buildah build --push

Would internally do

buildah build followed by buildah push.

I honestly didn't think that is how we'd want to push. On the outset, --push seems like it should be added serially to build but I felt it might not be extensible given there could be modifications/additions to the --output flag in future.

For the "output=registry" case, setting IsDir in a BuildOutputOption while also adding a Type field that overlaps its meaning is confusing. Making that a special case after we've extracted the output image's rootfs as an archive (an archive which doesn't get used if we're just going to push the image) looks wrong; pushing to a registry probably needs to be an entirely separate branch in generateBuildOutput().

You're right, that was an oversight.

danishprakash · 2023-03-23T11:30:53Z

I concur should we just do this at podman, and only add --output=type=registry to buildah ?

So adding the --push flag to podman which would be translated to --output within buildah? Sounds about right to me. That was the original plan as mentioned here.

pkg/parse/parse.go

pkg/cli/common.go

rhatdan · 2023-04-21T09:56:38Z

internal/util/util.go

+
+		image := opts.Attrs["name"]
+		destSpec := opts.Attrs["name"]
+		dest, err := alltransports.ParseImageName(destSpec)


I am not sure this code is correct, or necessary. I know that @mtrmac and @vrothberg frown on parsing specs outside of github.com/containers/image or github.com/containers/common/libimage

Yes; as a general principle I think the CLI layer, or something as close to it as possible, should be responsible for turning strings into a unique types.ImageReference.

Admittedly that’s not always possible, but it’s the thing to strive for.

Does it help if it's moved to util? #4677 (comment)

mtrmac · 2023-04-21T21:34:19Z

define/types.go

@@ -100,6 +100,8 @@ type Secret struct {

 // BuildOutputOptions contains the the outcome of parsing the value of a build --output flag
 type BuildOutputOption struct {
+	Type     string
+	Attrs    map[string]string


Why is this adding an untyped map instead simple Go fields that exactly represent what we need?

This was referenced from buildx but the idea remains the same, for every type, there are (and would be in the future) multiple arguments/options which are consolidated under attributes here. It's possible for us to have individual fields here but those would eventually proliferate into a heterogenous struct type and I'm not sure how ideal that would be.

This is ultimately up to Buildah maintainers, not me.

IMHO individual fields, or individual types, is much better than Attrs, because Attrs is equally heterogenous, just with much worse compiler / linter support.

We can do interfaces, type checks, wrappers, …

That's a good point. Perhaps we can have embedded structs here for individual Types as and when the situation arises? Looking for inputs

Up to Buildah maintainers.

(My personal style would be:

Type field first

Then separate sections for individual subtypes, separated by blank lines and with a comment “valid if Type == … ”

so that future maintainers can just look at the type definition and don’t have to chase individual field uses in an IDE.

I agree that separate structs, or even type checks, might be overkill for this small scope.)

define/types.go

pkg/parse/parse.go

pkg/cli/build.go

pkg/cli/common.go

internal/util/util.go

mtrmac · 2023-04-21T21:41:29Z

internal/util/util.go

+		libimageOptions.Writer = os.Stdout
+		runtime, err := libimage.RuntimeFromStore(store, &libimage.RuntimeOptions{SystemContext: &types.SystemContext{}})
+		destString := fmt.Sprintf("%s:%s", dest.Transport().Name(), dest.StringWithinTransport())
+		_, err = runtime.Push(context.Background(), image, destString, libimageOptions)


(Eventually, could libImage have an API that accepts type.ImageReference directly?)

internal/util/util.go

mtrmac

It’s quite likely I don’t have a full context of the goals / constraints of the Buildx feature set or compatibility; I’m looking basically only at this PR in isolation.

I’ll fully defer to actual Buildah maintainers.

define/types.go

internal/util/util.go

pkg/parse/parse.go

define/types.go

internal/util/util.go

mtrmac · 2023-04-25T17:47:34Z

util/util.go

@@ -50,6 +51,31 @@ func StringInSlice(s string, slice []string) bool {
 	return util.StringInSlice(s, slice)
 }

+func StringToImageReference(image string) (types.ImageReference, error) {


UIArgumentToImageReference? HeuristicUserInputToImageReference? Something along those lines? (Up to Buildah maintainers.)

I, fairly desperately, don’t want this code to be used to “round-trip” between ImageReference and string [there already are other functions for that], and this plain naming of the function suggests that it could be used for that purpose.

I, fairly desperately, don’t want this code to be used to “round-trip” between ImageReference and string

Sorry but would you mind explaining what you mean? Some sort of abuse this function would get by being used in unintended places?

Also, how about ImageStringToImageReference?

Some sort of abuse this function would get by being used in unintended places?

Yes. The function is a heuristic that can parse the input using several different format/semantics, depending on what happens to work.

A failure case I’ve seen fairly frequently in this space is finding some pre-existing function like that, similarly targeted at the UI and heuristically doing “what the user meant”, and embedding it somewhere deeper in the call stack, calling it with a string produced from a Go value, without accounting for the details of the heuristics, or possibly the CLI-level heuristics being expanded to add new user-level features. The outcome is that the Go value → string → Go value transformation (which might, sometimes, be unavoidable, e.g. for multi-machine operations) becomes inexact, hard to predict, and possibly even leads to security issues.

Hence all of the concern about not using strings as an internal representation if at all possible, as the easiest preventative measure.

Secondarily, using the right string representation / API. The types.ImageReference values already have an API for producing a string representation, and for creating one from a string representation. That one (alltransports.ParseImageName) should be used anywhere below the CLI heuristic layers, and the new function should be named so that it isn’t accidentally used instead.

So that’s why I would like some mention of UI, and preferably “heuristics” in the name.

“Image string” does not help because it does not disambiguate “a lossy heuristic subject to change” from “a stable round-trip-safe value conversion”, either would apply to images and strings.

internal/util/util.go

StarpTech · 2023-05-20T20:15:12Z

Hi, I just created containers/podman#18642 It looks related to this PR.

I am no fan of mixing multiple commands as it breaks separation of concerns. buildah push has over a dozen command-line options. How would they be exposed in buildah build?

I agree but a good reason to combine them is to enable parallelism.

rhatdan · 2023-06-27T21:16:20Z

@danishprakash Are you still working on this?

rhatdan · 2023-12-17T11:46:38Z

@danishprakash still working on this?

danishprakash · 2023-12-20T06:06:24Z

Yes, going to get back to this end of this week.

packit-as-a-service · 2024-02-15T12:39:35Z

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

jianzhangbjz · 2024-05-27T08:04:20Z

Any updates? Thanks! And, one more question, for now, once build finished, how can I push all arch images to the registry at once if no --push? Thanks!

Error: unknown flag: --push
See 'podman buildx build --help'

rhatdan · 2024-06-03T19:54:53Z

@danishprakash would you like me to try to get someone to take this over?

danishprakash · 2024-06-05T16:41:08Z

Tests were pending, I had added them a couple of months back. Two RPM tests are failing, I'm not sure if they're related. Also, this still needs a final pass from @mtrmac.

rhatdan · 2024-09-13T09:28:45Z

@mtrmac PTAL

rhatdan · 2024-09-13T09:31:06Z

@nalind @flouthoc PTAL

rhatdan · 2024-09-13T09:34:19Z

imagebuildah/stage_executor.go

+				return fmt.Errorf("failed to export build output: %w", err)
+			}
+		}
+		return nil


Is this right it does no output if not opts.Push?

iiuc, type=image outputs a container image format, only when you specify --output, it additionally pushes the image to the registry.

internal/util/util.go

* pass systemCtx to push util * fix test Signed-off-by: Danish Prakash <[email protected]>

pasquale95 · 2024-09-30T13:26:07Z

Hi, is this still WIP?

danishprakash · 2024-10-03T13:30:28Z

Hi, is this still WIP?

Yes

openshift-ci bot added do-not-merge/work-in-progress kind/feature Categorizes issue or PR as related to a new feature. labels Mar 21, 2023

TomSweeneyRedHat reviewed Mar 21, 2023

View reviewed changes

define/build.go Outdated Show resolved Hide resolved

danishprakash force-pushed the bud-push-output branch from 07d26e5 to f934001 Compare March 23, 2023 11:25

danishprakash mentioned this pull request Apr 21, 2023

[Feature]: Add support for "buildx build --push" and/or "buildx build --output=type=registry" #4671

Open

rhatdan reviewed Apr 21, 2023

View reviewed changes

pkg/parse/parse.go Show resolved Hide resolved

rhatdan reviewed Apr 21, 2023

View reviewed changes

pkg/cli/common.go Show resolved Hide resolved

rhatdan reviewed Apr 21, 2023

View reviewed changes

mtrmac reviewed Apr 21, 2023

View reviewed changes

danishprakash force-pushed the bud-push-output branch 4 times, most recently from e22f351 to d6925cd Compare April 25, 2023 06:18

mtrmac reviewed Apr 25, 2023

View reviewed changes

danishprakash force-pushed the bud-push-output branch from d6925cd to bd7e5cf Compare April 26, 2023 13:13

mtrmac reviewed May 2, 2023

View reviewed changes

internal/util/util.go Show resolved Hide resolved

internal/util/util.go Outdated Show resolved Hide resolved

danishprakash marked this pull request as ready for review June 8, 2023 11:51

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 8, 2023

danishprakash force-pushed the bud-push-output branch from bd7e5cf to 4b19451 Compare June 8, 2023 12:18

github-actions bot removed the stale-pr label Dec 18, 2023

danishprakash force-pushed the bud-push-output branch 3 times, most recently from 0036033 to f9d20a2 Compare January 18, 2024 05:59

danishprakash force-pushed the bud-push-output branch from f9d20a2 to 61ed32a Compare February 15, 2024 12:37

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2024

danishprakash force-pushed the bud-push-output branch from 61ed32a to 7a1ef03 Compare February 15, 2024 12:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2024

danishprakash force-pushed the bud-push-output branch from 7a1ef03 to b2c313d Compare March 13, 2024 07:58

danishprakash force-pushed the bud-push-output branch from b2c313d to a5d4921 Compare June 5, 2024 12:57

ebensh mentioned this pull request Jun 26, 2024

ROX-24040: Set Podman instead of Docker to fix RHEL8 builders stackrox/acs-fleet-manager#1917

Closed

danishprakash force-pushed the bud-push-output branch 4 times, most recently from 5c25e1e to 0efaf1f Compare September 12, 2024 14:09

rhatdan reviewed Sep 13, 2024

View reviewed changes

internal/util/util.go Outdated Show resolved Hide resolved

build: add support for --push

80defed

* pass systemCtx to push util * fix test Signed-off-by: Danish Prakash <[email protected]>

danishprakash force-pushed the bud-push-output branch from 0efaf1f to 80defed Compare September 17, 2024 12:19

barryoneill mentioned this pull request Sep 26, 2024

Add dockerBuildxPlatforms scala-steward-org/scala-steward#3419

Merged

cmd/build: add support for --push #4677

Are you sure you want to change the base?

cmd/build: add support for --push #4677

Conversation

danishprakash commented Mar 21, 2023

What type of PR is this?

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

rhatdan commented Mar 21, 2023

rhatdan commented Mar 21, 2023

nalind commented Mar 21, 2023

TomSweeneyRedHat commented Mar 21, 2023

vrothberg commented Mar 22, 2023

rhatdan commented Mar 22, 2023

flouthoc commented Mar 22, 2023

danishprakash commented Mar 23, 2023

danishprakash commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StarpTech commented May 20, 2023 • edited Loading

rhatdan commented Jun 27, 2023

rhatdan commented Dec 17, 2023

danishprakash commented Dec 20, 2023

packit-as-a-service bot commented Feb 15, 2024

jianzhangbjz commented May 27, 2024 • edited Loading

rhatdan commented Jun 3, 2024

danishprakash commented Jun 5, 2024

rhatdan commented Sep 13, 2024

rhatdan commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pasquale95 commented Sep 30, 2024

danishprakash commented Oct 3, 2024

cmd/build: add support for `--push` #4677

cmd/build: add support for `--push` #4677

mtrmac Apr 25, 2023 •

edited

Loading

StarpTech commented May 20, 2023 •

edited

Loading

jianzhangbjz commented May 27, 2024 •

edited

Loading