-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Rewrite the Quadlet documentation. #26929
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jankaluza The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
8558e91
to
6612832
Compare
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
c55dc16
to
534eb3f
Compare
This commit does the following: - Splits the podman-systemd.unit.5.md into multiple files - one for each quadlet file type, podman-quadlet.7.md for general quadlet information and podman-quadlet-basic-usage.7.md for quadlet examples. - Removes the original podman-systemd.unit.5.md file. - Adds support for jinja2 templating language in the markdown_preprocess. - Uses jinja2 in options/*.md to use the single .md file for both podman subcommands man-pages and quadlet man-pages. This deduplicates the Quadlet man-pages a lot. - Adds new `@@option quadlet:source.md` preprocess command to import such .md files from options directory. Signed-off-by: Jan Kaluza <[email protected]>
534eb3f
to
e7916da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a proper review just some drive by comments
hack/markdown-preprocess
Outdated
@@ -11,6 +11,7 @@ import glob | |||
import os | |||
import re | |||
import sys | |||
from jinja2 import Template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jinja2 is not in the std lib, which means we somehow much install it it everywhere the preprocessor runs (including the redthedocs build process)
Line 32 in 07bb670
path = os.path.join(os.path.abspath(os.path.dirname( |
While I really like the de duplication I think maybe it would be better if we can avoid jinja2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I was thinking about extending the existing parser in some way, but to be honest doing my own if-logic in python seems like reinventing the wheel. I was also playing with some other syntax ideas, but they looked like one big hack in the end.
I will check it back on Monday when fresh to see if I get better idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I agree the current way is a ugly and horrible. having a proper template engine might be better.
But I do really wonder if hand rolling our own thing even when using a custom template is the right thing? I think there must be better doc generation tools existing already somewhere that could be used instead. I know it would be a lot of work and maybe outside of the scope for this but maybe it would be better to write our docs in rst where we can have proper includes instead of this markdown hacking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do not remove this, there are endless of people linking that page or are used to typing man podman-systemd.unit that we break with this.
IMO the podman-quadlet content should be in this file instead and we should not have a second podman-quadlet man page which is confusing when there is both a .1 and .7 file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. That's good point. I will update it.
- **`.container`** — Defines and manages a single container. See **podman-container.unit(5)**. | ||
- **`.pod`** — Creates a Podman pod that containers can join. See **podman-pod.unit(5)**. | ||
- **`.volume`** — Ensures a named Podman volume exists. See **podman-volume.unit(5)**. | ||
- **`.network`** — Creates a Podman network for containers and pods. See **podman-network.unit(5)**. | ||
- **`.image`** — Pulls and caches a container image. See **podman-image.unit(5)**. | ||
- **`.build`** — Builds a container image from a Containerfile. See **podman-build.unit(5)**. | ||
- **`.kube`** — Deploys containers from Kubernetes YAML using `podman kube play`. See **podman-kube.unit(5)**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these should all be actual links so users can click on the on the website
## Quadlet section [Quadlet] | ||
Some quadlet specific configuration is shared between different unit types. Those settings | ||
can be configured in the `[Quadlet]` section. | ||
|
||
Valid options for `[Quadlet]` are listed below: | ||
|
||
| **[Quadlet] options** | **Description** | | ||
|----------------------------|---------------------------------------------------| | ||
| DefaultDependencies=false | Disable implicit network dependencies to the unit | | ||
|
||
### `DefaultDependencies=` | ||
|
||
Add Quadlet's default network dependencies to the unit (default is `true`). | ||
|
||
When set to false, Quadlet will **not** add a dependency (After=, Wants=) to | ||
`network-online.target`/`podman-user-wait-network-online.service` to the generated unit. | ||
|
||
Note, this option is set in the `[Quadlet]` section. The _systemd_ `[Unit]` section | ||
has an option with the same name but a different meaning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a global section supported by all file types so it should be documented in podman-systemd.unit
# FILE LOCATIONS | ||
|
||
Place `.build` files in one of the following: | ||
|
||
### Rootless | ||
|
||
- `$XDG_RUNTIME_DIR/containers/systemd/` | ||
- `$XDG_CONFIG_HOME/containers/systemd/` or `~/.config/containers/systemd/` | ||
- `/etc/containers/systemd/users/$(UID)` | ||
- `/etc/containers/systemd/users/` | ||
|
||
### Rootful | ||
|
||
- `/run/containers/systemd/` | ||
- `/etc/containers/systemd/` | ||
- `/usr/share/containers/systemd/` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is duplication you have in any file, IMO it is enough to document this only once in the main file
|
||
``` | ||
[Image] | ||
Image=quay.io/centos/centos:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
he example here made me think ... earlier you mentioned that people should use fully qualified names for performance. I would argue it is for security as much as anything. And that said, upon seeing this, I wondered if we should also be recommending, generally, that people use a tagged image instead of latest
to avoid things changing underneath them? Just a thought (oh, an apologies if i missed it or its somewhere where i havent reviewed yet)
# SEE ALSO | ||
|
||
[systemd.unit(5)](https://www.freedesktop.org/software/systemd/man/systemd.unit.html), | ||
[podman-kube-play(1)](https://docs.podman.io/en/latest/markdown/podman-kube-play.1.html), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should kube-generate be in here?
|
||
Run the container in a new user namespace using the supplied GID mapping. This | ||
option conflicts with the **--userns** and **--subgidname** options. This | ||
un the container in a new user namespace using the supplied GID mapping. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo:
un the container -> Run the container
|
||
Run the container in a new user namespace using the supplied GID mapping. This | ||
option conflicts with the **--userns** and **--subgidname** options. This | ||
un the container in a new user namespace using the supplied GID mapping. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo:
un the container -> Run the container
PR needs rebase. 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-sigs/prow repository. |
Signed-off-by: Jan Kaluza <[email protected]>
This commit does the following:
@@option quadlet:source.md
preprocess command to import such .md files from options directory.Does this PR introduce a user-facing change?