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

added proxy support for build #1965

Merged
merged 3 commits into from Mar 11, 2024

Conversation

relief-melone
Copy link
Contributor

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

@@ -351,7 +352,14 @@ gulp.task('package:createVSIX', () =>
);

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
Contributor Author

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
Contributor Author

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
@@ -67,6 +67,7 @@
"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
Contributor 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
Contributor Author

relief-melone commented Mar 11, 2024

@microsoft-github-policy-service agree

@relief-melone
Copy link
Contributor 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
@VSCodeTriageBot VSCodeTriageBot added this to the March 2024 milestone Mar 11, 2024
@connor4312 connor4312 merged commit 24bfaf8 into microsoft:main Mar 11, 2024
5 of 7 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.

None yet

4 participants