yellow team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad Yellow Squad (yellow): code

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

(Some of these changes may be in Benji's branch.)

* Standardize calling of main and logging of script enter/exit.

* Various clean up.

* Change the slave name to be the JUJU_REMOTE_UNIT string.  Retrieve name,passwd from the local cache rather than (incorrectly) trying to get it from relation-get.

* Don't return a name,passwd pair to the slave unless builders have been provided.
-- 
https://code.launchpad.net/~bac/charms/oneiric/buildbot-master/bbm-bugs/+merge/92128
Your team Launchpad Yellow Squad is requested to review the proposed merge of lp:~bac/charms/oneiric/buildbot-master/bbm-bugs into lp:~yellow/charms/oneiric/buildbot-master/trunk.

=== modified file 'hooks/buildbot-relation-changed'
--- hooks/buildbot-relation-changed	2012-02-07 17:30:54 +0000
+++ hooks/buildbot-relation-changed	2012-02-08 20:27:18 +0000
@@ -3,8 +3,12 @@
 # Copyright 2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import os
+
 from helpers import (
     log,
+    log_entry,
+    log_exit,
     relation_get,
     relation_set,
     )
@@ -25,7 +29,7 @@
     return value
 
 
-def update_slave_json(builders, name, passwd):
+def update_slave_json(name, passwd, builders):
     slave_info = slave_json.get()
     slave_info[name] = (passwd, builders)
     slave_json.set(slave_info)
@@ -36,17 +40,25 @@
     builders = filter(
         None,
         (b.strip() for b in relation_get('builders').split(',')))
+    if not builders:
+        log("No builders set.  Returning.")
+        return
     log("builders: {}".format(builders))
-    name = get_or_create('name', prefix='slave-')
-    passwd = get_or_create('passwd')
-    if builders:
-        update_slave_json(builders, name, passwd)
-        log("Reconfiguring buildbot.")
-        buildbot_reconfig()
+    name = os.environ.get('JUJU_REMOTE_UNIT')
+    slave_info = slave_json.get()
+    passwd, dummy = slave_info.get(name, (None, None))
+    if passwd is None:
+        passwd = generate_string()
+    update_slave_json(name, passwd, builders)
+    log("Reconfiguring buildbot.")
+    buildbot_reconfig()
     log("Sending name and password to the slave.")
     relation_set(name=name, passwd=passwd)
 
 
 if __name__ == '__main__':
-    log('BUILDBOT-RELATION-CHANGED HOOK:')
-    main()
+    log_entry()
+    try:
+        main()
+    finally:
+        log_exit()

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2012-02-08 09:57:46 +0000
+++ hooks/config-changed	2012-02-08 20:27:18 +0000
@@ -18,6 +18,8 @@
     get_config,
     install_extra_repository,
     log,
+    log_entry,
+    log_exit,
     run,
     )
 from local import (
@@ -65,6 +67,7 @@
 
     # Add a new repository if it was just added.
     added_or_changed = diff.added_or_changed
+
     if extra_repo and 'extra-repository' in added_or_changed:
         install_extra_repository(extra_repo)
         restart_required = True
@@ -124,6 +127,7 @@
 
 def initialize_buildbot(config):
     # Initialize the buildbot directory and (re)start buildbot.
+    log("Initializing buildbot")
     buildbot_dir =  config.get('installdir')
     master_cfg_path = os.path.join(buildbot_dir, 'master.cfg')
     shutil.move(master_cfg_path, master_cfg_path + '.original')
@@ -158,9 +162,8 @@
     if not os.path.exists(buildbot_dir):
         os.makedirs(buildbot_dir)
 
-    restart_required = (
-        handle_config_changes(config, diff) or
-        configure_buildbot(config, diff))
+    restart_required = handle_config_changes(config, diff)
+    restart_required |= configure_buildbot(config, diff)
 
     master_cfg_path = os.path.join(buildbot_dir, 'master.cfg')
     if restart_required and os.path.exists(master_cfg_path):
@@ -172,5 +175,8 @@
 
 
 if __name__ == '__main__':
-    log('CONFIG-CHANGED HOOK:')
-    main()
+    log_entry()
+    try:
+        main()
+    finally:
+        log_exit()

=== modified file 'hooks/helpers.py'
--- hooks/helpers.py	2012-02-07 14:37:13 +0000
+++ hooks/helpers.py	2012-02-08 20:27:18 +0000
@@ -13,6 +13,8 @@
     'grep',
     'install_extra_repository',
     'log',
+    'log_entry',
+    'log_exit',
     'run',
     'relation_get',
     'relation_set',
@@ -58,7 +60,6 @@
 
 
 def run(*args):
-    log('run: ' + str(args))
     return subprocess.check_output(args, shell=False)
 
 
@@ -108,6 +109,18 @@
     return line.split('=')[1].strip('"\' ')
 
 
+def _script_name():
+    return os.path.basename(sys.argv[0])
+
+
+def log_entry():
+    log("<-- Entering {}".format(_script_name()))
+
+
+def log_exit():
+    log("--> Exiting {}".format(_script_name()))
+
+
 class DictDiffer:
     """
     Calculate the difference between two dictionaries as:
@@ -179,7 +192,6 @@
 
     def __init__(self, path, default=None, serialize=None, deserialize=None):
         self.path = path
-        log("Serializer path: " + self.path)
         self.default = default or {}
         self.serialize = serialize or json.dump
         self.deserialize = deserialize or json.load

=== modified file 'hooks/install'
--- hooks/install	2012-02-07 17:30:54 +0000
+++ hooks/install	2012-02-08 20:27:18 +0000
@@ -11,6 +11,8 @@
     apt_get_install,
     get_config,
     log,
+    log_entry,
+    log_exit,
     run,
     )
 from local import (
@@ -44,5 +46,8 @@
 
 
 if __name__ == '__main__':
-    log('INSTALL HOOK:')
-    main()
+    log_entry()
+    try:
+        main()
+    finally:
+        log_exit()

=== modified file 'hooks/local.py'
--- hooks/local.py	2012-02-07 14:34:40 +0000
+++ hooks/local.py	2012-02-08 20:27:18 +0000
@@ -11,6 +11,7 @@
     'config_json',
     'create_slave',
     'generate_string',
+    'HTTP_PORT_PROTOCOL',
     'slave_json',
     ]
 
@@ -26,6 +27,9 @@
     )
 
 
+HTTP_PORT_PROTOCOL = '8010/TCP'
+
+
 def _get_buildbot_dir():
     config = get_config()
     return config.get('installdir')

=== modified file 'hooks/start'
--- hooks/start	2012-02-07 13:29:08 +0000
+++ hooks/start	2012-02-08 20:27:18 +0000
@@ -5,15 +5,21 @@
 
 from helpers import (
     log,
+    log_entry,
+    log_exit,
     run,
     )
+from local import HTTP_PORT_PROTOCOL
 
 
 def main():
     # Actually starting the buildbot happens in config-changed.
-    run('open-port', '8010/TCP')
+    run('open-port', HTTP_PORT_PROTOCOL)
 
 
 if __name__ == '__main__':
-    log('START HOOK:')
-    main()
+    log_entry()
+    try:
+        main()
+    finally:
+        log_exit()

=== modified file 'hooks/stop'
--- hooks/stop	2012-02-07 13:29:08 +0000
+++ hooks/stop	2012-02-08 20:27:18 +0000
@@ -6,18 +6,24 @@
 from helpers import (
     get_config,
     log,
+    log_entry,
+    log_exit,
     run,
     )
+from local import HTTP_PORT_PROTOCOL
 
 
 def main():
     config = get_config()
     log('Stopping buildbot in {}'.format(config['installdir']))
-    run('close-port', '8010/TCP')
+    run('close-port', HTTP_PORT_PROTOCOL)
 
     log('Finished stopping buildbot')
 
 
 if __name__ == '__main__':
-    log('STOP HOOK:')
-    main()
+    log_entry()
+    try:
+        main()
+    finally:
+        log_exit()