feat: implement proxy usage #11

BlackDark · 2021-08-18T15:13:31Z

Closes #10

Have written small changes for proxy usage.
Tested in my environment where i need a proxy.

Released necessary package and adapted changes to the github action:

Tried using it in an action

      - name: Setup terraform
        uses: BlackDark/setup-terraform@proxy-custom-pkg
        with:
          terraform_version: ^1.0.0

Works:

hashicorp-cla · 2021-08-18T15:13:34Z

All committers have signed the CLA.

bahag-welscha · 2021-09-28T07:03:10Z

What is the progress of this PR? I faced the same issue that the binaries can't be downloaded when a proxy is used.
With the Fork from @BlackDark i have no problem to download the binaries with active proxy.

duxbuse · 2021-11-15T03:36:13Z

+1 please merge this

jpogran · 2021-11-18T15:01:16Z

Hi @BlackDark, apologies for the delay in response. Thank you for the contribution! This looks like a good addition but there are a few things to get sorted first.

We had to update openpgp to latest stable in another PR, but that has made conflicting changes for you. Can you please rebase on main and fold those in?

It's hard to tell what changes are necessary to get proxy support added when you're reformatted the entire file. Can you please walk back the formatting changes so we can review this PR?

BlackDark · 2021-11-19T08:12:34Z

Hi @BlackDark, apologies for the delay in response. Thank you for the contribution! This looks like a good addition but there are a few things to get sorted first.

We had to update openpgp to latest stable in another PR, but that has made conflicting changes for you. Can you please rebase on main and fold those in?

It's hard to tell what changes are necessary to get proxy support added when you're reformatted the entire file. Can you please walk back the formatting changes so we can review this PR?

@jpogran Rebased and cleanup up commit

jpogran

Minor request not to print proxy details to the console. Going to look into how this library is handling errors versus the presentation here.

jpogran · 2021-11-19T20:14:22Z

src/utils.ts

+  console.log(`Found proxy setting. Using to download files.`);
+  console.log(`HTTP: ${httpProxy}`);
+  console.log(`HTTPS: ${httpProxy}`);


Suggested change

console.log(`Found proxy setting. Using to download files.`);

console.log(`HTTP: ${httpProxy}`);

console.log(`HTTPS: ${httpProxy}`);

We shouldn't be printing to the console proxy details

jpogran · 2021-11-19T20:14:44Z

src/utils.ts

+  url: string,
+  options: AxiosRequestConfig = {}
+): Promise<any> {
+  console.log(`Request to: ${url} with options: ${JSON.stringify(options)}`);


Suggested change

console.log(`Request to: ${url} with options: ${JSON.stringify(options)}`);

We shouldn't be printing to the console proxy details

What would be the best solution to add debug functionality to check if the correct proxy is used?
The default set up job (on Github Enterprise) also prints proxy configs if setup:

This is a good question, and is something that has some variance in implementations. In this case we expect the application using this library to output information like this, not the library itself.

Logging information is an important task of any library, but there isn't a way in this library to currently log anything. A future PR could introduce an interface and method overload that allows logging to different targets, but this is out of scope here.

jpogran · 2021-11-19T20:19:44Z

src/utils.ts

+    console.error(e.toJSON());
+    throw new Error(`Request for terraform binary failed: ${e && e.message}`);


Going to ask internally about the usage for this library, but I think we should follow the previous pattern of the resolve/reject with promises instead of try/catch and printing a message to the console.

Reviewed with the team and we agree the approach here is good with the exception of catching the error here. We shouldn't print the error to the console and throw a new formatted here.

This library is used for more than downloading terraform, so instead we should let the caller handle the exception.

Remove the try catch so the caller must handle the error

jpogran · 2021-11-19T20:25:49Z

Looking good so far. If you could add a short note to the README about the environment variables needed in order to activate this functionality I'd appreciate it. If not I can do that as a follow up.

BlackDark · 2021-12-07T09:55:03Z

Looking good so far. If you could add a short note to the README about the environment variables needed in order to activate this functionality I'd appreciate it. If not I can do that as a follow up.

Hi @jpogran found some time working on my PR. Updated your comments as suggested and updated the README.

dbanck · 2021-12-10T16:01:48Z

Thanks a lot for the updates.

I noticed one thing when running npm install.

found 8 vulnerabilities (6 moderate, 1 high, 1 critical)
  run `npm audit fix` to fix them, or `npm audit` for details

Most of the vulnerabilities are related to dev packages and not part of this PR, but the critical one is. It was introduced by a dependency of proxy-agent and can be fixed by running npm update vm2 --depth 5.

Do you mind updating the deps accordingly?

BlackDark · 2021-12-13T15:23:31Z

npm update vm2 --depth 5

Updated the dependency :)

