-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
support proxies through environment variables #235
base: master
Are you sure you want to change the base?
Conversation
IMO, this isn't breaking. It is a bug fix... |
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.
Can we add a couple of tests ensuring proxying works?
|
||
### Proxy | ||
|
||
Proxies are supported through `https_proxy`, `http_proxy`, and `no_proxy` environment variables: |
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.
Let's have these in the README as both SCREAMING_SNAKE_CASE
and lower case, or otherwise make it clearer that we are operating insensitively.
I tried to add tests. But struggled with creating a proxy to use in the tests. I tried to write a simple proxy myself using I don't have much experience working with proxies to be honest. Maybe there is an architectural issue with what I'm trying? Here is my latest attempt using QUnit.module('proxy', async function (hooks) {
hooks.beforeEach(async function () {
const port = await getPort();
httpProxy
.createProxyServer({
target: process.env.EMBER_SOURCE_CHANNEL_URL_HOST,
})
.listen(port);
this.proxyPort = port;
});
hooks.afterEach(async function () {
// clean-up proxy environment variables
Object.keys(process.env).forEach((envVariable) => {
const proxyEnvVariables = ['http_proxy', 'https_proxy', 'no_proxy'];
const isProxyEnvVariable = proxyEnvVariables.includes(envVariable.toLocaleLowerCase());
if (isProxyEnvVariable) {
delete process.env[envVariable];
}
});
// shutdown proxy
await this.proxy.close();
});
['http_proxy', 'https_proxy'].forEach((envVariable) => {
QUnit.only(`supports ${envVariable} environment variable`, async function (assert) {
const { assetPath, proxyPort, server } = this;
// arrange: set proxy environment variable
process.env[envVariable] = `http://localhost:${proxyPort}`;
// act
const actual = await getChannelURL('canary');
// assert
const expected = `http://${server.host}:${server.port}/builds.emberjs.com${assetPath}`;
assert.equal(actual, expected);
});
});
}); Any idea? |
This adds support for
https_proxy
,http_proxy
, andno_proxy
environment variables. If these are set, a proxy will be used for network requests. This is important for some corporate networks, which require proxy usage for network requests to public internet.Most environment variables are uppercase. But for
https_proxy
,http_proxy
, andno_proxy
lowercase is more commonly used. There are some programs (e.g.wget
), which support lowercase but not uppercase. The implementation proposed here is case-insensitive.This change enables running ember-try scenarios using a proxy. The other tools used by ember-try supports these environment variables already. Only ember-source-channel-url does not support proxy usage yet at all.
Not sure if it should be considered a breaking changes. It changes behavior if proxy environment variables are set. But you could argue that a consumer, which sets the environment variables expects a program to use a proxy.
Closes #4