-
Notifications
You must be signed in to change notification settings - Fork 7
Only set proxy environment variables in proxy environments #71
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
base: main
Are you sure you want to change the base?
Only set proxy environment variables in proxy environments #71
Conversation
| # Unset proxy variables for all login shells | ||
| COPY unset-proxy.sh /etc/bash_completion.d/unset-proxy.sh |
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.
is this script still needed? My expectation is that this Dockerfile is only used in cases where proxy variables are actually set.
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.
Yes, the script is only needed where proxy variables are used
6858bb6 to
aa93b25
Compare
| "build": { | ||
| // Installs latest version from the Distribution | ||
| "dockerfile": "./Dockerfile", | ||
| "dockerfile": "./${localEnv:DEVCONTAINER_DOCKERFILE_NAME:Dockerfile}", |
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.
Are you sure this works as expected? I think this will kick in only during pre-building of the container, in GitHub Actions. And there, proxy variables are never set.
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.
IMHO the current / main solution works the same. Environment variables are set in the pre-built image and are taken from the build environment. As such I expect at the moment the people using the devcontainer in environments with a proxy will have to build the devcontainer themselves.
I guess an alternative is to use env files: https://code.visualstudio.com/remote/advancedcontainers/environment-variables#_option-2-use-an-env-file
e8eb525 to
51c5bf6
Compare
The approach using /etc/profile.d/ to unset empty variables does not work, because it is never executed in the devcontainer. The proposed solution should be more robust, because now the variables are never present, if not needed and the build will fail, if it is broken. Files in /etc/bash_completion.d/ will be executed whenever a new bash instance is spawned and should work as a /etc/profile.d/ replacement.
51c5bf6 to
ffcf6ed
Compare
The approach using
/etc/profile.d/to unset empty variables does not work, because it is never executed in the devcontainer.The proposed solution should be more robust, because now the variables are never present, if not needed and the build will fail, if it is broken. Files in
/etc/bash_completion.d/will be executed whenever a new bash instance is spawned and should work as a/etc/profile.d/replacement.Caused by #52