Merge lp:~teknico/charms/precise/juju-gui/cleanup-charm-code into lp:~juju-gui/charms/precise/juju-gui/trunk

[Nicola Larosa]

More charm code cleanup.

Most of these changes were split from the former branch that added
backend unit tests, it was getting too big and unreadable.

No behavior changes, only formatting and two occasions of different
expressions of the same meaning.

https://codereview.appspot.com/9125046/

[Nicola Larosa (teknico)]

Reviewers: mp+162315_code.launchpad.net,

Message:
Please take a look.

Description:
More charm code cleanup.

Most of these changes were split from the former branch that added
backend unit tests, it was getting too big and unreadable.

No behavior changes, only formatting and two occasions of different
expressions of the same meaning.

https://code.launchpad.net/~teknico/charms/precise/juju-gui/cleanup-charm-code/+merge/162315

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/9125046/

Affected files:
   A [revision details]
   M hooks/backend.py
   M hooks/config-changed
   M hooks/start
   M hooks/utils.py
   M hooks/web-relation-joined
   M tests/test_utils.py

[lp:~teknico/charms/precise/juju-gui/cleanup-charm-code]

54. By Nicola Larosa on 2013-05-03

Merge from trunk.

[Nicola Larosa (teknico)]

Please take a look.

https://codereview.appspot.com/9125046/

[Francesco Banconi (frankban)]

LGTM, thank you!

https://codereview.appspot.com/9125046/diff/3001/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/9125046/diff/3001/tests/test_utils.py#newcode51
tests/test_utils.py:51: class TestAttrDict(unittest.TestCase):
Just curious: why this change in all testcase names?

https://codereview.appspot.com/9125046/

[Nicola Larosa (teknico)]

Thanks for the review!

https://codereview.appspot.com/9125046/diff/3001/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/9125046/diff/3001/tests/test_utils.py#newcode51
tests/test_utils.py:51: class TestAttrDict(unittest.TestCase):
frankban wrote:
> Just curious: why this change in all testcase names?

Because having "Test" as a prefix is the convention usually followed by
unittest testcases, and also because that way it is easier to
distinguish between actual testcases and support classes.

https://codereview.appspot.com/9125046/

[Gary Poster (gary)]

LGTM. Thank you.

https://codereview.appspot.com/9125046/diff/3001/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/9125046/diff/3001/hooks/backend.py#newcode66
hooks/backend.py:66: charmhelpers.log('Setting up haproxy and Apache
start up scripts.')
Heh, benji and I were just commenting on this yesterday. Thank you.

https://codereview.appspot.com/9125046/

[Nicola Larosa (teknico)]

*** Submitted:

More charm code cleanup.

Most of these changes were split from the former branch that added
backend unit tests, it was getting too big and unreadable.

No behavior changes, only formatting and two occasions of different
expressions of the same meaning.

R=frankban, gary.poster
CC=
https://codereview.appspot.com/9125046

https://codereview.appspot.com/9125046/

=== modified file 'hooks/backend.py'
--- hooks/backend.py	2013-05-02 16:51:14 +0000
+++ hooks/backend.py	2013-05-03 10:39:24 +0000
@@ -1,5 +1,5 @@
 """
-A composition system for creating backend object.
+A composition system for creating backend objects.
 
 Backends implement start(), stop() and install() methods. A backend is composed
 of many mixins and each mixin will implement any/all of those methods and all
@@ -57,13 +57,13 @@
 class UpstartMixin(object):
     """Manage (install, start, stop, etc.) some service via Upstart."""
 
-    upstart_scripts = ('haproxy.conf', )
+    upstart_scripts = ('haproxy.conf',)
     debs = ('curl', 'openssl', 'haproxy', 'apache2')
 
     def install(self, backend):
-        """Set up haproxy and nginx upstart configuration files."""
+        """Set up haproxy and Apache upstart configuration files."""
         utils.setup_apache()
-        charmhelpers.log('Setting up haproxy and nginx start up scripts.')
+        charmhelpers.log('Setting up haproxy and Apache start up scripts.')
         config = backend.config
         if backend.different(
                 'ssl-cert-path', 'ssl-cert-contents', 'ssl-key-contents'):
@@ -120,7 +120,7 @@
 
 
 class ImprovMixin(object):
-    debs = ('zookeeper', )
+    debs = ('zookeeper',)
 
     def install(self, backend):
         config = backend.config
@@ -138,7 +138,7 @@
 
 
 class GoMixin(object):
-    debs = ('python-yaml', )
+    debs = ('python-yaml',)
 
 
 class Backend(object):
@@ -185,7 +185,7 @@
         mixins.append(GuiMixin)
         mixins.append(UpstartMixin)
 
-        # record our choice mapping classes to instances
+        # Record our choice mapping classes to instances.
         for i, b in enumerate(mixins):
             if callable(b):
                 mixins[i] = b()

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2013-04-30 10:14:57 +0000
+++ hooks/config-changed	2013-05-03 10:39:24 +0000
@@ -10,17 +10,14 @@
     get_config,
     log,
 )
-from shelltoolbox import (
-    DictDiffer,
-)
+from shelltoolbox import DictDiffer
 
+from backend import Backend
 from utils import (
     config_json,
     log_hook,
 )
 
-from backend import Backend
-
 
 def main():
     config = get_config()

=== modified file 'hooks/start'
--- hooks/start	2013-04-30 10:14:57 +0000
+++ hooks/start	2013-05-03 10:39:24 +0000
@@ -1,15 +1,10 @@
 #!/usr/bin/env python2
 #-*- python -*-
 
-from charmhelpers import (
-    get_config,
-)
-
-from utils import (
-    log_hook,
-)
+from charmhelpers import get_config
 
 from backend import Backend
+from utils import log_hook
 
 
 def main():
@@ -17,6 +12,7 @@
     backend = Backend(config)
     backend.start()
 
+
 if __name__ == '__main__':
     with log_hook():
         main()

=== modified file 'hooks/utils.py'
--- hooks/utils.py	2013-05-03 08:50:46 +0000
+++ hooks/utils.py	2013-05-03 10:39:24 +0000
@@ -49,6 +49,9 @@
 import tempfile
 from urlparse import urlparse
 
+import apt
+import tempita
+
 from launchpadlib.launchpad import Launchpad
 from shelltoolbox import (
     Serializer,
@@ -69,9 +72,6 @@
     unit_get,
 )
 
-import apt
-import tempita
-
 
 AGENT = 'juju-api-agent'
 APACHE = 'apache2'
@@ -574,7 +574,7 @@
     Each method is called in the context of its mixin instance, and its
     argument is the Backend instance.
     """
-    # chain method calls through all implementing mixins
+    # Chain method calls through all implementing mixins.
     def method(self):
         for mixin in self.mixins:
             a_callable = getattr(type(mixin), name, None)
@@ -589,7 +589,7 @@
     """Helper to merge a property from a set of strategy objects
     into a unified set.
     """
-    # return merged property from every providing backend as a set
+    # Return merged property from every providing mixin as a set.
     @property
     def method(self):
         result = set()

=== modified file 'hooks/web-relation-joined'
--- hooks/web-relation-joined	2013-04-30 10:14:57 +0000
+++ hooks/web-relation-joined	2013-05-03 10:39:24 +0000
@@ -2,6 +2,7 @@
 #-*- python -*-
 
 import socket
+
 from charmhelpers import (
     get_config,
     relation_set,
@@ -12,12 +13,10 @@
 def main():
     hostname = socket.getfqdn()
     config = get_config()
-    if config['secure']:
-    	port = 443
-    else:
-    	port = 80
+    port = 443 if config['secure'] else 80
     relation_set(port=port, hostname=hostname)
 
+
 if __name__ == '__main__':
     with log_hook():
         main()

=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2013-05-03 08:50:46 +0000
+++ tests/test_utils.py	2013-05-03 10:39:24 +0000
@@ -6,10 +6,10 @@
 from simplejson import dumps
 from subprocess import CalledProcessError
 import tempfile
-import tempita
 import unittest
 
 import charmhelpers
+import tempita
 import yaml
 
 from backend import InstallMixin
@@ -42,12 +42,13 @@
     """A dict with the ability to access keys as attributes."""
 
     def __getattr__(self, attr):
-        if attr in self:
+        try:
             return self[attr]
-        raise AttributeError
-
-
-class AttrDictTest(unittest.TestCase):
+        except KeyError:
+            raise AttributeError
+
+
+class TestAttrDict(unittest.TestCase):
 
     def test_key_as_attribute(self):
         # Ensure attributes can be used to retrieve dict values.
@@ -61,7 +62,7 @@
             AttrDict().myattr
 
 
-class FirstPathInDirTest(unittest.TestCase):
+class TestFirstPathInDir(unittest.TestCase):
 
     def setUp(self):
         self.directory = tempfile.mkdtemp()
@@ -83,7 +84,7 @@
         self.assertRaises(IndexError, first_path_in_dir, self.directory)
 
 
-class GetApiAddressTest(unittest.TestCase):
+class TestGetApiAddress(unittest.TestCase):
 
     def setUp(self):
         self.base_dir = tempfile.mkdtemp()
@@ -109,7 +110,7 @@
         self.assertRaises(IOError, get_api_address, self.unit_dir)
 
 
-class LegacyJujuTest(unittest.TestCase):
+class TestLegacyJuju(unittest.TestCase):
 
     def setUp(self):
         self.base_dir = tempfile.mkdtemp()
@@ -142,7 +143,7 @@
     return [AttrDict({attr: value}) for value in values]
 
 
-class MakeCollectionTest(unittest.TestCase):
+class TestMakeCollection(unittest.TestCase):
 
     def test_factory(self):
         # Ensure the factory returns the expected object instances.
@@ -152,7 +153,7 @@
             self.assertEqual(num, instance.myattr)
 
 
-class GetByAttrTest(unittest.TestCase):
+class TestGetByAttr(unittest.TestCase):
 
     attr = 'myattr'
     collection = make_collection(attr, range(5))
@@ -186,7 +187,7 @@
         return self.file_link
 
 
-class GetReleaseFileUrlTest(unittest.TestCase):
+class TestGetReleaseFileUrl(unittest.TestCase):
 
     project = AttrDict(
         series=(
@@ -307,7 +308,7 @@
         self.assertEqual('http://example.com/0.1.0.tgz', url)
 
 
-class GetZookeeperAddressTest(unittest.TestCase):
+class TestGetZookeeperAddress(unittest.TestCase):
 
     def setUp(self):
         self.zookeeper_address = 'example.com:2000'
@@ -323,7 +324,7 @@
         self.assertEqual(self.zookeeper_address, address)
 
 
-class LogHookTest(unittest.TestCase):
+class TestLogHook(unittest.TestCase):
 
     def setUp(self):
         # Monkeypatch the charmhelpers log function.
@@ -366,7 +367,7 @@
         self.assertIn('<<< Exiting', self.output[-1])
 
 
-class ParseSourceTest(unittest.TestCase):
+class TestParseSource(unittest.TestCase):
 
     def setUp(self):
         # Monkey patch utils.CURRENT_DIR.
@@ -417,7 +418,7 @@
         self.assertTupleEqual(expected, parse_source('url:foo/bar'))
 
 
-class RenderToFileTest(unittest.TestCase):
+class TestRenderToFile(unittest.TestCase):
 
     def setUp(self):
         self.destination_file = tempfile.NamedTemporaryFile()
@@ -436,7 +437,7 @@
         self.assertEqual(expected, self.destination_file.read())
 
 
-class SaveOrCreateCertificatesTest(unittest.TestCase):
+class TestSaveOrCreateCertificates(unittest.TestCase):
 
     def setUp(self):
         base_dir = tempfile.mkdtemp()
@@ -465,7 +466,8 @@
         self.assertEqual('KeyCertificate', open(pem_file).read())
 
 
-class CmdLogTest(unittest.TestCase):
+class TestCmdLog(unittest.TestCase):
+
     def setUp(self):
         # Patch the charmhelpers 'command', which powers get_config.  The
         # result of this is the mock_config dictionary will be returned.
@@ -485,7 +487,7 @@
         self.assertTrue(line.endswith(': juju-gui@INFO \nfoo\n'))
 
 
-class StartStopTest(unittest.TestCase):
+class TestStartStop(unittest.TestCase):
 
     def setUp(self):
         self.service_names = []