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

no_proxy doesn't appear to be honoured #1267

Closed
codefish1 opened this issue Oct 12, 2022 · 11 comments · Fixed by #1269
Closed

no_proxy doesn't appear to be honoured #1267

codefish1 opened this issue Oct 12, 2022 · 11 comments · Fixed by #1269

Comments

@codefish1
Copy link

Subject of the issue

We are using Github Enterprise with self hosted runners running behind a cooperate web proxy.

The no_proxy value is set, to include the specific host eg github.foo.com and also a .foo.com

Steps to reproduce

I can't share the workflow, but use the action behind a proxy with the following set for the whole runner:
HTTPS_PROXY
HTTP_PROXY
NO_PROXY

@peter-evans
Copy link
Owner

Hi @codefish1

Support for no_proxy was added in this PR: #1205

The current implementation doesn't support wildcards, so I don't think .foo.com will work. You need to provide full hostnames. Feel free to contribute an implementation.

@codefish1
Copy link
Author

codefish1 commented Oct 14, 2022

How come it's not using the default proxy-agent as documented in https://github.com/octokit/octokit.js/#proxy-servers-nodejs-only which makes use of the standard environment variables?

I can make a change but I need to understand how to test the change correctly

@peter-evans
Copy link
Owner

Hi @codefish1

Originally, I followed the advice here: octokit/octokit.js#2098 (comment)

But it now seems there is a better way to handle this and the new advice has been updated in the docs that you linked. I'm going to try changing the implementation so that the proxy environment variables are handled automatically.

@peter-evans
Copy link
Owner

@codefish1 It would be great if you could test the new implementation and check that it works well for you.

You can test it by changing the action version to proxy.

      - uses: peter-evans/create-pull-request@proxy

@codefish1
Copy link
Author

@peter-evans I've tested it (commented on the PR) and it all works fine. I can also see where I went wrong when I tried it (didn't import the library in package.json) so thanks for helping me learn something new

@peter-evans
Copy link
Owner

@codefish1 Thank you for testing it!

Released as v4.2.0 / v4

@peter-evans
Copy link
Owner

Hi @codefish1

After adding this proxy implementation it became clear that it added a huge chain of dependencies and a lot of functionality that this action doesn't need. It's also caused this warning because some of the dependencies in the chain are very old and perhaps even unmaintained.

I've come up with another implementation that is a lot lighter. It uses the same code under-the-hood as ProxyAgent to fetch and parse the environment variables, so hopefully you won't notice any difference.

If you have chance, it would be great if you could test the new implementation. It's in the v5-beta branch because I'm planning to release it in a new major version. Please let me know if you have any issues. Feel free to use v5-beta in active workflows, I will keep that version available and make sure there is plenty of warning before it's deleted after the v5 release.

      - uses: peter-evans/create-pull-request@v5-beta

@codefish1
Copy link
Author

Sure, I'll make some time to look at it later this week. Unfortunately I'm off sick with COVID atm

@codefish1
Copy link
Author

Tested it just not and it all appears to be working fine

@peter-evans
Copy link
Owner

Great. Thank you for testing!

@peter-evans
Copy link
Owner

@codefish1 v5.0.0 / v5 is now released.

Please update to v5. I will remove pre-release versions after 4 weeks. Thank you for testing the pre-release version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment