Skip to content

feat: allow saving specific platforms for an image #3218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LaurentGoderre
Copy link

@LaurentGoderre LaurentGoderre commented Jun 26, 2025

What does this PR do?

This introduces a way to pass image save options (like platform) to the image client. This allows saving a specific architecture for an image.

Why is it important?

With OCI multi-arch images, it is sometimes needed to save image for a different architecture than the current one. This changes allows to control the behavior instead of defaulting to the current platform.

@LaurentGoderre LaurentGoderre requested a review from a team as a code owner June 26, 2025 18:27
Copy link

netlify bot commented Jun 26, 2025

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit a1213e5
🔍 Latest deploy log https://app.netlify.com/projects/testcontainers-go/deploys/685d97b79584d400086243b5
😎 Deploy Preview https://deploy-preview-3218--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@LaurentGoderre LaurentGoderre force-pushed the save-image-multi-arch branch from 2dcc37c to b5a1892 Compare June 26, 2025 18:30
@LaurentGoderre LaurentGoderre changed the title feat: Allow saving specific platforms for an image feat: allow saving specific platforms for an image Jun 26, 2025
@LaurentGoderre LaurentGoderre force-pushed the save-image-multi-arch branch from b5a1892 to a1213e5 Compare June 26, 2025 18:55
Copy link
Contributor

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Thanks for this, I've done a quick pass with a few improvement suggestions.

@@ -1784,8 +1784,30 @@ func (p *DockerProvider) ListImages(ctx context.Context) ([]ImageInfo, error) {
return images, nil
}

func SaveImageOptionWithPlatforms(plaforms ...specs.Platform) SaveImageOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: add comment for all public methods, more in other files.

question: could this have a simpler name, WithPlatforms or SaveImagesWithPlatforms

return p.SaveImagesWithOps(ctx, output, images)
}

// SaveImages exports a list of images as an uncompressed tar
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: correct description which refers to the old name

Platforms []specs.Platform
}

type SaveImageOption func(*SaveOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: return an error so its future proof for extensions.

@@ -10,9 +12,16 @@ type ImageInfo struct {
Name string
}

type SaveOptions struct {
Platforms []specs.Platform
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: keep details private so changes can be used without breaking the API.

Copy link
Author

Choose a reason for hiding this comment

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

The reason I need it is for the K3S module so I would have to implement a duplicate type in that module

require.NoErrorf(t, err, "failed to get provider")

defer func() {
_ = provider.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: check the error.

Copy link
Contributor

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

Clicking the right option this time 😄

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.

2 participants