-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Honor users requests in quadlet files #24334
Conversation
@ygalblum PTAL |
pkg/systemd/quadlet/quadlet.go
Outdated
// The default syslog identifier is the exec basename (podman) which isn't very useful here | ||
if _, ok := service.Lookup(ServiceGroup, "SyslogIdentifier"); !ok { | ||
service.Set(ServiceGroup, "SyslogIdentifier", "%N") | ||
} | ||
if _, ok := service.Lookup(ServiceGroup, "Type"); !ok { | ||
service.Set(ServiceGroup, "Type", "oneshot") | ||
} | ||
if _, ok := service.Lookup(ServiceGroup, "RemainAfterExit"); !ok { | ||
service.Set(ServiceGroup, "RemainAfterExit", "yes") | ||
} |
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.
this is duplicated three times, so it should really be in its own function an reused
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.
Small nits
pkg/systemd/quadlet/quadlet.go
Outdated
// which isn't very useful here | ||
"SyslogIdentifier", "%N") | ||
|
||
defaultServiceGroup(service, false) |
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.
We should indicate in the release comment that the default behaviour for .build
files has changed
test/e2e/quadlet/ipv6.network
Outdated
@@ -1,5 +1,13 @@ | |||
## assert-podman-final-args systemd-ipv6 | |||
## assert-podman-args "--ipv6" | |||
## assert-key-is Service Type exec | |||
## assert-key-is Service RemainAfterExit yes |
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.
Is this test passing? You set the value to no
and assert for yes
This PR has been marked for 5.3 inclusion but it must be merged prior to Nov 5 for inclusion in 5.3 RC3. PRs not merged by that date are considered on a case by case basis for backporting. |
Fixes: containers#24322 Signed-off-by: Daniel J Walsh <[email protected]>
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, rhatdan 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 |
containers/podman#24334 Quadlet build units no longer use RemainAfterExit=yes by default.
containers/podman#24334 Quadlet build units no longer use RemainAfterExit=yes by default.
Change dir variable name Co-authored-by: Felix Niederwanger <[email protected]> Don't parse JSON Exclude pod validation for SLE<16 Enable for SLE Allow only for tumbleweed and SLE 16+ Revert "Exclude pod validation for SLE<16" This reverts commit d8e3064. Remove unneeded JSON import Add image pull phase and increase image build timeout Improve run conditions and increase image pull delay Improve file population and cleanup daemon-reload Adjust for upstream Quadlet update containers/podman#24334 Quadlet build units no longer use RemainAfterExit=yes by default.
Fixes: #24322
Does this PR introduce a user-facing change?