)]}'
{"jenkins/__init__.py":[{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":469,"context_line":"            if response is None:"},{"line_number":470,"context_line":"                raise EmptyResponseException("},{"line_number":471,"context_line":"                    \"Error communicating with server[%s]: \""},{"line_number":472,"context_line":"                    \"empty response\" % self.server)"},{"line_number":473,"context_line":""},{"line_number":474,"context_line":"            if six.PY2:"},{"line_number":475,"context_line":"                return response.info().getheader(\u0027X-Jenkins\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_43a5e801","side":"PARENT","line":472,"updated":"2015-08-11 11:36:36.000000000","message":"Not raised by the handler.","commit_id":"35a5938d0f179942e52cf099b6eda3057702f239"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":53,"context_line":"import warnings"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"import requests"},{"line_number":56,"context_line":"import requests.exceptions as req_exc"},{"line_number":57,"context_line":"import six"},{"line_number":58,"context_line":"from six.moves.http_client import BadStatusLine"},{"line_number":59,"context_line":"from six.moves.urllib.error import URLError"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_835e90e7","line":56,"updated":"2015-08-11 11:36:36.000000000","message":"I\u0027ve found it a little easier to follow by ensuring it\u0027s clear where the exceptions are coming from instead of previously just having the class name as sometimes it wasn\u0027t obvious if it was a python-jenkins or a library exception.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":300,"context_line":"            response \u003d self.jenkins_open(requests.Request("},{"line_number":301,"context_line":"                \u0027GET\u0027, self._build_url(JOB_NAME, locals())"},{"line_number":302,"context_line":"            ))"},{"line_number":303,"context_line":"        except (req_exc.HTTPError, NotFoundException):"},{"line_number":304,"context_line":"            return None"},{"line_number":305,"context_line":"        else:"},{"line_number":306,"context_line":"            actual \u003d json.loads(response)[\u0027name\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_83a1b0ab","line":303,"updated":"2015-08-11 11:36:36.000000000","message":"Oops, this is a mistake, should only return None for the NotFoundException.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"06c95ae817c9f35d06f521cbca6e52b7921d1135","unresolved":false,"context_lines":[{"line_number":355,"context_line":"        except req_exc.HTTPError as e:"},{"line_number":356,"context_line":"            # Jenkins\u0027s funky authentication means its nigh impossible to"},{"line_number":357,"context_line":"            # distinguish errors."},{"line_number":358,"context_line":"            if e.response.status_code in [401, 403, 500]:"},{"line_number":359,"context_line":"                # six.moves.urllib.error.req_exc.HTTPError provides a \u0027reason\u0027"},{"line_number":360,"context_line":"                # attribute for all python version except for ver 2.6"},{"line_number":361,"context_line":"                # Falling back to req_exc.HTTPError.msg since it contains the"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_590b7e24","line":358,"updated":"2015-08-11 17:13:27.000000000","message":"For a first-cut conversion like this, doing this in an exception handler makes sense, but it might be worth considering handling the response codes directly, *then* calling raise_for_status() (and doing the empty body check)…","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"af44de0bdc54689feb58f39b363f437eb5b32744","unresolved":false,"context_lines":[{"line_number":355,"context_line":"        except req_exc.HTTPError as e:"},{"line_number":356,"context_line":"            # Jenkins\u0027s funky authentication means its nigh impossible to"},{"line_number":357,"context_line":"            # distinguish errors."},{"line_number":358,"context_line":"            if e.response.status_code in [401, 403, 500]:"},{"line_number":359,"context_line":"                # six.moves.urllib.error.req_exc.HTTPError provides a \u0027reason\u0027"},{"line_number":360,"context_line":"                # attribute for all python version except for ver 2.6"},{"line_number":361,"context_line":"                # Falling back to req_exc.HTTPError.msg since it contains the"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_225f3f10","line":358,"in_reply_to":"1a4dcd0f_590b7e24","updated":"2015-08-12 14:54:29.000000000","message":"Agreed, probably also the correct next step before implementing support for custom response handlers.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"af44de0bdc54689feb58f39b363f437eb5b32744","unresolved":false,"context_lines":[{"line_number":359,"context_line":"                # six.moves.urllib.error.req_exc.HTTPError provides a \u0027reason\u0027"},{"line_number":360,"context_line":"                # attribute for all python version except for ver 2.6"},{"line_number":361,"context_line":"                # Falling back to req_exc.HTTPError.msg since it contains the"},{"line_number":362,"context_line":"                # same info as reason"},{"line_number":363,"context_line":"                raise JenkinsException("},{"line_number":364,"context_line":"                    \u0027Error in request. \u0027 +"},{"line_number":365,"context_line":"                    \u0027Possibly authentication failed [%s]: %s\u0027 % ("}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_a2534f15","line":362,"updated":"2015-08-12 14:54:29.000000000","message":"Guess this comment is no longer relevant either.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":405,"context_line":"            else:"},{"line_number":406,"context_line":"                raise JenkinsException(\u0027job[%s] number[%d] does not exist\u0027"},{"line_number":407,"context_line":"                                       % (name, number))"},{"line_number":408,"context_line":"        except (req_exc.HTTPError, NotFoundException):"},{"line_number":409,"context_line":"            raise JenkinsException(\u0027job[%s] number[%d] does not exist\u0027"},{"line_number":410,"context_line":"                                   % (name, number))"},{"line_number":411,"context_line":"        except ValueError:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_e3719445","line":408,"updated":"2015-08-11 11:36:36.000000000","message":"I suspect we should only catch the NotFoundException here and allow any other HTTPError from requests to propagate.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":467,"context_line":"            ))"},{"line_number":468,"context_line":"        except (req_exc.HTTPError, BadStatusLine):"},{"line_number":469,"context_line":"            raise BadHTTPException(\"Error communicating with server[%s]\""},{"line_number":470,"context_line":"                                   % self.server)"},{"line_number":471,"context_line":"        except ValueError:"},{"line_number":472,"context_line":"            raise JenkinsException(\"Could not parse JSON info for server[%s]\""},{"line_number":473,"context_line":"                                   % self.server)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_03de006d","line":470,"updated":"2015-08-11 11:36:36.000000000","message":"I suspect we could actually skip handling many of these errors here now and delegate to the _response_handler method instead, but as a first pass I didn\u0027t want to deviate too much from the existing code/behaviour.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":492,"context_line":""},{"line_number":493,"context_line":"            return response.headers[\u0027X-Jenkins\u0027]"},{"line_number":494,"context_line":""},{"line_number":495,"context_line":"        except (req_exc.HTTPError, BadStatusLine):"},{"line_number":496,"context_line":"            raise BadHTTPException(\"Error communicating with server[%s]\""},{"line_number":497,"context_line":"                                   % self.server)"},{"line_number":498,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_a385cc59","line":495,"updated":"2015-08-11 11:36:36.000000000","message":"I suspect we could actually skip handling many of these errors here now and delegate to the _response_handler method instead, but as a first pass I didn\u0027t want to deviate too much from the existing code/behaviour.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":793,"context_line":"                return json.loads(response)"},{"line_number":794,"context_line":"            else:"},{"line_number":795,"context_line":"                raise JenkinsException(\u0027node[%s] does not exist\u0027 % name)"},{"line_number":796,"context_line":"        except (req_exc.HTTPError, NotFoundException):"},{"line_number":797,"context_line":"            raise JenkinsException(\u0027node[%s] does not exist\u0027 % name)"},{"line_number":798,"context_line":"        except ValueError:"},{"line_number":799,"context_line":"            raise JenkinsException(\"Could not parse JSON info for node[%s]\""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_838f7022","line":796,"updated":"2015-08-11 11:36:36.000000000","message":"Perhaps we should just catch NotFoundException?","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":949,"context_line":"            else:"},{"line_number":950,"context_line":"                raise JenkinsException(\u0027job[%s] number[%d] does not exist\u0027"},{"line_number":951,"context_line":"                                       % (name, number))"},{"line_number":952,"context_line":"        except (req_exc.HTTPError, NotFoundException):"},{"line_number":953,"context_line":"            raise JenkinsException(\u0027job[%s] number[%d] does not exist\u0027"},{"line_number":954,"context_line":"                                   % (name, number))"},{"line_number":955,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_03a4c0a3","line":952,"updated":"2015-08-11 11:36:36.000000000","message":"Maybe just catch NotFoundException?","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"7969577701df60a4cc48b5b6cf793b9f684f39c2","unresolved":false,"context_lines":[{"line_number":966,"context_line":"        try:"},{"line_number":967,"context_line":"            response \u003d self.jenkins_open(requests.Request("},{"line_number":968,"context_line":"                \u0027GET\u0027, self._build_url(VIEW_NAME, locals())))"},{"line_number":969,"context_line":"        except (req_exc.HTTPError, NotFoundException):"},{"line_number":970,"context_line":"            return None"},{"line_number":971,"context_line":"        else:"},{"line_number":972,"context_line":"            actual \u003d json.loads(response)[\u0027name\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_c3ad7889","line":969,"updated":"2015-08-11 11:36:36.000000000","message":"Oops, this should definitely only catch the NotFoundException.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"3628abca580e0ff370f76022aaf08b57ad3a6af7","unresolved":false,"context_lines":[{"line_number":253,"context_line":"            self.server \u003d url + \u0027/\u0027"},{"line_number":254,"context_line":"        if username is not None and password is not None:"},{"line_number":255,"context_line":"            if requests_kerberos is not None:"},{"line_number":256,"context_line":"                self.auth \u003d requests_kerberos.HTTPKerberosAuth()"},{"line_number":257,"context_line":"            else:"},{"line_number":258,"context_line":"                self.auth \u003d requests.auth.HTTPBasicAuth("},{"line_number":259,"context_line":"                    username.encode(\u0027utf-8\u0027), password.encode(\u0027utf-8\u0027))"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_b4e7c8e4","line":256,"updated":"2018-01-05 17:11:48.000000000","message":"This really doesn\u0027t look right to me.  We\u0027re basing the decision on whether to go with Kerberos authentication solely on the presence of the requests_kerberos module; so, if someone has requests_kerberos installed for some other reason, they\u0027re forced to use Kerberos authentication even if their target Jenkins isn\u0027t actually set up to support it.  I\u0027d say we really need to have another argument to the constructor to control whether we use Kerberos or basic authentication.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"536c4275d5598f48af26921bfbc76ab6d14be76b","unresolved":false,"context_lines":[{"line_number":253,"context_line":"            self.server \u003d url + \u0027/\u0027"},{"line_number":254,"context_line":"        if username is not None and password is not None:"},{"line_number":255,"context_line":"            if requests_kerberos is not None:"},{"line_number":256,"context_line":"                self.auth \u003d requests_kerberos.HTTPKerberosAuth()"},{"line_number":257,"context_line":"            else:"},{"line_number":258,"context_line":"                self.auth \u003d requests.auth.HTTPBasicAuth("},{"line_number":259,"context_line":"                    username.encode(\u0027utf-8\u0027), password.encode(\u0027utf-8\u0027))"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_ba356f48","line":256,"in_reply_to":"9f91af0f_b4e7c8e4","updated":"2018-01-05 18:30:53.000000000","message":"Is that any different to the decision being based on whether they have \u0027kerberos\u0027 installed with the existing codebase?\n\nI\u0027m happy to add another argument to the constructor, but is it ok to break with existing behaviour then by not automagically using kerberos if a particular library is present?\n\nActually looking closer at the existing code, what might need to be done is test if the kerberos authentication works, and then fall back to the BasicAuth if it doesn\u0027t, setting on the session then for all subsequent requests. But still this should probably only be done on the first requests.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"a4f34692a316ef626aa7820e91fa3aff3473fe13","unresolved":false,"context_lines":[{"line_number":253,"context_line":"            self.server \u003d url + \u0027/\u0027"},{"line_number":254,"context_line":"        if username is not None and password is not None:"},{"line_number":255,"context_line":"            if requests_kerberos is not None:"},{"line_number":256,"context_line":"                self.auth \u003d requests_kerberos.HTTPKerberosAuth()"},{"line_number":257,"context_line":"            else:"},{"line_number":258,"context_line":"                self.auth \u003d requests.auth.HTTPBasicAuth("},{"line_number":259,"context_line":"                    username.encode(\u0027utf-8\u0027), password.encode(\u0027utf-8\u0027))"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_8e65d0e3","line":256,"in_reply_to":"9f91af0f_ba356f48","updated":"2018-01-05 18:45:54.000000000","message":"Yeah, sounds like we\u0027ve shot ourselves in the foot if the existing behavior is to go kerberos if a particular library is present.  Doing the fall-back to BasicAuth would be acceptable to me, though; that may be something we could do with a custom authentication type, btw.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"3628abca580e0ff370f76022aaf08b57ad3a6af7","unresolved":false,"context_lines":[{"line_number":261,"context_line":"            self.auth \u003d None"},{"line_number":262,"context_line":"        self.crumb \u003d None"},{"line_number":263,"context_line":"        self.timeout \u003d timeout"},{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_140a3489","line":264,"updated":"2018-01-05 17:11:48.000000000","message":"If we\u0027re allocating a session, I\u0027d argue that we should set the authentication on the session directly, rather than on each request, which is what we appear to be doing right now.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"aa6c768b9c1d3bf1e10832bb071ec7a44c281985","unresolved":false,"context_lines":[{"line_number":261,"context_line":"            self.auth \u003d None"},{"line_number":262,"context_line":"        self.crumb \u003d None"},{"line_number":263,"context_line":"        self.timeout \u003d timeout"},{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5f93b717_8c3c7d93","line":264,"in_reply_to":"5f93b717_31e586dc","updated":"2018-02-02 18:51:58.000000000","message":"Geez, having looked at that a fair bit, can understand how I missed that piece of info, thanks, will retest with that piece.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"c6c008787e28b17124283a86e044fb2458e7fb04","unresolved":false,"context_lines":[{"line_number":261,"context_line":"            self.auth \u003d None"},{"line_number":262,"context_line":"        self.crumb \u003d None"},{"line_number":263,"context_line":"        self.timeout \u003d timeout"},{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5f93b717_31e586dc","line":264,"in_reply_to":"5f93b717_9c011808","updated":"2018-02-02 18:26:16.000000000","message":"I actually wondered about that, because of how we\u0027re passing in Request rather than allocating one in the context of the session.  That might come back to the argument over whether to pass Request to jenkins_open() vs. having jenkins_open() make the appropriate request by calling, e.g., _session.get()/_session.post().\n\nOn another topic, I looked at the requests documentation on sessions[1], and it points out this same phenomenon, and says this: \"However, the above code will lose some of the advantages of having a Requests Session object. In particular, Session-level state such as cookies will not get applied to your request. To get a PreparedRequest with that state applied, replace the call to Request.prepare() with a call to Session.prepare_request()[.]\"\n\nBased on that, we might want to look at using something along the lines of:\n\n    self._session.prepare_request(req)\n\nbefore the rest of the logic in jenkins_open()\n\n[1] http://docs.python-requests.org/en/master/user/advanced/#session-objects","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"64f9bb1d144c3cb70955b81da6156b627b61b217","unresolved":false,"context_lines":[{"line_number":261,"context_line":"            self.auth \u003d None"},{"line_number":262,"context_line":"        self.crumb \u003d None"},{"line_number":263,"context_line":"        self.timeout \u003d timeout"},{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"}],"source_content_type":"text/x-python","patch_set":16,"id":"5f93b717_9c011808","line":264,"in_reply_to":"9f91af0f_140a3489","updated":"2018-02-02 12:36:35.000000000","message":"I\u0027ve tested this extensively now, and it doesn\u0027t appear to work the way you\u0027d expect. Setting self._session.auth to the desired value does not appear to get applied to a \u0027PreppedRequest\u0027 that is then sent via Session.send(...).\n\nYou can set defaults in the Session, but it seems it\u0027s limited to simple properties. I suspect the problem is that auth is a called class that will apply headers to the request rather than a attribute. If using simple values (user, pass) it apparently works, but I suspect it\u0027s because it then passes it to a default Basic auth class to apply the headers. Using one of the auth classes directly as we need to do, unfortunately doesn\u0027t work.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"536c4275d5598f48af26921bfbc76ab6d14be76b","unresolved":false,"context_lines":[{"line_number":261,"context_line":"            self.auth \u003d None"},{"line_number":262,"context_line":"        self.crumb \u003d None"},{"line_number":263,"context_line":"        self.timeout \u003d timeout"},{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_3ae5dfa8","line":264,"in_reply_to":"9f91af0f_140a3489","updated":"2018-01-05 18:30:53.000000000","message":"good idea","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"3628abca580e0ff370f76022aaf08b57ad3a6af7","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"},{"line_number":268,"context_line":"            if k in [\"name\", \"msg\", \"short_name\", \"from_short_name\","},{"line_number":269,"context_line":"                     \"to_short_name\", \"folder_url\", \"from_folder_url\", \"to_folder_url\"]:"},{"line_number":270,"context_line":"                params[k] \u003d quote(params[k])"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_54ef5caf","line":267,"updated":"2018-01-05 17:11:48.000000000","message":"There\u0027s no need to coerce the output of keys() to a list; regardless of Python version, the result will be an iterable, which we then iterate over.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"9891f69ba7cb7ea68dd59af944889f9671816138","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"},{"line_number":268,"context_line":"            if k in [\"name\", \"msg\", \"short_name\", \"from_short_name\","},{"line_number":269,"context_line":"                     \"to_short_name\", \"folder_url\", \"from_folder_url\", \"to_folder_url\"]:"},{"line_number":270,"context_line":"                params[k] \u003d quote(params[k])"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_ca02c93d","line":267,"in_reply_to":"9f91af0f_0e9f80b4","updated":"2018-01-08 11:16:15.000000000","message":"I don\u0027t think it\u0027s mutating them, but with python3 because params.keys(), params.values() \u0026 params.items() are all view objects, even replacing a key\u0027s value can result in a slightly different layout altering any existing views in a way where you could end up skipping an element you were previously going to iterate over.\n\nI\u0027ll test, but I suspect the reason I included the change here was because I was hitting this issue.\n\nSee https://stackoverflow.com/a/6777832 and https://cito.github.io/blog/never-iterate-a-changing-dict/\n\nOnce confirmed, I\u0027ll move it out to a separate patch in any case.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"a4f34692a316ef626aa7820e91fa3aff3473fe13","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"},{"line_number":268,"context_line":"            if k in [\"name\", \"msg\", \"short_name\", \"from_short_name\","},{"line_number":269,"context_line":"                     \"to_short_name\", \"folder_url\", \"from_folder_url\", \"to_folder_url\"]:"},{"line_number":270,"context_line":"                params[k] \u003d quote(params[k])"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_0e9f80b4","line":267,"in_reply_to":"9f91af0f_1af49b5a","updated":"2018-01-05 18:45:54.000000000","message":"Probably, but I don\u0027t see why we would mutate params if we\u0027re then going to return the mutated version.  I think we need to look closer at how this routine is used (I didn\u0027t during my previous review) and think about other ways of addressing the need…","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"536c4275d5598f48af26921bfbc76ab6d14be76b","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"},{"line_number":268,"context_line":"            if k in [\"name\", \"msg\", \"short_name\", \"from_short_name\","},{"line_number":269,"context_line":"                     \"to_short_name\", \"folder_url\", \"from_folder_url\", \"to_folder_url\"]:"},{"line_number":270,"context_line":"                params[k] \u003d quote(params[k])"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_1af49b5a","line":267,"in_reply_to":"9f91af0f_54ef5caf","updated":"2018-01-05 18:30:53.000000000","message":"good point, not sure why I did that, wonder if there is an issue if you update keys in a dict while iterating over it, I\u0027ve a feeling that that I was assuming that it could sometimes result in entries being skipped.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"718cfc9b2208b0c3300c9b240299afa73c08c51a","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"},{"line_number":268,"context_line":"            if k in [\"name\", \"msg\", \"short_name\", \"from_short_name\","},{"line_number":269,"context_line":"                     \"to_short_name\", \"folder_url\", \"from_folder_url\", \"to_folder_url\"]:"},{"line_number":270,"context_line":"                params[k] \u003d quote(params[k])"}],"source_content_type":"text/x-python","patch_set":16,"id":"7f96bb07_5cd01dca","line":267,"in_reply_to":"9f91af0f_9ef87a49","updated":"2018-01-22 19:02:03.000000000","message":"I\u0027ve reverted this piece temporarily, I think it\u0027s only encountered when running python 3.6, which means we\u0027ll need to fix at some point, but it\u0027s not part of this change.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"533dc47aad92dacd20cb5fbc9e7650a93e50db1e","unresolved":false,"context_lines":[{"line_number":264,"context_line":"        self._session \u003d requests.Session()"},{"line_number":265,"context_line":""},{"line_number":266,"context_line":"    def _get_encoded_params(self, params):"},{"line_number":267,"context_line":"        for k in list(params.keys()):"},{"line_number":268,"context_line":"            if k in [\"name\", \"msg\", \"short_name\", \"from_short_name\","},{"line_number":269,"context_line":"                     \"to_short_name\", \"folder_url\", \"from_folder_url\", \"to_folder_url\"]:"},{"line_number":270,"context_line":"                params[k] \u003d quote(params[k])"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_9ef87a49","line":267,"in_reply_to":"9f91af0f_ca02c93d","updated":"2018-01-08 16:57:52.000000000","message":"Yep, it makes sense to me that modifying the dict could make keys unstable.  I honestly wouldn\u0027t expect that to be a problem during iteration—the size of the dict isn\u0027t being altered—but I haven\u0027t looked at the details of how Python does dicts, so…","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"3628abca580e0ff370f76022aaf08b57ad3a6af7","unresolved":false,"context_lines":[{"line_number":267,"context_line":"        for k in list(params.keys()):"},{"line_number":268,"context_line":"            if k in [\"name\", \"msg\", \"short_name\", \"from_short_name\","},{"line_number":269,"context_line":"                     \"to_short_name\", \"folder_url\", \"from_folder_url\", \"to_folder_url\"]:"},{"line_number":270,"context_line":"                params[k] \u003d quote(params[k])"},{"line_number":271,"context_line":"        return params"},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"    def _build_url(self, format_spec, variables\u003dNone):"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_746dc030","line":270,"updated":"2018-01-05 17:11:48.000000000","message":"I\u0027m not sure I understand why we went from iterating over .items() to iterating over .keys() and getting the value from params.  If we\u0027re going to do that, then maybe we should change this whole thing to:\n\n    _valid_keys \u003d [\"name\", \"msg\", …]\n    …\n    def _get_encoded_params(self, params):\n        return {k: quote(params[k]) for k in self._valid_keys\n                if k in params}","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"536c4275d5598f48af26921bfbc76ab6d14be76b","unresolved":false,"context_lines":[{"line_number":267,"context_line":"        for k in list(params.keys()):"},{"line_number":268,"context_line":"            if k in [\"name\", \"msg\", \"short_name\", \"from_short_name\","},{"line_number":269,"context_line":"                     \"to_short_name\", \"folder_url\", \"from_folder_url\", \"to_folder_url\"]:"},{"line_number":270,"context_line":"                params[k] \u003d quote(params[k])"},{"line_number":271,"context_line":"        return params"},{"line_number":272,"context_line":""},{"line_number":273,"context_line":"    def _build_url(self, format_spec, variables\u003dNone):"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_1ac25bea","line":270,"in_reply_to":"9f91af0f_746dc030","updated":"2018-01-05 18:30:53.000000000","message":"I might drop this piece from the change anyway, as it shouldn\u0027t impact and we can update as needed in the future.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"3628abca580e0ff370f76022aaf08b57ad3a6af7","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        if self.crumb is None:"},{"line_number":285,"context_line":"            try:"},{"line_number":286,"context_line":"                response \u003d self.jenkins_open(requests.Request("},{"line_number":287,"context_line":"                    \u0027GET\u0027, self._build_url(CRUMB_URL)), add_crumb\u003dFalse)"},{"line_number":288,"context_line":"            except (NotFoundException, EmptyResponseException):"},{"line_number":289,"context_line":"                self.crumb \u003d False"},{"line_number":290,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_34817890","line":287,"updated":"2018-01-05 17:11:48.000000000","message":"We\u0027re using this idiom over and over again, where we call self.jenkins_open() on a requests.Request() that we create inline, or possibly before the jenkins_open() in some places.  I have to wonder if we could instead have jenkins_open() create the request for us?  Maybe that needs to be a future patch, since this is just converting from Request to requests.Request, but something to think about.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"533dc47aad92dacd20cb5fbc9e7650a93e50db1e","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        if self.crumb is None:"},{"line_number":285,"context_line":"            try:"},{"line_number":286,"context_line":"                response \u003d self.jenkins_open(requests.Request("},{"line_number":287,"context_line":"                    \u0027GET\u0027, self._build_url(CRUMB_URL)), add_crumb\u003dFalse)"},{"line_number":288,"context_line":"            except (NotFoundException, EmptyResponseException):"},{"line_number":289,"context_line":"                self.crumb \u003d False"},{"line_number":290,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_f9a200fc","line":287,"in_reply_to":"9f91af0f_0a38b13a","updated":"2018-01-08 16:57:52.000000000","message":"You have a good point about that.  The way I\u0027ve usually approached creating Python clients has been to create _get(), _post(), etc. methods that take a path and other data, then have those methods call the appropriate methods on the session created by the constructor…but that\u0027s hardly the only way to accomplish this task, and the jenkins_open() method is just as valid, and has the advantage of letting users set up their own calls.  Given how extensive the Jenkins API is, and how easily Jenkins plugins can add to it, sticking with the existing methodology makes sense.  I do have to point out that we are changing the interface, though, as now users have to allocate a requests.Request instead of a urllib Request.  That may require some additional communication and a version bump, if we truly consider jenkins_open() to be an exposed interface…","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"536c4275d5598f48af26921bfbc76ab6d14be76b","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        if self.crumb is None:"},{"line_number":285,"context_line":"            try:"},{"line_number":286,"context_line":"                response \u003d self.jenkins_open(requests.Request("},{"line_number":287,"context_line":"                    \u0027GET\u0027, self._build_url(CRUMB_URL)), add_crumb\u003dFalse)"},{"line_number":288,"context_line":"            except (NotFoundException, EmptyResponseException):"},{"line_number":289,"context_line":"                self.crumb \u003d False"},{"line_number":290,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_9a518b65","line":287,"in_reply_to":"9f91af0f_34817890","updated":"2018-01-05 18:30:53.000000000","message":"Possibly, though it would certainly complicate the jenkins_open call, and also change the the API, though maybe this change is sufficient that we would need a major version bump before releasing, in which case should definitely make the change before continuing.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"a4f34692a316ef626aa7820e91fa3aff3473fe13","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        if self.crumb is None:"},{"line_number":285,"context_line":"            try:"},{"line_number":286,"context_line":"                response \u003d self.jenkins_open(requests.Request("},{"line_number":287,"context_line":"                    \u0027GET\u0027, self._build_url(CRUMB_URL)), add_crumb\u003dFalse)"},{"line_number":288,"context_line":"            except (NotFoundException, EmptyResponseException):"},{"line_number":289,"context_line":"                self.crumb \u003d False"},{"line_number":290,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_aecc54bc","line":287,"in_reply_to":"9f91af0f_9a518b65","updated":"2018-01-05 18:45:54.000000000","message":"*nod* I\u0027d say we\u0027re making a pretty major change going to requests anyway, so we can probably take the opportunity to additionally clean up some API elements.","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"9891f69ba7cb7ea68dd59af944889f9671816138","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        if self.crumb is None:"},{"line_number":285,"context_line":"            try:"},{"line_number":286,"context_line":"                response \u003d self.jenkins_open(requests.Request("},{"line_number":287,"context_line":"                    \u0027GET\u0027, self._build_url(CRUMB_URL)), add_crumb\u003dFalse)"},{"line_number":288,"context_line":"            except (NotFoundException, EmptyResponseException):"},{"line_number":289,"context_line":"                self.crumb \u003d False"},{"line_number":290,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_0a38b13a","line":287,"in_reply_to":"9f91af0f_aecc54bc","updated":"2018-01-08 11:16:15.000000000","message":"Thinking about it over the weekend, the one advantage is that you can construct a requests object to have whatever arguments you need and pass it to jenkins_open(), without the function needing to know anything more than it is a requests object and whether to add the crumb or not.\n\nTo achieve the same flexibility we would eventually end up with a definition like the following:\n\n def jenkins_open(self, add_crumb\u003dTrue, *args, **kwargs):\n \n     req \u003d requests.Request(*args, **kwargs)\n\n     ....\n\n\nIs that better or worse than the current?","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"3628abca580e0ff370f76022aaf08b57ad3a6af7","unresolved":false,"context_lines":[{"line_number":445,"context_line":"            # distinguish errors."},{"line_number":446,"context_line":"            if e.response.status_code in [401, 403, 500]:"},{"line_number":447,"context_line":"                raise JenkinsException("},{"line_number":448,"context_line":"                    \u0027Error in request. \u0027 +"},{"line_number":449,"context_line":"                    \u0027Possibly authentication failed [%s]: %s\u0027 % ("},{"line_number":450,"context_line":"                        e.response.status_code, e.response.reason)"},{"line_number":451,"context_line":"                )"}],"source_content_type":"text/x-python","patch_set":16,"id":"9f91af0f_74db80ec","line":448,"updated":"2018-01-05 17:11:48.000000000","message":"I wonder if we could drop the extraneous \u0027+\u0027 operator here…","commit_id":"59bcc28f1d8de91b54ad5a7679f3232e56927b5c"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"718cfc9b2208b0c3300c9b240299afa73c08c51a","unresolved":false,"context_lines":[{"line_number":265,"context_line":"            self._auths.append(None)"},{"line_number":266,"context_line":""},{"line_number":267,"context_line":"        if requests_kerberos is not None:"},{"line_number":268,"context_line":"            self._auths.append(requests_kerberos.HTTPKerberosAuth())"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"        self.crumb \u003d None"},{"line_number":271,"context_line":"        self.timeout \u003d timeout"}],"source_content_type":"text/x-python","patch_set":17,"id":"7f96bb07_1cb13590","line":268,"updated":"2018-01-22 19:02:03.000000000","message":"The alternative to this is a custom auth class that tries kerberos first if available and then basic auth, but that seemed so messy that I defaulted to just trying one and then the other below, defaulting to the last authentication method.","commit_id":"42b08eeb7438651cab889f1c1e6b08cf36b72de1"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"718cfc9b2208b0c3300c9b240299afa73c08c51a","unresolved":false,"context_lines":[{"line_number":322,"context_line":"                    continue"},{"line_number":323,"context_line":"                except IndexError:"},{"line_number":324,"context_line":"                    break"},{"line_number":325,"context_line":"            self._session.auth \u003d auth"},{"line_number":326,"context_line":""},{"line_number":327,"context_line":"        self.auth \u003d self._session.auth"},{"line_number":328,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"7f96bb07_9c622516","line":325,"updated":"2018-01-22 19:02:03.000000000","message":"Is there any risk around multi threaded applications here?","commit_id":"42b08eeb7438651cab889f1c1e6b08cf36b72de1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"831c9fd0f75ab9dc1c843551a9faa55750da1af0","unresolved":false,"context_lines":[{"line_number":322,"context_line":"                    continue"},{"line_number":323,"context_line":"                except IndexError:"},{"line_number":324,"context_line":"                    break"},{"line_number":325,"context_line":"            self._session.auth \u003d auth"},{"line_number":326,"context_line":""},{"line_number":327,"context_line":"        self.auth \u003d self._session.auth"},{"line_number":328,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"7f96bb07_25941044","line":325,"in_reply_to":"7f96bb07_9c622516","updated":"2018-01-22 20:26:49.000000000","message":"TBH, I haven\u0027t been thinking about using this library in any kind of threaded way; I\u0027m not sure I\u0027d be comfortable with that.  Is there a risk?  Yes, multiple threads could invoke _maybe_add_auth() simultaneously.  That wouldn\u0027t be an issue for, say, maybe_add_crumb(), since it shouldn\u0027t matter which wins, but for authentication, it could be an issue.  The solution would be to lock around the method.\n\nMy quick perusal—which is emphatically not a formal analysis, and is based solely on the contents of __init__()—is that the authentication setup represented here is likely the only part of the class vulnerable to some sort of race condition.  That problem could be mitigated by moving the authentication into the __init__(), as discussed below (I\u0027m going through the comments in reverse order, FYI).  Another alternative *could* be to switch to a for loop on the contents of _auths—so it doesn\u0027t get modified.  That way, if a race condition does occur, both threads should come to the same conclusion and store the same auth into the session.  Finally, creating a specialized auth class for _session would move the threading race condition problem out of the Jenkins class and into that auth class.","commit_id":"42b08eeb7438651cab889f1c1e6b08cf36b72de1"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"718cfc9b2208b0c3300c9b240299afa73c08c51a","unresolved":false,"context_lines":[{"line_number":467,"context_line":"        \u0027\u0027\u0027"},{"line_number":468,"context_line":"        try:"},{"line_number":469,"context_line":"            if not self._auth_resolved:"},{"line_number":470,"context_line":"                self._maybe_add_auth()"},{"line_number":471,"context_line":"            if add_crumb:"},{"line_number":472,"context_line":"                self.maybe_add_crumb(req)"},{"line_number":473,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"7f96bb07_bcc041e2","line":470,"updated":"2018-01-22 19:02:03.000000000","message":"So this seems hacky, should I call this in the init? or as done here?\n\nDoing it here matches the existing behaviour, in which we try to resolve the authentication at the first request. Putting it in the init will mean an initial request will be made first.","commit_id":"42b08eeb7438651cab889f1c1e6b08cf36b72de1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"831c9fd0f75ab9dc1c843551a9faa55750da1af0","unresolved":false,"context_lines":[{"line_number":467,"context_line":"        \u0027\u0027\u0027"},{"line_number":468,"context_line":"        try:"},{"line_number":469,"context_line":"            if not self._auth_resolved:"},{"line_number":470,"context_line":"                self._maybe_add_auth()"},{"line_number":471,"context_line":"            if add_crumb:"},{"line_number":472,"context_line":"                self.maybe_add_crumb(req)"},{"line_number":473,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"7f96bb07_45c03c7d","line":470,"in_reply_to":"7f96bb07_bcc041e2","updated":"2018-01-22 20:26:49.000000000","message":"I don\u0027t have any problem with putting it in the init, personally; that would let us know as soon as possible whether authentication would be successful.  Of course, that is a change in behavior, and I suspect most clients delay the authentication step anyway.  (The ones that don\u0027t delay authentication probably need to use the provided credentials to get some sort of session token for the subsequent requests, but even there it\u0027s possible to delay.)\n\nThe only change I\u0027d suggest making—and this is totally optional, and probably something of a personal style thing—is to move the \"if not self._auth_resolved\" into the _maybe_add_auth() method, or move the \"self._auth_resolved \u003d True\" back to here after the call.  That way, everything that manipulates _auth_resolved is in one place, making it a bit easier to figure out what\u0027s going on.","commit_id":"42b08eeb7438651cab889f1c1e6b08cf36b72de1"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"4d292b7ebe253b6704f441eb25edb818324fdac4","unresolved":false,"context_lines":[{"line_number":314,"context_line":"            # This is a hack to allow the transparent use of kerberos to work"},{"line_number":315,"context_line":"            # in future, we should require explicit request to use kerberos"},{"line_number":316,"context_line":"            failures \u003d []"},{"line_number":317,"context_line":"            for name, auth in self._auths:"},{"line_number":318,"context_line":"                try:"},{"line_number":319,"context_line":"                    self.jenkins_open("},{"line_number":320,"context_line":"                        requests.Request(\u0027GET\u0027, self._build_url(INFO),"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f93b717_6cc011e2","line":317,"updated":"2018-02-02 19:35:21.000000000","message":"I think you meant to do this reversed—i.e., try kerberos first, then fall back to basic.","commit_id":"4f3a9901c40710318f9c1faaa1249a9a00fe157f"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"5a516b292bbee0272c817fa2c357306f81503191","unresolved":false,"context_lines":[{"line_number":314,"context_line":"            # This is a hack to allow the transparent use of kerberos to work"},{"line_number":315,"context_line":"            # in future, we should require explicit request to use kerberos"},{"line_number":316,"context_line":"            failures \u003d []"},{"line_number":317,"context_line":"            for name, auth in self._auths:"},{"line_number":318,"context_line":"                try:"},{"line_number":319,"context_line":"                    self.jenkins_open("},{"line_number":320,"context_line":"                        requests.Request(\u0027GET\u0027, self._build_url(INFO),"}],"source_content_type":"text/x-python","patch_set":20,"id":"3fa0c359_5db5fe7c","line":317,"in_reply_to":"5f93b717_6cc011e2","updated":"2018-02-12 11:18:04.000000000","message":"Thanks, when I tested originally tested that approach I was getting a lot of errors, so I switched it while working on the code and forgot to retest with it again afterwards.","commit_id":"4f3a9901c40710318f9c1faaa1249a9a00fe157f"},{"author":{"_account_id":13807,"name":"Thanh Ha","display_name":"zxiiro","email":"zxiiro@gmail.com","username":"zxiiro"},"change_message_id":"a6ee0279101959c686890270eeebd81b4bcd03c9","unresolved":false,"context_lines":[{"line_number":1114,"context_line":"            Plugin:mailer, Plugin:jquery, Plugin:antisamy-markup-formatter,"},{"line_number":1115,"context_line":"            Plugin:maven-plugin, Plugin:pam-auth]\u0027"},{"line_number":1116,"context_line":"        \u0027\u0027\u0027"},{"line_number":1117,"context_line":"        return self.jenkins_open("},{"line_number":1118,"context_line":"            requests.Request("},{"line_number":1119,"context_line":"                \u0027POST\u0027, self._build_url(SCRIPT_TEXT),"},{"line_number":1120,"context_line":"                data\u003d\"script\u003d\".encode(\u0027utf-8\u0027) + quote(script).encode(\u0027utf-8\u0027)))"},{"line_number":1121,"context_line":""},{"line_number":1122,"context_line":"    def install_plugin(self, name, include_dependencies\u003dTrue):"},{"line_number":1123,"context_line":"        \u0027\u0027\u0027Install a plugin and its dependencies from the Jenkins public"}],"source_content_type":"text/x-python","patch_set":21,"id":"5f7c97a3_5c0afed1","line":1120,"range":{"start_line":1117,"start_character":8,"end_line":1120,"end_character":80},"updated":"2018-06-11 00:06:55.000000000","message":"This code broke then run_script() api. The code returns 200 OK though \u0027\"POST /scriptText HTTP/1.1\" 200 0\u0027 but the groovy script does not actually run on the server.\n\nIt also appears that requests.Request() does not return a text object that we can print back to the user as far as I can tell so we just get a blank object if we try to print the result of print(jenkins.run_script())","commit_id":"34cca0c4d939b0b681d34cf204ae0be53e46069e"},{"author":{"_account_id":13807,"name":"Thanh Ha","display_name":"zxiiro","email":"zxiiro@gmail.com","username":"zxiiro"},"change_message_id":"166c12c3169d07213572d732966746b89f16444d","unresolved":false,"context_lines":[{"line_number":1114,"context_line":"            Plugin:mailer, Plugin:jquery, Plugin:antisamy-markup-formatter,"},{"line_number":1115,"context_line":"            Plugin:maven-plugin, Plugin:pam-auth]\u0027"},{"line_number":1116,"context_line":"        \u0027\u0027\u0027"},{"line_number":1117,"context_line":"        return self.jenkins_open("},{"line_number":1118,"context_line":"            requests.Request("},{"line_number":1119,"context_line":"                \u0027POST\u0027, self._build_url(SCRIPT_TEXT),"},{"line_number":1120,"context_line":"                data\u003d\"script\u003d\".encode(\u0027utf-8\u0027) + quote(script).encode(\u0027utf-8\u0027)))"},{"line_number":1121,"context_line":""},{"line_number":1122,"context_line":"    def install_plugin(self, name, include_dependencies\u003dTrue):"},{"line_number":1123,"context_line":"        \u0027\u0027\u0027Install a plugin and its dependencies from the Jenkins public"}],"source_content_type":"text/x-python","patch_set":21,"id":"5f7c97a3_f27205e3","line":1120,"range":{"start_line":1117,"start_character":8,"end_line":1120,"end_character":80},"in_reply_to":"5f7c97a3_5c0afed1","updated":"2018-06-11 15:23:28.000000000","message":"Submitted a fix proposal here Ibaffb768ea82c76b44ec5a6cccde8563afe1783f","commit_id":"34cca0c4d939b0b681d34cf204ae0be53e46069e"}],"tests/test_jenkins.py":[{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"06c95ae817c9f35d06f521cbca6e52b7921d1135","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"def build_response_mock(status_code, json_body\u003dNone, headers\u003dNone, **kwargs):"},{"line_number":17,"context_line":"    real_response \u003d jenkins.requests.Response()"},{"line_number":18,"context_line":"    real_response.status_code \u003d status_code"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"    text \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_d95c6e1b","line":17,"updated":"2015-08-11 17:13:27.000000000","message":"I\u0027d rather import requests directly, then use \"requests.Response()\"; makes it a little more explicit that we\u0027re doing something with requests and reduces how much you need to type…","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"af44de0bdc54689feb58f39b363f437eb5b32744","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"def build_response_mock(status_code, json_body\u003dNone, headers\u003dNone, **kwargs):"},{"line_number":17,"context_line":"    real_response \u003d jenkins.requests.Response()"},{"line_number":18,"context_line":"    real_response.status_code \u003d status_code"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"    text \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_4a27fdb5","line":17,"in_reply_to":"1a4dcd0f_d95c6e1b","updated":"2015-08-12 14:54:29.000000000","message":"Was going for trying to use directly from the jenkins module so any changes there to imports would immediately break the tests, but probably doesn\u0027t make much difference here or help much.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"af44de0bdc54689feb58f39b363f437eb5b32744","unresolved":false,"context_lines":[{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    def setUp(self):"},{"line_number":69,"context_line":"        super(JenkinsTest, self).setUp()"},{"line_number":70,"context_line":"        self.opener \u003d build_opener()"},{"line_number":71,"context_line":""},{"line_number":72,"context_line":"    def _check_requests(self, requests):"},{"line_number":73,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_a23d8f8f","line":70,"updated":"2015-08-12 14:54:29.000000000","message":"Forgot to strip this as well.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"06c95ae817c9f35d06f521cbca6e52b7921d1135","unresolved":false,"context_lines":[{"line_number":129,"context_line":"            u\u0027http://example.com/job/Test%20Job/config.xml\u0027)"},{"line_number":130,"context_line":"        self._check_requests(jenkins_mock.call_args_list)"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @patch(\u0027jenkins.requests.Session.send\u0027, autospec\u003dTrue)"},{"line_number":133,"context_line":"    def test_maybe_add_crumb(self, session_send_mock):"},{"line_number":134,"context_line":"        session_send_mock.return_value \u003d build_response_mock("},{"line_number":135,"context_line":"            404, reason\u003d\"Not Found\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_59f5beea","line":132,"updated":"2015-08-11 17:13:27.000000000","message":"You should be able to mock \u0027requests.Session.send\u0027 and not even involve the jenkins module.  I tend to find that a bit clearer.  It\u0027s also worth pointing out that, in cases where I\u0027ve imported a module (like jenkins here, or like requests, as I suggest above), I typically use mock.patch.object().  Assuming the \"import requests\", that would change this to be:\n\n    @patch.object(requests, \u0027Session.send\u0027, autospec\u003dTrue)","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"e1436e113d2467fd5aacb2550fda16ccd513ee8f","unresolved":false,"context_lines":[{"line_number":129,"context_line":"            u\u0027http://example.com/job/Test%20Job/config.xml\u0027)"},{"line_number":130,"context_line":"        self._check_requests(jenkins_mock.call_args_list)"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @patch(\u0027jenkins.requests.Session.send\u0027, autospec\u003dTrue)"},{"line_number":133,"context_line":"    def test_maybe_add_crumb(self, session_send_mock):"},{"line_number":134,"context_line":"        session_send_mock.return_value \u003d build_response_mock("},{"line_number":135,"context_line":"            404, reason\u003d\"Not Found\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_5c89cb6d","line":132,"in_reply_to":"1a4dcd0f_25c72d97","updated":"2015-08-13 11:39:41.000000000","message":"Mocking at where the lookup occurs is consistent across modules, objects and methods. This seems like a sensible idea which will help contributors just copy the existing mocking approach and a apply to any as needed without needing to be aware of an subtle changes in behaviour depending on what is being mocked.\n\nMocking at the lookup point also includes information about where in the API being unit tested that an object needs to be mocked to exercise the code. While this is currently less important for python-jenkins, given it currently has a single module, it still is a useful default.\n\nAn example of this: https://github.com/openstack-infra/jenkins-job-builder/blob/master/tests/cmd/subcommands/test_test.py#L102-L132 tells you exactly where in the modules the mocking of os.walk and os.path.isdir is intended to affect so you know where to focus looking in the code.\n\n\nReferencing the underlying module has the appearance of reaching in behind the curtain and changing internal behaviour instead of mocking where the object is being accessed. The effect may be ultimately the same, but it hides where in the code being tested it\u0027s intended to affect.\n\n\nIt does appear that in a number of cases nova is doing the exact same as here, nova/tests/unit/db/test_db_api.py mocks a number of methods under \u0027nova.db.sqlalchemy.api\u0027 which turn out to be mainly imports from sqlalchemy. Where they use @mock.patch.object(\u0027sqlalchemy_api\u0027, \u003cfunction\u003e) it turns out the functions are their internal ones instead of references to what is implemented by sqlalchemy.\n\nAs far as I can tell, there are multiple approaches being taken, sometimes the mock is directly on the system module being utilised, sometimes it\u0027s done using where the lookup occurs.\n\nWithout a better reason than less characters, it appears more beneficial to follow the recommendation from the mock documentation, and only deviate from that as and when needed.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"20b025383b89117da0b2251d79f7add71da630b4","unresolved":false,"context_lines":[{"line_number":129,"context_line":"            u\u0027http://example.com/job/Test%20Job/config.xml\u0027)"},{"line_number":130,"context_line":"        self._check_requests(jenkins_mock.call_args_list)"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @patch(\u0027jenkins.requests.Session.send\u0027, autospec\u003dTrue)"},{"line_number":133,"context_line":"    def test_maybe_add_crumb(self, session_send_mock):"},{"line_number":134,"context_line":"        session_send_mock.return_value \u003d build_response_mock("},{"line_number":135,"context_line":"            404, reason\u003d\"Not Found\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_6983d903","line":132,"in_reply_to":"1a4dcd0f_477b7dbe","updated":"2015-08-12 15:16:07.000000000","message":"1. That does work; that particular idiom is used all over nova, novaclient, and virtually everything I\u0027ve written.\n\n2. I have never seen that advice you\u0027ve linked to, and as I say, everything I\u0027ve seen that uses mock mocks out from the underlying package.  This is, in fact, one of the reasons nova and other OpenStack projects enforce the convention of importing modules rather than symbols from modules.  I can definitely see the advantages of this advice in the case where you import the symbols, but that\u0027s not what we\u0027re doing here; in essence, \"jenkins.requests.Session.send\" is simply an alias for \"requests.Session.send\", and if you patch the former, you are *also* patching the latter.  In that situation, I think I prefer the shorter version, to make it clear exactly what\u0027s being patched.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"af44de0bdc54689feb58f39b363f437eb5b32744","unresolved":false,"context_lines":[{"line_number":129,"context_line":"            u\u0027http://example.com/job/Test%20Job/config.xml\u0027)"},{"line_number":130,"context_line":"        self._check_requests(jenkins_mock.call_args_list)"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @patch(\u0027jenkins.requests.Session.send\u0027, autospec\u003dTrue)"},{"line_number":133,"context_line":"    def test_maybe_add_crumb(self, session_send_mock):"},{"line_number":134,"context_line":"        session_send_mock.return_value \u003d build_response_mock("},{"line_number":135,"context_line":"            404, reason\u003d\"Not Found\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_477b7dbe","line":132,"in_reply_to":"1a4dcd0f_59f5beea","updated":"2015-08-12 14:54:29.000000000","message":"That wouldn\u0027t work, or at least is bad practice if it does, for python mock, you should mock in the namespace that the module/object is imported into. See https://docs.python.org/3/library/unittest.mock.html#where-to-patch\n\nWhen mocking the session object I want to mock the one imported into the jenkins namespace, if for some reason we want to use the Session object locally to perform another task we wouldn\u0027t want to get one that is mocked.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":1054,"name":"Darragh Bailey","email":"daragh.bailey@gmail.com","username":"dbailey-k"},"change_message_id":"b53d09391304162b01383b81fcab2abe562531ee","unresolved":false,"context_lines":[{"line_number":129,"context_line":"            u\u0027http://example.com/job/Test%20Job/config.xml\u0027)"},{"line_number":130,"context_line":"        self._check_requests(jenkins_mock.call_args_list)"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @patch(\u0027jenkins.requests.Session.send\u0027, autospec\u003dTrue)"},{"line_number":133,"context_line":"    def test_maybe_add_crumb(self, session_send_mock):"},{"line_number":134,"context_line":"        session_send_mock.return_value \u003d build_response_mock("},{"line_number":135,"context_line":"            404, reason\u003d\"Not Found\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_8cd2eecf","line":132,"in_reply_to":"1a4dcd0f_6983d903","updated":"2015-08-12 17:14:57.000000000","message":"That advice has been in the mock docs for some time, and I\u0027ve found that it is more consistent to patch the object where it is being looked up instead.\n\nSimple example of where this falls down:\n\ntest1.py\n--\n\n class DummyClass(object):\n     def method(self):\n         return \"Hello World\"\n\n\ntest2.py\n--\n\n from test1 import DummyClass\n \n \n def create():\n     d \u003d DummyClass()\n     print(d.method())\n\ntest.py\n--\n\n import test2\n import test1\n import mock\n\n\n @mock.patch(\u0027test1.DummyClass\u0027)\n def example1(dummyclass_mock):\n     dummyclass_mock.return_value.method.return_value \u003d \"Goodbye World!\"\n\n     test2.create()\n\n example1()\n\n\nWill print \u0027Hello World\u0027, change the patch to mock \u0027test2.DummyClass\u0027 and it\u0027ll print \u0027Goodbye World!\u0027. This tends to catch people by surprise.\n\nUse the following mock and you\u0027ll get \u0027Hello World\u0027\n\n @mock.patch.object(test1, \u0027DummyClass\u0027)\n\nChange to the following and you\u0027ll get \u0027Goodbye World\u0027\n\n @mock.patch.object(test2, \u0027DummyClass\u0027)\n\n\n\nWhen mocking methods on objects, you\u0027ll get the expected behaviour, but due to the quirks around mocking entire classes I\u0027ve defaulted to using the recommendation from the mock docs to mock from where the object will be looked up so as to avoid surprises when switching between mocking the class versus methods.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"},{"author":{"_account_id":679,"name":"Kevin L. Mitchell","email":"klmitch@mit.edu","username":"klmitch"},"change_message_id":"e28496adef5d91a72d403f5140ccb36ce5bc7889","unresolved":false,"context_lines":[{"line_number":129,"context_line":"            u\u0027http://example.com/job/Test%20Job/config.xml\u0027)"},{"line_number":130,"context_line":"        self._check_requests(jenkins_mock.call_args_list)"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @patch(\u0027jenkins.requests.Session.send\u0027, autospec\u003dTrue)"},{"line_number":133,"context_line":"    def test_maybe_add_crumb(self, session_send_mock):"},{"line_number":134,"context_line":"        session_send_mock.return_value \u003d build_response_mock("},{"line_number":135,"context_line":"            404, reason\u003d\"Not Found\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"1a4dcd0f_25c72d97","line":132,"in_reply_to":"1a4dcd0f_8cd2eecf","updated":"2015-08-12 18:08:09.000000000","message":"Yep, of course, but allow me to point out again: you\u0027re importing an object, not a module, so mocking the class in the importing module is the only way to effect the mock.  When you import the *module*, both forms of mock work the same, just as you point out when you talk about mocking out a *method* on the class.","commit_id":"c588c77f2a2826516984078602eed4a685da23df"}]}