podman generate systemd: handle --sdnotify correctly #15056
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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 |
openshift-ci
bot
added
the
approved
|
@vrothberg @eriksjolund PTAL |
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
When a container was created with `--sdnotify value` we would remove this arg instead of using it like with `--sdnotfiy=value`. Also when the arg is set to ignore we should force conmon in order to make the resulting Type=notify units work. Fixes containers#15052 Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
generate-systemd-sdnotify
branch
from
edcc3fa to
6a9338a
Compare
| func removeSdNotifyArg(args []string, argCount int) []string { | ||
| processed := make([]string, 0, len(args)) | ||
| for i := 0; i < len(args)-argCount; i++ { | ||
| s := args[i] | ||
|
|
||
| switch { | ||
| case s == "--sdnotify": | ||
| i++ | ||
| continue | ||
| case strings.HasPrefix(s, "--sdnotify="): | ||
| continue | ||
| } | ||
| processed = append(processed, s) | ||
| } | ||
| processed = append(processed, args[len(args)-argCount:]...) | ||
| return processed | ||
| } | ||
|
|
There was a problem hiding this comment.
I guess command-line options can't have the value --sdnotify?
(That could confuse the parsing)
Edit
(If someone could influence the value of a command-line option, it could
lead to incorrect parsing).
You can probably ignore this comment as describing a too unrealistic scenario.
There was a problem hiding this comment.
This is not possible, the option always needs an argument.
There was a problem hiding this comment.
I was thinking about --some-other-option value where value is --sdnotify.
Maybe it's not possible. In any case, it's too unrealistic.
There was a problem hiding this comment.
Yeah that could be a problem. However the other flags are handled in the same way. I think we can ignore this for now until somebody comes up with a real problem. Note that we will never touch the arguments after the image since they are for the container process.
I think it would be better to if It improves the user experience as the error message can give a clear direct information about what is wrong. (Maybe changing that would be a breaking change ...) Another thing that might not be directly related to this PR: I'm thinking that the default value of --sdnotify should be conmon. |
Yes I agree but we should discus this a new issue or discussion and not on this PR.
I don't know if there even is somebody using ignore so I don't know if they would like a hard error or just an overwrite. We already overwrite other options such as --cidfile so I kept it the same for --sdnotify, --sdnotify=ignore feels more like an option useful for testing. In the real world just do not set the NOTIFY_SOCKET env. |
Yes, you're right.
One advantage of failing is also that documentation can of be written shorter. (I'm just trying to promote more use of the "failing approach" in Podman instead of the "translate bad input to good input" approach) |
|
@vrothberg good to merge? |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
When a container was created with
--sdnotify valuewe would removethis arg instead of using it like with
--sdnotfiy=value.Also when the arg is set to ignore we should force conmon in order to
make the resulting Type=notify units work.
Fixes #15052
Does this PR introduce a user-facing change?