Merge lp:~frankban/charms/precise/juju-gui/api-address into lp:~juju-gui/charms/precise/juju-gui/trunk

[Francesco Banconi]

Take the juju API address from the environment

The charm now tries to retrieve the juju API
address from the hook context before parsing
the machiner agent file.

This way we take advantage of a change in juju-core
currently in review, which introduces the corresponding
functionality.

https://codereview.appspot.com/9810044/

[Francesco Banconi (frankban)]

Reviewers: mp+166808_code.launchpad.net,

Message:
Please take a look.

Description:
Take the juju API address from the environment

The charm now tries to retrieve the juju API
address from the hook context before parsing
the machiner agent file.

This way we take advantage of a change in juju-core
currently in review, which introduces the corresponding
functionality.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/api-address/+merge/166808

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M hooks/utils.py
   M revision
   M tests/test_utils.py

[Nicola Larosa (teknico)]

LGTM, nice code and tests. A few trivials.

https://codereview.appspot.com/9810044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/9810044/diff/1/hooks/utils.py#newcode135
hooks/utils.py:135: base_dir = os.path.abspath(base_dir)
Sorry, can't resist: :-)

     segs = (CURRENT_DIR, '..', '..') if unit_dir is None else (unit_dir,
'..')
     base_dir = os.path.abspath(os.path.join(*segs))

https://codereview.appspot.com/9810044/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/9810044/diff/1/tests/test_utils.py#newcode97
tests/test_utils.py:97: If addresses are provided, also create a
machiner directory and an
What's a "machiner"?

https://codereview.appspot.com/9810044/diff/1/tests/test_utils.py#newcode133
tests/test_utils.py:133: # The API address is correctly retrieved from
the machiner agent file.
"machiner" again.

https://codereview.appspot.com/9810044/

[Brad Crittenden (bac)]

LGTM, thanks

https://codereview.appspot.com/9810044/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/9810044/diff/1/tests/test_utils.py#newcode98
tests/test_utils.py:98: agent file containing the addresses.
So I looked at the juju-core code and they strongly have the concept of
a 'machiner', an agent that oversees the lifecycle of a machine. So as
odd as it looks in this context I guess we should go with it.

https://codereview.appspot.com/9810044/

[Francesco Banconi (frankban)]

*** Submitted:

Take the juju API address from the environment

The charm now tries to retrieve the juju API
address from the hook context before parsing
the machiner agent file.

This way we take advantage of a change in juju-core
currently in review, which introduces the corresponding
functionality.

R=
CC=
https://codereview.appspot.com/9810044

https://codereview.appspot.com/9810044/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/9810044/diff/1/hooks/utils.py#newcode135
hooks/utils.py:135: base_dir = os.path.abspath(base_dir)
On 2013/05/31 15:22:04, teknico wrote:
> Sorry, can't resist: :-)

> segs = (CURRENT_DIR, '..', '..') if unit_dir is None else
(unit_dir, '..')
> base_dir = os.path.abspath(os.path.join(*segs))

I agree this avoids some code duplication and it's cool, but I still
consider my version slightly more readable.

https://codereview.appspot.com/9810044/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/9810044/diff/1/tests/test_utils.py#newcode98
tests/test_utils.py:98: agent file containing the addresses.
On 2013/06/03 13:08:36, bac wrote:
> So I looked at the juju-core code and they strongly have the concept
of a
> 'machiner', an agent that oversees the lifecycle of a machine. So as
odd as it
> looks in this context I guess we should go with it.

Ok.

https://codereview.appspot.com/9810044/

[Francesco Banconi (frankban)]

Thanks fro the reviews Nicola and Brad!

https://codereview.appspot.com/9810044/

=== modified file 'hooks/utils.py'
--- hooks/utils.py	2013-05-17 09:07:28 +0000
+++ hooks/utils.py	2013-05-31 14:29:31 +0000
@@ -115,13 +115,24 @@
     cmd_log(apt_get_install(*DEB_BUILD_DEPENDENCIES))
 
 
-def get_api_address(unit_dir):
-    """Return the Juju API address stored in the uniter agent.conf file."""
+def get_api_address(unit_dir=None):
+    """Return the Juju API address.
+
+    If not present in the hook context as an environment variable, try to
+    retrieve the address parsing the machiner agent.conf file.
+    """
+    api_addresses = os.getenv("JUJU_API_ADDRESSES")
+    if api_addresses is not None:
+        return api_addresses.split()[0]
+    # The JUJU_API_ADDRESSES environment variable is not included in the hooks
+    # context in older releases of juju-core.  Retrieve it from the machiner
+    # agent file instead.
     import yaml  # python-yaml is only installed if juju-core is used.
-    # XXX 2013-03-27 frankban bug=1161443:
-        # currently the uniter agent.conf file does not include the API
-        # address. For now retrieve it from the machine agent file.
-    base_dir = os.path.abspath(os.path.join(unit_dir, '..'))
+    if unit_dir is None:
+        base_dir = os.path.join(CURRENT_DIR, '..', '..')
+    else:
+        base_dir = os.path.join(unit_dir, '..')
+    base_dir = os.path.abspath(base_dir)
     for dirname in os.listdir(base_dir):
         if dirname.startswith('machine-'):
             agent_conf = os.path.join(base_dir, dirname, 'agent.conf')
@@ -390,7 +401,7 @@
         api_address = '127.0.0.1:{0}'.format(API_PORT)
     else:
         # Retrieve the juju-core API server address.
-        api_address = get_api_address(os.path.join(CURRENT_DIR, '..'))
+        api_address = get_api_address()
     context = {
         'api_address': api_address,
         'api_pem': JUJU_PEM,

=== modified file 'revision'
--- revision	2013-05-23 13:34:03 +0000
+++ revision	2013-05-31 14:29:31 +0000
@@ -1,1 +1,1 @@
-47
+48

=== modified file 'tests/test_utils.py'
--- tests/test_utils.py	2013-05-11 00:18:47 +0000
+++ tests/test_utils.py	2013-05-31 14:29:31 +0000
@@ -9,6 +9,7 @@
 import unittest
 
 import charmhelpers
+from shelltoolbox import environ
 import tempita
 import yaml
 
@@ -86,28 +87,70 @@
 
 class TestGetApiAddress(unittest.TestCase):
 
-    def setUp(self):
-        self.base_dir = tempfile.mkdtemp()
-        self.addCleanup(shutil.rmtree, self.base_dir)
-        self.unit_dir = tempfile.mkdtemp(dir=self.base_dir)
-        self.machine_dir = os.path.join(self.base_dir, 'machine-1')
-
-    def test_retrieving_address(self):
-        # The API address is correctly returned.
-        address = 'example.com:17070'
-        os.mkdir(self.machine_dir)
-        with open(os.path.join(self.machine_dir, 'agent.conf'), 'w') as conf:
-            yaml.dump({'apiinfo': {'addrs': [address]}}, conf)
-        self.assertEqual(address, get_api_address(self.unit_dir))
-
-    def test_missing_file(self):
+    env_address = 'env.example.com:17070'
+    agent_address = 'agent.example.com:17070'
+
+    @contextmanager
+    def agent_file(self, addresses=None):
+        """Set up a directory structure similar to the one created by juju.
+
+        If addresses are provided, also create a machiner directory and an
+        agent file containing the addresses.
+        Remove the directory structure when exiting from the context manager.
+        """
+        base_dir = tempfile.mkdtemp()
+        unit_dir = tempfile.mkdtemp(dir=base_dir)
+        machine_dir = os.path.join(base_dir, 'machine-1')
+        if addresses is not None:
+            os.mkdir(machine_dir)
+            with open(os.path.join(machine_dir, 'agent.conf'), 'w') as conf:
+                yaml.dump({'apiinfo': {'addrs': addresses}}, conf)
+        try:
+            yield unit_dir, machine_dir
+        finally:
+            shutil.rmtree(base_dir)
+
+    def test_retrieving_address_from_env(self):
+        # The API address is correctly retrieved from the environment.
+        with environ(JUJU_API_ADDRESSES=self.env_address):
+            self.assertEqual(self.env_address, get_api_address())
+
+    def test_multiple_addresses_in_env(self):
+        # If multiple API addresses are listed in the environment variable,
+        # the first one is returned.
+        addresses = '{} foo.example.com:42'.format(self.env_address)
+        with environ(JUJU_API_ADDRESSES=addresses):
+            self.assertEqual(self.env_address, get_api_address())
+
+    def test_both_env_and_agent_file(self):
+        # If the API address is included in both the environment and the
+        # agent.conf file, the environment variable takes precedence.
+        with environ(JUJU_API_ADDRESSES=self.env_address):
+            with self.agent_file([self.agent_address]) as (unit_dir, _):
+                self.assertEqual(self.env_address, get_api_address(unit_dir))
+
+    def test_retrieving_address_from_agent_file(self):
+        # The API address is correctly retrieved from the machiner agent file.
+        with self.agent_file([self.agent_address]) as (unit_dir, _):
+            self.assertEqual(self.agent_address, get_api_address(unit_dir))
+
+    def test_multiple_addresses_in_agent_file(self):
+        # If multiple API addresses are listed in the agent file, the first
+        # one is returned.
+        addresses = [self.agent_address, 'foo.example.com:42']
+        with self.agent_file(addresses) as (unit_dir, _):
+            self.assertEqual(self.agent_address, get_api_address(unit_dir))
+
+    def test_missing_env_and_agent_file(self):
         # An IOError is raised if the agent configuration file is not found.
-        os.mkdir(self.machine_dir)
-        self.assertRaises(IOError, get_api_address, self.unit_dir)
+        with self.agent_file() as (unit_dir, machine_dir):
+            os.mkdir(machine_dir)
+            self.assertRaises(IOError, get_api_address, unit_dir)
 
-    def test_missing_directory(self):
+    def test_missing_env_and_agent_directory(self):
         # An IOError is raised if the machine directory is not found.
-        self.assertRaises(IOError, get_api_address, self.unit_dir)
+        with self.agent_file() as (unit_dir, _):
+            self.assertRaises(IOError, get_api_address, unit_dir)
 
 
 class TestLegacyJuju(unittest.TestCase):