feat(copy): Support copying artifacts across multiple platforms.#1954
feat(copy): Support copying artifacts across multiple platforms.#1954hellocn9 wants to merge 4 commits intooras-project:mainfrom
Conversation
|
@shizhMSFT @FeynmanZhou Can you take a look? |
There was a problem hiding this comment.
Pull request overview
This PR adds support for copying artifacts across multiple platforms by allowing users to specify comma-separated platform architectures with the --platform flag (e.g., --platform linux/amd64,linux/arm64,linux/arm/v7). Previously, only a single platform could be specified at a time.
Key changes:
- Extended platform parsing to support comma-separated lists of platforms
- Added new
copyMultiplePlatformsfunction to handle copying filtered manifests from indexes - Implemented platform matching logic to filter index manifests based on specified platforms
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| cmd/oras/internal/option/platform.go | Added Platforms field to support multiple platforms, updated parsing logic to handle comma-separated platform strings, and refactored single platform parsing into a separate method |
| cmd/oras/root/cp.go | Added import for metadata handler, updated documentation with multi-platform example, implemented copyMultiplePlatforms function with index filtering and platform matching logic |
Comments suppressed due to low confidence (1)
cmd/oras/internal/option/platform.go:126
- The
parseSinglePlatformfunction doesn't populate thePlatformsslice when a single platform is parsed. This creates inconsistency wherePlatformis set butPlatformsremains nil/empty. This could cause issues in code that checkslen(opts.Platform.Platforms) > 0to determine if platforms were specified. For a single platform, the code at line 146 in cp.go would evaluate to false even when a platform is specified.
func (opts *Platform) parseSinglePlatform(platformStr string) error {
// OS[/Arch[/Variant]][:OSVersion]
// If Arch is not provided, will use GOARCH instead
var platformPart string
var p ocispec.Platform
platformPart, p.OSVersion, _ = strings.Cut(platformStr, ":")
parts := strings.Split(platformPart, "/")
switch len(parts) {
case 3:
p.Variant = parts[2]
fallthrough
case 2:
p.Architecture = parts[1]
case 1:
p.Architecture = runtime.GOARCH
default:
return fmt.Errorf("failed to parse platform %q: expected format os[/arch[/variant]]", platformStr)
}
p.OS = parts[0]
if p.OS == "" {
return fmt.Errorf("invalid platform: OS cannot be empty")
}
if p.Architecture == "" {
return fmt.Errorf("invalid platform: Architecture cannot be empty")
}
opts.Platform = &p
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: hellocn9 <zhangyancn9@126.com>
…form filtering and error handling Signed-off-by: hellocn9 <zhangyancn9@126.com>
…opying Signed-off-by: hellocn9 <zhangyancn9@126.com>
Done! Added unit & E2E tests for all platform-related changes. |
|
@shizhMSFT I’ve addressed all your feedback — please take a look when you have time! |
…form filtering Signed-off-by: hellocn9 <zhangyancn9@126.com>
| // Variant: optional; if specified, must match exactly, otherwise it is ignored | ||
| //if targetPlatform.Variant == "" && manifestPlatform.Variant != "" { | ||
| // return false | ||
| //} |
There was a problem hiding this comment.
| // Variant: optional; if specified, must match exactly, otherwise it is ignored | |
| //if targetPlatform.Variant == "" && manifestPlatform.Variant != "" { | |
| // return false | |
| //} |
| statusHandler, metadataHandler := display.NewCopyHandler(opts.Printer, opts.TTY, dst) | ||
|
|
||
| // Check if multiple platforms are specified | ||
| if len(opts.Platform.Platforms) > 1 && !opts.recursive { |
There was a problem hiding this comment.
This part is confusing me here, why is this checking for not recursive? If there are multiple platforms specified don't we want to copy the multiple platforms?
| opts.FlagDescription = "request platform" | ||
| } | ||
| fs.StringVarP(&opts.platform, "platform", "", "", opts.FlagDescription+" in the form of `os[/arch][/variant][:os_version]`") | ||
| fs.StringSliceVarP(&opts.platform, "platform", "", nil, opts.FlagDescription+" in the form of `os[/arch][/variant][:os_version]` or comma-separated list for multiple platforms (supported in oras cp only)") |
There was a problem hiding this comment.
Unrelated to this PR, but that bit about "supported in oras cp only" is a good argument for composition over inheritance
| func (opts *ArtifactPlatform) ApplyFlags(fs *pflag.FlagSet) { | ||
| opts.FlagDescription = "set artifact platform" | ||
| fs.StringVarP(&opts.platform, "artifact-platform", "", "", "[Experimental] "+opts.FlagDescription+" in the form of `os[/arch][/variant][:os_version]`") | ||
| fs.StringSliceVarP(&opts.platform, "artifact-platform", "", nil, "[Experimental] "+opts.FlagDescription+" in the form of `os[/arch][/variant][:os_version]`") |
| } | ||
|
|
||
| // Push the new index to the destination | ||
| if err = dst.Push(ctx, newIndexDesc, strings.NewReader(string(newIndexContent))); err != nil { |
There was a problem hiding this comment.
| if err = dst.Push(ctx, newIndexDesc, strings.NewReader(string(newIndexContent))); err != nil { | |
| if err = dst.Push(ctx, newIndexDesc, bytes.NewReader(newIndexContent)); err != nil { |
What this PR does / why we need it:
In certain scenarios, only the images for the
linux/amd64andlinux/arm64platforms need to be synchronized. However, in the current version, if--platformis not specified, images for all platforms are synchronized; whereas when-- platformis specified, only a single platform architecture can be provided. To address this issue, this PR introduces support for specifying multiple platform architectures with--platform, separated by commas.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #1945
Please check the following list: