Add fallback to CONTAINER_DEFAULT_PLATFORM for image architecture#1286
Add fallback to CONTAINER_DEFAULT_PLATFORM for image architecture#1286Willjianger9 wants to merge 4 commits intoapple:mainfrom
Conversation
|
@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. |
|
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 😊 |
jglogan
left a comment
There was a problem hiding this comment.
@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)) |
There was a problem hiding this comment.
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
containerthat's using the APIs, stderr output may not be desired. What do you think about the following?- Make
printNoticeprivate, and add alog: Loggerparameter 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 theprintNoticeinvocations inresolveandresolveWithDefaultsintofromEnvironment.
- Make
- Add docc for the public func parameters and return values.
There was a problem hiding this comment.
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
|
@Willjianger9 @jglogan can you help me understand here: I noticed that
I saw some tests cases were missing
|
|
@Ronitsabhaya75 Thanks for the feedback! Here is my thinking on these (and @jglogan, feel free to jump in if I missed any context):
I totally agree on the tests, I have gone ahead and added two missing test cases |
|
@Willjianger9 Thanks for the explanation! That all makes sense. You're completely right about Thanks for confirming that |
this. |
Type of Change
Motivation and Context
Closes #1252 (part of #913).
Commands that accept
--platformcurrently require it to be passed explicitly every time. Users working consistently with a non-native platform (e.g.linux/amd64on Apple Silicon) have no way to set a default, unlike Docker'sDOCKER_DEFAULT_PLATFORM.This adds support for the
CONTAINER_DEFAULT_PLATFORMenvironment variable as a fallback when--platformis not explicitly provided.Changes
DefaultPlatform.swift— centralized logic for reading, validating, and resolving the environment variable with clear precedence rules.--platform:run,create,image pull,image push,image save, andbuild.--platformflag →--os/--archflags →CONTAINER_DEFAULT_PLATFORM→ native host architecture.Notice: Using platform "linux/amd64" from CONTAINER_DEFAULT_PLATFORM).--platformoptions now show[environment: CONTAINER_DEFAULT_PLATFORM].Testing
20 unit tests added in
DefaultPlatformTests.swiftcovering:resolve()(pull/push/save): explicit flags override env varresolveWithDefaults()(run/create): env var overrides os/arch defaults--platformis provided