yellow team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Review: Approve

Thanks for putting this MP together Brad. Here's my comments / TODO list. I figure we should merge  lp:~bac/charms/oneiric/buildbot-slave/dynamic-relationship into ~yellow/.../trunk and then fix these things up, so I'm going to mark the MP approved.

TODO list (slave):

[1] Pickle stuff needs removing from hooks/config-changed. 
[2]
 
243	+    if buildbot_pkg and 'buildbot-pkg' not in diff.unchanged:

I really hate this double-neg pattern. We should use:

    if buildbot_pkg and 'buildbot-pkg' in diff.added_or_changed:
        ...

(unchanged is a micro-optimisation, AIUI, but added_or_changed is so much clearer to me).

[3] Copyright info still needs to be added to every file after the #!.

[4]

IGNORE THIS POINT. I realised it was nonsense, but can't be bothered to renumber ;).

[5]

681	+from subprocess import CalledProcessError
682	+

Nit: imports not in alphabetical order.

[6]

309	+
310	+log('INSTALL HOOK:')
311	+config = get_config()
312	+
313	+bzr_ = command('bzr')
314	+
315	+buildbot_dir = config.get('installdir')
316	+

These should go in the __name__ == '__main__' block or in main().
`config` should be passed as a parameter to functions as necessary.

[7]

317	+def bzr(source, path):
318	+    apt_get_install('bzr')
319	+    target = tempfile.mktemp()
320	+    bzr('branch', source, target)

Line 320 is going to blow up. We also have a bzr_ command defined at
line 313 for actual bzr commands. We should rename bzr() to bzr_fetch()
or something (and rename all the other fetch functions, too, come to
that).

[8]

416	+class testCommand(unittest.TestCase):

Should be TestCommand.

[9]

424	+    def testArguments(self):

This may be a silly time to raise coding standards complaints, but we
seem to be sticking with PEP8 most places. Why not do it here too,
rather than using LP/Zope's hybrid standards? You just know that some
non-LP developer is going to complain about this somewhere down the
line...

[10]

459	=== removed file 'revision'

Will Juju let us get away with this?

[11]

466	=== added file 'tests/100_buildbot-slave.config.test'
540	=== added file 'tests/200_buildbot-slave.test'

(We knew this already but) these need to be Pythonised. Also, last we
checked, they don't actually work, let alone fail.

[12]

662	=== added file 'tests/helpers.py'

We probably need a better way of getting necessary helpers into tests/
than just having a copy of helpers.py there. Another symlink mebbe?


-- 
https://code.launchpad.net/~bac/charms/oneiric/buildbot-slave/dynamic-relationship/+merge/91739
Your team Launchpad Yellow Squad is requested to review the proposed merge of lp:~bac/charms/oneiric/buildbot-slave/dynamic-relationship into lp:~yellow/charms/oneiric/buildbot-slave/trunk.