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

Configure npm proxy when proxy environmentals exist #712

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

jmedelmann
Copy link
Contributor

PR for issue #702 (Node feature does not pick up proxy settings).
Docker automatically sets up proxy environmentals in the container when configured (see https://docs.docker.com/network/proxy/) so usually everything works without additional configuration. An exception is npm, which does not pick up these environmentals and needs to be configured manually. This PR adds the necessary configuration steps.

@jmedelmann jmedelmann requested a review from a team as a code owner October 3, 2023 16:28
@jmedelmann
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing this change.

Currently, this PR is automatically setting proxy if the env variables exist. Instead, can we wrap this logic inside a condition on a new Feature option (say useProxyConfig:boolean with default false). This way, the users are willingly enabling it as per their needs, and we won't have to worry about folks raising security concerns.

As per https://docs.docker.com/network/proxy/#configure-the-docker-client, adding proxies to ~/.docker/config.json seems like a manual approach where users are opting into it. So wrapping it inside a Feature option makes sense to me!

@jmedelmann
Copy link
Contributor Author

jmedelmann commented Oct 3, 2023

Currently, this PR is automatically setting proxy if the env variables exist. Instead, can we wrap this logic inside a condition on a new Feature option (say useProxyConfig:boolean with default false). This way, the users are willingly enabling it as per their needs, and we won't have to worry about folks raising security concerns.

As per https://docs.docker.com/network/proxy/#configure-the-docker-client, adding proxies to ~/.docker/config.json seems like a manual approach where users are opting into it. So wrapping it inside a Feature option makes sense to me!

These env variables are a kind of standard and almost all programs will use them automatically. If we introduce an additional option for just npm we will create an inconsistent state with no benefit. Note that other programs already use these environmentals. For example, apt-get automatically uses the proxy environmentals in install.sh. If we want to make them optional it would require to make them optional for the other programs as well. This would be out of scope of this issue which is about the special treatment required for npm.

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a quick chat with the team, turns out the devcontainers/cli supports the automatic support as well. Approving! ✅

@samruddhikhandale samruddhikhandale merged commit 0d7ad7c into devcontainers:main Oct 4, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants