-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Allow users to specify TMPDIR in containers.conf #8725
Conversation
Currently we hard code TMPDIR environment variable to /var/tmp if it is not set in the Environment. This causes TMPDIR environment variable to be ignored if set in containers.conf. This change now uses the host environment TMPDIR, followed by containers.conf and then hard codes TMPDIR, if it was not set. Signed-off-by: Daniel J Walsh <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
Truly fixes #5412 |
@containers/podman-maintainers PTAL |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
I meant to ask earlier if there was any way to test this. I assume that this change updates the precedence, but I couldn't find a way to confirm. How is |
Self-followup: found a way using $ cat ~/.config/containers/containers.conf
[engine]
env = ["TMPDIR=/this/is/set/in/containers.conf"]
$ env -u TMPDIR ./bin/podman unshare /usr/bin/env|grep TMPD
TMPDIR=/this/is/set/in/containers.conf <--- Good. Pre-8725, this prints /var/tmp
$ TMPDIR=/this/is/set/in/user/environment ./bin/podman unshare /usr/bin/env|grep TMPD
TMPDIR=/this/is/set/in/user/environment <--- Good. Same thing pre-8725 Day late and dollar short, but /lgtm :-) I do not plan to write a system test for this: I try hard to avoid mucking with |
Sorry I could not figure a good way of testing this out. So I instrumented the tool to see what TMPDIR was set to and then tried every scenario I could try. |
Currently we hard code TMPDIR environment variable to /var/tmp
if it is not set in the Environment. This causes TMPDIR environment
variable to be ignored if set in containers.conf.
This change now uses the host environment TMPDIR, followed by
containers.conf and then hard codes TMPDIR, if it was not set.
Signed-off-by: Daniel J Walsh [email protected]