Merge lp://staging/~bac/charms/oneiric/buildbot-master/dynamic-relationship into lp://staging/~yellow/charms/oneiric/buildbot-master/trunk
Proposed by
Brad Crittenden
| Status: | Merged |
|---|---|
| Merged at revision: | 13 |
| Proposed branch: | lp://staging/~bac/charms/oneiric/buildbot-master/dynamic-relationship |
| Merge into: | lp://staging/~yellow/charms/oneiric/buildbot-master/trunk |
| Diff against target: |
1124 lines (+689/-248) 19 files modified
README.txt (+31/-0) config.yaml (+5/-4) encode (+18/-0) examples/master.cfg (+118/-0) hooks/buildbot-relation-changed (+56/-0) hooks/config-changed (+150/-127) hooks/helpers.py (+57/-21) hooks/install (+46/-28) hooks/local.py (+131/-0) hooks/master.cfg (+38/-0) hooks/relation-name-relation-broken (+0/-2) hooks/relation-name-relation-changed (+0/-9) hooks/relation-name-relation-departed (+0/-5) hooks/relation-name-relation-joined (+0/-5) hooks/start (+14/-17) hooks/stop (+19/-16) hooks/tests.py (+0/-14) juju_wrapper (+3/-0) tests/buildbot-master.test (+3/-0) |
| To merge this branch: | bzr merge lp://staging/~bac/charms/oneiric/buildbot-master/dynamic-relationship |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Francesco Banconi | code | Approve | |
|
Review via email:
|
|||
Description of the change
Cleaned up helpers.py and local.py. Merged Graham's cleanup branch.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
Thanks Brad for this merge of really different branches. I think this should be approved in order to have a nice place in ~yellow where we can start addressing remaining issues.
Hooks:
------
*install*
This hook actually sets up minimum apt dependencies and takes care of environment re-initialization: the install dir is removed, json files are flushed, and that's ok in my opinion, since juju doesn't provide a destroy hook (e.g. invoked when `juju destroy-service` is called).
- config is handled by main(), we don't need it at module level.
The installation of buildbot itself is deferred and takes place in...
*config-changed*
Here buildbot is installed and configured.
Installing buildbot here doesn't sound nice. I suppose this is needed because we have to handle installdir and buildbot-pkg config options, and again I suppose those options are there to make lpbuildbot work... but maybe I'm wrong.
- At line 400 of the diff 'w' is not quoted.
- I've seen the double negation `something not in diff.unchanged` is often used:
what's the difference between that and `something in diff.added_
Is it just a micro-optimization? I think the latter pattern is more readable.
- slave_json is imported but not used
*start and stop*
- we should import functions using brackets.
*buildbot-
I think the master-slave handshake is well handled here, but it is not tested.
- the names json, os, get_config are imported but not used
- same for the *_pickle functions: we should remove them from helpers.py
Helpers:
--------
*helpers.py*
- generate_string is in __all__ but actually present in locals.py.
- re is used in grep() but not imported.
- we should remove the pickle functions.
I agree to use apt_get_install rather than apt_install, reusing `command`.
- we need to provide missing docstrings and UNIT TESTS.
*locals.py*
- missing trailing comma at line 762 of the diff.
- dedent is imported but not used.
- the function _get_slave_
- great idea to change the path of slave_json, so we can safely initialize it in the install hook, but we need to change that path in *hooks/master.cfg* too.
*tests.py*
- CalledProcessError is imported twice.
- pickle tests are no longer needed.
In all files we need to check that __all__ is correct and add copyright info. Concerning this, we are using AGPL in `hooks/helpers.py`, `hooks/local.py` and `tests/helpers.py`, while GPL3 is the license present in `copyright`. We should take a decision.