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

Refactor common man page options, phase 2 #15250

Merged

Conversation

edsantiago
Copy link
Member

Followup to #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 #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]

man page cleanup: consolidate common options

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 edsantiago added the kind/documentation Categorizes issue or PR as related to documentation. label Aug 9, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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 Aug 9, 2022
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.

Changes LGTM

@mheon
Copy link
Member

mheon commented Aug 9, 2022

Tests failing. Looks like Cirrus issues, not flakes? Restarted, regardless.

Changes LGTM
/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2022
@edsantiago
Copy link
Member Author

Thanks for the restart. The flake seems to be yet another access.redhat.com one:

Error: copying system image from manifest list: Source image rejected: parsing signature https://access.redhat.com/.../signature-7: unrecognized signature format, starting with binary 0x3c

@edsantiago
Copy link
Member Author

Tests are green. As with previous iterations of this PR, here is how I verified the diffs:

$ git co main; make clean; make docs; rm -rf docdiff;mkdir docdiff;mv docs/build/man docdiff/main
$ git co docs_dedup_phase2;make clean;make docs;diff -ur docdiff/main docs/build/man | cdif | less

(that is: compare the built man directory on main against the one built here. Differences are what I expect. I also greped the output for Only, as in Only in one-of-the-directories, which would indicate that a man page didn't build. No matches.)

@edsantiago
Copy link
Member Author

(I don't want to release the hold. This is an enormous change and I would greatly appreciate many more reviewers).

@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2022

Did you save the MD files before this PR and then add this PR and regenerate the MD files and then compare them?

@edsantiago
Copy link
Member Author

edsantiago commented Aug 9, 2022

I did, by a somewhat more complicated set of rsync and for-loop and diff and grep -v. Results are much harder for the human eye to parse, due to the extra whitespace added by the [//]: # (etc) markdown comments, but here it is for those interested:

$ git checkout main; make docs; mkdir  -p docdiff/md; rsync -av docs/source/markdown/*.md docdiff/md/
$ git checkout docs_dedup_phase2; make clean; make docs; for i in docs/source/markdown/*.in;do f=${i%.in}; cdif -ub docdiff/md/$(basename $f) <(grep -v 'included file' $f);done |less

@rhatdan
Copy link
Member

rhatdan commented Aug 9, 2022

/hold cancel
LGTM

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2022
@openshift-merge-robot openshift-merge-robot merged commit 6d887bd into containers:main Aug 9, 2022
@edsantiago edsantiago deleted the docs_dedup_phase2 branch August 9, 2022 19:29
@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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 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. kind/documentation Categorizes issue or PR as related to documentation. 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.

5 participants