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

[CI:DOCS] Improve podman-save and podman-push documentation #20537

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

Conversation

amluto
Copy link

@amluto amluto commented Oct 30, 2023

podman-save has awkward semantics with regard tagging the saved image in the output. Improve the documentation, and suggest using podman push as an alternative.

Add a corresponding example to podman push's manpage.

See #20515

Does this PR introduce a user-facing change?

I think None. It's user-facing in the sense that it updates documentation. I don't think it needs a release note. Certainly no action is required.

If a release note is required, I suggest:

Updated podman-save documentation to suggest using podman push instead.

podman-save has awkward semantics with regard tagging the saved image
in the output.  Improve the documentation, and suggest using podman push
as an alternative.

Add a corresponding example to podman push's manpage.

Signed-off-by: Andy Lutomirski <[email protected]>
@@ -40,6 +40,9 @@ $ sudo podman push myimage docker-daemon:docker.io/library/myimage:33

# Push to a tarball in the OCI format
$ podman push myimage oci-archive:/tmp/myimage

# Push to a directory, with a relative path, in the OCI format with another tag
Copy link
Member

@rhatdan rhatdan Oct 30, 2023

Choose a reason for hiding this comment

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

Add something about this being a good alternative to podman save

One you reference it, make sure you add podman save to the See Also section below.

@rhatdan rhatdan changed the title Improve podman-save and podman-push documentation [CI:DOCS] Improve podman-save and podman-push documentation Oct 30, 2023
@rhatdan
Copy link
Member

rhatdan commented Oct 30, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2023
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! I'm unable to understand how/why podman push solves the problem reported in #20515 (IMHO it defers the name-finding question, it does not solve it). But I'll leave that for others to argue. Inline, a few suggestions.

Comment on lines 44 to 45
# Push to a directory, with a relative path, in the OCI format with another tag
$ podman push myimage oci:directory:tag
Copy link
Member

Choose a reason for hiding this comment

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

IMO the use of a relative path here makes everything more confusing: the casual reader has no way of knowing if the word directory is a keyword or an argument. oci:/tmp/myimage:mytag would be clearer, and oci:/tmp/myimagedir:quay.io/myrepo/myimage:mytag even more so.

Copy link
Author

Choose a reason for hiding this comment

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

I feel like adding a name like quay.io/myrepo/myimage:mytag makes the result more complicated.

@@ -22,6 +22,8 @@ Note: `:` is a restricted character and cannot be part of the file name.

**podman save [OPTIONS] NAME[:TAG]**

Most or all of **podman save**'s functionality is available, with more flexibility, in **[podman-push(1)](podman-push.1.md)**.
Copy link
Member

Choose a reason for hiding this comment

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

This is debatable. It might be friendlier just to not add this.

Copy link
Author

Choose a reason for hiding this comment

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

The comments in #20515 made it sounds like podman save is mostly for Docker compatibility. I'll defer to people actually involved in podman.

Copy link
Member

Choose a reason for hiding this comment

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

I'd not include this

Copy link
Member

Choose a reason for hiding this comment

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

I think the statement is correct but also thin that the text above is sufficient to highlight that podman-push can be used to create archives as well.

@@ -39,6 +41,14 @@ An image format to produce, one of:
| **oci-dir** | A directory using the OCI Image Format |
| **docker-dir** | **dir** transport (see **containers-transports(5)**) with v2s2 manifest type |

If **oci-archive** or **oci-dir** saves an image specified by tag, then it will save it in the OCI Image with the same tag. If **oci-archive** or **oci-dir** saves an image specified by hash, it will not tag it at all, which may make using the resulting image unnecessarily complicated.
Copy link
Member

Choose a reason for hiding this comment

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

The word "tag" is overloaded, hence ambiguous. Replacing the first two instances of tag with name might be more readable.

And, would you consider removing the "which may" clause? That may be the case for some workflows but certainly not for others.

Copy link
Author

Choose a reason for hiding this comment

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

I reworded it.

I'm not that familiar with all the various OCI Image-consuming tools out there, but so far the only thing I've found that can work with these unnamed images is podman load.

This comes from PR containers#20537 comments.

Signed-off-by: Andy Lutomirski <[email protected]>
@amluto amluto closed this Oct 30, 2023
@amluto amluto deleted the save-docs branch October 30, 2023 20:12
@amluto
Copy link
Author

amluto commented Oct 30, 2023

Bah, I made a silly typo pushing an update and github interpreted that as closing the PR.

@amluto amluto reopened this Oct 30, 2023
@@ -39,6 +41,14 @@ An image format to produce, one of:
| **oci-dir** | A directory using the OCI Image Format |
| **docker-dir** | **dir** transport (see **containers-transports(5)**) with v2s2 manifest type |

