Code review comment for lp:~frankban/charms/oneiric/buildbot-slave/use-lpsetup

Revision history for this message
Gary Poster (gary) wrote :

Hi Francesco. Cool!

We'll want to have this merged into the official charm as well (https://code.launchpad.net/~charmers/charms/precise/buildbot-slave/trunk and https://code.launchpad.net/~charmers/charms/oneiric/buildbot-slave/trunk), so some of my comments will come from that perspective.

I think we should keep url for backwards compatibility. Say that url is deprecated, and it is an error to provide both url and source. I know it makes the code messy, but I think it is the right thing to do. I'm happy to have the associated variables named "source" in the code, once the backwards compatibility hacks have happened. Also, all the examples should use the "source" variants.

In lpbuildbot.yaml, isn't --use-urandom (or similar) an option in lpsetup also? Or do we always make the urandom->random hack with lpsetup? If we always do it, we should probably change that, IMO: people will not want (or likely need?) that for their dev machines, for instance.

That's it. Thanks!

review: Approve

« Back to merge proposal