Merge lp:~allanlesage/qa-coverage-dashboard/jenkinsapi-singleton into lp:qa-coverage-dashboard

[Allan LeSage]

The public Jenkins is really really slow so don't login every time, just login once!

[Chris Gagnon (chris.gagnon)]

This looks good except for a pep8 issue

68 + if JENKINSAPI_JENKINS == None:

should be
if JENKINSAPI_JENKINS is None:

review: Needs Fixing

[lp:~allanlesage/qa-coverage-dashboard/jenkinsapi-singleton]

769. By Allan LeSage on 2014-05-21

Change == to is per chris.gagnon's request.

[Brendan Donegan (brendan-donegan)]

I don't like playing devils advocate, but globals should really be avoided - I would personally prefer:

_JENKINS_API = None

def get_jenkinsapi_jenkins():
    if _JENKINS_API is None:
        _JENKINS_API = Jenkins(JENKINS_URL)
    else:
         ...

This way the _JENKINS_API singleton can only be accessed by jenkins_pull._JENKINS_API, which is clear it is not meant to be used directly.

review: Needs Fixing

[Allan LeSage (allanlesage)]

Brendan, I need the global just to fix the setting of _JENKINS_API, i.e. your code appears to require the global declaration also. We would add the leading underscore if the use of these global constants (in settings files) was not already rampant in Django :) . (Offering this paste as demonstration: http://pastebin.ubuntu.com/7496861/ .)

[lp:~allanlesage/qa-coverage-dashboard/jenkinsapi-singleton]

770. By Allan LeSage on 2014-05-21

Adjust to make JENKINSAPI_JENKINS local to jenkins util module.

[Allan LeSage (allanlesage)]

Brendan persuaded me using logic; ChrisGagnon pls. approve and then we'll merge.

[Brendan Donegan (brendan-donegan)]

Don't you just hate logic!

[lp:~allanlesage/qa-coverage-dashboard/jenkinsapi-singleton]

771. By Allan LeSage on 2014-05-21

Further hide the jenkinsapi.Jenkins.

[Allan LeSage (allanlesage)]

Discussed with thomi and ChrisGagnon; thomi demonstrated the use of functools.partial but I don't want to use that here because it requires instantiating the jenkinsapi.Jenkins at a level above, and I don't want the top-level code to have to know about Jenkins. Instead I'm hiding the instantiated singleton in a function attribute as thomi also demonstrated.

[Chris Gagnon (chris.gagnon)]

lgtm

review: Approve

[Brendan Donegan (brendan-donegan)]

I'm fine with that too

review: Approve

=== modified file 'gaps/tests/test_jenkins_pull.py'
--- gaps/tests/test_jenkins_pull.py	2014-04-21 00:36:37 +0000
+++ gaps/tests/test_jenkins_pull.py	2014-05-21 14:35:29 +0000
@@ -65,13 +65,13 @@
     def setUp(self):
         super(UrlArtifactListTestCase, self).setUp()
 
-        self.jenkinsapi_jenkins_mock = MagicMock(
-            spec=Jenkins)
-        jenkinsapi_jenkins_patch = patch(
-            'gaps.util.jenkins_pull.Jenkins',
-            new=self.jenkinsapi_jenkins_mock)
-        jenkinsapi_jenkins_patch.start()
-        self.addCleanup(jenkinsapi_jenkins_patch.stop)
+        self.get_jenkinsapi_jenkins_mock = MagicMock(
+            return_value=MagicMock(spec=Jenkins))
+        get_jenkinsapi_jenkins_patch = patch(
+            'gaps.util.jenkins_pull.get_jenkinsapi_jenkins',
+            new=self.get_jenkinsapi_jenkins_mock)
+        get_jenkinsapi_jenkins_patch.start()
+        self.addCleanup(get_jenkinsapi_jenkins_patch.stop)
 
         self.jenkinsapi_job_mock = MagicMock(
             spec=job.Job)
@@ -105,7 +105,7 @@
             return_value=self.jenkinsapi_build_mock
         )
 
-        self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
+        self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
             return_value=self.jenkinsapi_job_mock
         )
 
@@ -150,7 +150,7 @@
             result)
 
     def test_return_false_if_job_not_found(self):
-        self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
+        self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
             return_value=self.jenkinsapi_job_mock,
             side_effect=OSError,
         )
@@ -168,7 +168,7 @@
             ),
             get_build=MagicMock(return_value=self.jenkinsapi_build_mock)
         )
-        self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
+        self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
             return_value=jenkinsapi_job
         )
         result = url_artifact_list(

=== modified file 'gaps/util/jenkins_pull.py'
--- gaps/util/jenkins_pull.py	2014-05-13 01:56:03 +0000
+++ gaps/util/jenkins_pull.py	2014-05-21 14:35:29 +0000
@@ -9,6 +9,19 @@
 logger = logging.getLogger('qa_dashboard')
 
 
+def get_jenkinsapi_jenkins():
+    """Log into Jenkins (if necessary), return our jenkinsapi.Jenkins."""
+    if not getattr(get_jenkinsapi_jenkins, '_api', False):
+        try:
+            logger.info("Initializing jenkinsapi.")
+            setattr(get_jenkinsapi_jenkins, '_api', Jenkins(JENKINS_URL))
+        except:
+            logger.debug("Failed to initialize jenkinsapi, trying again.")
+            time.sleep(5)
+            setattr(get_jenkinsapi_jenkins, '_api', Jenkins(JENKINS_URL))
+    return getattr(get_jenkinsapi_jenkins, '_api', None)
+
+
 def _get_artifact_urls(build, artifact_name):
     """Return artifact urls from build
 
@@ -38,11 +51,7 @@
 
     :return: List of urls
     """
-    try:
-        jenkapi = Jenkins(JENKINS_URL)
-    except:
-        time.sleep(5)
-        jenkapi = Jenkins(JENKINS_URL)
+    jenkapi = get_jenkinsapi_jenkins()
     try:
         job = jenkapi.get_job(jenkins_job_name)
     except: