Conversation
cli/command/image/convert.go
Outdated
| args.Src = posArgs[0] | ||
| args.Dst = posArgs[1] |
There was a problem hiding this comment.
Change into flag arguments.
vvoland
force-pushed
the
c8d-multiplatform-convert
branch
2 times, most recently
from
54d1564 to
4b3b15f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4983 +/- ##
==========================================
- Coverage 61.34% 61.23% -0.11%
==========================================
Files 295 296 +1
Lines 20712 20751 +39
==========================================
+ Hits 12705 12707 +2
- Misses 7104 7142 +38
+ Partials 903 902 -1 |
vvoland
force-pushed
the
c8d-multiplatform-convert
branch
from
e41109a to
e09e6da
Compare
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
vvoland
force-pushed
the
c8d-multiplatform-convert
branch
from
e09e6da to
d9775f8
Compare
| flags.BoolVar(&args.NoAttestations, "no-attestations", false, "Do not include image attestations") | ||
| flags.BoolVar(&args.OnlyAvailablePlatforms, "available", false, "Only include platforms locally available to the daemon") | ||
| flags.StringArrayVar(&args.Dst, "to", nil, "Target image references") | ||
| flags.StringVar(&args.Src, "from", "", "Source image reference") |
There was a problem hiding this comment.
nerdctl already has nerdctl image convert SRC DST with a different purpose (akin to ctr images convert) :
https://github.com/containerd/nerdctl/blob/main/docs/command-reference.md#nerd_face-nerdctl-image-convert
Would it be possible to adopt the same syntax as nerdctl image convert SRC DST, or change the command name to avoid conflict?
There was a problem hiding this comment.
I think in general, the purpose is the same. Unfortunately I don't have any better idea for a name... Do you have any suggestions?
We deliberately chose the src and dst to be flag arguments instead of positional arguments as it gives more clarity, especially when multiple destinations might be specified.
There was a problem hiding this comment.
I don't think we should change our syntax (esp. since we settled on this one as the most user-friendly/better option) just for compatibility's sake.
Unless there's a more explicit/as clear name, I also don't think we should change the name to avoid "colliding" with nerdctls command, although I do understand the possible confusion if we have two commands with the same name/subcommand and different syntax.
There was a problem hiding this comment.
We deliberately chose the src and dst to be flag arguments instead of positional arguments as it gives more clarity, especially when multiple destinations might be specified.
What about making it (docker|nerdctl) image convert SRC DST1 DST2 DST3, or (docker|nerdctl) image convert SRC DST1,DST2,DST3?
There was a problem hiding this comment.
Definitely a fan of positional for this but I haven't looked at the PR yet, just this thread.
There was a problem hiding this comment.
cc @tianon - I think you had a strong opinion on it being a flag argument 😄
There was a problem hiding this comment.
Took me a minute, but I remember now what my thoughts were. 😅
IMO, this command should match docker build -- ala, positional arguments for input, and --tag/-t for output(s).
(I think we could make the same argument for docker push -t foo -t bar -t baz image-to-push)
In the case of convert, we might have multiple inputs as well, right? For example, if we have a way to remove things from an index, wouldn't it make sense to also have a way to add things to one / create one? That could then be multiple positional arguments (now unambiguous if we avoid destination being positional).
| } | ||
|
|
||
| flags := cmd.Flags() | ||
| flags.StringArrayVar(&args.Platforms, "platforms", nil, "Include only the specified platforms in the destination image") |
There was a problem hiding this comment.
This is named "platform" rather than "platforms" in nerdctl and ctr
7 participants
Add
docker image convertcommand- Description for the changelog