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

Proposed by Allan LeSage on 2014-05-13
Status: Merged
Approved by: Chris Gagnon on 2014-05-22
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 on 2014-05-21
Chris Gagnon (community) 2014-05-13 Approve on 2014-05-21
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.
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 on 2014-05-21

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

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
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 on 2014-05-21

Adjust to make JENKINSAPI_JENKINS local to jenkins util module.

Allan LeSage (allanlesage) wrote :

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

Brendan Donegan (brendan-donegan) wrote :

Don't you just hate logic!

771. By Allan LeSage on 2014-05-21

Further hide the jenkinsapi.Jenkins.

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.

Chris Gagnon (chris.gagnon) wrote :

lgtm

review: Approve
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
=== 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 @@
65 def setUp(self):65 def setUp(self):
66 super(UrlArtifactListTestCase, self).setUp()66 super(UrlArtifactListTestCase, self).setUp()
6767
68 self.jenkinsapi_jenkins_mock = MagicMock(68 self.get_jenkinsapi_jenkins_mock = MagicMock(
69 spec=Jenkins)69 return_value=MagicMock(spec=Jenkins))
70 jenkinsapi_jenkins_patch = patch(70 get_jenkinsapi_jenkins_patch = patch(
71 'gaps.util.jenkins_pull.Jenkins',71 'gaps.util.jenkins_pull.get_jenkinsapi_jenkins',
72 new=self.jenkinsapi_jenkins_mock)72 new=self.get_jenkinsapi_jenkins_mock)
73 jenkinsapi_jenkins_patch.start()73 get_jenkinsapi_jenkins_patch.start()
74 self.addCleanup(jenkinsapi_jenkins_patch.stop)74 self.addCleanup(get_jenkinsapi_jenkins_patch.stop)
7575
76 self.jenkinsapi_job_mock = MagicMock(76 self.jenkinsapi_job_mock = MagicMock(
77 spec=job.Job)77 spec=job.Job)
@@ -105,7 +105,7 @@
105 return_value=self.jenkinsapi_build_mock105 return_value=self.jenkinsapi_build_mock
106 )106 )
107107
108 self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(108 self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
109 return_value=self.jenkinsapi_job_mock109 return_value=self.jenkinsapi_job_mock
110 )110 )
111111
@@ -150,7 +150,7 @@
150 result)150 result)
151151
152 def test_return_false_if_job_not_found(self):152 def test_return_false_if_job_not_found(self):
153 self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(153 self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
154 return_value=self.jenkinsapi_job_mock,154 return_value=self.jenkinsapi_job_mock,
155 side_effect=OSError,155 side_effect=OSError,
156 )156 )
@@ -168,7 +168,7 @@
168 ),168 ),
169 get_build=MagicMock(return_value=self.jenkinsapi_build_mock)169 get_build=MagicMock(return_value=self.jenkinsapi_build_mock)
170 )170 )
171 self.jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(171 self.get_jenkinsapi_jenkins_mock.return_value.get_job = MagicMock(
172 return_value=jenkinsapi_job172 return_value=jenkinsapi_job
173 )173 )
174 result = url_artifact_list(174 result = url_artifact_list(
175175
=== 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 @@
9logger = logging.getLogger('qa_dashboard')9logger = logging.getLogger('qa_dashboard')
1010
1111
12def get_jenkinsapi_jenkins():
13 """Log into Jenkins (if necessary), return our jenkinsapi.Jenkins."""
14 if not getattr(get_jenkinsapi_jenkins, '_api', False):
15 try:
16 logger.info("Initializing jenkinsapi.")
17 setattr(get_jenkinsapi_jenkins, '_api', Jenkins(JENKINS_URL))
18 except:
19 logger.debug("Failed to initialize jenkinsapi, trying again.")
20 time.sleep(5)
21 setattr(get_jenkinsapi_jenkins, '_api', Jenkins(JENKINS_URL))
22 return getattr(get_jenkinsapi_jenkins, '_api', None)
23
24
12def _get_artifact_urls(build, artifact_name):25def _get_artifact_urls(build, artifact_name):
13 """Return artifact urls from build26 """Return artifact urls from build
1427
@@ -38,11 +51,7 @@
3851
39 :return: List of urls52 :return: List of urls
40 """53 """
41 try:54 jenkapi = get_jenkinsapi_jenkins()
42 jenkapi = Jenkins(JENKINS_URL)
43 except:
44 time.sleep(5)
45 jenkapi = Jenkins(JENKINS_URL)
46 try:55 try:
47 job = jenkapi.get_job(jenkins_job_name)56 job = jenkapi.get_job(jenkins_job_name)
48 except:57 except:

Subscribers

People subscribed via source and target branches

to all changes: