:fix :(docs): changed env PODMAN_BIN to CONTAINER_TOOL #305

soham4abc · 2024-08-03T18:02:47Z

This PR changes the environment variable PODMAN_BIN to CONTAINER_TOOL

as kantra falls back to docker if podman is not present, this variable name makes much more sense.

@dagood Please do have a look and provide any necessary feedback for the same.

Closes #168

dagood · 2024-08-05T16:24:43Z

cmd/settings.go

@@ -24,7 +24,7 @@ const (

 type Config struct {
 	RootCommandName      string `env:"CMD_NAME" default:"kantra"`
-	PodmanBinary         string `env:"PODMAN_BIN" default:"/usr/bin/podman"`
+	PodmanBinary         string `env:"CONTAINER_TOOL" default:"/usr/bin/podman"`


My only question here would be whether kantra should keep supporting PODMAN_BIN as an alias of CONTAINER_TOOL for compatibility reasons. (Realized my paragraph about this was unclear in #168, so I've edited it to reword.)

I'm not sure either way. (And to be clear, I'm not a maintainer of the repo.)

@eemcmullan your views on this?

I think it's fine to move ahead with CONTAINER_TOOL without supporting PODMAN_BIN. We will then probably want to update instances of the podmanBinary field to something like containerToolBinary, but I don't think that was covered in the issue, and it's not a user-facing var, so I am okay with these changes. Thanks for the contribution!

Welcome @eemcmullan do let me know if further changes are required before merging!!

I am wondering if during the loading, if PODMAN_BIN is set in line 57 below we could use that.

@eemcmullan won't a user set this env var to use docker if they want?

eemcmullan · 2024-08-05T20:19:21Z

cmd/settings.go

@@ -24,7 +24,7 @@ const (

 type Config struct {
 	RootCommandName      string `env:"CMD_NAME" default:"kantra"`
-	PodmanBinary         string `env:"PODMAN_BIN" default:"/usr/bin/podman"`
+	PodmanBinary         string `env:"CONTAINER_TOOL" default:"/usr/bin/podman"`


I think it's fine to move ahead with CONTAINER_TOOL without supporting PODMAN_BIN. We will then probably want to update instances of the podmanBinary field to something like containerToolBinary, but I don't think that was covered in the issue, and it's not a user-facing var, so I am okay with these changes. Thanks for the contribution!

eemcmullan · 2024-08-06T15:07:30Z

@soham4abc apologies for the confusion. After discussing with @shawn-hurley we think it's best to support PODMAN_BIN for a while longer. Can we first check for this var? If CONTAINER_TOOL is also set, we will want to use that instead.

soham4abc · 2024-08-06T15:21:54Z

@soham4abc apologies for the confusion. After discussing with @shawn-hurley we think it's best to support PODMAN_BIN for a while longer. Can we first check for this var? If CONTAINER_TOOL is also set, we will want to use that instead.

Sure... Do you want me to revert the changes to ```PODMAN_BIN?

eemcmullan · 2024-08-06T15:34:42Z

@soham4abc We still want to move forward with CONTAINER_TOOL. But for the sake of compatibility, we should probably check for PODMAN_BIN and CONTAINER_TOOL together. If both are set, use CONTAINER_TOOL. If only one or the other, use that.

Signed-off-by: soham4abc <[email protected]>

soham4abc · 2024-08-06T15:55:01Z

@soham4abc We still want to move forward with CONTAINER_TOOL. But for the sake of compatibility, we should probably check for PODMAN_BIN and CONTAINER_TOOL together. If both are set, use CONTAINER_TOOL. If only one or the other, use that.

hey @eemcmullan I have made the changes can you please have a look and test on your end!

Signed-off-by: soham4abc <[email protected]>

eemcmullan · 2024-08-06T18:44:37Z

cmd/settings.go

@@ -24,6 +24,7 @@ const (

 type Config struct {
 	RootCommandName      string `env:"CMD_NAME" default:"kantra"`
+	ContainerBinary      string `env:"CONTAINER_TOOL" default:"/usr/bin/podman"`


CI is failing because this field will need to also be updated where it is being accessed in the rest of the project. This will be where the different commands run. Please let me know if you have any questions on it.

sure, Any particular directory this should be pointing to as default?

As of the latest changes I removed the default path.

Signed-off-by: soham4abc <[email protected]>

soham4abc · 2024-08-06T23:55:06Z

@eemcmullan can you please have a look and run the CI tests?

Signed-off-by: soham4abc <[email protected]>

eemcmullan · 2024-08-07T20:14:45Z

cmd/settings.go

-	// Respect existing PODMAN_BIN setting.
-	if os.Getenv("PODMAN_BIN") != "" {
+	// Respect existing CONTAINER_TOOL setting.
+	if os.Getenv("CONTAINER_TOOL") != "" || os.Getenv("PODMAN_BIN") != "" {


I think the check should first look for CONTAINER_TOOL, and then PODMAN_BIN after, only if the former is empty, so CONTAINER_TOOL takes priority.

For the Config struct, we do not need PodmanBinary any longer - just ContainerBinary. This field is used throughout the rest of the project to start the different commands, as the commands run in containers. For example, an analyze command will look something like /usr/bin/podman run --entrypoint <analyzer_bin>.... <image>... where /usr/bin/podman is set from this config struct. You can see an example of this here: https://github.com/konveyor/kantra/blob/main/cmd/analyze.go#L1476

Once these commands are updated with the new struct field name, they should be able to run, and the CI testing should pass.

Thanks a lot for the review @eemcmullan. I have pushed the recent changes!

…rity Signed-off-by: soham4abc <[email protected]>

soham4abc · 2024-08-10T05:18:46Z

@shawn-hurley can you please have a look at the changes and provide your reviews?

shawn-hurley

One minor nit

shawn-hurley · 2024-08-12T13:57:38Z

docs/developer.md

-Two environment variables control the container runtime and the kantra image: `PODMAN_BIN` and `RUNNER_IMG`:
- `PODMAN_BIN`: path to your container runtime (podman or docker)
+Two environment variables control the container runtime and the kantra image: `CONTAINER_TOOL` and `RUNNER_IMG`:
+- `CONTAINER_TOOL`: path to your container runtime (podman or docker)


nit: container tool not container runtime.

Container Runtimes are actually different things https://kubernetes.io/docs/setup/production-environment/container-runtimes/ and in this case we don't work with most container runtimes we work with a container tool which is IMO something different.

We can fix this on a follow up though

Thanks for the review @shawn-hurley!

@eemcmullan do let me know if this PR needs any more changes

soham4abc force-pushed the env_change branch from 2cff97d to 9bbbd42 Compare August 3, 2024 18:03

dagood reviewed Aug 5, 2024

View reviewed changes

eemcmullan approved these changes Aug 5, 2024

View reviewed changes

soham4abc added 2 commits August 6, 2024 21:24

:fix :(docs): changed env PODMAN_BIN to CONTAINER_TOOL

5b94681

Signed-off-by: soham4abc <[email protected]>

:fix :(bug): added backward compatibility for the env PODMAN_BIN

326743a

Signed-off-by: soham4abc <[email protected]>

soham4abc force-pushed the env_change branch from 179444a to 326743a Compare August 6, 2024 15:54

soham4abc added 2 commits August 6, 2024 21:29

:fix :(env) : formatting changes

b2c1761

Signed-off-by: soham4abc <[email protected]>

:fix: :(bug): removed default path for CONTAINER_TOOL

38fce9a

Signed-off-by: soham4abc <[email protected]>

eemcmullan reviewed Aug 6, 2024

View reviewed changes

:bug :(env): changed default path for CONTAINER_TOOL

8ecf097

Signed-off-by: soham4abc <[email protected]>

:bug :(env): changed default path for PODMAN_BIN

f921c51

Signed-off-by: soham4abc <[email protected]>

soham4abc requested review from eemcmullan, dagood and shawn-hurley August 7, 2024 14:16

eemcmullan reviewed Aug 7, 2024

View reviewed changes

:bug :(env): updated the config struct and set CONTAINER_TOOL as prio…

41f7d4d

…rity Signed-off-by: soham4abc <[email protected]>

soham4abc requested a review from eemcmullan August 7, 2024 23:34

eemcmullan approved these changes Aug 9, 2024

View reviewed changes

shawn-hurley approved these changes Aug 12, 2024

View reviewed changes

eemcmullan merged commit 9da4a1f into konveyor:main Aug 12, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

:fix :(docs): changed env PODMAN_BIN to CONTAINER_TOOL #305

:fix :(docs): changed env PODMAN_BIN to CONTAINER_TOOL #305

soham4abc commented Aug 3, 2024

dagood Aug 5, 2024

soham4abc Aug 5, 2024

eemcmullan Aug 5, 2024

soham4abc Aug 5, 2024

shawn-hurley Aug 6, 2024

eemcmullan Aug 5, 2024

eemcmullan commented Aug 6, 2024

soham4abc commented Aug 6, 2024

eemcmullan commented Aug 6, 2024

soham4abc commented Aug 6, 2024

eemcmullan Aug 6, 2024

soham4abc Aug 6, 2024

soham4abc Aug 6, 2024

soham4abc commented Aug 6, 2024

eemcmullan Aug 7, 2024 •

edited

Loading

soham4abc Aug 7, 2024

soham4abc commented Aug 10, 2024

shawn-hurley left a comment

shawn-hurley Aug 12, 2024

soham4abc Aug 12, 2024

:fix :(docs): changed env PODMAN_BIN to CONTAINER_TOOL #305

:fix :(docs): changed env PODMAN_BIN to CONTAINER_TOOL #305

Conversation

soham4abc commented Aug 3, 2024

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

eemcmullan commented Aug 6, 2024

soham4abc commented Aug 6, 2024

eemcmullan commented Aug 6, 2024

soham4abc commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soham4abc commented Aug 6, 2024

eemcmullan Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soham4abc commented Aug 10, 2024

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eemcmullan Aug 7, 2024 •

edited

Loading