Allow configuration of podmansh #1993

grisu48 · 2024-05-13T13:40:44Z

Adds a new configuration section podmansh to configure the shell,
container and the timeout for podmansh.

See containers/podman#22683 for more context.

rhatdan · 2024-05-13T13:45:22Z

@lsm5 @bachradsusi PTAL

packit-as-a-service · 2024-05-13T13:50:04Z

Ephemeral COPR build failed. @containers/packit-build please check.

Luap99

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.

pkg/config/config.go

rhatdan · 2024-05-13T15:12:50Z

I agree lets create a
[podmansh]
Section of the containers.conf

grisu48 · 2024-05-14T06:47:57Z

Needs documentation and basic unit tests.

Agreed.

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?

There are cases in which people might need to have /bin/sh and /bin/bash, e.g. when they run a distribution with shell scripts that for some reason required /bin/sh. While bash and sh are very close, there are many people out there that run a different shell on a daily basis but still require /bin/sh for running shell scripts.

If one would e.g. use a podmansh container with a fish shell as default, then linking /bin/sh to /bin/fish would likely break a good portion of the shell scripts with the #!/bin/sh shebang.
I think this motivates the configuration possibility.

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 $home/.config/containers/containers.conf. Allowing users to easily configure their environment is the whole point for this PR.

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.

Makes sense, will have a look.

Luap99 · 2024-05-14T11:07:50Z

Thanks, yes these are good arguments for the option.

lsm5

@grisu48 thanks for the PR. Agreed with @Luap99 and @rhatdan 's comments on unit test and new containers.conf section

rhatdan · 2024-05-15T11:30:27Z

pkg/config/config.go

+	// 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"`


Add Timeout here and deprecate podmansh_timeout, and remove it from man page and containers.conf.

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.

I will use

common/pkg/config/config.go

Line 341 in 477496c

ImageBuildFormat string `toml:"image_build_format,omitempty"`

as a template and keep the old value, while making it deprecated.

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.

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 }

Done, I moved this part over to containers/podman#22683

pkg/config/config.go

rhatdan · 2024-05-16T12:43:28Z

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.

danishprakash · 2024-05-16T14:11:06Z

pkg/config/testdata/containers_default.conf

@@ -307,6 +307,14 @@ crun = [
 	    "/usr/local/bin/crun",
 ]

+[podmansh]


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

**podmansh_timeout**=30

Number of seconds to wait for podmansh logins.

Done. I've also added a podmansh section therein.

Luap99 · 2024-05-16T14:42:08Z

pkg/config/new.go

+	// 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)
+		}
+	}


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.

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 · 2024-05-21T09:13:28Z

Amended, please @rhatdan @Luap99 @danishprakash could you have another look?

docs/containers.conf.5.md

rhatdan · 2024-05-21T10:45:18Z

I would like to see three tests satisfied.

1 engine.podman_timeout set inside of containers.conf and no setting for podmansh.timeout.
2 podmansh.timeout set no setting for engine.podman_timeout
3 Both set.

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.

grisu48 · 2024-05-21T12:29:15Z

I would like to see three tests satisfied.

1 engine.podman_timeout set inside of containers.conf and no setting for podmansh.timeout. 2 podmansh.timeout set no setting for engine.podman_timeout 3 Both set.

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 engine.podmansh_timeout and podmansh.timeout settings from the config files and the new "Check podmansh timeout settings" unit test checks if the PodmanshTimeout() method behaves according to the three cases you requested.

I think this should satisfy your request.

pkg/config/config.go

pkg/config/testdata/containers_default.conf

lsm5 · 2024-05-21T19:00:31Z

LGTM pending validate tests.

pkg/config/config.go

TomSweeneyRedHat · 2024-05-21T20:09:16Z

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

pkg/config/containers.conf

docs/containers.conf.5.md

Adds a new configuration section `podmansh` to configure the shell, container and the timeout for podmansh. Signed-off-by: phoenix <felix.niederwanger@suse.com>

grisu48 · 2024-05-22T11:33:59Z

Incorporated the latest suggestions and amended the PR.

Luap99

LGTM

openshift-ci · 2024-05-22T16:01:32Z

