Skip to content

Add fallback to CONTAINER_DEFAULT_PLATFORM for image architecture#1286

Open
Willjianger9 wants to merge 4 commits intoapple:mainfrom
Willjianger9:main
Open

Add fallback to CONTAINER_DEFAULT_PLATFORM for image architecture#1286
Willjianger9 wants to merge 4 commits intoapple:mainfrom
Willjianger9:main

Conversation

@Willjianger9
Copy link

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Motivation and Context

Closes #1252 (part of #913).

Commands that accept --platform currently require it to be passed explicitly every time. Users working consistently with a non-native platform (e.g. linux/amd64 on Apple Silicon) have no way to set a default, unlike Docker's DOCKER_DEFAULT_PLATFORM.

This adds support for the CONTAINER_DEFAULT_PLATFORM environment variable as a fallback when --platform is not explicitly provided.

Changes

  • New file DefaultPlatform.swift — centralized logic for reading, validating, and resolving the environment variable with clear precedence rules.
  • Wired into all commands with --platform: run, create, image pull, image push, image save, and build.
  • Precedence: explicit --platform flag → --os/--arch flags → CONTAINER_DEFAULT_PLATFORM → native host architecture.
  • User feedback: prints a notice to stderr when the environment variable is active (e.g. Notice: Using platform "linux/amd64" from CONTAINER_DEFAULT_PLATFORM).
  • Help text: all --platform options now show [environment: CONTAINER_DEFAULT_PLATFORM].
  • Error handling: invalid values in the environment variable produce a clear error message naming the variable.

Testing

  • Tested locally
  • Added/updated tests
  • Added/updated docs

20 unit tests added in DefaultPlatformTests.swift covering:

  • Environment variable parsing (valid platforms, empty, missing, invalid)
  • Precedence for resolve() (pull/push/save): explicit flags override env var
  • Precedence for resolveWithDefaults() (run/create): env var overrides os/arch defaults
  • Invalid env var is skipped when explicit --platform is provided
  • All 134 pre-existing unit tests pass with no regressions

@jglogan
Copy link
Contributor

jglogan commented Mar 3, 2026

@Willjianger9 Thanks for the contribution! Could you make sure that your commit is signed and verified? See https://github.com/apple/containerization/blob/main/CONTRIBUTING.md#pull-requests for details.

@Willjianger9
Copy link
Author

Thanks for bringing that to my attention! I've signed the commit and force-pushed so it should now show as verified. Please let me know if there's anything else that needs to be addressed 😊

Copy link
Contributor

@jglogan jglogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Willjianger9 thanks, this looks like a nice refactor!

Signature looks good now, just a couple of changes suggested so that we have ContainerClient code that works as expected with the CLI, but makes output optional for non-CLI clients.

/// Prints a notice to stderr indicating that the environment variable is being used.
public static func printNotice(_ platform: ContainerizationOCI.Platform) {
let message = "Notice: Using platform \"\(platform.description)\" from \(environmentVariable)\n"
FileHandle.standardError.write(Data(message.utf8))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things here:

  • For the CLI we manage stderr output through a Logger, and use structured logging. Furthermore, if it's something other than container that's using the APIs, stderr output may not be desired. What do you think about the following?
    • Make printNotice private, and add a log: Logger parameter and log the result at warn level. Have a look at the logging statements in ContainerCommand for structured logging examples. The log message should contain no interpolated values.
    • For the other functions, add a log: Logger? parameter and move the printNotice invocations in resolve and resolveWithDefaults into fromEnvironment.
  • Add docc for the public func parameters and return values.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I do agree that using a Logger makes the API much more flexible for non-CLI clients and cleans up the stderr output, which is something I overlooked in my changes. I'll get started on updating the logging structure, adjusting the parameters, and adding the missing docc comments

@Ronitsabhaya75
Copy link
Contributor

@Willjianger9 @jglogan can you help me understand here:

I noticed that BuildCommand manually calls fromEnvironment() inline, whereas the other commands (ImagePull/Push/Save) delgating to DefaultPlatform.resolve(). Should BuildCommand also use a centralized helper so the precedence logic doesn't drift out of sync?

resolve() currently uses the raw Platform(from:) OCI initializer, while resolveWithDefaults() uses Parser.platform(from:). Since the original code used Parser.platform(), could this cause any subtle behavioral changes?

I saw some tests cases were missing

  • A test for a platform with a variant (like linux/arm/v7) via the environment variable.
  • A test for resolve() where an invalid env var is provided but --platform is explicit.

@Willjianger9
Copy link
Author

Willjianger9 commented Mar 4, 2026

@Ronitsabhaya75 Thanks for the feedback!

Here is my thinking on these (and @jglogan, feel free to jump in if I missed any context):

  • BuildCommand vs. others: BuildCommand needs to produce a Set<Platform> from an array of strings, while the other commands only need a single Platform?, which is why it can't use resolve() directly. However, it does use the centralized DefaultPlatform.fromEnvironment() to read the env var, so the validation logic won't drift. The precedence is still explicit --platform > CONTAINER_DEFAULT_PLATFORM > --os/--arch
  • Parser logic: Parser.platform(from:) is just a thin wrapper that calls try .init(from: platform), so it’s functionally identical to using Platform(from:). There shouldn't be any subtle behavioral changes, and I just kept them separate to preserve the original behavior of the specific callsites (pull/push/save vs. run/create)

I totally agree on the tests, I have gone ahead and added two missing test cases

@Ronitsabhaya75
Copy link
Contributor

@Willjianger9 Thanks for the explanation!

That all makes sense.

You're completely right about Set<Platform> vs Platform?. Since it still goes through fromEnvironment() to parse the env var, the validation logic is safely centralized.

Thanks for confirming that Parser.platform(from:) is just a thin wrapper around the OCI initializer. Preserving the original behavior of the local call sites is definitely the safest move here.

@jglogan
Copy link
Contributor

jglogan commented Mar 4, 2026

  • BuildCommand vs. others: BuildCommand needs to produce a Set<Platform> from an array of strings, while the other commands only need a single Platform?

this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Request]: Choose platform with CONTAINER_DEFAULT_PLATFORM.

3 participants