{"action":"created","comment":{"url":"https://api.github.com/repos/artefactual/automation-tools/pulls/comments/183554261","pull_request_review_id":114555643,"id":183554261,"diff_hunk":"@@ -40,9 +51,14 @@ def _call_url_json(url, params, method='GET'):\n         try:\n             return response.json()\n         except ValueError:  # JSON could not be decoded\n-            LOGGER.warning('Could not parse JSON from response: %s',\n-                           response.text)\n-            return errors.ERR_PARSE_JSON\n+            if not response.ok:\n+                LOGGER.warning('Could not parse JSON from response: %s',\n+                               response.text)\n+                return errors.ERR_PARSE_JSON\n+            pass","path":"transfers/utils.py","position":43,"original_position":43,"commit_id":"d1f443e8b0e6fde22aa42ac835ff991a136c357b","original_commit_id":"e543aa981b1182b31d30bb179ab45238ae9e74eb","user":{"login":"jrwdunham","id":2504271,"avatar_url":"https://avatars1.githubusercontent.com/u/2504271?v=4","gravatar_id":"","url":"https://api.github.com/users/jrwdunham","html_url":"https://github.com/jrwdunham","followers_url":"https://api.github.com/users/jrwdunham/followers","following_url":"https://api.github.com/users/jrwdunham/following{/other_user}","gists_url":"https://api.github.com/users/jrwdunham/gists{/gist_id}","starred_url":"https://api.github.com/users/jrwdunham/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jrwdunham/subscriptions","organizations_url":"https://api.github.com/users/jrwdunham/orgs","repos_url":"https://api.github.com/users/jrwdunham/repos","events_url":"https://api.github.com/users/jrwdunham/events{/privacy}","received_events_url":"https://api.github.com/users/jrwdunham/received_events","type":"User","site_admin":false},"body":"We are already checking `if not response.ok` on line 46 so I don't think the block in 55-57 can ever be reached. \r\n\r\nAlso, it is confusing (though not introduced by this PR) that we have nested `try/except` blocks. Can we separate out these different error conditions?\r\n\r\nI think what you're trying to do here is allow for a non-JSON response. So maybe it would be better to have a `json` (or `assume_json`) kwarg in the function that defaults to `True` but when `False` returns `response.text`. How about something like the following?:\r\n\r\n    def _call_url_json(url, params, method=METHOD_GET, headers=None,\r\n                    assume_json=True):\r\n        \"\"\"Helper to GET a URL where the expected response is 200 with JSON.\r\n\r\n        :param str url: URL to call\r\n        :param dict params: Params to pass as HTTP query string or JSON body\r\n        :param str method: HTTP method (e.g., 'GET')\r\n        :param dict headers: HTTP headers\r\n        :param bool assume_json: set to False if the response body should not be decoded as JSON\r\n        :returns: Dict of the returned JSON or an integer error\r\n                code to be looked up\r\n        \"\"\"\r\n        method = method.upper()\r\n        LOGGER.debug('URL: %s; params: %s; method: %s', url, params, method)\r\n        try:\r\n            if method == METHOD_GET:\r\n                response = requests.request(method, url=url, params=params,\r\n                                            headers=headers)\r\n            else:\r\n                response = requests.request(method, url=url, data=params,\r\n                                            headers=headers)\r\n                                            headers=headers)\r\n            LOGGER.debug('Response: %s', response)\r\n            LOGGER.debug('type(response.text): %s ', type(response.text))\r\n            LOGGER.debug('Response content-type: %s',\r\n                        response.headers['content-type'])\r\n        except (urllib3.exceptions.NewConnectionError,\r\n                requests.exceptions.ConnectionError) as err:\r\n            LOGGER.error(\"Connection error %s\", err)\r\n            return errors.ERR_SERVER_CONN\r\n        if not response.ok:\r\n            LOGGER.warning('%s Request to %s returned %s %s', method, url,\r\n                        response.status_code, response.reason)\r\n            LOGGER.debug('Response: %s', response.text)\r\n            return errors.ERR_INVALID_RESPONSE\r\n        if assume_json:\r\n            try:\r\n                return response.json()\r\n            except ValueError:  # JSON could not be decoded\r\n                LOGGER.warning('Could not parse JSON from response: %s',\r\n                            response.text)\r\n                return errors.ERR_PARSE_JSON\r\n        return response.text\r\n","created_at":"2018-04-23T22:17:17Z","updated_at":"2018-04-23T22:51:44Z","html_url":"https://github.com/artefactual/automation-tools/pull/57#discussion_r183554261","pull_request_url":"https://api.github.com/repos/artefactual/automation-tools/pulls/57","author_association":"CONTRIBUTOR","_links":{"self":{"href":"https://api.github.com/repos/artefactual/automation-tools/pulls/comments/183554261"},"html":{"href":"https://github.com/artefactual/automation-tools/pull/57#discussion_r183554261"},"pull_request":{"href":"https://api.github.com/repos/artefactual/automation-tools/pulls/57"}}},"pull_request":{"url":"https://api.github.com/repos/artefactual/automation-tools/pulls/57","id":180235792,"html_url":"https://github.com/artefactual/automation-tools/pull/57","diff_url":"https://github.com/artefactual/automation-tools/pull/57.diff","patch_url":"https://github.com/artefactual/automation-tools/pull/57.patch","issue_url":"https://api.github.com/repos/artefactual/automation-tools/issues/57","number":57,"state":"open","locked":false,"title":"Adds AM Client Capability in support of Reingest","user":{"login":"ross-spencer","id":1880412,"avatar_url":"https://avatars1.githubusercontent.com/u/1880412?v=4","gravatar_id":"","url":"https://api.github.com/users/ross-spencer","html_url":"https://github.com/ross-spencer","followers_url":"https://api.github.com/users/ross-spencer/followers","following_url":"https://api.github.com/users/ross-spencer/following{/other_user}","gists_url":"https://api.github.com/users/ross-spencer/gists{/gist_id}","starred_url":"https://api.github.com/users/ross-spencer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ross-spencer/subscriptions","organizations_url":"https://api.github.com/users/ross-spencer/orgs","repos_url":"https://api.github.com/users/ross-spencer/repos","events_url":"https://api.github.com/users/ross-spencer/events{/privacy}","received_events_url":"https://api.github.com/users/ross-spencer/received_events","type":"User","site_admin":false},"body":"This PR adds capability to AM Client in support of the API functions\r\nrequired for automated reingest.\r\n\r\n* Adds new functions to AM Client, including reingest and transfer status.\r\n* Provides unit testing for the additional function calls.\r\n* Extends utils.py request mechanism to enable http authentication headers\r\n  to be set.\r\n* A formatting issue was discovered in models.py while testing which\r\n  is now corrected.\r\n* Warning for non-JSON responses are only output if there is a non-OK\r\n  HTTP response where responses from the API are not always JSON.\r\n\r\n\r\nSubmitted as part of Redmine ticket 11686 for CCA in support of automated reingest, cc. @timothyryanwalsh.","created_at":"2018-04-09T07:56:03Z","updated_at":"2018-04-23T22:51:44Z","closed_at":null,"merged_at":null,"merge_commit_sha":"8aaa71423250954b871ce55c8696a88cc8b0bf1c","assignee":{"login":"ross-spencer","id":1880412,"avatar_url":"https://avatars1.githubusercontent.com/u/1880412?v=4","gravatar_id":"","url":"https://api.github.com/users/ross-spencer","html_url":"https://github.com/ross-spencer","followers_url":"https://api.github.com/users/ross-spencer/followers","following_url":"https://api.github.com/users/ross-spencer/following{/other_user}","gists_url":"https://api.github.com/users/ross-spencer/gists{/gist_id}","starred_url":"https://api.github.com/users/ross-spencer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ross-spencer/subscriptions","organizations_url":"https://api.github.com/users/ross-spencer/orgs","repos_url":"https://api.github.com/users/ross-spencer/repos","events_url":"https://api.github.com/users/ross-spencer/events{/privacy}","received_events_url":"https://api.github.com/users/ross-spencer/received_events","type":"User","site_admin":false},"assignees":[{"login":"ross-spencer","id":1880412,"avatar_url":"https://avatars1.githubusercontent.com/u/1880412?v=4","gravatar_id":"","url":"https://api.github.com/users/ross-spencer","html_url":"https://github.com/ross-spencer","followers_url":"https://api.github.com/users/ross-spencer/followers","following_url":"https://api.github.com/users/ross-spencer/following{/other_user}","gists_url":"https://api.github.com/users/ross-spencer/gists{/gist_id}","starred_url":"https://api.github.com/users/ross-spencer/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ross-spencer/subscriptions","organizations_url":"https://api.github.com/users/ross-spencer/orgs","repos_url":"https://api.github.com/users/ross-spencer/repos","events_url":"https://api.github.com/users/ross-spencer/events{/privacy}","received_events_url":"https://api.github.com/users/ross-spencer/received_events","type":"User","site_admin":false}],"requested_reviewers":[{"login":"sevein","id":606459,"avatar_url":"https://avatars2.githubusercontent.com/u/606459?v=4","gravatar_id":"","url":"https://api.github.com/users/sevein","html_url":"https://github.com/sevein","followers_url":"https://api.github.com/users/sevein/followers","following_url":"https://api.github.com/users/sevein/following{/other_user}","gists_url":"https://api.github.com/users/sevein/gists{/gist_id}","starred_url":"https://api.github.com/users/sevein/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sevein/subscriptions","organizations_url":"https://api.github.com/users/sevein/orgs","repos_url":"https://api.github.com/users/sevein/repos","events_url":"https://api.github.com/users/sevein/events{/privacy}","received_events_url":"https://api.github.com/users/sevein/received_events","type":"User","site_admin":false},{"login":"jraddaoui","id":637040,"avatar_url":"https://avatars2.githubusercontent.com/u/637040?v=4","gravatar_id":"","url":"https://api.github.com/users/jraddaoui","html_url":"https://github.com/jraddaoui","followers_url":"https://api.github.com/users/jraddaoui/followers","following_url":"https://api.github.com/users/jraddaoui/following{/other_user}","gists_url":"https://api.github.com/users/jraddaoui/gists{/gist_id}","starred_url":"https://api.github.com/users/jraddaoui/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jraddaoui/subscriptions","organizations_url":"https://api.github.com/users/jraddaoui/orgs","repos_url":"https://api.github.com/users/jraddaoui/repos","events_url":"https://api.github.com/users/jraddaoui/events{/privacy}","received_events_url":"https://api.github.com/users/jraddaoui/received_events","type":"User","site_admin":false}],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/artefactual/automation-tools/pulls/57/commits","review_comments_url":"https://api.github.com/repos/artefactual/automation-tools/pulls/57/comments","review_comment_url":"https://api.github.com/repos/artefactual/automation-tools/pulls/comments{/number}","comments_url":"https://api.github.com/repos/artefactual/automation-tools/issues/57/comments","statuses_url":"https://api.github.com/repos/artefactual/automation-tools/statuses/d1f443e8b0e6fde22aa42ac835ff991a136c357b","head":{"label":"artefactual:dev/cca-11686-amclient-additions","ref":"dev/cca-11686-amclient-additions","sha":"d1f443e8b0e6fde22aa42ac835ff991a136c357b","user":{"login":"artefactual","id":1163545,"avatar_url":"https://avatars1.githubusercontent.com/u/1163545?v=4","gravatar_id":"","url":"https://api.github.com/users/artefactual","html_url":"https://github.com/artefactual","followers_url":"https://api.github.com/users/artefactual/followers","following_url":"https://api.github.com/users/artefactual/following{/other_user}","gists_url":"https://api.github.com/users/artefactual/gists{/gist_id}","starred_url":"https://api.github.com/users/artefactual/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/artefactual/subscriptions","organizations_url":"https://api.github.com/users/artefactual/orgs","repos_url":"https://api.github.com/users/artefactual/repos","events_url":"https://api.github.com/users/artefactual/events{/privacy}","received_events_url":"https://api.github.com/users/artefactual/received_events","type":"Organization","site_admin":false},"repo":{"id":25843990,"name":"automation-tools","full_name":"artefactual/automation-tools","owner":{"login":"artefactual","id":1163545,"avatar_url":"https://avatars1.githubusercontent.com/u/1163545?v=4","gravatar_id":"","url":"https://api.github.com/users/artefactual","html_url":"https://github.com/artefactual","followers_url":"https://api.github.com/users/artefactual/followers","following_url":"https://api.github.com/users/artefactual/following{/other_user}","gists_url":"https://api.github.com/users/artefactual/gists{/gist_id}","starred_url":"https://api.github.com/users/artefactual/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/artefactual/subscriptions","organizations_url":"https://api.github.com/users/artefactual/orgs","repos_url":"https://api.github.com/users/artefactual/repos","events_url":"https://api.github.com/users/artefactual/events{/privacy}","received_events_url":"https://api.github.com/users/artefactual/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/artefactual/automation-tools","description":"Tools to aid automation of Archivematica and AtoM.","fork":false,"url":"https://api.github.com/repos/artefactual/automation-tools","forks_url":"https://api.github.com/repos/artefactual/automation-tools/forks","keys_url":"https://api.github.com/repos/artefactual/automation-tools/keys{/key_id}","collaborators_url":"https://api.github.com/repos/artefactual/automation-tools/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/artefactual/automation-tools/teams","hooks_url":"https://api.github.com/repos/artefactual/automation-tools/hooks","issue_events_url":"https://api.github.com/repos/artefactual/automation-tools/issues/events{/number}","events_url":"https://api.github.com/repos/artefactual/automation-tools/events","assignees_url":"https://api.github.com/repos/artefactual/automation-tools/assignees{/user}","branches_url":"https://api.github.com/repos/artefactual/automation-tools/branches{/branch}","tags_url":"https://api.github.com/repos/artefactual/automation-tools/tags","blobs_url":"https://api.github.com/repos/artefactual/automation-tools/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/artefactual/automation-tools/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/artefactual/automation-tools/git/refs{/sha}","trees_url":"https://api.github.com/repos/artefactual/automation-tools/git/trees{/sha}","statuses_url":"https://api.github.com/repos/artefactual/automation-tools/statuses/{sha}","languages_url":"https://api.github.com/repos/artefactual/automation-tools/languages","stargazers_url":"https://api.github.com/repos/artefactual/automation-tools/stargazers","contributors_url":"https://api.github.com/repos/artefactual/automation-tools/contributors","subscribers_url":"https://api.github.com/repos/artefactual/automation-tools/subscribers","subscription_url":"https://api.github.com/repos/artefactual/automation-tools/subscription","commits_url":"https://api.github.com/repos/artefactual/automation-tools/commits{/sha}","git_commits_url":"https://api.github.com/repos/artefactual/automation-tools/git/commits{/sha}","comments_url":"https://api.github.com/repos/artefactual/automation-tools/comments{/number}","issue_comment_url":"https://api.github.com/repos/artefactual/automation-tools/issues/comments{/number}","contents_url":"https://api.github.com/repos/artefactual/automation-tools/contents/{+path}","compare_url":"https://api.github.com/repos/artefactual/automation-tools/compare/{base}...{head}","merges_url":"https://api.github.com/repos/artefactual/automation-tools/merges","archive_url":"https://api.github.com/repos/artefactual/automation-tools/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/artefactual/automation-tools/downloads","issues_url":"https://api.github.com/repos/artefactual/automation-tools/issues{/number}","pulls_url":"https://api.github.com/repos/artefactual/automation-tools/pulls{/number}","milestones_url":"https://api.github.com/repos/artefactual/automation-tools/milestones{/number}","notifications_url":"https://api.github.com/repos/artefactual/automation-tools/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/artefactual/automation-tools/labels{/name}","releases_url":"https://api.github.com/repos/artefactual/automation-tools/releases{/id}","deployments_url":"https://api.github.com/repos/artefactual/automation-tools/deployments","created_at":"2014-10-27T23:26:50Z","updated_at":"2018-04-13T08:41:52Z","pushed_at":"2018-04-16T09:52:19Z","git_url":"git://github.com/artefactual/automation-tools.git","ssh_url":"git@github.com:artefactual/automation-tools.git","clone_url":"https://github.com/artefactual/automation-tools.git","svn_url":"https://github.com/artefactual/automation-tools","homepage":null,"size":439,"stargazers_count":18,"watchers_count":18,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":18,"mirror_url":null,"archived":false,"open_issues_count":18,"license":{"key":"other","name":"Other","spdx_id":null,"url":null},"forks":18,"open_issues":18,"watchers":18,"default_branch":"master"}},"base":{"label":"artefactual:master","ref":"master","sha":"6dc6c4fd69db99a4666d24722dd463a16cf45c5d","user":{"login":"artefactual","id":1163545,"avatar_url":"https://avatars1.githubusercontent.com/u/1163545?v=4","gravatar_id":"","url":"https://api.github.com/users/artefactual","html_url":"https://github.com/artefactual","followers_url":"https://api.github.com/users/artefactual/followers","following_url":"https://api.github.com/users/artefactual/following{/other_user}","gists_url":"https://api.github.com/users/artefactual/gists{/gist_id}","starred_url":"https://api.github.com/users/artefactual/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/artefactual/subscriptions","organizations_url":"https://api.github.com/users/artefactual/orgs","repos_url":"https://api.github.com/users/artefactual/repos","events_url":"https://api.github.com/users/artefactual/events{/privacy}","received_events_url":"https://api.github.com/users/artefactual/received_events","type":"Organization","site_admin":false},"repo":{"id":25843990,"name":"automation-tools","full_name":"artefactual/automation-tools","owner":{"login":"artefactual","id":1163545,"avatar_url":"https://avatars1.githubusercontent.com/u/1163545?v=4","gravatar_id":"","url":"https://api.github.com/users/artefactual","html_url":"https://github.com/artefactual","followers_url":"https://api.github.com/users/artefactual/followers","following_url":"https://api.github.com/users/artefactual/following{/other_user}","gists_url":"https://api.github.com/users/artefactual/gists{/gist_id}","starred_url":"https://api.github.com/users/artefactual/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/artefactual/subscriptions","organizations_url":"https://api.github.com/users/artefactual/orgs","repos_url":"https://api.github.com/users/artefactual/repos","events_url":"https://api.github.com/users/artefactual/events{/privacy}","received_events_url":"https://api.github.com/users/artefactual/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/artefactual/automation-tools","description":"Tools to aid automation of Archivematica and AtoM.","fork":false,"url":"https://api.github.com/repos/artefactual/automation-tools","forks_url":"https://api.github.com/repos/artefactual/automation-tools/forks","keys_url":"https://api.github.com/repos/artefactual/automation-tools/keys{/key_id}","collaborators_url":"https://api.github.com/repos/artefactual/automation-tools/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/artefactual/automation-tools/teams","hooks_url":"https://api.github.com/repos/artefactual/automation-tools/hooks","issue_events_url":"https://api.github.com/repos/artefactual/automation-tools/issues/events{/number}","events_url":"https://api.github.com/repos/artefactual/automation-tools/events","assignees_url":"https://api.github.com/repos/artefactual/automation-tools/assignees{/user}","branches_url":"https://api.github.com/repos/artefactual/automation-tools/branches{/branch}","tags_url":"https://api.github.com/repos/artefactual/automation-tools/tags","blobs_url":"https://api.github.com/repos/artefactual/automation-tools/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/artefactual/automation-tools/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/artefactual/automation-tools/git/refs{/sha}","trees_url":"https://api.github.com/repos/artefactual/automation-tools/git/trees{/sha}","statuses_url":"https://api.github.com/repos/artefactual/automation-tools/statuses/{sha}","languages_url":"https://api.github.com/repos/artefactual/automation-tools/languages","stargazers_url":"https://api.github.com/repos/artefactual/automation-tools/stargazers","contributors_url":"https://api.github.com/repos/artefactual/automation-tools/contributors","subscribers_url":"https://api.github.com/repos/artefactual/automation-tools/subscribers","subscription_url":"https://api.github.com/repos/artefactual/automation-tools/subscription","commits_url":"https://api.github.com/repos/artefactual/automation-tools/commits{/sha}","git_commits_url":"https://api.github.com/repos/artefactual/automation-tools/git/commits{/sha}","comments_url":"https://api.github.com/repos/artefactual/automation-tools/comments{/number}","issue_comment_url":"https://api.github.com/repos/artefactual/automation-tools/issues/comments{/number}","contents_url":"https://api.github.com/repos/artefactual/automation-tools/contents/{+path}","compare_url":"https://api.github.com/repos/artefactual/automation-tools/compare/{base}...{head}","merges_url":"https://api.github.com/repos/artefactual/automation-tools/merges","archive_url":"https://api.github.com/repos/artefactual/automation-tools/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/artefactual/automation-tools/downloads","issues_url":"https://api.github.com/repos/artefactual/automation-tools/issues{/number}","pulls_url":"https://api.github.com/repos/artefactual/automation-tools/pulls{/number}","milestones_url":"https://api.github.com/repos/artefactual/automation-tools/milestones{/number}","notifications_url":"https://api.github.com/repos/artefactual/automation-tools/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/artefactual/automation-tools/labels{/name}","releases_url":"https://api.github.com/repos/artefactual/automation-tools/releases{/id}","deployments_url":"https://api.github.com/repos/artefactual/automation-tools/deployments","created_at":"2014-10-27T23:26:50Z","updated_at":"2018-04-13T08:41:52Z","pushed_at":"2018-04-16T09:52:19Z","git_url":"git://github.com/artefactual/automation-tools.git","ssh_url":"git@github.com:artefactual/automation-tools.git","clone_url":"https://github.com/artefactual/automation-tools.git","svn_url":"https://github.com/artefactual/automation-tools","homepage":null,"size":439,"stargazers_count":18,"watchers_count":18,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":18,"mirror_url":null,"archived":false,"open_issues_count":18,"license":{"key":"other","name":"Other","spdx_id":null,"url":null},"forks":18,"open_issues":18,"watchers":18,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/artefactual/automation-tools/pulls/57"},"html":{"href":"https://github.com/artefactual/automation-tools/pull/57"},"issue":{"href":"https://api.github.com/repos/artefactual/automation-tools/issues/57"},"comments":{"href":"https://api.github.com/repos/artefactual/automation-tools/issues/57/comments"},"review_comments":{"href":"https://api.github.com/repos/artefactual/automation-tools/pulls/57/comments"},"review_comment":{"href":"https://api.github.com/repos/artefactual/automation-tools/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/artefactual/automation-tools/pulls/57/commits"},"statuses":{"href":"https://api.github.com/repos/artefactual/automation-tools/statuses/d1f443e8b0e6fde22aa42ac835ff991a136c357b"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"jrwdunham"}}