Merge lp:~abentley/charms/precise/juju-reports/major-updates into lp:~juju-qa/charms/precise/juju-reports/trunk

[Aaron Bentley]

Major updates to charm.

Major updates to charm.
1. all code is common code. Anything that runs python does
2. 'dist-clean' is introduced so that devs can turn of automatic "make dist-clean" and update much faster.
3. open-port and close-port are done as part of config now, not database-relation-changed
4. open / close port use the configured ports.
5. almost all use of in_juju_reports is removed.
6. restart is removed in favour of stop (before updating source) and start (after updating source).
7. LP key is written in update_source, not install hook.
8. port is closed when the service cannot operate.
9. Launchpad user is juju-qa-bot
10. start hook fails gracefully.
11. config is parsed as yaml instead of being looked up one-by-one. This also preserves value types.
12. use os.unlink instead of subprocess.call(['rm', ...])
13. stop early if mongodb url is not known.

[Curtis Hovey (sinzui)]

In update_source() you use a try/except block...I expected a context manger. The lines aren't wrong, I just expect context managers for resources in code that YOU write.

In that same function, I see we login as the bot. This has bothered me for some time. We don't logout. Should we? Am I paranoid. I ask because I am pondering a jenkin-juju-ci subordinate charm and I feel safer if the charm can login to get private branches, but logs out when done. The user is not left with powers that have no legitimate need.

In install_cronjob() file() is used. The function is deprecated. Does this work?
    open('/etc/cron.d/ubuntu', 'w').write(str(t))

review: Needs Information (code)

[Aaron Bentley (abentley)]

> In update_source() you use a try/except block...I expected a context manger.
> The lines aren't wrong, I just expect context managers for resources in code
> that YOU write.

I think I just figured that it was specific to the context, not something that could be generalized usefully. It also does sys.exit, which would be poor behaviour for a library function (and you can't return in a context manager).

> In that same function, I see we login as the bot.

It actually just sets the Launchpad user ID. It's not like Launchpadlib. It doesn't establish OAuth credentials or anything.

> This has bothered me for
> some time. We don't logout. Should we? Am I paranoid. I ask because I am
> pondering a jenkin-juju-ci subordinate charm and I feel safer if the charm can
> login to get private branches, but logs out when done. The user is not left
> with powers that have no legitimate need.

The actual secret is the SSH key, in this case. We can delete it after pulling, if we think that's useful. But it's security-by-obscurity-- the SSH key will still be accessible in the box, just not on the filesystem.

To embrace the principle of least privilege, we'd want to grant this machine access only to the one branch it needs. But that means a separate account, and I don't like doing that.

>
> In install_cronjob() file() is used. The function is deprecated. Does this
> work?
> open('/etc/cron.d/ubuntu', 'w').write(str(t))

Oh, it's not marked as deprecated in the Python 2 docs, but I see it's not in Python 3. Learn something every day. I'll change it.

[Curtis Hovey (sinzui)]

Thank you for explaining he situation and replacing the call to file(). r=me

review: Approve (code)

[lp:~abentley/charms/precise/juju-reports/major-updates]

26. By Aaron Bentley on 2014-05-08

Use 'open' instead of 'file'.

=== modified file 'config.yaml'
--- config.yaml	2014-01-03 16:39:13 +0000
+++ config.yaml	2014-05-08 21:35:11 +0000
@@ -20,3 +20,7 @@
     type: string
     default: ""
     description: "SSH key for accessing private code on launchpad."
+  dist-clean:
+    type: boolean
+    default: true
+    description: "If enabled, run dist-clean when updating source code."

=== modified file 'hooks/common.py'
--- hooks/common.py	2014-04-18 19:19:28 +0000
+++ hooks/common.py	2014-05-08 21:35:11 +0000
@@ -1,6 +1,14 @@
-from ConfigParser import ConfigParser
+from ConfigParser import (
+    ConfigParser,
+    NoSectionError)
+import errno
 import os
+from StringIO import StringIO
 import subprocess
+import sys
+
+from Cheetah.Template import Template
+import yaml
 
 
 CHARM_DIR = os.getcwd()
@@ -17,20 +25,143 @@
     return dec_fn
 
 
+def get_ini_path():
+    return os.path.join(PROJECT_DIR, INI)
+
+
+def get_ini():
+    config = ConfigParser()
+    config.read(get_ini_path())
+    return config
+
+
 def update_or_install_ini(mongo_url=None):
-    ini_file = os.path.join(PROJECT_DIR, INI)
-    config = ConfigParser()
-    config.read([ini_file])
+    config = get_ini()
     if mongo_url is not None:
         config.set('app:main', 'mongo.url', mongo_url)
-    config.write(file(ini_file, 'w'))
-
-
-@in_juju_reports
-def restart():
+    config.write(open(get_ini_path(), 'w'))
+    return config
+
+
+def set_port(ini, open_port=True):
+    port = ini_get(ini, 'server:main', 'port')
+    if port is None:
+        return
+    cmd = 'open-port' if open_port else 'close-port'
+    subprocess.check_call([cmd, '%s/tcp' % port])
+
+
+def call_script(script_path):
     env = os.environ.copy()
     ini = os.path.join(PROJECT_DIR, INI)
     env['HOME'] = HOME
     env['INI'] = ini
-    subprocess.check_call(['scripts/stop'], env=env)
-    subprocess.check_call(['scripts/run'], env=env)
+    subprocess.check_call([script_path], env=env, cwd=PROJECT_DIR)
+    return env
+
+
+def stop():
+    try:
+        call_script('scripts/stop')
+    except OSError as e:
+        if e.errno != errno.ENOENT:
+            raise
+
+
+def start():
+    call_script('scripts/run')
+
+
+def get_config():
+    config = yaml.safe_load(StringIO(subprocess.check_output('config-get')))
+    for key in ['lp-key', 'source']:
+        if config[key] == '':
+            raise IncompleteConfig('%s not specified.' % key)
+    return config
+
+
+class IncompleteConfig(ValueError):
+    """Error class indicating that ssh is not set to a valid value."""
+
+
+@in_juju_reports
+def update_source(source_url, revno, ssh_key):
+    fd = os.open('/root/.ssh/lp_rsa', os.O_WRONLY | os.O_TRUNC | os.O_CREAT,
+                 0600)
+    try:
+        os.write(fd, ssh_key)
+    finally:
+        os.close(fd)
+    # Login to LP
+    bzr_cmd = ['bzr', 'lp-login', 'juju-qa-bot']
+    subprocess.check_call(bzr_cmd)
+
+    # Grab the code
+    revno = 'revno:%s' % revno
+    bzr_cmd = ['bzr', 'pull', '--overwrite', source_url, '-r', revno, '-d',
+               PROJECT_DIR]
+    subprocess.check_call(bzr_cmd)
+
+    # Remove the unnecessary INI files--if something tries to use them we need
+    # a failure.
+    for ini in ['test.ini', 'development.ini']:
+        try:
+            os.unlink(os.path.join(PROJECT_DIR, ini))
+        except OSError as e:
+            if e.errno != errno.ENOENT:
+                raise
+
+
+@in_juju_reports
+def install_production(dist_clean):
+    subprocess.check_call(['make', 'deploydeps'])
+    if dist_clean:
+        subprocess.check_call(['make', 'distclean'])
+    subprocess.check_call(['make', 'prodinstall'])
+    subprocess.check_call(['apt-get', 'autoremove', '--yes'])
+
+
+def install_cronjob(interval, error_email):
+    t = Template(file='templates/crontab.tmpl')
+    t.interval = interval
+    if error_email is not None:
+        t.error_email = 'MAILTO=%s' % error_email
+    else:
+        t.error_email = ''
+    t.reports_home = PROJECT_DIR
+    open('/etc/cron.d/ubuntu', 'w').write(str(t))
+
+
+def ini_get(ini, section, key):
+    try:
+        return ini.get(section, key)
+    except NoSectionError:
+        return
+
+
+def update_from_config(mongo_url=None):
+    ini = get_ini()
+    if mongo_url is None:
+        mongo_url = ini_get(ini, 'app:main', 'mongo.url')
+    try:
+        try:
+            if mongo_url is None:
+                raise IncompleteConfig('mongo url is missing.')
+            config = get_config()
+        except:
+            set_port(ini, False)
+            stop()
+            raise
+    except IncompleteConfig as e:
+        subprocess.call(['juju-log', 'Incomplete config: %s' % e])
+        sys.exit(0)
+    set_port(ini, False)
+    stop()
+    update_source(config['source'], config['revno'], config['lp-key'])
+    # ini may be updated by update_source
+    ini = get_ini()
+    install_production(config['dist-clean'])
+    ini = update_or_install_ini(mongo_url)
+    install_cronjob(config['cron-interval'], config['error-email'])
+    start()
+    set_port(ini, True)

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2014-04-18 19:19:28 +0000
+++ hooks/config-changed	2014-05-08 21:35:11 +0000
@@ -1,66 +1,5 @@
 #!/usr/bin/env python
-import os
-import subprocess
-
-from Cheetah.Template import Template
-
-from common import (
-    in_juju_reports,
-    PROJECT_DIR,
-    restart,
-    update_or_install_ini,
-)
-
-
-def rm_ini(ini_name):
-    if ini_name in os.listdir('.'):
-        subprocess.check_call(['rm', ini_name])
-
-
-@in_juju_reports
-def update_source(source_url, revno):
-    # Login to LP
-    bzr_cmd = ['bzr', 'lp-login', 'jcsackett']
-    subprocess.check_call(bzr_cmd)
-
-    # Grab the code
-    if revno == -1:
-        revno = 'HEAD'
-    revno = 'revno:%s' % revno
-    bzr_cmd = ['bzr', 'pull', '--overwrite', source_url, '-r', revno]
-    subprocess.check_call(bzr_cmd)
-
-    #Remove the unnecessary INI files--if something tries to use them we need a
-    #failure.
-    rm_ini('test.ini')
-    rm_ini('development.ini')
-
-
-@in_juju_reports
-def install_production():
-    subprocess.check_call(['make', 'deploydeps'])
-    subprocess.check_call(['make', 'distclean'])
-    subprocess.check_call(['make', 'prodinstall'])
-
-
-def install_cronjob(interval, error_email):
-    t = Template(file='templates/crontab.tmpl')
-    t.interval = interval
-    if error_email is not None:
-        t.error_email = 'MAILTO=%s' % error_email
-    else:
-        t.error_email = ''
-    t.reports_home = PROJECT_DIR
-    file('/etc/cron.d/ubuntu', 'w').write(str(t))
-
+from common import update_from_config
 
 if __name__ == '__main__':
-    source_url = subprocess.check_output(['config-get', 'source']).strip()
-    revno = subprocess.check_output(['config-get', 'revno']).strip()
-    update_source(source_url, revno)
-    update_or_install_ini()
-    install_production()
-    restart()
-    interval = subprocess.check_output(['config-get', 'cron-interval']).strip()
-    error_email = subprocess.check_output(['config-get', 'error-email']).strip()
-    install_cronjob(interval, error_email)
+    update_from_config()

=== modified file 'hooks/database-relation-broken'
--- hooks/database-relation-broken	2014-04-02 20:02:37 +0000
+++ hooks/database-relation-broken	2014-05-08 21:35:11 +0000
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
-from common import update_or_install_ini
+from common import update_from_config
 
 
 if __name__ == '__main__':
-    update_or_install_ini()
+    update_from_config()

=== modified file 'hooks/database-relation-changed'
--- hooks/database-relation-changed	2014-02-11 20:19:28 +0000
+++ hooks/database-relation-changed	2014-05-08 21:35:11 +0000
@@ -4,8 +4,7 @@
 import sys
 
 from common import (
-    restart,
-    update_or_install_ini
+    update_from_config
 )
 
 
@@ -19,6 +18,4 @@
         # We don't have the environment data we're supposed to have; by
         # convention we exit silently.
         sys.exit()
-    update_or_install_ini(mongo_url)
-    subprocess.check_call(['open-port', '6543/tcp'])
-    restart()
+    update_from_config(mongo_url)

=== modified file 'hooks/database-relation-departed'
--- hooks/database-relation-departed	2014-04-02 20:02:37 +0000
+++ hooks/database-relation-departed	2014-05-08 21:35:11 +0000
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
-from common import update_or_install_ini
+from common import update_from_config
 
 
 if __name__ == '__main__':
-    update_or_install_ini()
+    update_from_config()

=== modified file 'hooks/install'
--- hooks/install	2014-01-30 16:17:47 +0000
+++ hooks/install	2014-05-08 21:35:11 +0000
@@ -12,18 +12,10 @@
 fi
 mkdir -p $project_dir
 
-# Set up the machinery to be able to pull the project's private branch.
-# Install the ssh key for launchpad.
-cat << EOT > /root/.ssh/lp_rsa
-`config-get lp-key`
-EOT
-chmod 600 /root/.ssh/lp_rsa
-
-# Tell ssh to use the above key for accessing launchpad.
+# Tell ssh what key to use for accessing launchpad.
 cat << EOT > /root/.ssh/config
 Host bazaar.launchpad.net
     IdentityFile ~/.ssh/lp_rsa
-    User jcsackett
 EOT
 
 # Make sure launchpad is a known host so ssh isn't prompted to accept it.

=== modified file 'hooks/start'
--- hooks/start	2014-02-11 20:19:28 +0000
+++ hooks/start	2014-05-08 21:35:11 +0000
@@ -4,5 +4,8 @@
 project_dir="$home/juju-reports"
 ini="$project_dir/production.ini"
 cd $project_dir
+if [ ! -f $project_dir/scripts/stop ]; then
+  exit 0
+fi
 su ubuntu -c "HOME=$home PROJECT_DIR=$project_dir INI=$ini scripts/stop"
 su ubuntu -c "HOME=$home PROJECT_DIR=$project_dir INI=$ini scripts/run"

=== modified file 'hooks/upgrade-charm'
--- hooks/upgrade-charm	2013-12-19 14:02:42 +0000
+++ hooks/upgrade-charm	2014-05-08 21:35:11 +0000
@@ -1,3 +1,2 @@
 #!/bin/bash
 hooks/install
-hooks/config-changed