Merge lp:~allanlesage/qa-coverage-dashboard/jenkinsapi-singleton into lp:qa-coverage-dashboard
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 |
Related bugs: |
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:
|
Description of the change
The public Jenkins is really really slow so don't login every time, just login once!
- 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_
if _JENKINS_API is None:
else:
...
This way the _JENKINS_API singleton can only be accessed by jenkins_
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://
- 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.
This looks good except for a pep8 issue
68 + if JENKINSAPI_JENKINS == None:
should be
if JENKINSAPI_JENKINS is None: