Hi James,

Here's another round of review. Some trivial things I've missed in the
previous one, but I think the UI needs to be reworked; see below for details.

Don't forget to add the SQL migration script. After merging from the new
private-builds branch you'll have a migrations/ toplevel directory and you can
just add your script there.

> === modified file 'lib/offspring/slave/slave.py'
> --- lib/offspring/slave/slave.py	2011-09-23 14:01:26 +0000
> +++ lib/offspring/slave/slave.py	2011-10-13 18:55:08 +0000
> @@ -57,9 +60,30 @@
>          self.server.stop()
>          self.server.join()
>  
> -    def startBuild(self, projectName, configBranchURL):
> +    def add_user_to_bzr_ssh_url(self, configBranchURL, lpUser):

I've missed this in the first review, but I think Offspring in general uses
camelCase for method names; would you mind renaming this to be consistency
with others?

> +        # check for lp: format URLs. Convert these to bzr+ssh ones
> +        configBranchURL = re.sub("lp:", "bzr+ssh://bazaar.launchpad.net/",
> +                                 configBranchURL)
> +
> +        # The assumption is that this function will only be called with a
> +        # bzr+ssh URL (hence the name!)
> +        assert(re.search("bzr\+ssh://", configBranchURL))
> +
> +        # Add user to URL
> +        configBranchURL = re.sub("bzr\+ssh://",
> +                                 "bzr+ssh://" + lpUser + "@", configBranchURL)
> +
> +        return configBranchURL
> +
> +    def startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
> +
> +        if lpUser:
> +            # We don't want to pass the user separately, we want it to be
> +            # integrated into the configBranchURL.
> +            configBranchURL = self.add_user_to_bzr_ssh_url(configBranchURL, lpUser)
> +
>          self.currentBuild = ProjectBuildProcess(
> -            self, projectName, configBranchURL)
> +            self, projectName, configBranchURL, lpSSHKey)
>          logging.info("Starting build: %s %s %s" % (
>              self.currentBuild.getCommandPath(), projectName, configBranchURL))
>          self.currentBuild.start()
> @@ -159,14 +186,34 @@
>  
>  class ProjectBuildProcess(MonitoredProcess):
>  
> -    def __init__(self, supervisor, projectName, configBranchURL):
> -        MonitoredProcess.__init__(
> -            self, supervisor, [projectName, configBranchURL],
> +    def __init__(self, supervisor, projectName, configBranchURL, lpSSHKey=None):
> +        args = [projectName, configBranchURL]
> +        self.ssh_key_file = None
> +
> +        if lpSSHKey:
> +            # Write SSH key to disk. tempfile will delete it after the build
> +            # when this class goes out of scope.
> +            # Need to have this here so we can pass the temp file name
> +            # containing the key to the builder process on its command line.
> +            self.ssh_key_file = tempfile.NamedTemporaryFile()
> +            args.append(self.ssh_key_file.name)
> +            self.ssh_key_file.write(lpSSHKey)
> +            self.ssh_key_file.flush()
> +
> +        MonitoredProcess.__init__(self, supervisor, args,

Nitpick: the indentation here was changed (all arguments are not alligned) and
is no longer PEP-8 compliant. ;)

>              environment_updates={
>                  'CONFIG_SCRIPT': os.path.join(
>                      offspring_root, 'bin', 'offspring-builder-config')})
>          self.projectName = projectName
>  
> +    def __del__(self):
> +        self.tidyUp()
> +
> +    def tidyUp(self):
> +        if self.ssh_key_file:
> +            self.ssh_key_file.close()
> +            self.ssh_key_file = None
> +
>      def setup(self):
>          self.buildName = None
>  
> @@ -189,6 +236,7 @@
>          logging.debug(
>              "Setting state of slave to %s" % LexbuilderSlave.STATE_IDLE)
>          self.master.state = LexbuilderSlave.STATE_IDLE
> +        self.tidyUp()
>  
>      def getCommandPath(self):
>          return config.slave("build_command")
> 
> === added directory 'lib/offspring/slave/tests'
> === added file 'lib/offspring/slave/tests/test_private_ssh_keys.py'
> --- lib/offspring/slave/tests/test_private_ssh_keys.py	1970-01-01 00:00:00 +0000
> +++ lib/offspring/slave/tests/test_private_ssh_keys.py	2011-10-13 18:55:08 +0000
> @@ -0,0 +1,110 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +import os
> +import subprocess
> +import offspring.slave
> +import unittest
> +import xmlrpclib
> +import re
> +import os.path

You don't need to import os.path as you already import os above.

Although not a big deal, we always try to sort imports alphabetically.

> +
> +
> +class TestLexbuilderSlavePrivateSSHKeys(unittest.TestCase):
> +
> +    def nothing(self):
> +        pass
> +
> +    def setUp(self):
> +        self.slave = offspring.slave.LexbuilderSlave(('localhost', 8760))
> +        self.slave.start()
> +        self.x = xmlrpclib.ServerProxy("http://admin:password@localhost:8760")
> +        self.PBP_start = offspring.slave.slave.ProjectBuildProcess.start
> +
> +    def tearDown(self):
> +        offspring.slave.slave.ProjectBuildProcess.start = self.PBP_start
> +        self.slave.stop()
> +
> +    def test_startBuildWithCustomSSHKey(self):
> +        # Test doesn't want to trigger an actual build. The following line
> +        # prevents this.
> +        offspring.slave.slave.ProjectBuildProcess.start = self.nothing
> +
> +        args = ["project_name", "lp:~dooferlad/offspring/config", "user", "key"]
> +
> +        self.x.startBuild(args[0], args[1], args[2], args[3])

Here you could just pass *args to startBuild().

> +
> +        passed_args = self.slave.currentBuild.arguments
> +
> +        bzr_path = re.sub("lp:",
> +                          "bzr+ssh://" + args[2] + "@bazaar.launchpad.net/",
> +                          args[1])
> +
> +        expected_args = [args[0], bzr_path,
> +                         self.slave.currentBuild.ssh_key_file.name]
> +
> +        self.assertEqual(expected_args, passed_args)
> +
> +    def test_checkSSHKeyDeletedWhenBuildFinished(self):
> +        # Test doesn't want to trigger an actual build. The following line
> +        # prevents this.
> +        offspring.slave.slave.ProjectBuildProcess.start = self.nothing
> +        
> +        args = ["project_name", "lp:~dooferlad/offspring/config", "user", "key"]
> +
> +        self.x.startBuild(args[0], args[1], args[2], args[3])

Same here.

> +
> +        file_path = self.slave.currentBuild.ssh_key_file.name
> +        read_key = open(file_path).read()
> +
> +        # We should have a key written to a file for the slave to read...
> +        self.assertEqual(read_key, args[3])
> +
> +        # Can't use XMLRPC interface here because it checks to see if a build
> +        # has actually started. We just want to test the clean up logic.
> +        self.slave.stopBuild()
> +        # Having stopped the slave, the key should have been erased
> +        self.assertFalse(os.path.exists(file_path))
> +
> +    def test_checkSSHAgent(self):
> +        # This is a wrapper around a shell script that runs the commands and
> +        # we just check the output.
> +
> +        dir_of_this_file = os.path.dirname(os.path.abspath(__file__))
> +        test_script = os.path.join(dir_of_this_file, "../../build/tests/test.sh")
> +
> +        p = subprocess.Popen(test_script, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
> +
> +        stdout = p.communicate()[0]
> +
> +        lines = stdout.split("\n")
> +
> +        # Sample output:
> +        # Agent pid 9839
> +        # Identity added: /offspring/lib/offspring/build/tests/offspring_test_key (/offspring/lib/offspring/build/tests/offspring_test_key)
> +        # After loading test key, keys are:
> +        # 2048 fe:47:60:de:1a:1f:39:de:1d:91:43:d1:88:97:36:9b /offspring/lib/offspring/build/tests/offspring_test_key (RSA)
> +        # After killing ssh agent, we shouldn't be able to find any keys...
> +        # Could not open a connection to your authentication agent.
> +
> +        # Expect the first line to be of the form "Agent pid [PID]"
> +        self.assertTrue(re.search("Agent pid \d+", lines[0]),
> +                        "Test build ssh-agent check failed: Expected to find "+
> +                        "Agent pid <PID>, found:\n" + lines[0])
> +
> +        # Expect the second line to be of the form Identity added: <path to...>/offspring_test_key
> +        self.assertTrue(re.search("Identity added:.*?offspring_test_key", lines[1]),
> +                        "Test build ssh-agent check failed: Expected to find "+
> +                        "Identity added: <path to...>/offspring_test_key, found:\n" + lines[1])
> +
> +        # Ignore the third line, it is just a fixed message
> +        # Expect the fourth line to be <key fingerprint> /path/to/offspring_test_key (RSA)
> +        self.assertTrue(re.search("offspring_test_key \(RSA\)", lines[3]),
> +                        "Test build ssh-agent check failed: Expected to find "+
> +                        "offspring_test_key (RSA), found:\n" + lines[3])
> +
> +        # Ignore the fifth line, it is just a fixed message
> +        # Expect the sixth line to be "Could not open a connection to your authentication agent."
> +        self.assertTrue(re.search("^Could not open a connection to your authentication agent\.$", lines[5]),
> +                        "Test build ssh-agent check failed: Expected to find "+
> +                        "Could not open a connection to your authentication agent, found:\n" + lines[5])

You know, some lines here could do with some wrapping. ;)

> \ No newline at end of file

Oh, and maybe teach your IDE to add a newline at the end of new files?

> 
> === renamed file 'lib/offspring/slave/tests.py' => 'lib/offspring/slave/tests/tests.py'
> === modified file 'lib/offspring/web/queuemanager/admin.py'
> --- lib/offspring/web/queuemanager/admin.py	2011-02-24 05:08:42 +0000
> +++ lib/offspring/web/queuemanager/admin.py	2011-10-13 18:55:08 +0000
> @@ -2,6 +2,7 @@
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  from django.contrib import admin
> +from django import forms
>  
>  from offspring.web.queuemanager.forms import ProjectBaseForm
>  from offspring.web.queuemanager.models import (
> @@ -43,7 +44,8 @@
>  
>  
>  class ProjectAdmin(admin.ModelAdmin):
> -    fields = ['title', 'name', 'arch', 'project_group', 'launchpad_project', 'suite', 'series', 'priority', 'status', 'is_active', 'config_url', 'notes']
> +    fields = ['title', 'name', 'arch', 'project_group', 'launchpad_project', 'suite', 'series', 'priority',
> +              'status', 'is_active','config_url', 'lp_user', 'lp_ssh_key_input', 'notes']
>      list_display = ('display_name', 'arch', 'series', 'project_group', 'launchpad_project', 'is_active', 'status', 'priority', 'config_url')
>      list_filter = ['arch', 'series', 'is_active', 'project_group', 'status']
>      search_fields = ['title', 'name', 'arch', 'notes']
> 
> === modified file 'lib/offspring/web/queuemanager/forms.py'
> --- lib/offspring/web/queuemanager/forms.py	2011-10-10 14:53:58 +0000
> +++ lib/offspring/web/queuemanager/forms.py	2011-10-13 18:55:08 +0000
> @@ -14,11 +15,87 @@
>  )
>  
>  from offspring.web.queuemanager.widgets import SelectWithAddNew
> +import re

This import should be at the top, together with the other names from the
stdlib.

> +
>  
>  class ProjectBaseForm(ModelForm):
>      status = fields.CharField(max_length=200,
>          widget=fields.Select(choices=Project.STATUS_CHOICES), required=True)
>  
> +    ssh_help = ("Enter a private SSH ASCII key block, complete with begin "+
> +                "and end markers. Saved keys are not shown. If you wish to "+
> +                "erase a saved key (rather than replacing it) enter the "+
> +                "string \"ERASE\".")

Where is the help text supposed to show up? I don't see it on the UI of a
project's +edit page.

> +
> +    lp_ssh_key_input = forms.CharField(label="Launchpad User's SSH key",
> +                                       required=False,
> +                                       widget=forms.Textarea(
> +                                               attrs={'cols': 73, 'rows' : 4}),
> +                                       help_text=ssh_help)

This is a matter of preference but you could move the first argument to a new
line, thus reducing wasted space. And that way you could even move the help
text to where it belongs and avoid using a class variable for it. Something
like

    lp_ssh_key_input = forms.CharField(
        label="Launchpad User's SSH key", required=False,
        widget=forms.Textarea(attrs={'cols': 73, 'rows' : 4}),
        help_text=("..."))

Or, if you prefer one argument per line:

    lp_ssh_key_input = forms.CharField(
        label="Launchpad User's SSH key",
        required=False,
        widget=forms.Textarea(attrs={'cols': 73, 'rows' : 4}),
        help_text=("..."))

> +
> +    def __init__(self, *args, **kwargs):
> +        super(ProjectBaseForm, self).__init__(*args, **kwargs)
> +
> +        # Make sure lp_ssh_key_input turns up next to lp_user. This is needed
> +        # because I insert a special lp_ssh_key_input field in this class
> +        # (above) and without doing this it will show up last.
> +        field_order = self.fields.keyOrder
> +        field_order.pop(field_order.index('lp_ssh_key_input'))
> +        lp_user_pos = field_order.index('lp_user')
> +        field_order.insert(lp_user_pos + 1, 'lp_ssh_key_input')
> +
> +        self.ui_messages = {"ssh set":
> +                             "An SSH key has already been saved for this "+
> +                             "project. To update it paste a new one here.",

I think it'd be nice to mention here why we don't show it. What do you think?

> +                            "ssh clear":
> +                             "No SSH key has been saved for this project. "+
> +                             "Paste one here to set."}

Here you could use the style I described above as well, if you'd like.

> +
> +        # Set the form fields based on the model object
> +        if kwargs.has_key('instance'):

has_key() is deprecated in favor of "'key' in dict".

> +            instance = kwargs['instance']
> +            if instance.lp_ssh_key and instance.lp_ssh_key != "":

What values can we have in instance.lp_ssh_key? If you want to check that it's
not None nor the empty string it's common to just do "if
instance.lp_ssh_key:".

> +                self.initial['lp_ssh_key_input'] = self.ui_messages["ssh set"]
> +            else:
> +                self.initial['lp_ssh_key_input'] = self.ui_messages["ssh clear"]
> +
> +    def clean_lp_ssh_key_input(self):
> +        new_key = self.cleaned_data['lp_ssh_key_input']
> +
> +        key_search_regexp = (r"-----BEGIN \w+ PRIVATE KEY-----"+
> +                             r".*"+
> +                             r"-----END \w+ PRIVATE KEY-----")
> +        is_key = re.search(key_search_regexp, new_key, re.DOTALL | re.MULTILINE)
> +        is_message = new_key in self.ui_messages.values()
> +        
> +        if not(is_key or new_key == "" or is_message or new_key == "ERASE"):
> +            raise forms.ValidationError(
> +                            "Key block doesn't look valid - not saved.")

This is not the nicest user experience as they'd get an error if they change
the info text in the text area, but it's probably not a big deal so it may
not be worth spending too much time on this. One easy thing we could do is
put this in the help text instead of inside the text area, what do you think?

That would simplify this code significantly.

> +
> +        # Always return the cleaned data, whether you have changed it or not.
> +        return new_key
> +
> +    def save(self, commit=True):
> +        model = super(ProjectBaseForm, self).save(commit=False)

I assume you don't want the parent to commit because you're going to commit
only once when you're done here? If so, care to leave a comment to that
effect?

> +
> +        # Save the SSH key
> +        new_key = self.cleaned_data['lp_ssh_key_input']
> +        is_message = new_key in self.ui_messages.values()
> +        
> +        if new_key == "" or is_message:
> +            pass # No new key entered
> +
> +        elif new_key == "ERASE":
> +            model.lp_ssh_key = "" # Erase stored key
> +
> +        else: # Save new key (validated in clean_lp_ssh_key_input)
> +            model.lp_ssh_key = new_key
> +
> +        if commit:
> +            model.save()
> +
> +        return model

I just realized that it's not possible to remove an existing ssh key -- you
need to add a new (valid!) one to achieve that.  Also, the current UI doesn't
make it clear that the SSH key and LP user are optional, nor what they're used
for, which is not ideal.  Maybe it'd be better to move the setting of the SSH
key and LP user to a separate form altogether?  That way we don't add clutter
(that will rarely be used) to the existing form and can have two form actions
on the new form: one to save the changes and another to the SSH key and LP user
altogether.

> +
>      class Meta:
>          model = Project
>          widgets = {
> @@ -26,6 +103,7 @@
>              'series' : TextInput(attrs={'style': 'text-transform: lowercase;'}),
>              'config_url': TextInput(attrs={'size': 50}),
>              'notes' : Textarea(attrs={'cols': 73, 'rows' : 4}),
> +            'lp_ssh_key': Textarea(attrs={'cols': 73, 'rows' : 4}),
>          }
>  
>      def clean_name(self):
> 
> === modified file 'lib/offspring/web/queuemanager/models.py'
> --- lib/offspring/web/queuemanager/models.py	2011-09-29 18:10:42 +0000
> +++ lib/offspring/web/queuemanager/models.py	2011-10-13 18:55:08 +0000
> @@ -218,6 +218,13 @@
>      launchpad_project = models.ForeignKey("LaunchpadProject", blank=True, null=True)
>      status = models.CharField('status', max_length=200, null=True, choices=STATUS_CHOICES)
>      config_url = models.CharField('config URL', max_length=200) # Add some kind of validator here.
> +
> +    # The Launchpad User and SSH key are stored per project. If we stored them
> +    # per LauncpadProject, anyone who could create a Project referencing that
> +    # LaunchpadProject could get access to the private data in it.
> +    lp_user = models.CharField('Launchpad User', max_length=30)
> +    lp_ssh_key = models.TextField("!!!", blank=True, null=True, editable=False)

Not one, but 3 exclamation marks! :)

> +
>      notes = models.TextField(blank=True, null=True)
>  
>      class Meta: