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.

Sign up for GitHub

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

Jump to bottom

add support to ; for comments in unit files as per systemd documentation #24987

Merged
merged 1 commit into from
Jan 13, 2025
Merged

add support to ; for comments in unit files as per systemd documentation #24987

merged 1 commit into from
Jan 13, 2025
+31 −1

Conversation

AhmedMoalla
Copy link
Contributor

@AhmedMoalla AhmedMoalla commented Jan 9, 2025 •
edited
Loading

Currently the unit file parser supports # and : characters to define comments. But, the documentation states that the characters used for comments are # and ; Systemd Documentation

This PR aims to add support to ; for comments in unit files as per systemd documentation. without breaking changes by leaving : as a supported character for commenting.

The character : should eventually not be supported anymore and does not cause problems at the moment because systemd ignores lines with wrong syntax.

An example of a systemd service:

[Unit]
Description=Hello World Service
After=systemd-user-sessions.service
# A comment
; another comment
: a bad comment

[Service]
Type=simple
ExecStart=/home/ahmed/hello-world.sh

By checking logs with journalctl -u hello-world -e I get this in the logs as a warning but the service runs correctly:

systemd[1]: /etc/systemd/system/hello-world.service:6: Missing '=', ignoring line.

Does this PR introduce a user-facing change?

Action required: Removed support for `:` as a character to comment a line in Quadlet unit files. Users using `:` should now use `;` or `#` instead to comment.

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jan 9, 2025
@AhmedMoalla AhmedMoalla force-pushed the fix-comment-char-in-quadlet-unit-file branch 2 times, most recently from f00d9c7 to 2528334 Compare January 10, 2025 00:45
@rhatdan
Copy link
Member

rhatdan commented Jan 10, 2025

LGTM
Thanks @AhmedMoalla
/approve

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jan 10, 2025
Luap99
Luap99 reviewed Jan 10, 2025
View reviewed changes
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.

It might be good to add one full e2e test (see test/e2e/quadlet) for such comment as well to ensure it actually works with the real binary.

pkg/systemd/parser/unitfile.go Outdated
@@ -204,7 +204,7 @@ func (f *UnitFile) Dup() *UnitFile {
}

func lineIsComment(line string) bool {
return len(line) == 0 || line[0] == '#' || line[0] == ':'
return len(line) == 0 || line[0] == '#' || line[0] == ':' || line[0] == ';'
Copy link
Member

Choose a reason for hiding this comment

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

was the colon char here maybe intended to be ; originally. Why would we allow : as comment line?Systemd does not seem to documented that value.

cc @ygalblum

Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually @alexlarsson's code. It could be just a typo. All the tests use # for comments. So, I can't know for sure if we meant to support :.

Copy link
Member

Choose a reason for hiding this comment

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

: is not a valid comment in systemd (per documentation). @AhmedMoalla could you please drop the line[0] == ':' case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will do. I initially did not do it to prevent it from breaking quadlet files for users using : to comment. But removing it is the right thing to do.

@AhmedMoalla
Copy link
Contributor Author

It might be good to add one full e2e test (see test/e2e/quadlet) for such comment as well to ensure it actually works with the real binary.

Will look into that

@AhmedMoalla AhmedMoalla force-pushed the fix-comment-char-in-quadlet-unit-file branch from 2528334 to b61d406 Compare January 11, 2025 00:24
@AhmedMoalla AhmedMoalla force-pushed the fix-comment-char-in-quadlet-unit-file branch from b61d406 to 75b4a1b Compare January 13, 2025 13:20
ygalblum
ygalblum approved these changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@ygalblum ygalblum 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
Copy link
Contributor

openshift-ci bot commented Jan 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AhmedMoalla, rhatdan, 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:

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

rhatdan commented Jan 13, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 9f1fee2 into containers:main Jan 13, 2025
80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luap99 Luap99 Luap99 left review comments

@giuseppe giuseppe giuseppe left review comments

@ygalblum ygalblum ygalblum approved these changes

Assignees

@rhatdan rhatdan

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. release-note-action-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AhmedMoalla @rhatdan @giuseppe @ygalblum @Luap99