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

podman-run(1): fixes #5192

Merged
merged 1 commit into from
Feb 14, 2020
Merged

Conversation

kolyshkin
Copy link
Contributor

I saw some bad formatting when reading "man podman-run" and
proceeded to fix it. I have now opened a can of worms...

This commit tries to fix some of the formatting, wording and
other bugs I came across (unfortunately not all of them).

Can't list every fix that I made here, but in general:

  • format lists as such (prepend items with "- ");
  • format examples as such (enclose in ```...```);
  • format literal values (option names, literal values) as bold;
  • format man page references as page(1).
  • format replacements (option values) and file names as italic;
  • remove some duplicate info (such as what's the default value);
  • move option value description to option syntax;
  • end sentences with a period.

To test:

$ make docs
$ man ./docs/build/man/podman-run.1 ### check terminal formatting
$ man -Tps ./docs/build/man/podman-run.1 > podman-run.ps
$ ps2pdf podman-run.ps ### optional
$ evince podman-run.pdf ### check printer formatting (or use ps viewr

NOTE

  • there is much more to do here;
  • I haven't checked any factual contents, this is about formatting

Signed-off-by: Kir Kolyshkin [email protected]

@kolyshkin kolyshkin changed the title podman(1): fixes podman-run(1): fixes Feb 12, 2020
@openshift-ci-robot
Copy link
Collaborator

Hi @kolyshkin. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L labels Feb 12, 2020
@mheon
Copy link
Member

mheon commented Feb 12, 2020

I feel like some of this conflicts with the formatting that @rpjday was using elsewhere in his manpage cleanups, we should try and iron that out to make things consistent before merging

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Feb 13, 2020

I feel like some of this conflicts with the formatting that @rpjday was using elsewhere in his manpage cleanups

@mheon can you please provide a link to the PR you refer to, so I can rebase?

I can only find #4976 and there are no conflicts.

@rhatdan
Copy link
Member

rhatdan commented Feb 13, 2020

Did you look at the man page in markdown as well? Does it look good there?

@giuseppe
Copy link
Member

thanks for the patch! Does create.md also need these changes?

@kolyshkin
Copy link
Contributor Author

Did you look at the man page in markdown as well? Does it look good there?

I am checking all three:

  • in-terminal man
  • for-printer man (i.e. PostScript)
  • and markdown (as rendered by github).

The last one can be seen from github -- when you look at the patch, click on context menu and choose "View file").

thanks for the patch! Does create.md also need these changes?

most probably yes, but let's do one step at a time.

@kolyshkin
Copy link
Contributor Author

I am checking all three

having said that, I'd appreciate another pair of eyes.

@rhatdan
Copy link
Member

rhatdan commented Feb 13, 2020

@kolyshkin Thanks.
Looks very nice, you have some trailing white space that is causing the gating issue on testing. Clean this up and we can merge.
LGTM

@rpjday
Copy link
Contributor

rpjday commented Feb 13, 2020

Once this is merged, I can resume contributing changes as well, I'll just work on top of this stuff.

@rhatdan
Copy link
Member

rhatdan commented Feb 13, 2020

Excellent.

@mheon
Copy link
Member

mheon commented Feb 13, 2020

LGTM once the whitespace issue is resolved


Limit the container's Real Time CPU usage. This flag tell the kernel to restrict the container's Real Time CPU usage to the period you specify.

**--cpu-rt-runtime**=*microseconds*

Limit the CPU real-time runtime in microseconds
Limit the CPU real-time runtime in microseconds.
Copy link
Member

Choose a reason for hiding this comment

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

Woot! 👍

**/etc/subgid**

## SEE ALSO
subgid(5), subuid(5), libpod.conf(5), systemd.unit(5), setsebool(8), slirp4netns(1), fuse-overlayfs(1)
**subgid**(5), **subuid**(5), **libpod.conf**(5), **systemd.unit**(5), **setsebool**(8), **slirp4netns**(1), **fuse-overlayfs**(1).

## HISTORY
September 2018, updated by Kunal Kushwaha <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

With the changes you've made, you really should take credit and add yourself as an updater here though. Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact I was thinking about the opposite. Maybe it makes sense to remove (or, rather, comment out ) this section entirely. Not to offend all the people who have contributed, but listing these names and dates does not really add any value to a reader (except maybe the bit that says docs were initially adopted from Docker).

But that is a separate discussion.

Copy link
Member

Choose a reason for hiding this comment

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

I smell a thumb wrestling match when we meet!

@TomSweeneyRedHat
Copy link
Member

LGTM once the you get past white space hell.
@mheon @dwalsh @baude: Once completed, should we list podman-run(1) as the style guide to emulate for other man pages in the contributing page?

@kolyshkin
Copy link
Contributor Author

Once completed, should we list podman-run(1) as the style guide to emulate for other man pages in the contributing page?

It's still far from being perfect :-\

In fact, I could come up with a tiny style guide of its own (or use some smaller man page for that).

I saw some bad formatting when reading "man podman-run" and
proceeded to fix it. I have now opened a can of worms...

This commit tries to fix some of the formatting, wording and
other bugs I came across (unfortunately not all of them).

Can't list every fix that I made here, but in general:
- format lists as such (prepend items with "- ");
- format examples as such (enclose in ```...```);
- format literal values (option names, literal values) as **bold**;
- format man page references as **page**(1).
- format replacements (option values) and file names as _italic_;
- remove some duplicate info (such as what's the default value);
- move option value description to option syntax;
- end sentences with a period.

To test:
```console
$ make docs
$ man ./docs/build/man/podman-run.1 ### check terminal formatting
$ man -Tps ./docs/build/man/podman-run.1 > podman-run.ps
$ ps2pdf podman-run.ps ### optional
$ evince podman-run.pdf ### check printer formatting (or use ps viewr
```

NOTE
 - there is much more to do here;
 - I haven't checked any factual contents, this is about formatting

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin
Copy link
Contributor Author

you have some trailing white space

And here I thought my trusted vim won't do that to me...

$ grep -En '[[:space:]]$' docs/source/markdown/podman-run.1.md
83:Set the cgroup namespace mode for the container. 

Fixed.

@rhatdan
Copy link
Member

rhatdan commented Feb 14, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2020
@TomSweeneyRedHat
Copy link
Member

LGTM
A style guide for the man pages would be a most welcomed addition.

@mheon
Copy link
Member

mheon commented Feb 14, 2020

/ok-to-test
/approve
/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 14, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin, mheon

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2020
@mheon
Copy link
Member

mheon commented Feb 14, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0e64493 into containers:master Feb 14, 2020
edsantiago added a commit to edsantiago/libpod that referenced this pull request Aug 9, 2022
Followup to containers#15174. These are the options that are easy(ish)
to review: those that have only drifted slightly, and need
only minor tweaks to bring back to sanity. For the most part,
I went with the text in podman-run because that was cleaned up
in containers#5192 way back in 2020. These diffs primarily consist of
using '**' (star star) instead of backticks, plus other
formatting and punctuation changes.

This PR also adds a README in the options dir, and a new
convention: <<container text...|pod text...>> which tries
to do the right thing based on whether the man page name
includes "-pod-" or not. Since that's kind of hairy code,
I've also added a test suite for it.

Finally, since this is impossible to review by normal means,
I'm temporarily committing hack/markdown-preprocess-review,
a script that will diff option-by-option. I will remove it
once we finish this cleanup, but be advised that there are
still 130+ options left to examine, and some of those are
going to be really hard to reunite.

Review script usage: simply run it (you need to have 'diffuse'
installed). It isn't exactly obvious, but it shouldn't take more
than a minute to figure out. The rightmost column (zzz-chosen.md)
is the "winner", the actual content that will be used henceforth.
You really want an ultrawide screen here.

Signed-off-by: Ed Santiago <[email protected]>
edsantiago added a commit to edsantiago/libpod that referenced this pull request Aug 30, 2022
Whew! This one started off identical everywhere, but the version
in podman-run got fixed in containers#1380, then again in containers#5192, with no
corresponding fixes to any of the other man pages.

I went with the podman-run version, with a small change in wording.

Signed-off-by: Ed Santiago <[email protected]>
edsantiago added a commit to edsantiago/libpod that referenced this pull request Sep 1, 2022
Only for podman-create and -run, unfortunately: all the
others are too different, and can't easily be combined.

I went with the podman-run version because it was most
recently updated in containers#5192.

Signed-off-by: Ed Santiago <[email protected]>
edsantiago added a commit to edsantiago/libpod that referenced this pull request Sep 12, 2022
podman-create and -run only. The SELinux text was added
to podman-run (but not -create) in containers#3631, and reformatted
in containers#5192. I assume here that it also applies to podman-create.

Per feedback from Dan, added :s0 to SELinux context

Signed-off-by: Ed Santiago <[email protected]>
edsantiago added a commit to edsantiago/libpod that referenced this pull request Sep 13, 2022
Only shared between podman-create and run. The latter was
updated in containers#5192, and that is the text I chose.

Signed-off-by: Ed Santiago <[email protected]>
@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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 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. ok-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants