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

[http_check] Add allow_redirects parameter #2586

Merged
merged 1 commit into from Aug 11, 2016

Conversation

n0ts
Copy link
Contributor

@n0ts n0ts commented Jun 10, 2016

Hi, I add allow_redirects parameter http_check. Because I want to check http status only 301 or 302.

Regards,

@truthbk
Copy link
Member

truthbk commented Jun 13, 2016

Hi @n0ts thank you very much for this. This definitely looks like something we were missing. We'll review this in-depth soon (although it already looks good - thanks for the test), and we hope to merge it soon. Thanks!

@truthbk truthbk added this to the 5.9.0 milestone Jun 13, 2016
@@ -243,7 +244,8 @@ def _check(self, instance):
self.log.debug("Weak Ciphers will be used for {0}. Suppoted Cipherlist: {1}".format(
base_addr, WeakCiphersHTTPSConnection.SUPPORTED_CIPHERS))

r = sess.request('GET', addr, auth=auth, timeout=timeout, headers=headers, proxies=instance_proxy,
r = sess.request('GET', addr, auth=auth, timeout=timeout, headers=headers,
proxies=instance_proxy, allow_redirects=allow_redirects,
Copy link
Member

Choose a reason for hiding this comment

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

In requests allow_redirects defaults to True as well so 👍 for the the default value as it maintains backward compatibility.

@truthbk
Copy link
Member

truthbk commented Aug 11, 2016

CI failures are unrelated, and this looks good to me! Especially with that test you wrote @n0ts

Thank you so much! :shipit:

@truthbk truthbk merged commit d5ec6f6 into DataDog:master Aug 11, 2016
@n0ts n0ts deleted the topic-http-check-redirects branch August 19, 2016 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants