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.

Sign up for GitHub

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

Jump to bottom

Allow Setting Proxy without Using HTTP_PROXY, etc. #180

Merged
merged 10 commits into from
Jan 30, 2020
Merged

Allow Setting Proxy without Using HTTP_PROXY, etc. #180

merged 10 commits into from
Jan 30, 2020

Conversation

KingAlex42
Copy link
Contributor

@KingAlex42 KingAlex42 commented Jul 31, 2019 •
edited by zippolyte
Loading

Right now, the only way to use a proxy to send stuff up to Datadog is to set the http_proxy environment variable. For my use case, I'd rather not run my application using this library with that variable set, so this PR allows setting a different environment variable, dd_proxy_http for configuring a proxy specifically for hitting the Datadog api.

@AlexanderAliotoTR
Added more options for setting proxy
5e0be34 
* The dogapi now allows setting env variables such as DOGAPI_HTTP_PROXY instead
of only allow HTTP_PROXY and the like
@AlexanderAliotoTR
incrementing version tag
1a75c67
@AlexanderAliotoTR
Revert "incrementing version tag"
ca8d35d 
This reverts commit 1a75c67.
@AlexanderAliotoTR
Removed an unused requires statement
0deebb3
@hithwen hithwen requested a review from gzussa August 26, 2019 12:05
@gzussa gzussa requested review from a team and removed request for gzussa August 27, 2019 12:42
@github-actions
Copy link

github-actions bot commented Jan 4, 2020

This issue has been automatically marked as stale because it has not had activity in the last 30 days.
Note that the issue will not be automatically closed, but this notification will remind us to investigate why there's been inactivity.

@github-actions github-actions bot added the stale Stale - Bot reminder label Jan 4, 2020
@github-actions github-actions bot closed this Jan 11, 2020
@KingAlex42
Copy link
Contributor Author

I assume this has been automatically closed due to inactivity?

If not, an explanation would be nice as my team would love to see this or something similar get merged in so we don't have to continue maintaining a fork to use this gem.

@gzussa gzussa reopened this Jan 14, 2020
@gzussa gzussa requested a review from dabcoder January 14, 2020 09:24
@dabcoder
Copy link
Contributor

I assume this has been automatically closed due to inactivity?

Indeed, apologies for that. We've now updated the closing issue/PR settings with a much longer timeframe so that it does not happen in the future.
@KingAlex42 we're reviewing your PR (thanks for your contribution!) and will get back to you.

dabcoder
dabcoder reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@dabcoder dabcoder left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left notes (thanks for adding tests by the way), let me know if I've understood your use case, thanks!

lib/dogapi/common.rb Outdated Show resolved Hide resolved
spec/unit/common_spec.rb Outdated Show resolved Hide resolved
spec/unit/common_spec.rb Outdated Show resolved Hide resolved
@gzussa gzussa added changelog/Added Added features results into a minor version bump kind/feature-request Feature request related issue and removed stale Stale - Bot reminder labels Jan 16, 2020
@gzussa gzussa requested a review from a team January 16, 2020 13:27
@KingAlex42
Copy link
Contributor Author

I've gone ahead and made your suggested changes. The variables are now of the form DD_PROXY_HTTP(S).

I think you got the gist of the use case; the idea was just to have some way of setting the proxies without having to set HTTP(S)_PROXY which could potentially cause other parts of my application to use the same proxy, which I don't want to happen.

jirikuncar
jirikuncar previously approved these changes Jan 24, 2020
View reviewed changes
jirikuncar
jirikuncar suggested changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@jirikuncar jirikuncar left a comment

Choose a reason for hiding this comment

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

I would suggest to keep the HTTP(S)_PROXY name and only add a DD_ prefix.

jirikuncar
jirikuncar previously approved these changes Jan 28, 2020
View reviewed changes
zippolyte
zippolyte reviewed Jan 30, 2020
View reviewed changes
lib/dogapi/common.rb
@@ -211,6 +210,12 @@ def Dogapi.find_localhost
@@hostname = Addrinfo.getaddrinfo(Socket.gethostname, nil, nil, nil, nil, Socket::AI_CANONNAME).first.canonname
end

def Dogapi.find_proxy
ENV['DD_PROXY_HTTPS'] || ENV['dd_proxy_https'] ||
ENV['DD_PROXY_HTTP'] || ENV['dd_proxy_http'] ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ENV['DD_PROXY_HTTP'] || ENV['dd_proxy_http'] ||
ENV['DD_PROXY_HTTP'] || ENV['dd_proxy_http'] ||

zippolyte
zippolyte reviewed Jan 30, 2020
View reviewed changes
lib/dogapi/common.rb
def Dogapi.find_proxy
ENV['DD_PROXY_HTTPS'] || ENV['dd_proxy_https'] ||
ENV['DD_PROXY_HTTP'] || ENV['dd_proxy_http'] ||
ENV['HTTPS_PROXY'] || ENV['https_proxy'] || ENV['HTTP_PROXY'] || ENV['http_proxy']
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ENV['HTTPS_PROXY'] || ENV['https_proxy'] || ENV['HTTP_PROXY'] || ENV['http_proxy']
ENV['HTTPS_PROXY'] || ENV['https_proxy'] || ENV['HTTP_PROXY'] || ENV['http_proxy']

@zippolyte zippolyte merged commit 39599ca into DataDog:master Jan 30, 2020
@zippolyte zippolyte mentioned this pull request Jan 30, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zippolyte zippolyte zippolyte approved these changes

@dabcoder dabcoder dabcoder left review comments

@jirikuncar jirikuncar jirikuncar left review comments

Assignees
No one assigned
Labels
changelog/Added Added features results into a minor version bump kind/feature-request Feature request related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@KingAlex42 @dabcoder @jirikuncar @zippolyte @gzussa @AlexanderAliotoTR