Use --arch and --os options to select architecture and os #2868

rhatdan · 2020-12-22T21:30:38Z

Remove --override-os and --override-arch flags.

Signed-off-by: Daniel J Walsh [email protected]

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci-robot · 2020-12-22T21:30:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

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

jamescassell · 2020-12-29T07:56:06Z

docs/buildah-pull.md

+
+Set the ARCH of the image to the provided value instead of using the architecture of the host.
+
+**--arch**="ARCH"


You've got this entry twice.

jamescassell · 2020-12-29T07:56:14Z

docs/buildah-pull.md

@@ -70,6 +78,10 @@ The [key[:passphrase]] to be used for decryption of images. Key can point to key

 If an image needs to be pulled from the registry, suppress progress output.

+**--os**="OS"
+
+Set the OS of the image to be pulled to the provided value instead of using the current operating system of the host.


mtrmac

This breaks any pre-existing users of the --override-* flags. If that is intentional, nothing in this PR records why, and that rationale will be impossible to reconstruct.
This was linked from arch: arm instead of arch: arm32v7 implies official images need to be referenced by archtecture explicitely #2873 as “helping fix this”, I can’t see that it does. arch: arm instead of arch: arm32v7 implies official images need to be referenced by archtecture explicitely #2873 (supposedly) fails because defaulting OS/arch to runtime.GO* values turns off autodetection (including variant autodetection), and this PR AFAICS changes nothing about that.

rhatdan · 2021-01-04T16:01:28Z

@mtrmac I setup aliases for --override-arch -> --arch and --override-os -> --os

So existing users should not be broken and these options were never documented.

rhatdan · 2021-01-04T16:03:12Z

This PR allows the specification of the variant, which was one of the requested changes in that issue, I believe.

mtrmac · 2021-01-04T16:26:19Z

@mtrmac I setup aliases for --override-arch -> --arch and --override-os -> --os

Ah, my mistake. Thanks.

This PR allows the specification of the variant, which was one of the requested changes in that issue, I believe.

Ah, OK. The PR description doesn’t say, and I have missed that.

Letting the user manually specify a variant provides a workaround; the way c/image works, removing the defaulting to runtime.GO* should make that workaround unnecessary for common uses (OTOH I haven’t checked whether it would break anything else).

rhatdan · 2021-01-04T17:48:44Z

@mtrmac Added a second patch to stop defaulting the OS and Arch if the user did not specify it. Is this what you meant?

TomSweeneyRedHat · 2021-01-04T23:51:14Z

@rhatdan tests aren't hip and you need a rebase

mtrmac · 2021-01-05T10:14:50Z

pkg/parse/parse.go


-	if selectedOS, err := c.Flags().GetString("os"); err == nil && selectedOS != runtime.GOOS {
-		os = selectedOS
+	if c.Flag("os").Changed {


This does not change SystemContextFromOptions

From a very quick skim, the results of this function are used to set the OS/arch inside the built image, and not passed to c/image at all. That probably should have some default.

It seems easier to follow to me to not have the runtime.GO* defaults for flag values (in cmd/buildah/) specified, if they are going to be ignored here, and if this function hard-codes a default anyway.

Disclaimer: This is based on a very cursory skim, I don’t understand the code base and data flow at all well.

The advantage of the defaults is the buildah --help information shows them

$ ./bin/buildah from --help | grep -- --os
--os OS prefer OS instead of the running OS when pulling images (default "linux")
$ ./bin/buildah from --help | grep -- --arch
--arch string set the ARCH of the image to the provided value instead of the architecture of the host (default "amd64")

The SystemContextFromOptions is a good point, and I added the .Changed check there as well.

Well, but it’s not the default if no --os and --os=$default behave differently.

It is also possible to embed that value into the flag description instead.

The change of the default return value of PlatformFromOptions still looks wrong to me, but I’ll leave that to Buildah experts.

Remove --override-os and --override-arch flags. Also use --platform option if specified when generating the SystemContext. Conflict --platform option with --os, --arch and --variant options. Signed-off-by: Daniel J Walsh <[email protected]>

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2021-01-06T13:05:37Z

@mtrmac @TomSweeneyRedHat Tests are passing and I think I addressed @mtrmac concerns.
@containers/podman-maintainers PTAL

We need to get this merged so I can continue on the manifests PR.

TomSweeneyRedHat · 2021-01-06T14:38:24Z

LGTM

rhatdan · 2021-01-06T15:43:34Z

@containers/podman-maintainers PTAL

rhatdan · 2021-01-06T15:43:41Z

@vrothberg PTAL

ashley-cui · 2021-01-06T19:32:35Z

LGTM

openshift-ci-robot added the approved label Dec 22, 2020

rhatdan force-pushed the override branch 9 times, most recently from 0c0b715 to 056ba46 Compare December 23, 2020 20:04

rhatdan mentioned this pull request Dec 24, 2020

podman run --override-arch do not download the image with correct architecture if the image with other architecture is exists. containers/podman#8001

Closed

jamescassell reviewed Dec 29, 2020

View reviewed changes

rhatdan force-pushed the override branch 2 times, most recently from 1b14dde to 809a09f Compare December 29, 2020 15:02

rhatdan mentioned this pull request Dec 29, 2020

arch: arm instead of arch: arm32v7 implies official images need to be referenced by archtecture explicitely #2873

Closed

rhatdan force-pushed the override branch 3 times, most recently from 515f610 to 8e45ae4 Compare December 30, 2020 19:57

mtrmac reviewed Jan 4, 2021

View reviewed changes

rhatdan force-pushed the override branch from a5f8824 to 2278e24 Compare January 4, 2021 17:51

mtrmac reviewed Jan 5, 2021

View reviewed changes

rhatdan force-pushed the override branch from 2278e24 to 479d0f2 Compare January 5, 2021 21:22

rhatdan added 2 commits January 6, 2021 06:43

Only grab the OS, Arch if the user actually specified them

47e4c3c

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the override branch from 479d0f2 to 47e4c3c Compare January 6, 2021 11:44

rhatdan mentioned this pull request Jan 6, 2021

podman build appears to ignore --arch option when pulling multiarch images containers/podman#8894

Closed

rhatdan added todo-backport-to-1.19 We should do or consider a backport to 1.19. Remove when we either decide not to, or open a PR. lgtm labels Jan 6, 2021

openshift-merge-robot merged commit 02b914b into containers:master Jan 6, 2021

mtrmac mentioned this pull request Jan 7, 2021

Fix handling of TMPDIR environment variable #2893

Merged

nalind removed the todo-backport-to-1.19 We should do or consider a backport to 1.19. Remove when we either decide not to, or open a PR. label Jan 14, 2021

github-actions bot added the locked - please file new issue/PR label Sep 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use --arch and --os options to select architecture and os #2868

Use --arch and --os options to select architecture and os #2868

rhatdan commented Dec 22, 2020

openshift-ci-robot commented Dec 22, 2020

jamescassell Dec 29, 2020

jamescassell Dec 29, 2020

mtrmac left a comment

rhatdan commented Jan 4, 2021

rhatdan commented Jan 4, 2021

mtrmac commented Jan 4, 2021

rhatdan commented Jan 4, 2021

TomSweeneyRedHat commented Jan 4, 2021

mtrmac Jan 5, 2021

rhatdan Jan 5, 2021

mtrmac Jan 6, 2021

rhatdan commented Jan 6, 2021

TomSweeneyRedHat commented Jan 6, 2021

rhatdan commented Jan 6, 2021

rhatdan commented Jan 6, 2021

ashley-cui commented Jan 6, 2021


		Set the ARCH of the image to the provided value instead of using the architecture of the host.

		--arch="ARCH"

Use --arch and --os options to select architecture and os #2868

Use --arch and --os options to select architecture and os #2868

Conversation

rhatdan commented Dec 22, 2020

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

openshift-ci-robot commented Dec 22, 2020

jamescassell Dec 29, 2020

Choose a reason for hiding this comment

jamescassell Dec 29, 2020

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

rhatdan commented Jan 4, 2021

rhatdan commented Jan 4, 2021

mtrmac commented Jan 4, 2021

rhatdan commented Jan 4, 2021

TomSweeneyRedHat commented Jan 4, 2021

mtrmac Jan 5, 2021

Choose a reason for hiding this comment

rhatdan Jan 5, 2021

Choose a reason for hiding this comment

mtrmac Jan 6, 2021

Choose a reason for hiding this comment

rhatdan commented Jan 6, 2021

TomSweeneyRedHat commented Jan 6, 2021

rhatdan commented Jan 6, 2021

rhatdan commented Jan 6, 2021

ashley-cui commented Jan 6, 2021