yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/charms/oneiric/buildbot-master/wip into lp:~yellow/charms/oneiric/buildbot-master/trunk.

Requested reviews:
  Gary Poster (gary): code

For more details, see:
https://code.launchpad.net/~bac/charms/oneiric/buildbot-master/wip/+merge/91357

Fix config-changed to save state and modify configs only as actually needed.

Add more helper functions and tests.

Added example lpbuildot.yaml configuration file.

Can be completely deployed with:

juju deploy --config <charmname>/examples/lpbuildbot.yaml --repository=. local:buildbot-master
-- 
https://code.launchpad.net/~bac/charms/oneiric/buildbot-master/wip/+merge/91357
Your team Launchpad Yellow Squad is subscribed to branch lp:~yellow/charms/oneiric/buildbot-master/trunk.

=== modified file 'config.yaml'
--- config.yaml	2012-01-25 15:21:32 +0000
+++ config.yaml	2012-02-02 21:47:30 +0000
@@ -1,4 +1,11 @@
 options:
+  buildbot-pkg:
+    description: |
+      The package name, possibly with versioning information, to be
+      installed for buildbot.  Example values are 'buildbot',
+      'buildbot/lucid', or 'buildbot=0.7.9'.
+    type: string
+    default: buildbot
   installdir:
     description: |
       The directory where the Buildbot master will be installed.
@@ -28,9 +35,22 @@
     description: |
       The user for access to a restricted URL.
     type: string
-  config-private-key: 
+  config-private-key:
     description: |
       The private key for `config-user`.
     type: string
-
-
+  extra-repository:
+    description: |
+      The full line to be inserted into an apt sources.list for a repository.
+      If called multiple times the new repository will be added but
+      ones added previously will not be removed. An example would be:
+      deb http://us.archive.ubuntu.com/ubuntu/ lucid main universe
+    type: string
+  extra-packages:
+    description: |
+      A space-separated list of packages to be installed.  The
+      repositories to use in getting these packages should have been
+      set prior to setting this value.  Calling multiple times will
+      install the newly specified packages while leaving the previous
+      ones installed.
+    type: string

=== added directory 'examples'
=== added file 'examples/lpbuildbot.yaml'
--- examples/lpbuildbot.yaml	1970-01-01 00:00:00 +0000
+++ examples/lpbuildbot.yaml	2012-02-02 21:47:30 +0000
@@ -0,0 +1,7 @@
+buildbot-master:
+  config-transport: bzr
+  config-url: http://bazaar.launchpad.net/~launchpad/lpbuildbot/public
+  extra-repository: deb http://us.archive.ubuntu.com/ubuntu/ lucid main universe
+  buildbot-pkg: buildbot/lucid
+  installdir: /var/lib/buildbot/masters/lpbuildbot
+

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2012-02-02 20:22:52 +0000
+++ hooks/config-changed	2012-02-02 21:47:30 +0000
@@ -1,42 +1,110 @@
 #!/usr/bin/python
 
-from helpers import command, Config, run
+from helpers import (
+    command,
+    DictDiffer,
+    get_config,
+    install_extra_repository,
+    load_pickle,
+    save_pickle,
+    run,
+    )
 from subprocess import CalledProcessError
 import base64
 import os
 import os.path
-
+import sys
 
 # config_file is changed via juju like:
 # juju set buildbot-master config-file=`uuencode master.cfg`
 
+CONFIG_PICKLE = "config.pkl"
+REQUIRED_KEYS = [
+    'buildbot-pkg',
+    'installdir',
+    ]
+SUPPORTED_TRANSPORTS = ['bzr']
+
+restart_required = False
+
 log = command('juju-log')
 bzr = command('bzr')
+apt_get_install = command('apt-get', 'install', '-y', '--force-yes')
 
 # Log the fact that we're about to begin the install step.
 log('--> config-changed')
 
-config = Config()
-buildbot_dir =  config['installdir']
-config_file = config['config-file']
-config_transport = config['config-transport']
-config_url = config['config-url']
+config = get_config()
+prev_config = load_pickle(CONFIG_PICKLE)
+
+diff = DictDiffer(config, prev_config)
+
+buildbot_pkg = config.get('buildbot-pkg')
+buildbot_dir =  config.get('installdir')
+config_file = config.get('config-file')
+config_transport = config.get('config-transport')
+config_url = config.get('config-url')
+extra_repo = config.get('extra-repository')
+extra_pkgs = config.get('extra-packages')
 
 log('Updating buildbot configuration.')
+log(str(config))
+
+if not diff.modified:
+    log("No configuration changes, exiting.")
+    sys.exit(0)
+
+# If all of the required keys are not present in the configuration then exit
+# with an error.
+if not all(config.get(k) for k in REQUIRED_KEYS):
+    log('All required items not configured: {}'.format(REQUIRED_KEYS))
+    log(str(config))
+    sys.exit(1)
+
+log('Configuration changes seen:')
+log(str(diff))
+
+# Add a new repository if it was just added.
+if extra_repo and 'extra-repository' in diff.added_or_changed:
+    install_extra_repository(extra_repo)
+    restart_required = True
+
+if extra_pkgs and 'extra_packages' not in diff.unchanged:
+    apt_get_install(extra_pkgs)
+    restart_required = True
+
+if 'buildbot-pkg' not in diff.unchanged:
+    apt_get_install(buildbot_pkg)
+    restart_required = True
+
+# Ensure the install directory exists.
+if not os.path.exists(buildbot_dir):
+    os.makedirs(buildbot_dir)
+
+# If asked to fetch the config file then it must be a transport we support.
+if config_url and config_transport not in SUPPORTED_TRANSPORTS:
+    log("{} is an unsupported transport".format(config_transport))
+    sys.exit(1)
+
 # Write the buildbot config to disk (fetching it if necessary).
-if config_file:
+if config_file and 'config-file' not in diff.unchanged:
+    if not os.path.exists(buildbot_dir):
+        os.makedirs(buildbot_dir)
     with open(os.path.join(buildbot_dir, 'master.cfg', 'w')) as f:
         f.write(base64.decode(config_file))
-    log('Config decoded and written.')
-elif config_transport == 'bzr' and config_url:
+    log('config_file decoded and written.')
+    restart_required = True
+elif (config_transport == 'bzr' and config_url and
+      'config-transport' not in diff.unchanged and
+      'config-url' not in diff.unchanged):
     # If the branch is private then more setup needs to be done.  The
     # gpg-agent needs to send the key over and the bzr launchpad-login
-    # needs to be set. 
-    lp_id = config['config-user']
+    # needs to be set.
+    lp_id = config.get('config-user')
     if lp_id:
         bzr('launchpad-login', lp_id)
 
-    private_key = config['config-private-key']
+    private_key = config.get('config-private-key')
     if private_key:
         # Set up the .ssh directory.
         ssh_dir = os.expanduser('~/.ssh')
@@ -44,27 +112,38 @@
         os.chmod(ssh_dir, 0700)
         with open(os.path.join(ssh_dir, 'lp_key', w)) as f:
             f.write(base64.decode(private_key))
-
     bzr('branch', '--use-existing-dir', config_url, buildbot_dir)
     run('chown', '-R', 'ubuntu:ubuntu', buildbot_dir)
+    log('configuration fetched from {}'.format(config_url))
+    restart_required = True
 else:
-    # XXX Is it an error to get to this point?
+    # Configuration file specifiers are unchanged or unrecognized.
     pass
 
 # Restart buildbot if it is running.
-pidfile = os.path.join(buildbot_dir, 'twistd.pid')
-if os.path.exists(pidfile):
-    buildbot_pid = open(pidfile).read().strip()
-    try:
-        # Is buildbot running?
-        run('kill', '-0', buildbot_pid)
-    except CalledProcessError:
-        # Buildbot isn't running, so no need to reconfigure it.
-        pass
+if restart_required:
+    pidfile = os.path.join(buildbot_dir, 'twistd.pid')
+    if os.path.exists(pidfile):
+        buildbot_pid = open(pidfile).read().strip()
+        try:
+            # Is buildbot running?
+            run('kill', '-0', buildbot_pid)
+        except CalledProcessError:
+            # Buildbot isn't running, so no need to reconfigure it.
+            pass
+        else:
+            # Buildbot is running, reconfigure it.
+            log('Reconfiguring buildbot')
+            run('buildbot', 'reconfig', buildbot_dir)
+            log('Buildbot reconfigured')
     else:
-        # Buildbot is running, reconfigure it.
-        log('Reconfiguring buildbot')
-        run('buildbot', 'reconfig', buildbot_dir)
-        log('Buildbot reconfigured')
+        # Buildbot isn't running so start afresh but only if properly
+        # configured.
+        if os.path.exists(os.path.join(buildbot_dir, 'master.cfg')):
+            run('buildbot', 'start', buildbot_dir)
+else:
+    log("Configuration changed but didn't require restarting.")
+
+save_pickle(config, CONFIG_PICKLE)
 
 log('<-- config-changed')

=== modified file 'hooks/helpers.py'
--- hooks/helpers.py	2012-02-02 18:35:04 +0000
+++ hooks/helpers.py	2012-02-02 21:47:30 +0000
@@ -1,17 +1,135 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Helper functions for writing hooks in python."""
+
+__metaclass__ = type
+__all__ = [
+    'command',
+    'Config',
+    'DictDiffer',
+    'install_extra_repository',
+    'log',
+    'run',
+    ]
+
 from collections import defaultdict
+import json
+import os
+import pickle
 import subprocess
+from textwrap import dedent
 import yaml
 
+
 def command(*base_args):
     def callable_command(*args):
-        return subprocess.check_output(base_args+args, shell=False)
+        all_args = base_args + args
+        return subprocess.check_output(all_args, shell=False)
 
     return callable_command
 
 
+log = command('juju-log')
+
+
 def run(*args):
+    log('run: ' + str(args))
     return subprocess.check_output(args, shell=False)
 
+def get_config():
+    config_get = command('config-get', '--format=json')
+    return json.loads(config_get())
+
+
+def install_extra_repository(extra_repository):
+    try:
+        run('apt-add-repository', extra_repository)
+        run('apt-get', 'update')
+    except CalledProcessError as e:
+        log("Error adding repository %s" % extra_repository)
+        log(e)
+        sys.exit(-1)
+
+
+def load_pickle(filepath):
+    prev_config = {}
+    if os.path.exists(filepath):
+        with open(filepath) as fd:
+            prev_config = pickle.load(fd)
+    return prev_config
+
+
+def save_pickle(obj, filepath):
+    with open(filepath, 'w') as fd:
+        pickle.dump(obj, fd)
+
+
+class DictDiffer:
+    """
+    Calculate the difference between two dictionaries as:
+    (1) items added
+    (2) items removed
+    (3) keys same in both but changed values
+    (4) keys same in both and unchanged values
+    """
+
+    # Based on answer by hughdbrown at:
+    # http://stackoverflow.com/questions/1165352
+
+    def __init__(self, current_dict, past_dict):
+        self.current_dict = current_dict
+        self.past_dict = past_dict
+        self.set_current = set(current_dict)
+        self.set_past = set(past_dict)
+        self.intersect = self.set_current.intersection(self.set_past)
+
+    @property
+    def added(self):
+        return self.set_current - self.intersect
+
+    @property
+    def removed(self):
+        return self.set_past - self.intersect
+
+    @property
+    def changed(self):
+        return set(key for key in self.intersect
+                   if self.past_dict[key] != self.current_dict[key])
+    @property
+    def unchanged(self):
+        return set(key for key in self.intersect
+                   if self.past_dict[key] == self.current_dict[key])
+    @property
+    def modified(self):
+        return self.current_dict != self.past_dict
+
+    @property
+    def added_or_changed(self):
+        return self.added.union(self.changed)
+
+    def _changes(self, keys):
+        new = {}
+        old = {}
+        for k in keys:
+            new[k] = self.current_dict.get(k)
+            old[k] = self.past_dict.get(k)
+        return "%s -> %s" % (old, new)
+    def __str__(self):
+        if self.modified:
+            s = dedent("""\
+            added: %s
+            removed: %s
+            changed: %s
+            unchanged: %s""") % (
+                self._changes(self.added),
+                self._changes(self.removed),
+                self._changes(self.changed),
+                list(self.unchanged))
+        else:
+            s = "no changes"
+        return s
+
 
 def unit_info(service_name, item_name, data=None):
     if data is None:
@@ -20,16 +138,3 @@
     units = services['units']
     item = units.items()[0][1][item_name]
     return item
-
-
-class Config(defaultdict):
-
-    def __init__(self):
-        super(defaultdict, self).__init__()
-        self.config_get = command('config-get')
-
-    def __missing__(self, key):
-        return self.config_get(key).strip()
-
-    def __setitem__(self, key, value):
-        raise RuntimeError('configuration is read-only')

=== modified file 'hooks/install'
--- hooks/install	2012-02-02 20:17:22 +0000
+++ hooks/install	2012-02-02 21:47:30 +0000
@@ -1,9 +1,8 @@
 #!/usr/bin/python
 
-from helpers import command, Config, run
+from helpers import command, get_config, run, log
 from subprocess import CalledProcessError
 import os
-import os.path
 import shutil
 
 log = command('juju-log')
@@ -11,18 +10,16 @@
 # Log the fact that we're about to begin the install step.
 log('--> install')
 
-config = Config()
+config = get_config()
+log("config:")
+log(str(config))
 
 buildbot_dir =  config['installdir']
 run('apt-get', 'install', '-y', 'sharutils', 'bzr')
 
-# Get the lucid version of buildbot.
-run('apt-add-repository',
-    'deb http://us.archive.ubuntu.com/ubuntu/ lucid main universe')
-run('apt-get', 'update')
-run('apt-get', 'install', '-y', 'buildbot/lucid')
+# Install the extra repository
+# Install the initially configured version of buildbot.
 
-log('Creating master in {}.'.format(buildbot_dir))
 # Since we may be installing into a pre-existing service, ensure the
 # buildbot directory is removed.
 if os.path.exists(buildbot_dir):
@@ -32,8 +29,10 @@
         # It probably wasn't running; just ignore the error.
         pass
     shutil.rmtree(buildbot_dir)
-lpbuildbot_checkout = os.path.join(os.environ['CHARM_DIR'], 'lpbuildbot')
-shutil.copytree(lpbuildbot_checkout, buildbot_dir)
+
+## run('buildbot', 'create-master', buildbot_dir)
+## shutil.copyfile(os.path.join(buildbot_dir, 'master.cfg.sample'),
+##                 os.path.join(buildbot_dir, 'master.cfg'))
 
 # Log the fact that the install step is done.
 log('<-- install')

=== modified file 'hooks/start'
--- hooks/start	2012-02-02 20:17:22 +0000
+++ hooks/start	2012-02-02 21:47:30 +0000
@@ -1,15 +1,18 @@
 #!/usr/bin/python
 
-from helpers import command, Config, run
-
-log = command('juju-log')
+from helpers import (
+    command,
+    Config,
+    log,
+    run,
+    )
 
 # Log the fact that we're about to begin the start step.
 log('--> start')
 
 config = Config()
 buildbot_dir =  config['installdir']
-run('buildbot', 'start', buildbot_dir)
+# Actually starting the buildbot happens in config-changed.
 run('open-port', '8010/TCP')
 
 # Log the fact that the start step is done.

=== modified file 'hooks/tests.py'
--- hooks/tests.py	2012-02-02 18:35:04 +0000
+++ hooks/tests.py	2012-02-02 21:47:30 +0000
@@ -1,9 +1,19 @@
+from subprocess import CalledProcessError
+import tempfile
 import unittest
+
+from helpers import (
+    command,
+    DictDiffer,
+    load_pickle,
+    save_pickle,
+    unit_info,
+    )
+
 from subprocess import CalledProcessError
-from helpers import command, unit_info
-
-
-class testCommand(unittest.TestCase):
+
+
+class TestCommand(unittest.TestCase):
 
     def testSimpleCommand(self):
         # Creating a simple command (ls) works and running the command
@@ -42,8 +52,55 @@
         ls = command('/bin/ls')
         ls('--help', '>')
 
-
-class testUnit_info(unittest.TestCase):
+    def testOneLongArgument(self):
+        ls = command('/bin/ls')
+        with self.assertRaises(CalledProcessError):
+            ls('tests.py install')
+
+    def testSplitStringWorks(self):
+        ls = command('/bin/ls')
+        ls(*'tests.py install'.split())
+
+
+class TestDictDiffer(unittest.TestCase):
+
+    def testStr(self):
+        a = dict(cow='moo', pig='oink')
+        b = dict(cow='moo', pig='oinkoink', horse='nay')
+        diff = DictDiffer(b, a)
+        s = str(diff)
+        self.assertIn("added: {'horse': None} -> {'horse': 'nay'}", s)
+        self.assertIn("removed: {} -> {}", s)
+        self.assertIn("changed: {'pig': 'oink'} -> {'pig': 'oinkoink'}", s)
+        self.assertIn("unchanged: ['cow']", s)
+
+    def testStrUnmodified(self):
+        a = dict(cow='moo', pig='oink')
+        diff = DictDiffer(a, a)
+        s = str(diff)
+        self.assertEquals('no changes', s)
+
+    def testAddedOrChanged(self):
+        a = dict(cow='moo', pig='oink')
+        b = dict(cow='moo', pig='oinkoink', horse='nay')
+        diff = DictDiffer(b, a)
+        expected = set(['horse', 'pig'])
+        self.assertEquals(expected, diff.added_or_changed)
+
+
+class TestPicklers(unittest.TestCase):
+
+    def testSaveAndLoad(self):
+        fd = tempfile.NamedTemporaryFile()
+        fn = fd.name
+        fd.close()
+        orig = dict(a=1, b=2)
+        save_pickle(orig, fn)
+        retrieved = load_pickle(fn)
+        self.assertEquals(orig, retrieved)
+
+
+class TestUnit_info(unittest.TestCase):
 
     def make_data(self, state='started'):
         return {