Support 'verify' option #12

oliverlockwood · 2017-10-17T14:08:35Z

Closes #11.

I have tested this manually as well with a local install and it does what I was looking for.

@kopf What do you think?

coveralls · 2017-10-17T14:32:30Z

Coverage decreased (-2.4%) to 92.8% when pulling 8e9c9de on oliverlockwood:support-verify into 969f30c on kopf:master.

kopf · 2017-10-17T17:28:25Z

Thanks for the contribution! I'll do the code review in-line

kopf · 2017-10-17T17:29:09Z

httsleep/main.py

@@ -34,6 +34,9 @@ class HttSleeper(object):
                     function that takes the response as an argument returning True.
    :param auth: a (username, password) tuple for HTTP authentication.
    :param headers: a dict of HTTP headers.
+    :param verify: (optional) Either a boolean, in which case it controls whether we verify
+                   the server's TLS certificate, or a string, in which case it must be a path
+                   to a CA bundle to use. Defaults to ``True``.


It should default to True, but at the moment it defaults to None. Can you fix this please?

TL;DR: I copied this description from the requests library, which we delegate to; they default to None on the API, but then in the depths of the code they set verify to the actual default of True if not already set.

I'm inclined to stick with this description - and code setting! as-is, but happy to discuss further if you like?

References:

Description copied from: https://github.com/requests/requests/blob/master/requests/api.py#L39-L41 (or https://github.com/requests/requests/blob/master/requests/sessions.py#L474-L476, it's the same).

API defaulting to None in the API: https://github.com/requests/requests/blob/master/requests/sessions.py#L444.

Where they turn a None into True: https://github.com/requests/requests/blob/master/requests/sessions.py#L597 (with reference to https://github.com/requests/requests/blob/master/requests/sessions.py#L367-L368).

I still feel it'd be more developer-friendly (especially for those using IDEs) if the kwarg matched the documentation. I don't see any downsides to setting the default value to True from the get-go.

(Sorry, I added that comment weeks ago, but clicked the wrong button on github...)

@kopf it's your library 😃 I've changed this as requested in the most recent commit.

kopf · 2017-10-17T17:32:05Z

tests/test_run.py

@@ -23,6 +23,17 @@ def test_run_success():


 @httpretty.activate
+def test_run_success_with_verify():
+    """Should return response when a success criteria has been reached"""


This test doesn't really do much - it would be better if it were """Should tell requests to skip SSL verification if verify==False""", testing whether session.send() was called with the verify kwarg set to False.

Checking the default behaviour would be great too.

Agreed. I'll have a play with doing this tomorrow. Thought I wouldn't spend too much time on it in the first pass just in case the library was unmaintained (which sometimes happens in OSS; it's nice to have a response so quickly after raising the PR - thanks!)

I've put something in for this. Feel free to point out ways to do it better; Python isn't my main language so I'm sure it could be nicer!

kopf · 2017-10-17T17:33:47Z

httsleep/main.py

@@ -34,6 +34,9 @@ class HttSleeper(object):
                     function that takes the response as an argument returning True.
    :param auth: a (username, password) tuple for HTTP authentication.
    :param headers: a dict of HTTP headers.
+    :param verify: (optional) Either a boolean, in which case it controls whether we verify


At the moment, the other kwargs (e.g. auth and headers and so on) aren't marked with "(optional)". It'd be best to stick to that style and I'll take care of them all at once.

(Unless you'd like to change them all :) I agree, marking it explicitly in the documentation is better.)

How we handle this probably depends on the outcome of the other discussion thread, on the whole of this description being copied from the requests library. So I'll wait until you've had a chance to respond to that before I decide which way to go!

kopf · 2017-10-17T17:38:39Z

One last thing - it would be great if you could make an entry in the changelog for this. You can simply put it under the heading "Next Version" at the very top.

Thanks again!

coveralls · 2017-10-18T07:31:05Z

Coverage increased (+0.04%) to 95.2% when pulling 61ad4b6 on oliverlockwood:support-verify into 969f30c on kopf:master.

coveralls · 2017-10-18T07:33:46Z

Coverage increased (+0.04%) to 95.2% when pulling 841c275 on oliverlockwood:support-verify into 969f30c on kopf:master.

coveralls · 2017-10-18T07:34:55Z

Coverage increased (+0.04%) to 95.2% when pulling 0b3915f on oliverlockwood:support-verify into 969f30c on kopf:master.

kopf · 2017-10-19T18:46:53Z

tests/test_run.py

+        httsleep.run()
+        assert mock_session_send.called
+        args, kwargs = mock_session_send.call_args
+        assert kwargs.get('verify') is None


I'd prefer to keep as little within the mocked scope as possible. okResp and the asserts are a little unpythonic. Could you change these to:

def test_propagate_verify(): """Should tell requests to skip SSL verification if verify==False""" resp = Response() resp.status_code = 200 httsleep = HttSleeper(URL, {'status_code': 200}, verify=False) with mock.patch('requests.sessions.Session.send') as mock_session_send: mock_session_send.return_value = resp httsleep.run() assert mock_session_send.called args, kwargs = mock_session_send.call_args assert 'verify' in kwargs assert kwargs['verify'] == False @httpretty.activate def test_default_does_not_send_verify(): """Should not send a value for 'verify' to requests by default""" resp = Response() resp.status_code = 200 httsleep = HttSleeper(URL, {'status_code': 200}) with mock.patch('requests.sessions.Session.send') as mock_session_send: mock_session_send.return_value = resp httsleep.run() assert mock_session_send.called args, kwargs = mock_session_send.call_args assert 'verify' not in kwargs

Thanks!

These changes made. Thank you for the Pythonic education!

To satisfy the second test, I had to change the implementation to avoid it sending verify=None.

coveralls · 2017-10-26T08:17:19Z

Coverage increased (+0.1%) to 95.276% when pulling 0dab89b on oliverlockwood:support-verify into 969f30c on kopf:master.

coveralls · 2017-11-08T08:53:04Z

Coverage increased (+0.1%) to 95.276% when pulling d3a50da on oliverlockwood:support-verify into 969f30c on kopf:master.

coveralls · 2017-11-08T11:52:42Z

Coverage increased (+0.1%) to 95.276% when pulling 0308a51 on oliverlockwood:support-verify into 969f30c on kopf:master.

kopf · 2017-11-08T20:15:49Z

httsleep/main.py

-             auth=None, headers=None,
+def httsleep(url_or_request, until=None, alarms=None, status_code=None,
+             json=None, jsonpath=None, text=None, callback=None,
+             auth=None, headers=None, verify=None,


verify=True

kopf · 2017-11-08T20:17:41Z

httsleep/main.py

@@ -84,6 +87,9 @@ def __init__(self, url_or_request, until=None, alarms=None,
                         'jsonpath': jsonpath, 'text': text, 'callback': callback}
            until.append({k: v for k, v in condition.items() if v})

+        self.kwargs = {}
+        if verify is not None:


This will now never happen.. I'd be fine with simply always passing verify to requests as a kwarg on L154 (no need for self.kwargs, which is a little ambiguous)

kopf · 2017-11-08T20:18:06Z

Sorry that this is dragging on! I'm sure that was the last round of comments....

coveralls · 2017-11-08T22:33:05Z

Coverage increased (+0.04%) to 95.2% when pulling 988ad5d on oliverlockwood:support-verify into 969f30c on kopf:master.

oliverlockwood · 2017-11-08T22:38:47Z

@kopf no worries; I should have self-reviewed the last change better.

Hopefully with the commits I've just added, this is there now...!

kopf · 2017-11-09T06:43:27Z

Nice one! Thanks again for the contribution!

oliverlockwood · 2017-11-09T07:52:30Z

My pleasure, thanks for guiding me through it.

Support 'verify' option

4e9469e

oliverlockwood force-pushed the support-verify branch from f132c2d to 71e2090 Compare October 17, 2017 14:31

oliverlockwood force-pushed the support-verify branch 2 times, most recently from 3b972b3 to 8e9c9de Compare October 17, 2017 14:46

kopf reviewed Oct 17, 2017

View reviewed changes

Improve testing of 'verify' propagation

50a53e7

oliverlockwood force-pushed the support-verify branch from 841c275 to 0b3915f Compare October 18, 2017 07:33

Add entry to CHANGELOG and CONTRIBUTORS

91bedd6

kopf reviewed Oct 19, 2017

View reviewed changes

oliverlockwood added 2 commits October 26, 2017 09:01

Changes to tests as suggested by kopf

8b53998

Fix code not to send 'verify' unless defined

eecf3b6

oliverlockwood mentioned this pull request Oct 26, 2017

Support passing in a Session object #13

Closed

Default 'verify' kwarg to True, instead of None

0308a51

oliverlockwood force-pushed the support-verify branch from d3a50da to 0308a51 Compare November 8, 2017 11:51

kopf reviewed Nov 8, 2017

View reviewed changes

oliverlockwood added 2 commits November 8, 2017 22:25

Remove generic kwargs as they are no longer necessary

ed5603d

Remove (optional) marker on param description

0d4ac8e

Fix init test

988ad5d

kopf merged commit 1a5e76d into kopf:master Nov 9, 2017

oliverlockwood deleted the support-verify branch November 9, 2017 07:52

oliverlockwood mentioned this pull request Nov 9, 2017

Support the use of a Session object #14

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'verify' option #12

Support 'verify' option #12

oliverlockwood commented Oct 17, 2017 •

edited

Loading

coveralls commented Oct 17, 2017 •

edited

Loading

kopf commented Oct 17, 2017

kopf Oct 17, 2017

oliverlockwood Oct 17, 2017

kopf Nov 5, 2017

kopf Nov 5, 2017

oliverlockwood Nov 8, 2017

kopf Oct 17, 2017

oliverlockwood Oct 17, 2017

oliverlockwood Oct 18, 2017

kopf Oct 17, 2017

oliverlockwood Oct 17, 2017

kopf commented Oct 17, 2017

coveralls commented Oct 18, 2017 •

edited

Loading

coveralls commented Oct 18, 2017 •

edited

Loading

coveralls commented Oct 18, 2017 •

edited

Loading

kopf Oct 19, 2017

oliverlockwood Oct 26, 2017

coveralls commented Oct 26, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

kopf Nov 8, 2017

kopf Nov 8, 2017

kopf commented Nov 8, 2017

coveralls commented Nov 8, 2017 •

edited

Loading

oliverlockwood commented Nov 8, 2017

kopf commented Nov 9, 2017

oliverlockwood commented Nov 9, 2017

Support 'verify' option #12

Support 'verify' option #12

Conversation

oliverlockwood commented Oct 17, 2017 • edited Loading

coveralls commented Oct 17, 2017 • edited Loading

kopf commented Oct 17, 2017

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

kopf commented Oct 17, 2017

coveralls commented Oct 18, 2017 • edited Loading

coveralls commented Oct 18, 2017 • edited Loading

coveralls commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 26, 2017 • edited Loading

coveralls commented Nov 8, 2017 • edited Loading

coveralls commented Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kopf commented Nov 8, 2017

coveralls commented Nov 8, 2017 • edited Loading

oliverlockwood commented Nov 8, 2017

kopf commented Nov 9, 2017

oliverlockwood commented Nov 9, 2017

oliverlockwood commented Oct 17, 2017 •

edited

Loading

coveralls commented Oct 17, 2017 •

edited

Loading

coveralls commented Oct 18, 2017 •

edited

Loading

coveralls commented Oct 18, 2017 •

edited

Loading

coveralls commented Oct 18, 2017 •

edited

Loading

coveralls commented Oct 26, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading