Don't use Docker adjective if we don't have to #820

rhatdan · 2021-11-08T20:17:50Z

Start using dir instead of docker-dir for podman save.

Use compat-archive rather then docker-archive.

Use docker-dir and docker-archive for compatibility only.

Signed-off-by: Daniel J Walsh [email protected]

openshift-ci · 2021-11-09T08:22:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg

I not a big fan of introducing a new term (i.e., compat). Those images are in the docker format and I would find it very confusing. Users browsing through docs, man pages, blogs and pretty much all existing resources will be trained to distinguish between docker and oci images.

vrothberg · 2021-11-09T09:15:24Z

libimage/save.go

@@ -56,13 +56,13 @@ func (r *Runtime) Save(ctx context.Context, names []string, format, path string,

 	// Dispatch the save operations.
 	switch format {
-	case "oci-archive", "oci-dir", "docker-dir":
+	case "oci-archive", "oci-dir", "docker-dir", "dir":


Why dir? That is not supported by podman save.

I have a WIP PR to support it. We refer to the "dir" transport in containers-transports, we should use the same terminology here.

Could we then make OCI the default for dir? Currently, it's docker.

That is a change in transports though? Would you want this to conflict?

The c/image dir: transport is “raw unmodified accept-anything”, it’s not tied to a manifest format like Docker/OCI, and it’s specified by neither.

I completely support not naming that destination docker-dir.

Making ”OCI” (the manifest format?) default for dir: (the transport) would defeat the original “debugging destination” purpose in c/image; I suppose Podman could set up saves to use the dir: format and force a conversion, but that seems worse in every way than just using the oci: (oci/layout) transport and including an OCI-native index blob and the like.

WRT whether podman save should default to dir:, oci: (a directory), or docker-archive: / oci-archive:, I don’t have a strong preference (I’d weakly say that the dir: debugging format is probably not the right default) — anyway that seems like a topic for a different PR.

I am not interested in changing the default. Just in removing the "docker" adjective from our man pages when possible, especially when it adds little value. 10 instances of Docker in the podman save man page, none of which add any value to the user IMHO.

man podman save | grep docker podman save saves an image to either docker-archive, oci-archive, oci- dir (directory with oci manifest type), or docker-dir (directory with v2s2 manifest type) on the local machine, default is docker-archive. dir transport i.e --format=oci-dir or --format=docker-dir Save image to docker-archive, oci-archive (see containers-trans‐ ports(5)), oci-dir (oci transport), or docker-dir (dir transport with --format docker-archive --format docker-dir docker-archive. The default for this option can be modified via the $ podman save --format docker-dir -o ubuntu-dir ubuntu

If it’s just that, the man page lists the available options three times, when doing so just once in the --format section would probably suffice.

The default value, and the conditionals for --compress and --multi-image-archive, can’t be eliminated that easily, to be fair.

the man page lists the available options three times, when doing so just once in the --format section would probably suffice.

containers/podman#12489

vrothberg · 2021-11-09T09:15:31Z

libimage/save.go

@@ -105,7 +105,7 @@ func (r *Runtime) saveSingleImage(ctx context.Context, name, format, path string
 		destRef, err = ociTransport.NewReference(path, tag)
 		options.ManifestMIMEType = ociv1.MediaTypeImageManifest

-	case "docker-dir":
+	case "docker-dir", "dir":


Why dir? That is not supported by podman save.

Start using dir instead of docker-dir for podman save. Use compat-archive rather then docker-archive. Use docker-dir and docker-archive for compatibility only. Signed-off-by: Daniel J Walsh <[email protected]>

vrothberg

LGTM
@mtrmac PTAL/FYI

mtrmac · 2021-12-02T12:03:21Z

libimage/save.go

 		if len(names) > 1 {
 			return errors.Errorf("%q does not support saving multiple images (%v)", format, names)
 		}
 		return r.saveSingleImage(ctx, names[0], format, path, options)

-	case "docker-archive":
+	case "docker-archive", "compat-archive":
 		options.ManifestMIMEType = manifest.DockerV2Schema2MediaType


(Completely unrelated, I don’t see why this is hard-coded here. The transport already contains only that value in SupportedManifestMIMETypes — so this maybe just silently ignores user’s explicit format value instead of failing.

Is there some non-obvious reason for doing this? If so, it might be worth a comment.)

Probably just old code. I can remove it.

I’m really after the knowledge, not after dropping the line. It came in #551 and looks fairly unlikely to be accidental, but it’s not obvious to me why it was added.

If we don’t know, let’s leave it it, at least in this PR.

mtrmac · 2021-12-02T12:09:35Z

I not a big fan of introducing a new term (i.e., compat). Those images are in the docker format and I would find it very confusing.

I agree. It immediately begs the question “compat with what?”.

I am also very much in favor of steering users away from both of the tar archive formats for any kind of transport, they are horribly inefficient. I don’t think naming it them “compat” doesn’t help that at all — if anything, it makes it more likely to be chosen because “that one is intended to be widely compatible”. That would be, from my point of view, a very regrettable outcome of this (lack of) branding choice.

rhatdan · 2021-12-02T14:36:46Z

How about just calling archive and dir.

mtrmac · 2021-12-02T18:22:48Z

libimage/save.go

@@ -105,7 +105,7 @@ func (r *Runtime) saveSingleImage(ctx context.Context, name, format, path string
 		destRef, err = ociTransport.NewReference(path, tag)
 		options.ManifestMIMEType = ociv1.MediaTypeImageManifest

-	case "docker-dir":
+	case "docker-dir", "dir":
 		destRef, err = dirTransport.NewReference(path)
 		options.ManifestMIMEType = manifest.DockerV2Schema2MediaType


So actually there’s this hard-coded format setting, making docker-dir not be the raw “preserve original format” destination that c/image dir: is.

And the history of that is containers/podman@d2ab53a , which has originally used oci-dir for dir: with OCI conversion forced, and docker-dir for dir: with v2s2 conversion forced. I.e. the docker- part in the name is historically very much intentional and semantically meaningful.

Only fairly recently was oci-dir changed to use the OCI repository format ( containers/podman#6814 ), making the oci-dir/docker-dir less of a clear-cut pair.

Still — right now it is not a “Docker-defined directory”, but it is a “Docker-format-containing directory”. I’m not too happy about dropping the format name from the format.

I’m tempted to complicate this further, leave docker-dir as is, and define a new dir to have the c/image meaning (i.e. no conversion unless the user explicitly asked for one).

I’m tempted to complicate this further, leave docker-dir as is, and define a new dir to have the c/image meaning (i.e. no conversion unless the user explicitly asked for one).

Actually a podman push $source dir:… should work for that already, so we don’t need to make this an explicit podman image save feature without any specific user demand.

Yup
man docker save shows none of these. So there is little value in us flooding the man pages with "docker".

docker save does not even support --format.

Podman does support --format, so Podman users need to know which value to use to interoperate with various tools, whether that is umoci or Docker. (Even if the default is to use a Docker-compatible format, and users don’t need to explicitly use --format to get that kind of output, I think the man page should document clearly enough that that’s what the command is intended to do, so that users can rely on that.)

mtrmac · 2021-12-02T18:36:36Z

I completely support not naming that destination docker-dir.

Looking more closely, given #820 (review) , I no longer think it’s obvious. Considering that the metadata format and file names are from the c/image dir: transport, and not defined elsewhere, I guess dir is acceptable, though using such a generic word is not quite ideal.

How about just calling archive and dir.

WRT a generic archive, I fairly strongly disagree. The format choice matters to users.

This format doesn’t have a formal name to my knowledge, so we are free to be descriptive, like docker-load-consumable or the-docker-engine-API-/images/load-format, but I can’t see that buys anything.

If we invent a completely new non-branded name (letters of the Greek alphabet or whatever), that just runs into the-artist-formerly-named-Prince problem, and results in even more explanatory blogs to be published.

rhatdan · 2021-12-02T20:05:07Z

podman save --format omicron :^)

I don't believe there are that many people changing the format of podman save. (I would bet you the number could fit into a small closet.) The old names would continue to be supported, just hidden from the user.

People are moving to Containerfile, better then I thought...

mtrmac · 2021-12-02T21:02:45Z

I don't believe there are that many people changing the format of podman save. (I would bet you the number could fit into a small closet.)

I think that’s very likely — but to me that suggests that trying to deemphasize it even further is not all that worthwhile.

rhatdan · 2021-12-03T15:29:44Z

Ok, not important enough to continue slogging this out would much rather get rid of the docker:// transport.

openshift-ci bot added the approved label Nov 8, 2021

saschagrunert approved these changes Nov 9, 2021

View reviewed changes

vrothberg reviewed Nov 9, 2021

View reviewed changes

rhatdan force-pushed the docker branch from 80a498a to c1cc6b7 Compare November 9, 2021 15:34

rhatdan mentioned this pull request Nov 9, 2021

[WIP] Switch docker-dir->dir & docker-archive->compat-archive podman save/load containers/podman#12228

Closed

rhatdan force-pushed the docker branch from bdbe8a3 to c1cc6b7 Compare November 10, 2021 20:10

rhatdan force-pushed the docker branch 3 times, most recently from d4c1add to 9dd504d Compare December 1, 2021 19:33

Don't use Docker adjective if we don't have to

60a6cb3

Start using dir instead of docker-dir for podman save. Use compat-archive rather then docker-archive. Use docker-dir and docker-archive for compatibility only. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the docker branch from 9dd504d to 60a6cb3 Compare December 1, 2021 20:34

vrothberg reviewed Dec 2, 2021

View reviewed changes

mtrmac reviewed Dec 2, 2021

View reviewed changes

mtrmac mentioned this pull request Dec 2, 2021

Improve documentation of (podman image save) containers/podman#12489

Merged

rhatdan closed this Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use Docker adjective if we don't have to #820

Don't use Docker adjective if we don't have to #820

rhatdan commented Nov 8, 2021

openshift-ci bot commented Nov 9, 2021

vrothberg left a comment

vrothberg Nov 9, 2021

rhatdan Nov 9, 2021

vrothberg Nov 9, 2021

rhatdan Nov 10, 2021

mtrmac Dec 2, 2021

rhatdan Dec 2, 2021

mtrmac Dec 2, 2021

mtrmac Dec 2, 2021

vrothberg Nov 9, 2021

rhatdan Nov 9, 2021

vrothberg left a comment

mtrmac Dec 2, 2021

rhatdan Dec 2, 2021

mtrmac Dec 2, 2021

mtrmac commented Dec 2, 2021

rhatdan commented Dec 2, 2021

mtrmac Dec 2, 2021

mtrmac Dec 2, 2021

rhatdan Dec 2, 2021 •

edited

Loading

rhatdan Dec 2, 2021

mtrmac Dec 2, 2021 •

edited

Loading

mtrmac commented Dec 2, 2021 •

edited

Loading

rhatdan commented Dec 2, 2021

mtrmac commented Dec 2, 2021

rhatdan commented Dec 3, 2021

Don't use Docker adjective if we don't have to #820

Don't use Docker adjective if we don't have to #820

Conversation

rhatdan commented Nov 8, 2021

openshift-ci bot commented Nov 9, 2021

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Dec 2, 2021

rhatdan commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

mtrmac commented Dec 2, 2021 • edited Loading

rhatdan commented Dec 2, 2021

mtrmac commented Dec 2, 2021

rhatdan commented Dec 3, 2021

rhatdan Dec 2, 2021 •

edited

Loading

mtrmac Dec 2, 2021 •

edited

Loading

mtrmac commented Dec 2, 2021 •

edited

Loading