Quadlet: ensure all keys are documented #21233

edsantiago · 2024-01-11T17:08:06Z

New CI validation check: all keys in quadlet.go must be
documented at least once in podman-systemd.unit.5.md.

And, because the md file lists keys in both table and block
form, make sure those all match.

And make sure everything is sorted in lexical order.

You'd think that by now I would know enough to add
[NO NEW TESTS NEEDED]

Signed-off-by: Ed Santiago [email protected]

None

edsantiago

Blocked pending resolution of many questions.

edsantiago · 2024-01-11T17:09:11Z

docs/source/markdown/podman-systemd.unit.5.md

+| RemapGid=FIXME                       | FIXME                                                |
+| RemapUid=FIXME                       | FIXME                                                |
+| RemapUidSize=FIXME                   | FIXME                                                |
+| RemapUsers=FIXME                     | FIXME                                                |


Someone please tell me how to fill these in, and if they're in the right unit section

These keys were deprecated and therefore remove from the documentation. They are still supported in the code to maintain backward compatibility.

edsantiago · 2024-01-11T17:10:24Z

hack/xref-quadlet-docs

+            my ($constname, $keystring) = ($1, $2);
+
+            # const name *should* be the same as the string, except (sigh)
+            # there's an inexplicable block of KeyNetworkFoo = "Foo".


"inexplicable" to me. If there's a great reason for it, please lmk so I can update the comment.

When I added these keys the idea was to indicate that they relate the .network files (and hence used in ConvertNetwork). I don't see any harm in removing Network if you want

edsantiago · 2024-01-11T17:11:07Z

hack/xref-quadlet-docs

+                # There's also, sigh again, some preexisting mismatches
+                unless ($constname =~ /Remap[GU]ID/) {


Barf. Since these are all undocumented, would it be possible to actually fix them?

As I explained, these keys were deprected. I see that the code looks for deprecated. So, I guess the solution is to add this comment to these lines

edsantiago · 2024-01-11T17:11:49Z

hack/xref-quadlet-docs

+                # sigh: "Driver" and "Options" also exist in KeyNetworkFoo space
+                warn "$ME: $path:$.: duplicate key string \"$keystring\"\n"
+                    unless $keystring =~ /^(Driver|Options)$/;


Barf again. Just checking: is it intentional to have multiple Driver and Options definitions?

I think that is something that should be fixed in the code.
I guess this is due the fact that we have Driver/Options for Volume and Network units for example. So it would need to be documented for each unit type. But yeah there is really no reason to define the same string const twice in the code unless we like to bloat the binary size by some bytes.

As I wrote, the Network part can be removed (and then also remove the duplications)

Luap99 · 2024-01-11T17:17:06Z

docs/source/markdown/podman-systemd.unit.5.md

+### `VolatileTmp=`
+
+FIXME


I would need to look through the git log but if I remember correctly this key is deprecated and thus not documented.
I like the idea of this but I think we need to have a way to ignore some keys. Maybe we could add // deprecated as comment in the code behind the key name and you script could see it and not force the docs or something like this.

Thank you! #20479. I had missed that. And I like the idea of // deprecated.

edsantiago · 2024-01-15T15:58:46Z

Thank you @ygalblum. I've removed the Network prefix, fixed the case mismatch in RemapXID, and removed those special-case exceptions from the script. For ease of review, I've pushed this as two commits.

ygalblum · 2024-01-15T17:17:51Z

docs/source/markdown/podman-systemd.unit.5.md

@@ -835,6 +836,7 @@ Valid options for `[Kube]` are listed below:
 | AutoUpdate=registry                 | --annotation "io.containers.autoupdate=registry"                 |
 | ConfigMap=/tmp/config.map           | --config-map /tmp/config.map                                     |
 | ContainersConfModule=/etc/nvd\.conf | --module=/etc/nvd\.conf                                          |
+| ExitCodePropagation=how             | How to propagate container error status              |


Please align | to the end of the table

ARGH! Thank you for catching that. Fixed, but I will not push until I see how CI does.

ygalblum

LGTM

openshift-ci · 2024-01-16T05:57:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, ygalblum

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:

~~OWNERS~~ [edsantiago,ygalblum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

- A number of keys had a "Network" prefix, for historical reasons that never panned out ("KeyNetworkGateway"). Remove that prefix and remove the two duplicates. - Three RemapXXX keys were mismatched in case ("UID" vs "Uid"). Make those consistent. Signed-off-by: Ed Santiago <[email protected]>

New CI validation check: all keys in quadlet.go must be documented at least once in podman-systemd.unit.5.md. Adding '// deprecated' next to an enum definition will exclude said key from the documentation cross-checks. And, because the md file lists keys in both table and block form, make sure those all match. And make sure everything is sorted in lexical order, in both .go source and in man page. And add a validation check to make sure it stays that way. Signed-off-by: Ed Santiago <[email protected]>

edsantiago · 2024-01-18T17:42:18Z

rebased out of paranoia. This is ready to go.

rhatdan · 2024-01-18T18:41:08Z

/lgtm

edsantiago requested a review from ygalblum January 11, 2024 17:08

openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 11, 2024

edsantiago commented Jan 11, 2024

View reviewed changes

Luap99 reviewed Jan 11, 2024

View reviewed changes

edsantiago force-pushed the quadlet-docs-xref branch 3 times, most recently from 9a43ec7 to 11cde41 Compare January 15, 2024 15:56

edsantiago force-pushed the quadlet-docs-xref branch from 11cde41 to 2db11fa Compare January 15, 2024 16:32

ygalblum reviewed Jan 15, 2024

View reviewed changes

edsantiago force-pushed the quadlet-docs-xref branch from 2db11fa to 571bded Compare January 15, 2024 17:57

edsantiago changed the title ~~[DO NOT MERGE] Quadlet: ensure all keys are documented~~ Quadlet: ensure all keys are documented Jan 15, 2024

ygalblum approved these changes Jan 16, 2024

View reviewed changes

edsantiago added 2 commits January 18, 2024 06:17

edsantiago force-pushed the quadlet-docs-xref branch from 571bded to a5612bc Compare January 18, 2024 13:18

openshift-ci bot assigned rhatdan Jan 18, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024

openshift-merge-bot bot merged commit ce42c9f into containers:main Jan 18, 2024
91 of 92 checks passed

edsantiago deleted the quadlet-docs-xref branch January 18, 2024 20:57

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 Apr 18, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quadlet: ensure all keys are documented #21233

Quadlet: ensure all keys are documented #21233

edsantiago commented Jan 11, 2024 •

edited

Loading

edsantiago left a comment

edsantiago Jan 11, 2024

ygalblum Jan 14, 2024

edsantiago Jan 11, 2024

ygalblum Jan 14, 2024

edsantiago Jan 11, 2024

ygalblum Jan 14, 2024

edsantiago Jan 11, 2024

Luap99 Jan 11, 2024

ygalblum Jan 14, 2024

Luap99 Jan 11, 2024

edsantiago Jan 11, 2024

edsantiago commented Jan 15, 2024

ygalblum Jan 15, 2024

edsantiago Jan 15, 2024

ygalblum left a comment

openshift-ci bot commented Jan 16, 2024

edsantiago commented Jan 18, 2024

rhatdan commented Jan 18, 2024

		# There's also, sigh again, some preexisting mismatches
		unless ($constname =~ /Remap[GU]ID/) {

Quadlet: ensure all keys are documented #21233

Quadlet: ensure all keys are documented #21233

Conversation

edsantiago commented Jan 11, 2024 • edited Loading

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygalblum left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 16, 2024

edsantiago commented Jan 18, 2024

rhatdan commented Jan 18, 2024

edsantiago commented Jan 11, 2024 •

edited

Loading