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

quadlet: Support systemd style dropin files #20828

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

alexlarsson
Copy link
Contributor

For a source file like foo.container, look for drop in named foo.container.d/*.conf and merged them into the main file. The dropins are applied in alphabetical order, and files in earlier diretories override later files with same name.

This is similar to how systemd dropins work, see:
https://www.freedesktop.org/software/systemd/man/latest/systemd.unit.html

Also adds some tests for these

This seems useful for things like:
containers/qm#284
It also allows a non-auto-enabled quadlet, i.e. without an install section, to easily add one using a dropin.

Does this PR introduce a user-facing change?

 * Quadlet now support systemd-style dropins

@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2023

Please update the man page.

For a source file like `foo.container`, look for drop in named
`foo.container.d/*.conf` and merged them into the main file.  The
dropins are applied in alphabetical order, and files in earlier
diretories override later files with same name.

This is similar to how systemd dropins work, see:
https://www.freedesktop.org/software/systemd/man/latest/systemd.unit.html

Also adds some tests for these

Signed-off-by: Alexander Larsson <[email protected]>
@alexlarsson
Copy link
Contributor Author

@rhatdan Added

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.

Lovely idea, LGTM

Copy link
Contributor

openshift-ci bot commented Nov 29, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexlarsson, vrothberg

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 Nov 29, 2023
@vrothberg
Copy link
Member

@ygalblum PTAL

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Could you add more tests, i.e. test for [Service] and [Unit] sections as well as the other quadlet specific sections.

@ygalblum
Copy link
Contributor

Great addition and the code LGTM. But, I do have some comments/questions about the doc and tests.

I think I understand what happened in the override tests - the empty Environment= line in 20-second.conf caused any previously set value to be removed. But I wouldn't have understood it from the man page. I don't know, is this the way this is done also in systemd drop-ins?

Anyhow, I think the man page should elaborate some more on how the drop-in should look like.

Also, I think the tests should include different types of keys. The Environment key is a a key=value list that can appear multiple times. But, there are other types of keys. For example, does the code work correctly for the Image key?

@alexlarsson
Copy link
Contributor Author

I think I understand what happened in the override tests - the empty Environment= line in 20-second.conf caused any previously set value to be removed. But I wouldn't have understood it from the man page. I don't know, is this the way this is done also in systemd drop-ins?

This is just in general how the systemd syntax works for list-kind of keys. It also works inside a single file. I'm not sure we have a specific doc for it in the quadlet manpage, but anyone used to systemd will know about this, and I believe we link to the systemd docs for this in the quadlet manpage.

I'll add some more tests.

@dougsland
Copy link
Contributor

LGTM not sure what's wrong with CI but seems not specific with this patch.

@rhatdan
Copy link
Member

rhatdan commented Nov 29, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 29, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 572a769 into containers:main Nov 29, 2023
93 checks passed
@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 Feb 28, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
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.

6 participants