support copy multi arch instance #2612
Conversation
Signed-off-by: cleverhu <[email protected]>
| @@ -98,8 +99,65 @@ func validateCompressionVariantExists(input []OptionCompressionVariant) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func isPlatformSupported(list internalManifest.List, platform manifest.Schema2PlatformSpec) (digest.Digest, bool) { | |||
There was a problem hiding this comment.
It’s unexpected for a predicate-named function is… to return a non-boolean value.
| if digest, ok := isPlatformSupported(list, platform); !ok { | ||
| missingPlatforms = append(missingPlatforms, platform) | ||
| } else { | ||
| supportedInstance = append(supportedInstance, digest) |
There was a problem hiding this comment.
This is insufficient; there can be multiple instances for the same platform, and in that case, by default, all of them should be copied. (Compare EnsureCompressionVariantsExist).
There was a problem hiding this comment.
I am simulating Docker buildx with this parameter, and the examples passed in are linux/amd64,linux/arm64。If user need to copy all instance, they need to add such as linux/arm64,linux/arm/v6
There was a problem hiding this comment.
I don’t think that’s applicable. The manifest can, legitimately, contain two instances with exactly the same platform specification.
| return nil, fmt.Errorf("requested platforms not found in image: %s", strings.Join(platformStrings, ", ")) | ||
| } | ||
|
|
||
| if len(platforms) == 0 { |
There was a problem hiding this comment.
(If this were a special case, I’d expect it to be handled at the top, before starting the other logic.)
| // prepareInstanceCopies prepares a list of instances which needs to copied to the manifest list. | ||
| func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.Digest, options *Options) ([]instanceCopy, error) { | ||
| filteredInstanceDigests, err := filterInstancesByPlatforms(list, options.ImageListPlatforms) |
There was a problem hiding this comment.
The CopySpecificImages logic exists at the start of the existing range instanceDigest loop; splitting that into two separate locations is unexpected.
|
|
||
| return supportedInstance, nil | ||
| } | ||
|
|
||
| // prepareInstanceCopies prepares a list of instances which needs to copied to the manifest list. | ||
| func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.Digest, options *Options) ([]instanceCopy, error) { |
There was a problem hiding this comment.
prepareInstanceCopies exists, to a large extent, to be a strictly isolated ~functional piece of code that can be unit tested; the option handling, especially the way various options can interact, should have unit tests.
| @@ -285,6 +346,21 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, | |||
| return nil, fmt.Errorf("updating manifest list: %w", err) | |||
| } | |||
|
|
|||
| if len(c.options.Instances) > 0 { | |||
There was a problem hiding this comment.
I’d expect this logic to be driven by prepareInstanceCopies, therefore unit-testable, and unit-tested.
| if len(c.options.Instances) > 0 { | ||
| deletedInstanceDigests := []internalManifest.ListEdit{} | ||
| for _, instanceDigest := range updatedList.Instances() { | ||
| if !slices.Contains(c.options.Instances, instanceDigest) { | ||
| deletedInstanceDigests = append(deletedInstanceDigests, internalManifest.ListEdit{ | ||
| ListOperation: internalManifest.ListOpRemove, | ||
| UpdateOldDigest: instanceDigest, | ||
| }) | ||
| } | ||
| } | ||
| if err = updatedList.EditInstances(deletedInstanceDigests); err != nil { | ||
| return nil, fmt.Errorf("updating manifest list: %w", err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
We outright must not change the semantics of the existing Instances option to trigger manifest edits. c/image keeps a stable API, if at all possible.
| if targetIndex == -1 { | ||
| return fmt.Errorf("Schema2List.EditInstances: digest %s not found", editInstance.UpdateOldDigest) | ||
| } | ||
| index.Manifests = append(index.Manifests[:targetIndex], index.Manifests[targetIndex+1:]...) |
There was a problem hiding this comment.
This should use some form of slices.Delete*. And have a test.
| if targetIndex == -1 { | ||
| return fmt.Errorf("OCI1Index.EditInstances: digest %s not found", editInstance.UpdateOldDigest) | ||
| } | ||
| index.Manifests = append(index.Manifests[:targetIndex], index.Manifests[targetIndex+1:]...) |
There was a problem hiding this comment.
This should use some form of slices.Delete*. And have a test.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| options.Instances = filteredInstanceDigests |
There was a problem hiding this comment.
Outright overwriting and ignoring previously-defined options is not acceptable.
support copy multi arch instance