If **oci-archive** or **oci-dir** saves an image specified by name, then it will save it in the OCI Image with the same name (i.e. an **org.opencontainers.image.ref.name** annotation will be written). If **oci-archive** or **oci-dir** saves an image specified by hash, the image will not be given a name in the saved image, which may make using the resulting image with tools that expect named images difficult.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If **oci-archive** or **oci-dir** saves an image specified by name, then it will save it in the OCI Image with the same name (i.e. an **org.opencontainers.image.ref.name** annotation will be written). If **oci-archive** or **oci-dir** saves an image specified by hash, the image will not be given a name in the saved image, which may make using the resulting image with tools that expect named images difficult.
If **oci-archive** or **oci-dir** saves an image specified by name, then it will keep it in the OCI Image with the same name (i.e., an **org.opencontainers.image.ref.name** annotation will be written). If **oci-archive** or **oci-dir** saves an image specified by hash, the image will not be given a name in the saved image, which may make using the resulting image with tools that expect named images difficult.

Copy link
Member

Choose a reason for hiding this comment

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

I'm on the fence about keep/save. The comma would be good though.

Copy link
Author

Choose a reason for hiding this comment

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

I'll keep "save". Saying "keep" seems odd -- the image doesn't start out in the OCI Image at all, so the image isn't being kept.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Great work, thanks! We can merge once the comments are addressed. I really like the description of the behavior when only a digest/hash is specified.

@@ -22,6 +22,8 @@ Note: `:` is a restricted character and cannot be part of the file name.

**podman save [OPTIONS] NAME[:TAG]**

Most or all of **podman save**'s functionality is available, with more flexibility, in **[podman-push(1)](podman-push.1.md)**.
Copy link
Member

Choose a reason for hiding this comment

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

I think the statement is correct but also thin that the text above is sufficient to highlight that podman-push can be used to create archives as well.

@umohnani8
Copy link
Member

@amluto can you please address the comments above so we can get this merged.

@amluto
Copy link
Author

amluto commented Nov 14, 2023

I'm leaving the "Most or all of podman save's functionality is available" part for now -- the other text that conveys that is farther down and maybe easy to miss. But feel free to argue with me :)

Copy link

Cockpit tests failed for commit e9df8bd. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

Over night apparently quay.io was down, so all cockpit-revdeps tests failed with

Trying to pull quay.io/prometheus/busybox:latest...
Error: initializing source docker://quay.io/prometheus/busybox:latest: can't talk to a V1 container registry

It seems to be working again, so feel free to /packit retry-failed (or click "Re-run" here)

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Thank you for your patience. Comments inline. Please also rebase to a current main and squash commits.

@@ -40,6 +44,9 @@ $ sudo podman push myimage docker-daemon:docker.io/library/myimage:33

# Push to a tarball in the OCI format
$ podman push myimage oci-archive:/tmp/myimage

# Push to a directory in the OCI format with an explicitly specified tag
$ podman push myimage oci:/tmp/myimage:tag
Copy link
Member

Choose a reason for hiding this comment

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

This is a counterintuitive situation for many users. Please add a note making it unambiguously clear that the destination directory is /tmp/myimage and that tag is an annotation:

# Push to a directory ...
$ podman push myimage oci:/tmp/myimage:myname/mytag
$ jq '.manifests[].annotations' /tmp/myimage/index.json
{
  "org.opencontainers.image.ref.name": "myname/mytag"
}

@@ -39,6 +41,14 @@ An image format to produce, one of:
| **oci-dir** | A directory using the OCI Image Format |
| **docker-dir** | **dir** transport (see **containers-transports(5)**) with v2s2 manifest type |

If **oci-archive** or **oci-dir** saves an image specified by name, then it will save it in the OCI Image with the same name (i.e., an **org.opencontainers.image.ref.name** annotation will be written). If **oci-archive** or **oci-dir** saves an image specified by hash, the image will not be given a name in the saved image, which may make using the resulting image with tools that expect named images difficult.
Copy link
Member

Choose a reason for hiding this comment

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

Awkward phrasing, I find it hard to parse. At the very least please differentiate by name vs by hash to make it easier for a human reader.

@@ -39,6 +41,14 @@ An image format to produce, one of:
| **oci-dir** | A directory using the OCI Image Format |
| **docker-dir** | **dir** transport (see **containers-transports(5)**) with v2s2 manifest type |

If **oci-archive** or **oci-dir** saves an image specified by name, then it will save it in the OCI Image with the same name (i.e., an **org.opencontainers.image.ref.name** annotation will be written). If **oci-archive** or **oci-dir** saves an image specified by hash, the image will not be given a name in the saved image, which may make using the resulting image with tools that expect named images difficult.

To explicity specify the name to be used in the saved image, use the **oci-archive:** or **oci::** schemes with **[podman-push(1)](podman-push.1.md)** instead. For example:
Copy link
Member

Choose a reason for hiding this comment

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

Typo, "explicitly"

To explicity specify the name to be used in the saved image, use the **oci-archive:** or **oci::** schemes with **[podman-push(1)](podman-push.1.md)** instead. For example:

```
$ podman push IMAGE oci:directoryname:tagname
Copy link
Member

Choose a reason for hiding this comment

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

I have to insist that this is confusing: a nonexpert reader will have trouble figuring out what is an exact keyword and what is a mutable argument. See above for (IMO) clearer ways to say this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants