Skip to content
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

requested change to the behavior of the podman run --env-merge flag #18920

Closed
lastephey opened this issue Jun 16, 2023 · 5 comments · Fixed by #18969
Closed

requested change to the behavior of the podman run --env-merge flag #18920

lastephey opened this issue Jun 16, 2023 · 5 comments · Fixed by #18969
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@lastephey
Copy link

Feature request description

Dear Podman developers,

Thank you for providing the podman run --env-merge flag we requested in #15288.

We are working on getting this into production for our podman-hpc modules. We would like to prepend some directories to the user container LD_LIBRARY_PATH settings without overwriting whatever they may already have.

However, we have found that in the situation where an environment variable (ex: LD_LIBRARY_PATH) is not already set in the container, the --env-merge flag returns an empty LD_LIBRARY_PATH.

I've included an example below. In the first command, I am running the test container test:test that contains the setting LD_LIBRARY_PATH=foo. In the second command, I am running the ubuntu:jammy container which does not define LD_LIBRARY_PATH.

(base) DOE-7616476:test stephey$ cat Containerfile 
FROM ubuntu:jammy

ENV LD_LIBRARY_PATH=foo
(base) DOE-7616476:test stephey$ cat print-env-helper.sh 
#!/bin/bash
#
##helper script to print environment variable
#
echo $LD_LIBRARY_PATH
(base) DOE-7616476:test stephey$ podman run --rm -it --volume=$(pwd):/test --workdir=/test test:test ./print-env-helper.sh 
foo
(base) DOE-7616476:test stephey$ 
(base) DOE-7616476:test stephey$ podman run --rm --env-merge LD_LIBRARY_PATH=\${LD_LIBRARY_PATH},bar --volume=$(pwd):/test --workdir=/test test:test ./print-env-helper.sh 
foo,bar
(base) DOE-7616476:test stephey$ podman run --rm -it --volume=$(pwd):/test --workdir=/test ubuntu:jammy ./print-env-helper.sh 

(base) DOE-7616476:test stephey$ 
(base) DOE-7616476:test stephey$ podman run --rm --env-merge LD_LIBRARY_PATH=\${LD_LIBRARY_PATH},bar --volume=$(pwd):/test --workdir=/test ubuntu:jammy ./print-env-helper.sh 

(base) DOE-7616476:test stephey$ 

Suggest potential solution

Feature request: update the behavior of the --env-merge flag to preserve the requested additional variable setting, even if there is no original variable defined.

Desired behavior:

(base) DOE-7616476:test stephey$ podman run --rm --env-merge LD_LIBRARY_PATH=\${LD_LIBRARY_PATH},bar --volume=$(pwd):/test --workdir=/test ubuntu:jammy ./print-env-helper.sh 
bar
(base) DOE-7616476:test stephey$ 

Have you considered any alternatives?

We are open to alternatives.

Additional context

Thank you for considering this change.

@lastephey lastephey added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 16, 2023
@vrothberg
Copy link
Member

Thanks for reaching out, @lastephey.

I agree on your desired behavior and personally see it more as a bug fix. @Luap99, WDYT?

@Luap99
Copy link
Member

Luap99 commented Jun 19, 2023

Yeah I think that can be considered a bug fix. I would expect it to follow shell like behaviour and just have a empty string in place of an undefined var.
Although one important thing is that means your command would return ,bar not bar.

@danishprakash
Copy link
Contributor

Mind if I take a look at this?

@vrothberg
Copy link
Member

It's all yours, thank you, @danishprakash

@lastephey
Copy link
Author

Thanks very much for looking at this @danishprakash. Good point about the leading/trailing character that may be left behind @Luap99. I think this should be okay in our case since we will always be prepending rather than appending. I could see an issue for the case where people want to append and may end up with a leading :, so maybe that's worth noting as a caveat in the docs.

Thanks again and sorry for my late reply- yesterday was a holiday here in the US.

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Categorizes issue or PR as related to a new feature. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants