cmd: Use $TOOLBOX_DEFAULT_CONTAINER to populate --container#1774
cmd: Use $TOOLBOX_DEFAULT_CONTAINER to populate --container#1774whot wants to merge 1 commit intocontainers:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
If set, use the TOOLBOX_DEFAULT_CONTAINER environment variable in lieu of the --container argument. This allows for much easier scripting with partially interactive use-cases. For example, a user may have different toolboxes for different projects. With this patch one can use e.g. direnv to export the variable, allowing for workflows like this: $ cd path/to/myrepo # direnv sets TOOLBOX_DEFAULT_CONTAINER to myrepo $ toolbox run meson compile -C build # expands to --container=myrepo Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
whot
force-pushed
the
wip/default-container
branch
from
5b278c9 to
1914bd4
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a convenient feature by allowing the TOOLBOX_DEFAULT_CONTAINER environment variable to specify the default container for create, enter, and run commands. The implementation for reading the environment variable is correct. However, I've identified a potential issue with adding this variable to the list of preserved environment variables, which could lead to unexpected behavior by leaking host configuration into the container. My review includes a specific comment and suggestion to address this concern.
| "SHELL", | ||
| "SSH_AUTH_SOCK", | ||
| "TERM", | ||
| "TOOLBOX_DEFAULT_CONTAINER", |
There was a problem hiding this comment.
Adding TOOLBOX_DEFAULT_CONTAINER to preservedEnvironmentVariables causes it to be passed from the host into containers during toolbox run. This leaks host-side tool configuration into the container's environment, which can lead to unexpected behavior and breaks isolation. For example, a script inside a container might behave differently depending on the host's TOOLBOX_DEFAULT_CONTAINER setting.
To avoid this, please remove this variable from the list. The main feature of this PR (using the variable on the host) will still work correctly.
There was a problem hiding this comment.
Going to wait for human confirmation of this because if the above is correct, I misunderstood it completely :) in my testing this was needed to get a toolbox foo to run correctly when running within a toolbox itself.
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 13s |
1 participant
If set, use the TOOLBOX_DEFAULT_CONTAINER environment variable in lieu of the --container argument. This allows for much easier scripting with partially interactive use-cases.
For example, a user may have different toolboxes for different projects. With this patch one can use e.g. direnv to export the variable, allowing for workflows like this:
I've scripted around this locally but it seems like a nice feature to have. I have loads of different containers with the per-project specific builddeps etc so I constantly have to do "toolbox enter foo", "toolbox run --container=bar", etc.
I now have a
.use-toolboxfile with the container name and my ninja/meson wrappers read this so they auto-execute astoolbox run --container=$(cat .use-toolbox) $@. This works fine, but it's a local bespoke script which would be much nicer to have as built-in functionality.