[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:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2024-05-22T16:01:36Z

/lgtm

grisu48 · 2024-05-23T06:27:20Z

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?

grisu48 mentioned this pull request May 13, 2024

Add configuration for podmansh containers/podman#22683

Merged

grisu48 force-pushed the podmansh_sh branch from 3a3ace7 to 1991ba9 Compare May 13, 2024 13:43

Luap99 requested changes May 13, 2024

View reviewed changes

rhatdan reviewed May 13, 2024

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

grisu48 force-pushed the podmansh_sh branch from 1991ba9 to 4383bac Compare May 14, 2024 07:01

lsm5 reviewed May 14, 2024

View reviewed changes

grisu48 marked this pull request as draft May 15, 2024 09:32

openshift-ci bot added the do-not-merge/work-in-progress label May 15, 2024

rhatdan reviewed May 15, 2024

View reviewed changes

pkg/config/config.go Show resolved Hide resolved

grisu48 force-pushed the podmansh_sh branch from b3db2f3 to 17fbc63 Compare May 16, 2024 13:09

grisu48 marked this pull request as ready for review May 16, 2024 13:10

openshift-ci bot removed the do-not-merge/work-in-progress label May 16, 2024

danishprakash reviewed May 16, 2024

View reviewed changes

Luap99 reviewed May 16, 2024

View reviewed changes

grisu48 marked this pull request as draft May 21, 2024 07:25

openshift-ci bot added the do-not-merge/work-in-progress label May 21, 2024

grisu48 force-pushed the podmansh_sh branch 4 times, most recently from 7d772ee to b43c0bb Compare May 21, 2024 08:58

grisu48 mentioned this pull request May 21, 2024

Add configurations section for podmansh containers/podman#22767

Closed

grisu48 marked this pull request as ready for review May 21, 2024 09:13

openshift-ci bot removed the do-not-merge/work-in-progress label May 21, 2024

rhatdan reviewed May 21, 2024

View reviewed changes

docs/containers.conf.5.md Outdated Show resolved Hide resolved

grisu48 marked this pull request as draft May 21, 2024 12:03

openshift-ci bot added the do-not-merge/work-in-progress label May 21, 2024

grisu48 force-pushed the podmansh_sh branch from b43c0bb to 5571a35 Compare May 21, 2024 12:21

grisu48 marked this pull request as ready for review May 21, 2024 12:31

openshift-ci bot removed the do-not-merge/work-in-progress label May 21, 2024

lsm5 reviewed May 21, 2024

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

lsm5 reviewed May 21, 2024

View reviewed changes

pkg/config/testdata/containers_default.conf Show resolved Hide resolved

TomSweeneyRedHat reviewed May 21, 2024

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

TomSweeneyRedHat added the 5.1 label May 21, 2024

grisu48 force-pushed the podmansh_sh branch from 5571a35 to 7d80598 Compare May 22, 2024 06:11

Luap99 reviewed May 22, 2024

View reviewed changes

pkg/config/containers.conf Outdated Show resolved Hide resolved

pkg/config/containers.conf Outdated Show resolved Hide resolved

pkg/config/containers.conf Outdated Show resolved Hide resolved

docs/containers.conf.5.md Outdated Show resolved Hide resolved

Add configuration for podmansh

e41e56f

Adds a new configuration section `podmansh` to configure the shell, container and the timeout for podmansh. Signed-off-by: phoenix <felix.niederwanger@suse.com>

grisu48 force-pushed the podmansh_sh branch from 7d80598 to e41e56f Compare May 22, 2024 11:32

Luap99 approved these changes May 22, 2024

View reviewed changes

openshift-ci bot added the approved label May 22, 2024

openshift-ci bot assigned Luap99 May 22, 2024

openshift-ci bot added the lgtm label May 22, 2024

openshift-merge-bot bot merged commit 1a97fd1 into containers:main May 22, 2024
12 checks passed

grisu48 deleted the podmansh_sh branch May 23, 2024 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration of podmansh #1993

Allow configuration of podmansh #1993

grisu48 commented May 13, 2024 •

edited

Loading

rhatdan commented May 13, 2024

packit-as-a-service bot commented May 13, 2024

Luap99 left a comment

rhatdan commented May 13, 2024

grisu48 commented May 14, 2024 •

edited

Loading

Luap99 commented May 14, 2024

lsm5 left a comment

rhatdan May 15, 2024

grisu48 May 15, 2024

grisu48 May 15, 2024

grisu48 May 15, 2024

Luap99 May 16, 2024

grisu48 May 21, 2024

rhatdan commented May 16, 2024

danishprakash May 16, 2024

grisu48 May 21, 2024

Luap99 May 16, 2024

grisu48 May 21, 2024 •

edited

Loading

grisu48 commented May 21, 2024

rhatdan commented May 21, 2024

grisu48 commented May 21, 2024 •

edited

Loading

lsm5 commented May 21, 2024

TomSweeneyRedHat commented May 21, 2024

grisu48 commented May 22, 2024

Luap99 left a comment

openshift-ci bot commented May 22, 2024

Luap99 commented May 22, 2024

grisu48 commented May 23, 2024

	podmansh_timeout=30

	Number of seconds to wait for podmansh logins.

Allow configuration of podmansh #1993

Allow configuration of podmansh #1993

Conversation

grisu48 commented May 13, 2024 • edited Loading

rhatdan commented May 13, 2024

packit-as-a-service bot commented May 13, 2024

Luap99 left a comment

Choose a reason for hiding this comment

rhatdan commented May 13, 2024

grisu48 commented May 14, 2024 • edited Loading

Luap99 commented May 14, 2024

lsm5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grisu48 May 21, 2024 • edited Loading

Choose a reason for hiding this comment

grisu48 commented May 21, 2024

rhatdan commented May 21, 2024

grisu48 commented May 21, 2024 • edited Loading

lsm5 commented May 21, 2024

TomSweeneyRedHat commented May 21, 2024

grisu48 commented May 22, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 22, 2024

Luap99 commented May 22, 2024

grisu48 commented May 23, 2024

grisu48 commented May 13, 2024 •

edited

Loading

grisu48 commented May 14, 2024 •

edited

Loading

grisu48 May 21, 2024 •

edited

Loading

grisu48 commented May 21, 2024 •

edited

Loading