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

Adding proxy support for pagerDuty client #16

Merged
merged 6 commits into from
Apr 18, 2020

Conversation

bogdangherca
Copy link

@bogdangherca bogdangherca commented Nov 11, 2019

Hey folks,

I'm trying to use this awesome pagerDuty client behind a corporate proxy and I'm having the same issue as described here: #12

This PR introduces the ability to specify a proxy configuration when creating a PagerDutyEvents client. An alternative solution I've considered was to make the Unirest HTTP client automatically pick up system proxy env variables, but that might impact people already using this lib. Also, there are multiple env variables used for setting proxies since there is some convention around them, but not a standard (e.g. $http_proxy, $http.proxyHost)

Please take a look when you have a chance, I'm happy to address any of your comments here.

Thanks,
Bogdan.

@dikhan
Copy link
Owner

dikhan commented Mar 24, 2020

Hi @bogdangherca ,

Thanks for contributing to the repo, much appreciate it! And sorry for the delay it's taken to reply on this, I have been afk for a long time.

The build did not seem to pass so I took a look at it. After some reading, looks like there is an issue with Travis java builds and the Ubuntu distribution used by task. So created a PR #18 that seemed to fix that problem and after merging it to master I went ahead and rebased this branch with master to pick up the change. The build seems to be working again and looks like few tests are failing, I ran the tests locally and the tests are not passing either.

Are you able to run the tests successfully on your end? We need to fix the tests before the branch can be merged.

Thanks!

@dikhan
Copy link
Owner

dikhan commented Apr 18, 2020

@bogdangherca thanks again for opening the PR. I pushed few more commits to fix the build and the tests seem to be passing now so merging the PR.

@dikhan dikhan merged commit 970f09c into dikhan:master Apr 18, 2020
@bogdangherca
Copy link
Author

@dikhan Thanks for taking the time to fix the build and merge this, highly appreciated!

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.

3 participants