jpogran

Thank you for contributing this!

jpogran · 2022-02-01T13:19:52Z

Apologies on the delay, merging this now!

magnetikonline · 2022-04-29T05:47:51Z

Hey @BlackDark / @jpogran - I've been tracking an annoying deprecation warning I've been seeing in the https://github.com/hashicorp/setup-terraform GitHub Action recently.

(node:1617) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
(Use `node --trace-deprecation ...` to show where the warning was created)

This is actually seen in the opening screenshot 😄.

Tracking this back, it's been this change that's cause the message:

Inclusion of proxy-agent package.
This in-turn uses pac-proxy-agent
This in-turn uses get-uri.
This in-turn uses ftp (phew!)
The (very old) ftp package makes use of new Buffer(): FIXED: use Buffer.from(string) instead of Buffer(string) mscdex/node-ftp#230

Got there!

I wonder if there is an alternative to the proxy-agent package?

jpogran · 2022-04-29T13:04:13Z

@magnetikonline I've created a new issue for request with the content you've posted. We'll look into seeing what can be done about this..

Next time, you can open a new issue and link to the PR in question instead of posting to a closed PR. Not all projects keep tabs on closed PRs.

magnetikonline · 2022-04-29T13:45:56Z

Next time, you can open a new issue and link to the PR in question instead of posting to a closed PR. Not all projects keep tabs on closed PRs.

Fair call @jpogran - sorry about that.

BlackDark force-pushed the feat/proxy-implementation branch from 47db578 to 472f491 Compare November 19, 2021 08:07

feat: implement proxy usage

3d16347

BlackDark force-pushed the feat/proxy-implementation branch from 472f491 to 3d16347 Compare November 19, 2021 08:11

jpogran requested changes Nov 19, 2021

View reviewed changes

BlackDark added 2 commits December 7, 2021 10:47

fix: PR comment changes

ca6351f

docs: update README

9dc84d0

fix: update nested dep with critical vulnerability

54894d8

jpogran approved these changes Feb 1, 2022

View reviewed changes

jpogran merged commit 71512e5 into hashicorp:main Feb 1, 2022

dbanck mentioned this pull request Mar 14, 2022

Fix download and verify #27

Merged

jpogran mentioned this pull request Apr 29, 2022

[DEP0005] DeprecationWarning: Buffer() is deprecated #42

Closed

BlackDark deleted the feat/proxy-implementation branch August 22, 2022 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement proxy usage #11

feat: implement proxy usage #11

BlackDark commented Aug 18, 2021 •

edited

Loading

hashicorp-cla commented Aug 18, 2021 •

edited

Loading

bahag-welscha commented Sep 28, 2021

duxbuse commented Nov 15, 2021

jpogran commented Nov 18, 2021

BlackDark commented Nov 19, 2021

jpogran left a comment

jpogran Nov 19, 2021

BlackDark Dec 7, 2021

jpogran Nov 19, 2021

BlackDark Nov 22, 2021

jpogran Nov 29, 2021

BlackDark Dec 7, 2021

jpogran Nov 19, 2021

jpogran Nov 29, 2021

BlackDark Dec 7, 2021

jpogran commented Nov 19, 2021

BlackDark commented Dec 7, 2021

dbanck commented Dec 10, 2021

BlackDark commented Dec 13, 2021

jpogran left a comment

jpogran commented Feb 1, 2022

magnetikonline commented Apr 29, 2022

jpogran commented Apr 29, 2022

magnetikonline commented Apr 29, 2022

	console.log(`Found proxy setting. Using to download files.`);
	console.log(`HTTP: ${httpProxy}`);
	console.log(`HTTPS: ${httpProxy}`);

		console.error(e.toJSON());
		throw new Error(`Request for terraform binary failed: ${e && e.message}`);

feat: implement proxy usage #11

feat: implement proxy usage #11

Conversation

BlackDark commented Aug 18, 2021 • edited Loading

hashicorp-cla commented Aug 18, 2021 • edited Loading

bahag-welscha commented Sep 28, 2021

duxbuse commented Nov 15, 2021

jpogran commented Nov 18, 2021

BlackDark commented Nov 19, 2021

jpogran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpogran commented Nov 19, 2021

BlackDark commented Dec 7, 2021

dbanck commented Dec 10, 2021

BlackDark commented Dec 13, 2021

jpogran left a comment

Choose a reason for hiding this comment

jpogran commented Feb 1, 2022

magnetikonline commented Apr 29, 2022

jpogran commented Apr 29, 2022

magnetikonline commented Apr 29, 2022

BlackDark commented Aug 18, 2021 •

edited

Loading

hashicorp-cla commented Aug 18, 2021 •

edited

Loading