Skip to content
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

c8d: Add --platform flag to history, save and load #5331

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vvoland
Copy link
Collaborator

@vvoland vvoland commented Aug 8, 2024

Add --platform flag to history, save and load.

- Description for the changelog

containerd image store: `docker load` and `docker save` now support a `--platform` flag allowing to choose a specific single-platform image to load/save if the image is multi-platform
containerd image store: `docker history` now supports a `--platform` flag allowing to choose a specific platform to show the history for.

- A picture of a cute animal (not mandatory but encouraged)

If the platform is not specified, the host platform is preferred if it's available, otherwise any available platform is used.

Format: os[/arch[/variant]]
Example: "linux/amd64"`)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about including the full cmd?

Suggested change
Example: "linux/amd64"`)
Example: "docker image history --platform linux/amd64"`)

Copy link
Member

Choose a reason for hiding this comment

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

I (somewhat) assume that this was done to allow defining a global const for these flags and for them to be shared (similar to what we did for --format 🤔), but I need to check if they differ in other ways

Comment on lines 47 to 52
`Pick a single-platform to be loaded if the image is multi-platform.
Full multi-platform image will be load if not specified.

Format: os[/arch[/variant]]
Example: "linux/amd64"`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Pick a single-platform to be loaded if the image is multi-platform.
Full multi-platform image will be load if not specified.
Format: os[/arch[/variant]]
Example: "linux/amd64"`)
`Specify a platform from a multi-platform image to load.
If a platform is not specified, and the image is a multi-platform image, all platforms are loaded.
Format: os[/arch[/variant]]
Example: "docker image load --platform linux/amd64"`)

Comment on lines 45 to 49
`Pick a single-platform to be saved if the image is multi-platform.
Full multi-platform image will be saved if not specified.

Format: os[/arch[/variant]]
Example: "linux/amd64"`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Pick a single-platform to be saved if the image is multi-platform.
Full multi-platform image will be saved if not specified.
Format: os[/arch[/variant]]
Example: "linux/amd64"`)
`Specify a platform from a multi-platform image to save.
If a platform is not specified, and the image is a multi-platform image, all platform variants are saved.
Format: os[/arch[/variant]]
Example: "docker image save --platform linux/amd64"`)

@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 53.84615% with 18 lines in your changes missing coverage. Please review.

Project coverage is 59.73%. Comparing base (3a3fe84) to head (8ebf4cf).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5331      +/-   ##
==========================================
- Coverage   59.74%   59.73%   -0.02%     
==========================================
  Files         345      345              
  Lines       23432    23469      +37     
==========================================
+ Hits        14000    14019      +19     
- Misses       8458     8473      +15     
- Partials      974      977       +3     

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah marked this pull request as ready for review September 24, 2024 15:19
@thaJeztah thaJeztah requested a review from a team as a code owner September 24, 2024 15:19
Comment on lines +52 to +53
`Specify a platform from a multi-platform image to show the history for.
If the platform is not specified, the host platform is preferred if it's available, otherwise any available platform is used.
Copy link
Member

@thaJeztah thaJeztah Sep 24, 2024

Choose a reason for hiding this comment

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

@dvdksn I applied your suggestions; I'm slightly wondering if we should / could condense the information a bit; for example;

Specify a platform -> Platform from a multi-platform image ...

(I don't think we generally add Specify .... as that's a bit implicit for options?)

The "format" could possibly be included in the first line, something like;

Platform from a multi-platform image to show the history for (format: "os[/arch[/variant]]")

By default, the daemon's native platform is preferred, otherwise any available platform is used.

(wondering if the host platform is ambiguous, and if we need to emphasise daemon platform)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api area/ux containerd-integration Issues and PRs related to containerd integration impact/changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants