yellow team mailing list archive

[Francesco Banconi <frankban@xxxxxxxxx>]

Francesco Banconi has proposed merging lp:~frankban/charms/oneiric/buildbot-slave/merged-run-as-buildbot into lp:~yellow/charms/oneiric/buildbot-slave/trunk with lp:~frankban/charms/oneiric/buildbot-slave/run-as-buildbot-base as a prerequisite.

Requested reviews:
  Launchpad Yellow Squad (yellow)

For more details, see:
https://code.launchpad.net/~frankban/charms/oneiric/buildbot-slave/merged-run-as-buildbot/+merge/92543

This branch includes Gary's already approved *run-as-buildbot* one: lp:~gary/charms/oneiric/buildbot-slave/run-as-buildbot

The diff is against a custom branch containing changes from current trunk and *run-as-buildbot* (at least I hope so...).

Changes:

- Fixed tests.

- Increased timeout in tests: wait_for_unit (waiting for a nicer solution in helpers).

- Tests now use the `make_charm_config_file` helper function to dynamically generate configuration files.

- Changed how script arguments are dynamically formatted in install hook.

- Added a test for script download and execution using formatted args.


Tests:

$ RESOLVE_TEST_CHARMS=1 tests/buildbot-slave.test -v 
test_deploy (__main__.TestCharm) ... ok
test_master_slave_relationship (__main__.TestCharm) ... ok
test_script (__main__.TestCharm) ... ok
test_script_args_formatting (__main__.TestCharm) ... ok

----------------------------------------------------------------------
Ran 4 tests in 281.291s

OK

-- 
https://code.launchpad.net/~frankban/charms/oneiric/buildbot-slave/merged-run-as-buildbot/+merge/92543
Your team Launchpad Yellow Squad is requested to review the proposed merge of lp:~frankban/charms/oneiric/buildbot-slave/merged-run-as-buildbot into lp:~yellow/charms/oneiric/buildbot-slave/trunk.

=== modified file 'config.setuplxc.yaml'
--- config.setuplxc.yaml	2012-02-06 21:11:38 +0000
+++ config.setuplxc.yaml	2012-02-10 18:01:43 +0000
@@ -1,8 +1,10 @@
 buildbot-slave:
   builders: lucid_lp,lucid_db_lp
   script-retrieval-method: bzr_cat
-  script-url: "http://bazaar.launchpad.net/~gmb/launchpad/neuter-setuplxc/utilities/setuplxc.py";
+  script-url: "http://bazaar.launchpad.net/~gary/launchpad/setuplxc/utilities/setuplxc.py";
   script-path: setuplxc.py
-  script-args: "-u launchpad-pqm -e launchpad-pqm@xxxxxxxxxxxxx -f 'Launchpad PQM' /home/launchpad-pqm/launchpad/"
+  # The buildbot user's home directory is /var/lib/buildout.
+  # The LP dependencies will be in /var/lib/buildout/dependencies.
+  script-args: "-u buildbot -e launchpad-pqm@xxxxxxxxxxxxx -f 'Launchpad PQM' {installdir}"
   extra-repository: deb http://us.archive.ubuntu.com/ubuntu/ lucid main universe
   buildbot-pkg: buildbot/lucid

=== modified file 'config.yaml'
--- config.yaml	2012-02-06 21:11:38 +0000
+++ config.yaml	2012-02-10 18:01:43 +0000
@@ -40,4 +40,4 @@
     description: |
       The directory where the Buildbot slave will be installed.
     type: string
-    default: /tmp/buildslave
+    default: /var/lib/buildbot/slaves/slave

=== modified file 'hooks/install'
--- hooks/install	2012-02-09 10:44:48 +0000
+++ hooks/install	2012-02-10 18:01:43 +0000
@@ -7,11 +7,16 @@
     apt_get_install,
     command,
     get_config,
+    install_extra_repository,
     log,
     log_entry,
     log_exit,
     run,
     )
+from local import (
+    config_json,
+    create_slave,
+    )
 import os
 import shlex
 import subprocess
@@ -40,21 +45,21 @@
 
 def wget(source, path):
     target = os.path.join('/tmp', path)
-    command('wget', '-O', target, source)
+    run('wget', '-O', target, source)
     return target
 
 
 def hg_fetch(source, path):
     apt_get_install('mercurial')
     target = tempfile.mktemp()
-    command('hg', 'clone', source, target)
+    run('hg', 'clone', source, target)
     return os.path.join(target, path)
 
 
 def git_fetch(source, path):
     apt_get_install('git')
     target = tempfile.mktemp()
-    command('git', 'clone', source, target)
+    run('git', 'clone', source, target)
     return os.path.join(target, path)
 
 
@@ -81,9 +86,32 @@
     method = config.get('script-retrieval-method')
     url = config.get('script-url')
     path = config.get('script-path')
-    args = config.get('script-args')
+    args = config.get('script-args', '').format(**config)
+    buildbot_pkg = config.get('buildbot-pkg')
+    extra_repo = config.get('extra-repository')
+    buildbot_dir = config.get('installdir')
+
+    if extra_repo:
+        install_extra_repository(extra_repo)
+
+    if buildbot_pkg:
+        log('Installing ' + buildbot_pkg)
+        apt_get_install(buildbot_pkg)
+        log('Creating initial buildbot slave in ' + buildbot_dir)
+        create_slave('temporary', 'temporary', buildbot_dir=buildbot_dir)
+
+    config_json.set(config)
+
     retrieve = METHODS.get(method)
     if retrieve and url and path:
+        # Make buildbot user have a shell by editing /etc/passwd.
+        # Otherwise you cannot ssh as this user, which some scripts
+        # need (e.g. those that create lxc containers).  We choose sh as
+        # a standard and basic "system" shell.
+        run('usermod', '-s', '/bin/sh', 'buildbot')
+        # This is naive; we can make it more sophisticated (e.g., allowing
+        # escaping dollar signs) if we discover we need it.  For now,
+        # simplicity wins.
         sys.exit(handle_script(retrieve, url, path, args))
 
 

=== modified file 'tests/buildbot-slave.test'
--- tests/buildbot-slave.test	2012-02-10 09:39:18 +0000
+++ tests/buildbot-slave.test	2012-02-10 18:01:43 +0000
@@ -9,6 +9,7 @@
 from helpers import (
     command,
     encode_file,
+    make_charm_config_file,
     unit_info,
     wait_for_page_contents,
     wait_for_relation,
@@ -28,7 +29,6 @@
     def setUp(self):
         self.charm_name = 'buildbot-slave'
         self.environment = os.getenv('JUJU_ENVIRONMENT')
-        self.tests_dir = os.path.dirname(os.path.abspath(__file__))
         self.started_services = set()
 
     def tearDown(self):
@@ -39,8 +39,9 @@
         args = ['deploy', service_name]
         if self.environment is not None:
             args.append('--environment=' + self.environment)
-        if config:
-            args.append('--config=' + config)
+        if config is not None:
+            config_file = make_charm_config_file(config)
+            args.append('--config=' + config_file.name)
         juju(*args)
         self.started_services.add(service_name)
 
@@ -57,7 +58,11 @@
     def test_deploy(self):
         # Ensure the charm starts correctly when deployed.
         self.deploy(self.charm_name)
-        wait_for_unit(self.charm_name)
+        # XXX 2012-02-10 frankban:
+        #     increasing the timeout to 10 minutes to avoid runtime errors
+        #     when a charm is deployed and the corresponding instance
+        #     is not already created.
+        wait_for_unit(self.charm_name, timeout=600)
         self.assertEqual('started', unit_info(self.charm_name, 'state'))
 
     def test_master_slave_relationship(self):
@@ -66,8 +71,10 @@
         # We deploy both and then wait because it's quite a bit faster.
         self.deploy(master_charm_name)
         self.deploy(self.charm_name)
-        wait_for_unit(master_charm_name)
-        wait_for_unit(self.charm_name)
+        # XXX 2012-02-10 frankban:
+        #     see above in `test_deploy()`.
+        wait_for_unit(master_charm_name, timeout=600)
+        wait_for_unit(self.charm_name, timeout=600)
         juju('set', master_charm_name, 'extra-packages=git')
         config_path = os.path.join(os.path.dirname(__file__), 'test.cfg')
         config = encode_file(config_path)
@@ -79,13 +86,21 @@
         wait_for_page_contents(url, 'buildbot-slave/')
         wait_for_page_contents(url, 'Idle')
 
-    def test_script(self):
-        # Ensure the script is run when the charm is deployed.
-        config = os.path.join(self.tests_dir, 'config.test.yaml')
-        self.deploy(self.charm_name, config=config)
-        wait_for_unit(self.charm_name)
-        address = unit_info(self.charm_name, 'public-address')
-        ssh = command(
+    def get_script_config(self, service, options=None):
+        if options is None:
+            options = {}
+        options.update({
+            'script-retrieval-method': 'bzr_cat',
+            'script-url':
+                'http://bazaar.launchpad.net/~yellow/charms/'
+                'oneiric/buildbot-slave/trunk/tests/create_file.py',
+            'script-path': 'create_file.py',
+        })
+        return {service: options}
+
+    def get_ssh_command(self, service):
+        address = unit_info(service, 'public-address')
+        return command(
             'ssh',
             '-o', 'StrictHostKeyChecking=no',
             '-o', 'UserKnownHostsFile=/dev/null',
@@ -94,9 +109,35 @@
             # after it as command line options.
             '--',
             )
+
+    def test_script(self):
+        # Ensure the script is run when the charm is deployed.
+        config = self.get_script_config(self.charm_name)
+        self.deploy(self.charm_name, config=config)
+        # XXX 2012-02-10 frankban:
+        #     see above in `test_deploy()`.
+        wait_for_unit(self.charm_name, timeout=600)
+        ssh = self.get_ssh_command(self.charm_name)
         self.assertEqual(CONTENT, ssh('cat {}'.format(PATH)))
         ssh('rm {}'.format(PATH))
 
+    def test_script_args_formatting(self):
+        # Ensure the charm correctly formats script arguments taking values
+        # from config if necessary.
+        installdir = '/tmp/test/'
+        options = {
+            'installdir': installdir,
+            'script-args': '{installdir}'
+        }
+        config = self.get_script_config(self.charm_name, options)
+        self.deploy(self.charm_name, config=config)
+        # XXX 2012-02-10 frankban:
+        #     see above in `test_deploy()`.
+        wait_for_unit(self.charm_name, timeout=600)
+        ssh = self.get_ssh_command(self.charm_name)
+        self.assertEqual(installdir, ssh('cat {}'.format(PATH)))
+        ssh('rm {}'.format(PATH))
+
 
 if __name__ == '__main__':
     unittest.main()

=== removed file 'tests/config.test.yaml'
--- tests/config.test.yaml	2012-02-09 17:58:50 +0000
+++ tests/config.test.yaml	1970-01-01 00:00:00 +0000
@@ -1,4 +0,0 @@
-buildbot-slave:
-  script-retrieval-method: bzr_cat
-  script-url: "http://bazaar.launchpad.net/~yellow/charms/oneiric/buildbot-slave/charm-tests/tests/create_file.py";
-  script-path: create_file.py

=== modified file 'tests/create_file.py'
--- tests/create_file.py	2012-02-09 17:55:11 +0000
+++ tests/create_file.py	2012-02-10 18:01:43 +0000
@@ -8,14 +8,18 @@
 
 import pwd
 import os
+import sys
 
 CONTENT = 'IT WORKS!'
 PATH = '/tmp/buildslave_test'
 
+def write_file(path, content, owner):
+    with open(path, 'w') as f:
+        f.write(content)
+    os.chmod(path, 0777)
+    userdata = pwd.getpwnam(owner)
+    os.chown(path, userdata.pw_uid, userdata.pw_gid)
 
 if __name__ == "__main__":
-    with open(PATH, 'w') as f:
-        f.write(CONTENT)
-    os.chmod(PATH, 0777)
-    userdata = pwd.getpwnam('ubuntu')
-    os.chown(PATH, userdata.pw_uid, userdata.pw_gid)
+    content = sys.argv[1] if sys.argv[1:] else CONTENT
+    sys.exit(write_file(PATH, content, 'ubuntu'))