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] Document --format for additional man pages #17766

Merged
merged 7 commits into from
Mar 15, 2023

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Mar 13, 2023

Does this PR introduce a user-facing change?

Document --format in all man pages

Improves on #17472

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 13, 2023
@rhatdan rhatdan force-pushed the man branch 4 times, most recently from 8ec004e to acec469 Compare March 13, 2023 20:45
@rhatdan
Copy link
Member Author

rhatdan commented Mar 13, 2023

@edsantiago What do I now need to do to clean this up?

@edsantiago
Copy link
Member

@rhatdan first step is to put a space before all the dot-dot-dots:

-| .GraphDriver...      | Structure for the graph driver info                |
+| .GraphDriver ...     | Structure for the graph driver info                |

Next step is to document all the missing fields, basically, keep running hack/xref-helpmsgs-manpages until it exits zero. Like, add .HealthCheck ... to image-inspect, change .NameHistory to .NamesHistory (plural), and about 80 more.

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.

This is great work, thank you! LGTM with a few questions and suggestions

| .RepoDigests | Repository digests for the image |
| .RepoTags | Repository tags for the image |
| .RootFS ... | Structure for the root file system info |
| .Size | Size of image, in bytes [1] |
Copy link
Member

Choose a reason for hiding this comment

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

What does the [1] mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Cut and paste from somewhere else.

| .Size | Size of image, in bytes [1] |
| .User | Default user to execute the image as |
| .Version | Image Version |
| .VirtualSize | Virtual size of image, in bytes [1] |
Copy link
Member

Choose a reason for hiding this comment

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

Ibid

| .Comment | Image comment |
| .Config ... | Structure with config info |
| .Created | Image creation time (string, ISO3601) |
| .Digest | Image digest (sha256:+64-char hash) |
Copy link
Member

Choose a reason for hiding this comment

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

There's a tab character in here.

I don't know if it's something in my emacs setup, or if it's automatic in emacs, but when I have my cursor inside MD tables and press the TAB key, emacs instantly aligns all the vertical bars. Really sweet. HTH.

Copy link
Member Author

Choose a reason for hiding this comment

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

Must be something in your emacs, would love to have that.

Copy link
Member

Choose a reason for hiding this comment

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

Must be this markdown-mode that I have installed (unfortunately, not in Fedora repos).

| .Reclaimable | Human reclaimable size of each Type |
| .Size | Human size of each type |
| .Total | Total items for each type |
| .Type | Type of data |
Copy link
Member

Choose a reason for hiding this comment

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

"Human size" reads weirdly (to me). I like the parenthesized description you use elsewhere, would it make sense to write this table in that form?

.RawReclaimable | Reclaimable size (bytes)
.Reclaimable | Reclaimable size (human-readable)
...

| .Mountpoint | Source of volume mount point |
| .Name | Volume name |
| .NeedsChown | Indicates whether volume needs to be chowned |
| .NeedsCopyUp | Indicates if volume needs to be copied up to |
Copy link
Member

Choose a reason for hiding this comment

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

Dangling "to"?

| .Anonymous | Indicates whether volume is anonymous |
| .CreatedAt | Volume creation time |
| .Driver | Volume driver |
| .GID | GID to create volume with |
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "GID of volume"? "to create with" seems more applicable to volume create than ls.

| .Status | Status of the volume |
| .StorageID | StorageID of the volume |
| .Timeout | Timeout of the volume |
| .UID | UID to create volume with |
Copy link
Member

Choose a reason for hiding this comment

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

Ibid

@rhatdan
Copy link
Member Author

rhatdan commented Mar 14, 2023

@containers/podman-maintainers PTAL
@Luap99 PTAL

@edsantiago
Copy link
Member

LGTM

| .Name | Name of the machine |
| .Resources ... | Resources used by the machine |
| .SSHConfig ... | SSH configuration info for communitating with machine |
| .State ... | Machine state |
Copy link
Member

Choose a reason for hiding this comment

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

Possible extra space here?

Copy link
Member

Choose a reason for hiding this comment

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

Before the first '|'

Valid placeholders for the Go template are listed below:

| **Placeholder** | **Description** |
| ------------------- | ----------------------------------- |
Copy link
Member

Choose a reason for hiding this comment

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

hmm, I thought this divider would have to line up with the below, but it seems to work OK as is. It would be nice to keep them consistent though.

| .Status | Status of the volume |
| .StorageID | StorageID of the volume |
| .Timeout | Timeout of the volume |
| .UID | UID to create volume with |
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 you have the wrong tense? This command doesn't create the uid, it just shows what it is right?

Suggested change
| .UID | UID to create volume with |
| .UID | UID the volume was created with |

Copy link
Member

Choose a reason for hiding this comment

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

and below you use UID of volume, that might be better.

{
"Name": "myvol",
"Driver": "local",
"Mountpoint": "/home/dwalsh/.local/share/containers/storage/volumes/myvol/_data",
Copy link
Member

Choose a reason for hiding this comment

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

do you want to include /home/dwalsh here? I'd suggest something like /home/myusername

Copy link
Member

Choose a reason for hiding this comment

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

Ditto throughout

{
"Name": "myvol",
"Driver": "local",
"Mountpoint": "/home/dwalsh/.local/share/containers/storage/volumes/myvol/_data",
Copy link
Member

Choose a reason for hiding this comment

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

ditto dwalsh

@rhatdan rhatdan force-pushed the man branch 3 times, most recently from 0d3e385 to 2004317 Compare March 15, 2023 16:08
Valid placeholders for the Go template are listed below:

| **Placeholder** | **Description** |
| ------------------- | ------===================================----------------------------- |
Copy link
Member

Choose a reason for hiding this comment

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

EEK! Those equal signs make a complete mess of the table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dan, don't do stuff while running out to lunch.

@rhatdan rhatdan force-pushed the man branch 2 times, most recently from 18efb3f to f3abb00 Compare March 15, 2023 17:25
Valid placeholders for the Go template are listed below:

| **Placeholder** | **Description** |
| =====------------------- | ----------------------------------------------------------------- |
Copy link
Member

Choose a reason for hiding this comment

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

aaaaaannd, you fixed one, but introduced equals in another.

Ninety-nine little bugs in the code
Ninety-nine little bugs
Fix one bug
commit & re-push
One hundred little bugs in the code

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 wonder if Emacs is doing something strange here... Or just PEBCAK

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.

Forcing a block to make sure nobody inadvertently merges this with the broken equals.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, 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

@edsantiago
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2023
@openshift-merge-robot openshift-merge-robot merged commit 345aa34 into containers:main Mar 15, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants