Code review comment for lp:~jtv/launchpad/bug-499405-translationtemplates-buildmanager

Hi Jeroen-

  This branch looks good. I do have some questions though, and would like to have answers before I Approve this branch.

=== added file 'lib/canonical/buildd/generate_translation_templates.py'
--- lib/canonical/buildd/generate_translation_templates.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/buildd/generate_translation_templates.py 2010-01-21 12:50:24 +0000
@@ -0,0 +1,57 @@
+#! /usr/bin/python2.5
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import os.path
+import sys
+from subprocess import call
+
+
+class GenerateTranslationTemplates:
+ """Script to generate translation templates from a branch."""
+
+ def __init__(self, branch_spec):
+ """Prepare to generate templates for a branch.
+
+ :param branch_spec: Either a branch URL or the path of a local
+ branch. URLs are recognized by the occurrence of ':'. In
+ the case of a URL, this will make up a path for the branch
+ and check out the branch to there.
+ """
+ self.home = os.environ['HOME']
+ self.branch_spec = branch_spec
+
+ def _getBranch(self):
+ """Set `self.branch_dir`, and check out branch if needed."""
+ if ':' in self.branch_spec:
+ # This is a branch URL. Check out the branch.
+ self.branch_dir = os.path.join(self.home, 'source-tree')
+ self._checkout(self.branch_spec)
+ else:
+ # This is a local filesystem path. Use the branch in-place.
+ self.branch_dir = self.branch_spec
+
+ def _checkout(self, branch_url):
+ """Check out a source branch to generate from.
+
+ The branch is checked out to the location specified by
+ `self.branch_dir`.
+ """
+ command = ['/usr/bin/bzr', 'checkout', branch_url, self.branch_dir]
+ return call(command, cwd=self.home)

Is there a reason you're not using bzrlib? I know that the build slaves have special security there, but if you have bzr there, you have bzrlib.

+
+ def generate(self):
+ """Do It. Generate templates."""
+ self._getBranch()
+ # XXX JeroenVermeulen 2010-01-19 bug=509557: Actual payload goes here.
+ return 0
+

Does this mean that this is basically a no-op right now? I'm okay with this code, but I'd rather you wait for it to actually do something than land it now.

+
+if __name__ == '__main__':
+ if len(sys.argv) != 2:
+ print "Usage: %s branch" % sys.argv[0]
+ print "Where 'branch' is a branch URL or directory."
+ sys.exit(1)
+ sys.exit(GenerateTranslationTemplates(sys.argv[1]).generate())

=== added file 'lib/canonical/buildd/tests/test_generate_translation_templates.py'
--- lib/canonical/buildd/tests/test_generate_translation_templates.py 1970-01-01 00:00:00 +0000
+++ lib/canonical/buildd/tests/test_generate_translation_templates.py 2010-01-21 12:50:24 +0000
@@ -0,0 +1,57 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import os
+from unittest import TestLoader
+
+from lp.testing import TestCase
+
+from canonical.buildd.generate_translation_templates import (
+ GenerateTranslationTemplates)
+
+from canonical.launchpad.ftests.script import run_script
+from canonical.testing.layers import ZopelessDatabaseLayer
+
+
+class MockGenerateTranslationTemplates(GenerateTranslationTemplates):
+ """A GenerateTranslationTemplates with mocked _checkout."""
+ checked_out_branch = None
+
+ def _checkout(self, branch_url):
+ self.checked_out_branch = branch_url
+
+

Why does checked_out_branch exist? Is it just to demonstrate the a branch would have been checked out? Maybe that needs a comment, and maybe checked_out_branch should be a boolean.

+class TestGenerateTranslationTemplates(TestCase):
+ """Test slave-side generate-translation-templates script."""
+ def test_getBranch_url(self):
+ # If passed a branch URL, the template generation script will
+ # check out that branch into a directory called "source-tree."
+ branch_url = 'lp://~my/translation/branch'
+
+ generator = MockGenerateTranslationTemplates(branch_url)
+ generator._getBranch()
+
+ self.assertEqual(branch_url, generator.checked_out_branch)
+ self.assertTrue(generator.branch_dir.endswith('source-tree'))
+
+ def test_getBranch_dir(self):
+ # If passed a branch directory, the template generation script
+ # works directly in that directory.
+ branch_dir = '/home/me/branch'
+
+ generator = MockGenerateTranslationTemplates(branch_dir)
+ generator._getBranch()
+
+ self.assertEqual(None, generator.checked_out_branch)
+ self.assertEqual(branch_dir, generator.branch_dir)
+

In this case, why would generator.checked_out_branch be none? Shouldn't it be set to the value of branch_dir? _getBranch does call _checkout, and you've mocked it in a way that it shouldn't be None.