Allow configuration of podmansh #1993
Conversation
|
@lsm5 @bachradsusi PTAL |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
There was a problem hiding this comment.
Needs documentation and basic unit tests.
I like to see some proper design on why these option are required. For example why not just link /bin/sh to your actual shell?
Second, if we agree that we needs these options then should we move the existing PodmanShTimeout option to the new table to better group these options. We can still support the old option for backwards compat.
|
I agree lets create a |
Agreed.
There are cases in which people might need to have If one would e.g. use a podmansh container with a I also looked into different options for achieving this: Environment variables appeared to me as a bit clumsy for this use cases and container labels are not really user-configurable options. That's what motivated a new configuration section. Also: users can configure their own podmansh environment via
Makes sense, will have a look. |
|
Thanks, yes these are good arguments for the option. |
pkg/config/config.go
Outdated
| // Shell to start in container, default: "/bin/sh" | ||
| Shell string `toml:"shell"` | ||
| // Name of the container the podmansh user should join | ||
| Container string `toml:"container"` |
There was a problem hiding this comment.
Add Timeout here and deprecate podmansh_timeout, and remove it from man page and containers.conf.
There was a problem hiding this comment.
I'm in the process of doing so and need input: Do we have somewhere an example value of a deprecated value? Otherwise I would keep the old value in the internal struct and use this one, if the new value is zero.
There was a problem hiding this comment.
I will use
Line 341 in 477496c
There was a problem hiding this comment.
Done. @rhatdan @Luap99 please have a look at my suggestion.
I had to fiddle with the backwards compatability of podmansh_timeout. I wanted to have a solution, where podmansh.Timeout is preferred over the now deprecated podmansh_timeout value and the best idea I had was to use a default 0 value for both variables when they are not set. Then the newLocked function can use either of the two, if they are set and otherwise apply the default setting of 30.
This solution works, but it appears a bit clumsy. If any of you have a better idea for a more elegant solution please share.
Will work on the man page once we have resolved the remaining issues.
There was a problem hiding this comment.
I think you can leave the old default 30 on the deprecated field, then use 0 on the new one,
timeout := podmansh_timeout
if podmansh.Timeout > 0 {
timeout= podmansh.Timeout
}
There was a problem hiding this comment.
Done, I moved this part over to containers/podman#22683
Align with changes in containers/common#1993
|
Please squash your commits and make sure they are signed. If you don't want to squash your commits, then make sure that one does not add content that a subsequent one later removes of modifies. |
| @@ -307,6 +307,14 @@ crun = [ | |||
| "/usr/local/bin/crun", | |||
| ] | |||
|
|
|||
| [podmansh] | |||
There was a problem hiding this comment.
Could you also add a section documenting this in docs/containers.conf, removing podmansh_timeout?
common/docs/containers.conf.5.md
Lines 867 to 869 in fb9cee5
There was a problem hiding this comment.
Done. I've also added a podmansh section therein.
pkg/config/new.go
Outdated
| // engine.PodmanshTimeout is deprecated and should only be used, if podmansh.Timeout is not set | ||
| if config.Podmansh.Timeout == 0 { | ||
| if config.Engine.PodmanshTimeout > 0 { | ||
| config.Podmansh.Timeout = config.Engine.PodmanshTimeout | ||
| } else { | ||
| config.Podmansh.Timeout = uint(30) | ||
| } | ||
| } |
There was a problem hiding this comment.
I would not call this code in c/common at all, given there is only ever one user in podman that is only called in very rare cases (podmansh) I would do it there.
That way we do not have to run this for every podman command, sure it is not like this is slow but these things add up over time.
There was a problem hiding this comment.
I moved this part over to containers/podman#22683 which largely simplifies this code. I could remove a unit test and a test file, which makes this PR smaller 👍
grisu48
force-pushed
the
podmansh_sh
branch
4 times, most recently
from
7d772ee
to
b43c0bb
Compare
|
Amended, please @rhatdan @Luap99 @danishprakash could you have another look? |
|
I would like to see three tests satisfied. 1 engine.podman_timeout set inside of containers.conf and no setting for podmansh.timeout. It would be nice to have a config.PodmanshTimeout() function which gives you the right setting with the three examples above. This would allow podmansh to just call the function. |
Done. This happens in two tests: We check if podman reads the I think this should satisfy your request. |
|
LGTM pending validate tests. |
|
LGTM once the one spelling error is corrected to allow the tests to complete. Slapping a 5.1 label on this, we'd like to get this merged ASAP so we can add it to Podman v5.1 |
Adds a new configuration section `podmansh` to configure the shell, container and the timeout for podmansh. Signed-off-by: phoenix <felix.niederwanger@suse.com>
|
Incorporated the latest suggestions and amended the PR. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grisu48, 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 |
|
/lgtm |
|
Thank you everyone! containers/podman#22683 can be merged now as well, but it needs this dependency to be updated first. I guess renovate takes care of that? |
Adds a new configuration section
podmanshto configure the shell,container and the timeout for podmansh.
See containers/podman#22683 for more context.