Merge lp:~jcsackett/charmworld/better-jobs into lp:charmworld

[j.c.sackett]

Handle some failure modes with better logging.

This branch adds some exception handling and logging to known failure modes we
want to keep track of, as well as some better request handling.

charmworld/jobs/askbubuntu.py
-----------------------------
* Added simple backoff parameter handling, which stackexchange API sets on some
  responses.
* Added handling of previous error condition wherein job couldn't connect to API
  servers. This is now logged as an error.

charmworld/jobs/github.py
-------------------------
* Added handling of limit rates in github requests, and logs instances of
  exceeding the limit as an error.
* Added handling of previous error condition wherein job couldn't connect to API
  servers. This is now logged as an error.

charmworld/jobs/review.py
-------------------------
* Logs missing lp_credentials file, ie a filepath is provided but no file
  exists. In this instance also sets the lp_credentials var to None so we'll
  login anonymously, rather than accidentally launching lynx.
* Logs whether the credentials file exists and we're logging in as charmbot, or
  doesn't exist and we're logging in anonymously.

[lp:~jcsackett/charmworld/better-jobs]

468. By j.c.sackett on 2013-11-18

Merged trunk.

469. By j.c.sackett on 2013-11-18

Shut up, lint.

470. By j.c.sackett on 2013-11-18

Tests for askubuntu backoff.

471. By j.c.sackett on 2013-11-18

Tests actually, y'know, test things.

[Aaron Bentley (abentley)]

Looks good. Thanks for the tests.

review: Approve

[Juju Gui Bot (juju-gui-bot)]

review: Approve (continuous-integration)

=== modified file 'charmworld/jobs/askubuntu.py'
--- charmworld/jobs/askubuntu.py	2013-09-27 16:13:21 +0000
+++ charmworld/jobs/askubuntu.py	2013-11-18 15:35:18 +0000
@@ -30,6 +30,8 @@
 def get_questions(page):
     data = get_data_from_askubuntu(page)
     results = data['items']
+    backoff = int(data.get('backoff', 0))
+    time.sleep(backoff)
     if data['has_more']:
         results += get_questions(page + 1)
     return results
@@ -73,7 +75,11 @@
     conn = pymongo.Connection(MONGO_URL, safe=True)
     db = conn['juju']
     settings = get_ini()
-    questions = get_all_questions(log)
+    try:
+        questions = get_all_questions(log)
+    except requests.ConnectionError as e:
+        log.error("Couldn't complete request: %s" % e)
+        return
     entries = create_entries_from_questions(questions)
     try:
         with lock('askubuntu',

=== modified file 'charmworld/jobs/github.py'
--- charmworld/jobs/github.py	2013-10-10 17:59:16 +0000
+++ charmworld/jobs/github.py	2013-11-18 15:35:18 +0000
@@ -16,6 +16,10 @@
 from utils import LockHeld
 
 
+class GithubRateLimitError(Exception):
+    pass
+
+
 def make_github_request(endpoint_name, repo_name=None):
     if endpoint_name == 'repos':
         url = 'https://api.github.com/users/charms/repos'
@@ -24,6 +28,8 @@
             raise ValueError('repo_name cannot be None for pull requests')
         url = 'https://api.github.com/repos/charms/%s/pulls' % repo_name
     r = requests.get(url)
+    if int(r.headers.get('x-ratelimit-remaining', 1)) == 0:
+        raise GithubRateLimitError('Github ratelimit exceeded')
     return r.json()
 
 
@@ -77,8 +83,16 @@
     conn = pymongo.Connection(MONGO_URL, safe=True)
     db = conn['juju']
     settings = get_ini()
-    charm_repos = get_charm_repos(log)
-    pull_requests = get_pull_requests(charm_repos, log)
+    try:
+        charm_repos = get_charm_repos(log)
+    except (requests.ConnectionError, GithubRateLimitError) as e:
+        log.error("Couldn't complete request for repos: %s" % e)
+        return
+    try:
+        pull_requests = get_pull_requests(charm_repos, log)
+    except (requests.ConnectionError, GithubRateLimitError) as e:
+        log.error("Couldn't complete request for pull-requests: %s" % e)
+        return
     entries = create_entries_from_pull_requests(pull_requests)
     try:
         with lock('github',

=== modified file 'charmworld/jobs/review.py'
--- charmworld/jobs/review.py	2013-11-13 14:00:14 +0000
+++ charmworld/jobs/review.py	2013-11-18 15:35:18 +0000
@@ -17,6 +17,7 @@
 import datetime
 import itertools
 import logging
+import os
 
 from launchpadlib.launchpad import Launchpad
 import pymongo
@@ -42,14 +43,20 @@
         lp_credentials = get_ini()['lp_credentials_file']
     except KeyError:
         lp_credentials = None
+    if not os.path.exists(lp_credentials):
+        log.warn("lp_credentials path doesn't exist: %s" % lp_credentials)
+        lp_credentials = None
     with Timer() as timer:
         if lp_credentials:
+            log.info("Found lp_credentials; proceeding.")
             lp = Launchpad.login_with(application_name='charm-tools',
                                       service_root='production',
                                       version='devel',
                                       consumer_name='charmworld',
                                       credentials_file=lp_credentials)
         else:
+            log.info(
+                "Couldn't find lp_credentials; proceeding without latency.")
             lp = Launchpad.login_anonymously('charm-tools',
                                              'production',
                                              version='devel')

=== modified file 'charmworld/jobs/tests/test_askubuntu.py'
--- charmworld/jobs/tests/test_askubuntu.py	2013-10-15 20:04:46 +0000
+++ charmworld/jobs/tests/test_askubuntu.py	2013-11-18 15:35:18 +0000
@@ -14,8 +14,10 @@
 
 class FakeGet:
 
-    def __init__(self, has_more=False):
+    def __init__(self, has_more=False, backoff=None):
         self.response = {'items': [1, 2, 3], 'has_more': has_more}
+        if backoff:
+            self.response['backoff'] = backoff
         self.call_count = 0
 
     def __call__(self, *args, **kwargs):
@@ -38,11 +40,26 @@
     def test_get_questions_recursive(self):
         with patch(
             'charmworld.jobs.askubuntu.get_data_from_askubuntu',
-            FakeGet(True)) as mock:
+            FakeGet(has_more=True)) as mock:
             questions = get_questions(1)
             self.assertEqual(2, mock.call_count)
         self.assertEqual([1, 2, 3, 1, 2, 3], questions)
 
+    def test_get_questions_handles_backoff(self):
+        with patch(
+            'charmworld.jobs.askubuntu.get_data_from_askubuntu',
+            FakeGet(backoff='3')):
+            with patch('time.sleep') as time_mock:
+                get_questions(1)
+                time_mock.assert_called_once_with(3)
+
+    def test_get_questions_backoff_defaults_to_zero(self):
+        with patch(
+            'charmworld.jobs.askubuntu.get_data_from_askubuntu', FakeGet()):
+            with patch('time.sleep') as time_mock:
+                get_questions(1)
+                time_mock.assert_called_once_with(0)
+
     def test_create_entries_from_questions_empty(self):
         entries = create_entries_from_questions([])
         self.assertEqual([], entries)

=== modified file 'charmworld/jobs/tests/test_github.py'
--- charmworld/jobs/tests/test_github.py	2013-10-10 17:59:16 +0000
+++ charmworld/jobs/tests/test_github.py	2013-11-18 15:35:18 +0000
@@ -8,26 +8,29 @@
     create_entries_from_pull_requests,
     get_charm_repos,
     get_pull_requests,
+    GithubRateLimitError,
     make_github_request,
 )
 from charmworld.testing import MongoTestBase
 
 
 class FakeResponse:
-    def __init__(self, json={}):
+    def __init__(self, json={}, headers={}):
         self._json = json
+        self.headers = headers
 
     def json(self):
         return self._json
 
 
 class FakeGet:
-    def __init__(self, json=None):
+    def __init__(self, json=None, headers={}):
         self.json = json
+        self.headers = headers
 
     def __call__(self, url):
         self.url = url
-        return FakeResponse(self.json)
+        return FakeResponse(self.json, self.headers)
 
 
 class GithubTest(MongoTestBase):
@@ -47,6 +50,12 @@
         with patch('requests.get', FakeGet()):
             self.assertRaises(ValueError, make_github_request, 'pulls')
 
+    def test_make_github_request_rate_limit_check(self):
+        with patch(
+            'requests.get', FakeGet(headers={'x-ratelimit-remaining': 0})):
+            self.assertRaises(
+                GithubRateLimitError, make_github_request, 'repos')
+
     def test_get_charm_repos(self):
         test_json = json.load(file('charmworld/testing/data/repos.json'))
         with patch('requests.get', FakeGet(json=test_json)) as mock: