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] Remove future tense from man pages #18559

Merged
merged 1 commit into from
May 16, 2023

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented May 14, 2023

Remove all will, would, could, should and use present tense.

Does this PR introduce a user-facing change?

Man page cleanup.

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 14, 2023
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.

LGTM

@Luap99
Copy link
Member

Luap99 commented May 15, 2023

@TomSweeneyRedHat PTAL

@@ -117,8 +117,7 @@ The default is **false**.

#### **--latest**, **-l**

Instead of providing the *container ID* or *name*, use the last created *container*. If other methods than Podman are used to run *containers* such as `CRI-O`, the last started *container* could be from either of those methods.\
The default is **false**.\
Instead of providing the *container ID* or *name*, use the last created *container*. The default is **false**.
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove the CRI-O bit? It needs to be rewritten, but I think it still holds.

"If tools other than Podman are used to run a container, such as CRI-O or another container engine, the last started container could be from one of those engines.

Copy link
Member Author

Choose a reason for hiding this comment

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

cri-o does not create containers that --last would refer to. These containers are in the podman database, CRI-O does not use the Podman database.

*IMPORTANT: Conflicts with **--rmi** as the container is not being cleaned up so the image cannot be removed.*

#### **--latest**, **-l**

Instead of providing the *container ID* or *name*, use the last created *container*. If other methods than Podman are used to run *containers* such as `CRI-O`, the last started *container* could be from either of those methods.\
The default is **false**.\
Instead of providing the *container ID* or *name*, use the last created *container*. The default is **false**.
Copy link
Member

Choose a reason for hiding this comment

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

ditto CRI-O removal here.

The default is **false**.

#### **--latest**, **-l**

Instead of providing the *container ID* or *name*, use the last created *container*. If other tools than Podman are used to run *containers* such as `CRI-O`, the last started *container* could be from either tool.\
The default is **false**.\
Instead of providing the *container ID* or *name*, use the last created *container*. The default is **false**.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto CRI-o 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.

Ditto reason. :^)

@@ -17,7 +17,7 @@ Generating unit files for a pod requires the pod to be created with an infra con
- Note: The generated `podman run` command contains an `--sdnotify` option with the value taken from the container.
If the container does not have any explicitly set value or the value is set to __ignore__, the value __conmon__ is used.
The reason for overriding the default value __container__ is that almost no container workloads send notify messages.
Systemd would wait for a ready message that never comes, if the value __container__ is used for a container
Systemd waits for a ready message that never comes, if the value __container__ is used for a container
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 "will wait" is better than "waits" in this context, but it is a coinflip.

Suggested change
Systemd waits for a ready message that never comes, if the value __container__ is used for a container
Systemd will wait for a ready message that never comes if the value __container__ is used for a container

specify additional options via the `--storage-opt` flag.

#### **--storage-opt**=*value*

Specify a storage driver option. Default storage driver options are configured in `containers-storage.conf(5)`. The `STORAGE_OPTS` environment variable overrides the default. The --storage-opt specified options override all. Specify --storage-opt="" so no storage options will be used.
Specify a storage driver option. Default storage driver options are configured in `containers-storage.conf(5)`. The `STORAGE_OPTS` environment variable overrides the default. The --storage-opt specified options override all. Specify --storage-opt="" so no storage options is used.
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
Specify a storage driver option. Default storage driver options are configured in `containers-storage.conf(5)`. The `STORAGE_OPTS` environment variable overrides the default. The --storage-opt specified options override all. Specify --storage-opt="" so no storage options is used.
Specify a storage driver option. Default storage driver options are configured in `containers-storage.conf(5)`. The `STORAGE_OPTS` environment variable overrides the default. The --storage-opt specified options override all. Specify --storage-opt="" so no storage options are used.


**policy.json** (`/etc/containers/policy.json`)

Signature verification policy files are used to specify policy, e.g. trusted keys, applicable when deciding whether to accept an image, or individual signatures of that image, as valid.

**registries.conf** (`/etc/containers/registries.conf`, `$HOME/.config/containers/registries.conf`)

registries.conf is the configuration file which specifies which container registries should be consulted when completing image names which do not include a registry or domain portion.
registries.conf is the configuration file which specifies which container registries is consulted when completing image names which do not include a registry or domain portion.
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
registries.conf is the configuration file which specifies which container registries is consulted when completing image names which do not include a registry or domain portion.
registries.conf is the configuration file which specifies which container registries are consulted when completing image names that do not include a registry or domain portion.

Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

Away nits, away I SAY!

Done picking. The eyeballs were going, going, gone near the end, I'm sure a few slipped by. Please watch for text that's repeated across multiple man pages.

@Luap99
Copy link
Member

Luap99 commented May 16, 2023

@rhatdan Please squash the commits, Apply suggestions from code review isn't helpful to anyone going through the git log.

@rhatdan
Copy link
Member Author

rhatdan commented May 16, 2023

@Luap99 I did.

@lsm5
Copy link
Member

lsm5 commented May 16, 2023

@rhatdan woah was this manual or is there a tense-checker tool like the spellchecker one?

@lsm5
Copy link
Member

lsm5 commented May 16, 2023

LGTM given @TomSweeneyRedHat went through those already 😄

@lsm5
Copy link
Member

lsm5 commented May 16, 2023

@rhatdan woah was this manual or is there a tense-checker tool like the spellchecker one?

if there was a tool used, can we please mention it in the commit message? Otherwise, good to go for me.

Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2023

NOTE: The support for the network name pasta is deprecated and will be removed in the next major
NOTE: The support for the network name "pasta" is deprecated and is removed in the next major
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an actual future tense because it will actually happen in the future.

Copy link
Member

Choose a reason for hiding this comment

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

good catch!

@ashley-cui
Copy link
Member

ashley-cui commented May 16, 2023

Whoops, may have been a little fast, hopefully the hold will come in time?
/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 May 16, 2023
@ashley-cui
Copy link
Member

/lgtm cancel

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 16, 2023
duration in microseconds. Once the container's CPU quota is used up, it will
not be scheduled to run until the current period ends. Defaults to 100000
duration in microseconds. Once the container's CPU quota is used up, it is not
scheduled to run until the current period ends. Defaults to 100000
Copy link
Collaborator

Choose a reason for hiding this comment

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

This too, I believe, even if more nuanced, should be a future tense (once ... it will not).

Remove all will, would, could, should and use present tense.

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented May 16, 2023

"if there was a tool used, can we please mention it in the commit message? Otherwise, good to go for me."

Nope brute force on a Saturday morning with little to do.

@ashley-cui
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, sbrivio-rh

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

@rhatdan
Copy link
Member Author

rhatdan commented May 16, 2023

/hold cancel

@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 May 16, 2023
@openshift-merge-robot openshift-merge-robot merged commit 5e16451 into containers:main May 16, 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 Aug 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 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.

8 participants