-
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
generate systemd: handle --restart #11459
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
@Luap99 PTAL |
Added a bunch of new tests on top. |
LGTM |
if err := validateRestartPolicy(info.RestartPolicy); err != nil { | ||
return "", err | ||
if options.RestartPolicy != nil { | ||
if err := validateRestartPolicy(*options.RestartPolicy); err != nil { |
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 could warn and reset to the default restart policy here, not sure if this makes sense
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.
I think we should return an error if the user specified something wrong. Imagine it being used in a script or some kind of automation. Logs are usually only consulted when something went wrong, so we'd be hiding that.
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.
Yes returning an error is better, especially since warnings are not displayed on the remote client.
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
Handle custom restart policies of containers when generating the unit files; those should be set on the unit level and removed from ExecStart flags. Fixes: containers#11438 Signed-off-by: Valentin Rothberg <[email protected]>
ready to merge |
/lgtm |
Handle custom restart policies of containers when generating the unit
files; those should be set on the unit level and removed from ExecStart
flags.
Fixes: #11438
Signed-off-by: Valentin Rothberg [email protected]