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.

Sign up for GitHub

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

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

Closed
wants to merge 1 commit into from
+28 −23
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions libimage/image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import (
func TestImageFunctions(t *testing.T) {
// Note: this will resolve pull from the GCR registry (see
// testdata/registries.conf).
busyboxLatest := "docker.io/library/busybox:latest"
busyboxDigest := "docker.io/library/busybox@"
busyboxLatest := "quay.io/libpod/busybox:latest"
busyboxDigest := "quay.io/libpod/busybox@"

runtime, cleanup := testNewRuntime(t)
defer cleanup()
Expand Down Expand Up @@ -142,7 +142,7 @@ func TestImageFunctions(t *testing.T) {
require.False(t, hasDifferentDigest, "image with same digest should have the same manifest (and hence digest)")

// Different images -> different digests
remoteRef, err = alltransports.ParseImageName("docker://docker.io/library/alpine:latest")
remoteRef, err = alltransports.ParseImageName("docker://quay.io/libpod/alpine:latest")
require.NoError(t, err)
hasDifferentDigest, err = image.HasDifferentDigest(ctx, remoteRef, nil)
require.NoError(t, err)
Expand Down Expand Up @@ -191,7 +191,7 @@ func TestInspectHealthcheck(t *testing.T) {
func TestTag(t *testing.T) {
// Note: this will resolve pull from the GCR registry (see
// testdata/registries.conf).
busyboxLatest := "docker.io/library/busybox:latest"
busyboxLatest := "quay.io/libpod/busybox:latest"

runtime, cleanup := testNewRuntime(t)
defer cleanup()
Expand Down Expand Up @@ -242,7 +242,7 @@ func TestTag(t *testing.T) {
func TestUntag(t *testing.T) {
// Note: this will resolve pull from the GCR registry (see
// testdata/registries.conf).
busyboxLatest := "docker.io/library/busybox:latest"
busyboxLatest := "quay.io/libpod/busybox:latest"

runtime, cleanup := testNewRuntime(t)
defer cleanup()
Expand Down
6 changes: 3 additions & 3 deletions libimage/push_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func TestPush(t *testing.T) {
// Prefetch alpine.
pullOptions := &PullOptions{}
pullOptions.Writer = os.Stdout
_, err := runtime.Pull(ctx, "docker.io/library/alpine:latest", config.PullPolicyAlways, pullOptions)
_, err := runtime.Pull(ctx, "quay.io/libpod/alpine:latest", config.PullPolicyAlways, pullOptions)
require.NoError(t, err)

pushOptions := &PushOptions{}
Expand Down Expand Up @@ -76,7 +76,7 @@ func TestPushOtherPlatform(t *testing.T) {
pullOptions := &PullOptions{}
pullOptions.Writer = os.Stdout
pullOptions.Architecture = "arm64"
pulledImages, err := runtime.Pull(ctx, "docker.io/library/alpine:latest", config.PullPolicyAlways, pullOptions)
pulledImages, err := runtime.Pull(ctx, "quay.io/libpod/alpine:latest", config.PullPolicyAlways, pullOptions)
require.NoError(t, err)
require.Len(t, pulledImages, 1)

Expand All @@ -90,6 +90,6 @@ func TestPushOtherPlatform(t *testing.T) {
require.NoError(t, err)
tmp.Close()
defer os.Remove(tmp.Name())
_, err = runtime.Push(ctx, "docker.io/library/alpine:latest", "docker-archive:"+tmp.Name(), pushOptions)
_, err = runtime.Push(ctx, "quay.io/libpod/alpine:latest", "docker-archive:"+tmp.Name(), pushOptions)
require.NoError(t, err)
}
2 changes: 1 addition & 1 deletion libimage/remove_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
func TestRemoveImages(t *testing.T) {
// Note: this will resolve pull from the GCR registry (see
// testdata/registries.conf).
busyboxLatest := "docker.io/library/busybox:latest"
busyboxLatest := "quay.io/libpod/busybox:latest"

runtime, cleanup := testNewRuntime(t)
defer cleanup()
Expand Down
18 changes: 9 additions & 9 deletions libimage/save.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ type SaveOptions struct {
}

// Save saves one or more images indicated by `names` in the specified `format`
// to `path`. Supported formats are oci-archive, docker-archive, oci-dir and
// docker-dir. The latter two adhere to the dir transport in the corresponding
// oci or docker v2s2 format. Please note that only docker-archive supports
// to `path`. Supported formats are oci-archive, compat-archive, oci-dir and
// dir. The latter two adhere to the dir transport in the corresponding
// oci or docker v2s2 format. Please note that only compat-archive supports
// saving more than one images. Other formats will yield an error attempting
// to save more than one.
func (r *Runtime) Save(ctx context.Context, names []string, format, path string, options *SaveOptions) error {
Expand All @@ -46,8 +46,8 @@ func (r *Runtime) Save(ctx context.Context, names []string, format, path string,
case 1:
// All formats support saving 1.
default:
if format != "docker-archive" {
return errors.Errorf("unsupported format %q for saving multiple images (only docker-archive)", format)
if format != "docker-archive" && format != "compat-archive" {
return errors.Errorf("unsupported format %q for saving multiple images (only compat-archive)", format)
}
if len(options.AdditionalTags) > 0 {
return errors.Errorf("cannot save multiple images with multiple tags")
Expand All @@ -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":
Copy link
Member

Choose a reason for hiding this comment

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

Why dir? That is not supported by podman save.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

containers/podman#12489

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
Copy link
Contributor

Choose a reason for hiding this comment

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

(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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably just old code. I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return r.saveDockerArchive(ctx, names, path, options)
}
Expand All @@ -72,7 +72,7 @@ func (r *Runtime) Save(ctx context.Context, names []string, format, path string,
}

// saveSingleImage saves the specified image name to the specified path.
// Supported formats are "oci-archive", "oci-dir" and "docker-dir".
// Supported formats are "oci-archive", "oci-dir" and "docker-dir", "dir".
func (r *Runtime) saveSingleImage(ctx context.Context, name, format, path string, options *SaveOptions) error {
image, imageName, err := r.LookupImage(name, nil)
if err != nil {
Expand Down Expand Up @@ -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":
Copy link
Member

Choose a reason for hiding this comment

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

Why dir? That is not supported by podman save.

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

destRef, err = dirTransport.NewReference(path)
options.ManifestMIMEType = manifest.DockerV2Schema2MediaType
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@rhatdan rhatdan Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

docker save does not even support --format.

Copy link
Contributor

@mtrmac mtrmac Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.)


Expand Down
15 changes: 10 additions & 5 deletions libimage/save_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ func TestSave(t *testing.T) {
// Prefetch alpine, busybox.
pullOptions := &PullOptions{}
pullOptions.Writer = os.Stdout
_, err := runtime.Pull(ctx, "docker.io/library/alpine:latest", config.PullPolicyAlways, pullOptions)
_, err := runtime.Pull(ctx, "quay.io/libpod/alpine:latest", config.PullPolicyAlways, pullOptions)
require.NoError(t, err)
_, err = runtime.Pull(ctx, "docker.io/library/busybox:latest", config.PullPolicyAlways, pullOptions)
_, err = runtime.Pull(ctx, "quay.io/libpod/busybox:latest", config.PullPolicyAlways, pullOptions)
require.NoError(t, err)

// Save the two images into a multi-image archive. This way, we can
Expand All @@ -32,7 +32,7 @@ func TestSave(t *testing.T) {
require.NoError(t, err)
imageCache.Close()
defer os.Remove(imageCache.Name())
err = runtime.Save(ctx, []string{"alpine", "busybox"}, "docker-archive", imageCache.Name(), saveOptions)
err = runtime.Save(ctx, []string{"alpine", "busybox"}, "compat-archive", imageCache.Name(), saveOptions)
require.NoError(t, err)

loadOptions := &LoadOptions{}
Expand All @@ -58,13 +58,18 @@ func TestSave(t *testing.T) {
{[]string{"busybox"}, nil, "oci-archive", false, false},
// oci-archive doesn't support multi-image archives
{[]string{"busybox", "alpine"}, nil, "oci-archive", false, true},
// docker
// compat
{[]string{"busybox"}, nil, "docker-archive", false, false},
{[]string{"busybox"}, nil, "compat-archive", false, false},
{[]string{"busybox"}, []string{"localhost/tag:1", "quay.io/repo/image:tag"}, "docker-archive", false, false},
{[]string{"busybox"}, []string{"localhost/tag:1", "quay.io/repo/image:tag"}, "compat-archive", false, false},
{[]string{"busybox"}, nil, "docker-dir", true, false},
{[]string{"busybox"}, nil, "dir", true, false},
{[]string{"busybox", "alpine"}, nil, "docker-archive", false, false},
{[]string{"busybox", "alpine"}, nil, "compat-archive", false, false},
// additional tags and multi-images conflict
{[]string{"busybox", "alpine"}, []string{"tag"}, "docker-archive", false, true},
{[]string{"busybox", "alpine"}, []string{"tag"}, "compat-archive", false, true},
} {
// First clean up all images and load the cache.
_, rmErrors := runtime.RemoveImages(ctx, nil, nil)
Expand Down Expand Up @@ -100,7 +105,7 @@ func TestSave(t *testing.T) {
// Now make sure that all specified names (and tags) resolve to
// an image the local containers storage. Note that names are
// only preserved in archives.
if strings.HasSuffix(test.format, "-dir") {
if strings.HasSuffix(test.format, "dir") {
continue
}
_, err = runtime.ListImages(ctx, namesAndTags, nil)
Expand Down