fix: thread platform through dependency resolution for multi-platform builds #9922
Conversation
Summary of ChangesHello @shfc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a long-standing problem in Skaffold that caused failures when attempting to build containers for non-default or multi-platform architectures. Previously, Skaffold would incorrectly default to Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a good step towards improving multi-platform build support in Skaffold by threading the platform parameter through the dependency resolution chain. The changes are quite extensive and touch many files, which is expected for such a feature. However, I've identified a few areas where the platform information is either hardcoded to an empty value or not propagated correctly, which could undermine the fix for certain build types like local Docker and GCB. I've provided specific comments and suggestions to address these issues. Addressing these points will make the multi-platform support more robust.
| // only the COPY/ADD commands from the last image are syncable | ||
| fts, err := ReadCopyCmdsFromDockerfile(ctx, true, absDockerfilePath, workspace, buildArgs, cfg) | ||
| // Use empty platform as this is for file syncing, not platform-specific image retrieval | ||
| fts, err := ReadCopyCmdsFromDockerfile(ctx, true, absDockerfilePath, workspace, buildArgs, cfg, v1.Platform{}) |
There was a problem hiding this comment.
An empty platform is passed to ReadCopyCmdsFromDockerfile. The comment states this is for file syncing and not platform-specific, but if a base image has platform-dependent ONBUILD instructions, this could lead to an incorrect sync map. The SyncMap function is called from contexts where platform information is not available, which might be a limitation, but it's worth noting as a potential issue for complex multi-platform scenarios with file sync.
…ate platform in GCB and context creation - Update LocalDaemon.Build interface signature to include platform parameter - Extract platform from platform.Matcher in GCB cloud_build.go for dependency resolution - Thread platform parameter through CreateDockerTarContext call chain - Fix test mocks and calls to pass platform parameter
1 participant
Fixes: #9161
Related: N/A
Merge before/after: N/A
Description
Problem:
Skaffold fails when building platform-specific containers (Windows, ARM) with errors like:
no child with platform linux/amd64 in indexRoot cause:
Skaffold retrieves base images using the default platform
linux/amd64instead of the user-specified target platform.Solution
Thread the platform parameter through the dependency resolution chain:
Platform selection logic: