yellow team mailing list archive

[Francesco Banconi <frankban@xxxxxxxxx>]

Review: Approve code

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_or_changed`?
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-relation-changed*

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_info_path is defined but not used. Maybe is it used in the slave charm?

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

-- 
https://code.launchpad.net/~bac/charms/oneiric/buildbot-master/dynamic-relationship/+merge/91737
Your team Launchpad Yellow Squad is subscribed to branch lp:~yellow/charms/oneiric/buildbot-master/trunk.