Skip to content

Conversation

@relief-melone
Copy link

As stated in #1955 running npx gulp vsDebugServerBundle behind a proxy will currently fail as got does not honor proxy env vars by default.
This PR addresses that issue by using an https proxy agent in case env vars have been detected for an https proxy

gulp.task('l10n:bundle-download', async () => {
const res = await got('https://github.com/microsoft/vscode-loc/archive/main.zip').buffer();
const opts = {};
const proxy = process.env.https_proxy || process.env.HTTPS_PROXY || null;
Copy link
Member

Choose a reason for hiding this comment

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

process.env is already 'magically' case insensitive on Windows

Suggested change
const proxy = process.env.https_proxy || process.env.HTTPS_PROXY || null;
const proxy = process.env.HTTPS_PROXY || null;
Copy link
Author

@relief-melone relief-melone Mar 11, 2024

Choose a reason for hiding this comment

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

process.env is already 'magically' case insensitive on Windows

in my case it is running on linux as I build the package for use with nvim and the lsp

Copy link
Member

Choose a reason for hiding this comment

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

The uppercase version is the standard one I've seen, and is what e.g. curl uses

Copy link
Author

@relief-melone relief-melone Mar 11, 2024

Choose a reason for hiding this comment

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

don't really have a strong attachment to supporting upper and lowercase and I usually also prefer uppercase env vars. but I have seen multiple programs in linux that only use the lowercase variant. And in linux the lowercase variant also takes precedent over the uppercase one in curl

Like I said. Don't really have a strong opinion on that one. your choice

package.json Outdated
"execa": "^5.1.1",
"glob-stream": "^8.0.0",
"got": "^11.8.6",
"https-proxy-agent": "^7.0.4",
Copy link
Member

Choose a reason for hiding this comment

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

This should be in the devDependencies

Copy link
Author

Choose a reason for hiding this comment

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

agree. moved to devDependencies

@relief-melone
Copy link
Author

relief-melone commented Mar 11, 2024

@microsoft-github-policy-service agree

@relief-melone
Copy link
Author

not a mac user. Any idea what's the catch in that test stage?

@connor4312
Copy link
Member

That's a flake on main I need to fix still

@connor4312 connor4312 enabled auto-merge (squash) March 11, 2024 18:38
@vscodenpa vscodenpa added this to the March 2024 milestone Mar 11, 2024
@connor4312 connor4312 merged commit 24bfaf8 into microsoft:main Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants