Merge lp://staging/~gary/charms/oneiric/buildbot-slave/run-as-buildbot into lp://staging/~yellow/charms/oneiric/buildbot-slave/trunk

Proposed by Gary Poster
Status: Merged
Merged at revision: 13
Proposed branch: lp://staging/~gary/charms/oneiric/buildbot-slave/run-as-buildbot
Merge into: lp://staging/~yellow/charms/oneiric/buildbot-slave/trunk
Diff against target: 107 lines (+38/-7)
3 files modified
config.setuplxc.yaml (+4/-2)
config.yaml (+1/-1)
hooks/install (+33/-4)
To merge this branch: bzr merge lp://staging/~gary/charms/oneiric/buildbot-slave/run-as-buildbot
Reviewer Review Type Date Requested Status
Francesco Banconi Approve
Review via email: mp+92400@code.staging.launchpad.net

Description of the change

These changes, along with those in https://code.launchpad.net/~gary/charms/oneiric/buildbot-master/run-as-buildbot/+merge/92399 , make the buildbot slave run as the buildbot user, and generally matches the Debian setup better.

To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :

(As from the buildbot-master branch...)

The su contextmanager has an issue: the euid and egid are not set back properly when there is an error in the with block, for some reason. This can cause some problems: for some reason the juju log command hangs when the buildbot user tries to use it. Benji and I took a quick look at it and did not see a reason why they would do this. Perhaps it takes a moment to propagate?

I have not yet run the existing tests. I will do this before landing, resolving any issues. That said, this will bring up a master and slave for lpbuildbot, and at least check out the branch. Subsequently there's an error within Twisted/Buildbot itself, but it's on the right track.

Revision history for this message
Francesco Banconi (frankban) wrote :

Thank you Gary for this nice branch.

100 + # This is naive; we can make it more sophisticated (e.g., allowing
101 + # escaping dollar signs) if we discover we need it. For now,
102 + # simplicity wins.
103 + replaced_args = args.replace('$INSTALLDIR', buildbot_dir)

We could also do something like::

    formatted_args = args.format(**config)

And then, in the config file::

    script-args: "-u buildbot -e <email address hidden> -f 'Launchpad PQM' {installdir}"

It's as naive as your version, but maybe a little more generic?

60 - command('hg', 'clone', source, target)
61 + run('hg', 'clone', source, target)

Thank you for fixing all the script retrieval functions.
I have to make a note to myself: command helper returns a function!

Also thanks for your change on how the su context manager retrieves the user home directory:
please see the comment on the master MP for a suggestion on how to fix the exceptions problem too.

I've seen now buildslave is created in the install hook (great IMHO), but the same code is still present in config-changed: this could generate errors (buildslave created twice?), and requires further investigation.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: