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

Proposed by Allan LeSage
Status: Merged
Approved by: Chris Gagnon
Approved revision: 771
Merged at revision: 774
Proposed branch: lp:~allanlesage/qa-coverage-dashboard/jenkinsapi-singleton
Merge into: lp:qa-coverage-dashboard
Diff against target: 88 lines (+24/-15)
2 files modified
gaps/tests/test_jenkins_pull.py (+10/-10)
gaps/util/jenkins_pull.py (+14/-5)
To merge this branch: bzr merge lp:~allanlesage/qa-coverage-dashboard/jenkinsapi-singleton
Reviewer Review Type Date Requested Status
Brendan Donegan (community) Approve
Chris Gagnon (community) Approve
Review via email: mp+219383@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Chris Gagnon (chris.gagnon) wrote :

This looks good except for a pep8 issue

68 + if JENKINSAPI_JENKINS == None:

should be
if JENKINSAPI_JENKINS is None:

review: Needs Fixing
769. By Allan LeSage

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

Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

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
Revision history for this message
Allan LeSage (allanlesage) wrote :

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/ .)

770. By Allan LeSage

Adjust to make JENKINSAPI_JENKINS local to jenkins util module.

Revision history for this message
Allan LeSage (allanlesage) wrote :

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

Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

Don't you just hate logic!

771. By Allan LeSage

Further hide the jenkinsapi.Jenkins.

Revision history for this message
Allan LeSage (allanlesage) wrote :

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.

Revision history for this message
Chris Gagnon (chris.gagnon) wrote :

lgtm

review: Approve
Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

I'm fine with that too

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'gaps/tests/test_jenkins_pull.py'
2--- gaps/tests/test_jenkins_pull.py 2014-04-21 00:36:37 +0000
3+++ gaps/tests/test_jenkins_pull.py 2014-05-21 14:35:29 +0000
4@@ -65,13 +65,13 @@
5 def setUp(self):
6 super(UrlArtifactListTestCase, self).setUp()
7
8- self.jenkinsapi_jenkins_mock = MagicMock(
9- spec=Jenkins)
10- jenkinsapi_jenkins_patch = patch(
11- 'gaps.util.jenkins_pull.Jenkins',
12- new=self.jenkinsapi_jenkins_mock)
13- jenkinsapi_jenkins_patch.start()
14- self.addCleanup(jenkinsapi_jenkins_patch.stop)
15+ self.get_jenkinsapi_jenkins_mock = MagicMock(
16+ return_value=MagicMock(spec=Jenkins))
17+ get_jenkinsapi_jenkins_patch = patch(
18+ 'gaps.util.jenkins_pull.get_jenkinsapi_jenkins',
19+ new=self.get_jenkinsapi_jenkins_mock)
20+ get_jenkinsapi_jenkins_patch.start()
21+ self.addCleanup(get_jenkinsapi_jenkins_patch.stop)
22
23 self.jenkinsapi_job_mock = MagicMock(
24 spec=job.Job)
25@@ -105,7 +105,7 @@
26 return_value=self.jenkinsapi_build_mock
27 )
28
29- self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
30+ self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
31 return_value=self.jenkinsapi_job_mock
32 )
33
34@@ -150,7 +150,7 @@
35 result)
36
37 def test_return_false_if_job_not_found(self):
38- self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
39+ self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
40 return_value=self.jenkinsapi_job_mock,
41 side_effect=OSError,
42 )
43@@ -168,7 +168,7 @@
44 ),
45 get_build=MagicMock(return_value=self.jenkinsapi_build_mock)
46 )
47- self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
48+ self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
49 return_value=jenkinsapi_job
50 )
51 result = url_artifact_list(
52
53=== modified file 'gaps/util/jenkins_pull.py'
54--- gaps/util/jenkins_pull.py 2014-05-13 01:56:03 +0000
55+++ gaps/util/jenkins_pull.py 2014-05-21 14:35:29 +0000
56@@ -9,6 +9,19 @@
57 logger = logging.getLogger('qa_dashboard')
58
59
60+def get_jenkinsapi_jenkins():
61+ """Log into Jenkins (if necessary), return our jenkinsapi.Jenkins."""
62+ if not getattr(get_jenkinsapi_jenkins, '_api', False):
63+ try:
64+ logger.info("Initializing jenkinsapi.")
65+ setattr(get_jenkinsapi_jenkins, '_api', Jenkins(JENKINS_URL))
66+ except:
67+ logger.debug("Failed to initialize jenkinsapi, trying again.")
68+ time.sleep(5)
69+ setattr(get_jenkinsapi_jenkins, '_api', Jenkins(JENKINS_URL))
70+ return getattr(get_jenkinsapi_jenkins, '_api', None)
71+
72+
73 def _get_artifact_urls(build, artifact_name):
74 """Return artifact urls from build
75
76@@ -38,11 +51,7 @@
77
78 :return: List of urls
79 """
80- try:
81- jenkapi = Jenkins(JENKINS_URL)
82- except:
83- time.sleep(5)
84- jenkapi = Jenkins(JENKINS_URL)
85+ jenkapi = get_jenkinsapi_jenkins()
86 try:
87 job = jenkapi.get_job(jenkins_job_name)
88 except:

Subscribers

People subscribed via source and target branches

to all changes: