Merge lp:~frankban/juju-quickstart/quickstart-watch into lp:juju-quickstart

[Francesco Banconi]

Wait for the GUI to be ready and started.

Watch unit changes, open the browser at the
end of the process.

Tests: run `make check`.

QA assuming you have an ec2 Juju env named 'ec2':
- run `.venv/bin/python juju-quickstart -e ec2`.
The environment is bootstrapped, the GUI deployed,
relevant events on the GUI unit notified.
Your default browser shows the GUI when done.

https://codereview.appspot.com/15020043/

[Francesco Banconi (frankban)]

Reviewers: mp+191801_code.launchpad.net,

Message:
Please take a look.

Description:
Wait for the GUI to be ready and started.

Watch unit changes, open the browser at the
end of the process.

Tests: run `make check`.

QA assuming you have an ec2 Juju env named 'ec2':
- run `.venv/bin/python juju-quickstart -e ec2`.
The environment is bootstrapped, the GUI deployed,
relevant events on the GUI unit notified.
Your default browser shows the GUI when done.

https://code.launchpad.net/~frankban/juju-quickstart/quickstart-watch/+merge/191801

(do not edit description out of merge proposal)

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

Affected files (+296, -3 lines):
   A [revision details]
   M quickstart/__init__.py
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_utils.py
   M quickstart/utils.py

[Gary Poster (gary)]

code LGTM with small comment about comment. Trying qa now.

https://codereview.appspot.com/15020043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/15020043/diff/1/quickstart/app.py#newcode127
quickstart/app.py:127: def watch(env, service_name):
Nicely readable.

https://codereview.appspot.com/15020043/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/15020043/diff/1/quickstart/manage.py#newcode153
quickstart/manage.py:153: # there is no attached terminal.
I think the looking for an attached terminal is only important if we are
trying to ask the user for input. I originally thought we would ask the
user if he/she wanted us to open the browser, but in retrospect I think
simply defaulting to opening the browser, and having a flag to disable
this behavior, is sufficient. And fast. :-)

https://codereview.appspot.com/15020043/diff/1/quickstart/tests/test_app.py
File quickstart/tests/test_app.py (right):

https://codereview.appspot.com/15020043/diff/1/quickstart/tests/test_app.py#newcode310
quickstart/tests/test_app.py:310: # The glorious moments in the unit's
life are properly highlighted.
LOL

https://codereview.appspot.com/15020043/

[Gary Poster (gary)]

https://codereview.appspot.com/15020043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/15020043/diff/1/quickstart/app.py#newcode119
quickstart/app.py:119: charm_url = 'cs:precise/juju-gui-77'
Switch to 78? :-) This should make the deployment a lot faster since 77
is 40-something M and 78 is about 7M, IIRC.

https://codereview.appspot.com/15020043/

[Gary Poster (gary)]

make check: good. The obsessive compulsive in me wants to see coverage
for the run function, so we get 100% ;-)

I had some issues in QA but they seemed Juju-related, not quickstart
related, and as you suggested, my system has an old Juju. I will update
and try again.

During the qa, though, the text did confuse me.

---
connecting to wss://ec2-50-17-120-194.compute-1.amazonaws.com:17070
deploying Juju GUI
service deployed and exposed
juju-gui/0 deployment is pending
---

What would you think of changing the second and third lines to something
like these?

---
connecting to wss://ec2-50-17-120-194.compute-1.amazonaws.com:17070
requesting Juju GUI deployment
Juju GUI deployment request accepted
juju-gui/0 deployment is pending
---

Better ideas welcome, but I think it is weird to see "deployed and
exposed" but then "deployment is pending".

https://codereview.appspot.com/15020043/

[lp:~frankban/juju-quickstart/quickstart-watch]

17. By Francesco Banconi on 2013-10-18

Changes as per review.

18. By Francesco Banconi on 2013-10-18

Fixed tests.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/15020043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/15020043/diff/1/quickstart/app.py#newcode119
quickstart/app.py:119: charm_url = 'cs:precise/juju-gui-77'
On 2013/10/18 13:27:58, gary.poster wrote:
> Switch to 78? :-) This should make the deployment a lot faster since
77 is
> 40-something M and 78 is about 7M, IIRC.

Yeah, and it also includes the string encoding fix for the GUI server.

https://codereview.appspot.com/15020043/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/15020043/diff/1/quickstart/manage.py#newcode153
quickstart/manage.py:153: # there is no attached terminal.
On 2013/10/18 13:11:40, gary.poster wrote:
> I think the looking for an attached terminal is only important if we
are trying
> to ask the user for input. I originally thought we would ask the user
if he/she
> wanted us to open the browser, but in retrospect I think simply
defaulting to
> opening the browser, and having a flag to disable this behavior, is
sufficient.
> And fast. :-)

Cool, changed the XXX comment and the related card description.

https://codereview.appspot.com/15020043/

[lp:~frankban/juju-quickstart/quickstart-watch]

19. By Francesco Banconi on 2013-10-18

Other changes from the review.

[Gary Poster (gary)]

qa good. Very cool. Thank you!

https://codereview.appspot.com/15020043/

[Francesco Banconi (frankban)]

On 2013/10/18 13:45:07, gary.poster wrote:
> make check: good. The obsessive compulsive in me wants to see
coverage for the
> run function, so we get 100% ;-)

> I had some issues in QA but they seemed Juju-related, not quickstart
related,
> and as you suggested, my system has an old Juju. I will update and
try again.

> During the qa, though, the text did confuse me.

> ---
> connecting to wss://ec2-50-17-120-194.compute-1.amazonaws.com:17070
> deploying Juju GUI
> service deployed and exposed
> juju-gui/0 deployment is pending
> ---

> What would you think of changing the second and third lines to
something like
> these?

> ---
> connecting to wss://ec2-50-17-120-194.compute-1.amazonaws.com:17070
> requesting Juju GUI deployment
> Juju GUI deployment request accepted
> juju-gui/0 deployment is pending
> ---

> Better ideas welcome, but I think it is weird to see "deployed and
exposed" but
> then "deployment is pending".

Done, thanks for the review Gary!

https://codereview.appspot.com/15020043/

[Francesco Banconi (frankban)]

*** Submitted:

Wait for the GUI to be ready and started.

Watch unit changes, open the browser at the
end of the process.

Tests: run `make check`.

QA assuming you have an ec2 Juju env named 'ec2':
- run `.venv/bin/python juju-quickstart -e ec2`.
The environment is bootstrapped, the GUI deployed,
relevant events on the GUI unit notified.
Your default browser shows the GUI when done.

R=gary.poster
CC=
https://codereview.appspot.com/15020043

https://codereview.appspot.com/15020043/

=== modified file 'quickstart/__init__.py'
--- quickstart/__init__.py	2013-10-14 11:27:50 +0000
+++ quickstart/__init__.py	2013-10-18 14:16:02 +0000
@@ -19,7 +19,7 @@
 that it can be managed using a Web interface (the Juju GUI).
 """
 
-VERSION = (0, 0, 1)
+VERSION = (0, 1, 0)
 
 
 def get_version():

=== modified file 'quickstart/app.py'
--- quickstart/app.py	2013-10-17 16:51:55 +0000
+++ quickstart/app.py	2013-10-18 14:16:02 +0000
@@ -16,6 +16,8 @@
 
 """Juju Quickstart base application functions."""
 
+from __future__ import print_function
+import functools
 import json
 
 import jujuclient
@@ -114,9 +116,61 @@
     # XXX 2013-10-17 frankban:
         # Retrieve the URL of the last charm revision from
         # manage.jujucharms.com.
-    charm_url = 'cs:precise/juju-gui-77'
+    charm_url = 'cs:precise/juju-gui-78'
     try:
         env.deploy(service_name, charm_url, to=0)
         env.expose(service_name)
     except jujuclient.EnvError as err:
         raise ProgramExit('bad API server response: {}'.format(err.message))
+
+
+def watch(env, service_name):
+    """Start watching the first unit belonging to the given service.
+
+    Output a human readable messages from each time a relevant change is found.
+
+    The following changes are considered relevant for a healthy unit:
+        - the unit is pending;
+        - the unit is reachable;
+        - the unit is installed;
+        - the unit is started.
+
+    Stop watching and return the unit public address when the unit is started.
+    Raise a ProgramExit if the API server returns an error response, or if
+    the service unit is removed or in error.
+    """
+    unit_name = '{}/0'.format(service_name)
+    processor = functools.partial(utils.unit_changes, unit_name)
+    watcher = env.watch_changes(processor)
+    address = current_status = ''
+    while True:
+        try:
+            entity, action, data = watcher.next()
+        except jujuclient.EnvError as err:
+            raise ProgramExit(
+                'bad API server response: {}'.format(err.message))
+        # Exit with an error if the unit is removed.
+        if action == 'remove':
+            raise ProgramExit('{} unexpectedly removed'.format(unit_name))
+        # Exit with an error if the unit is in an error state.
+        status = data['Status']
+        if 'error' in status:
+            msg = '{} is in an error state: {}: {}'.format(
+                unit_name, status, data['StatusInfo'])
+            raise ProgramExit(msg)
+        # Notify when the unit becomes reachable.
+        if not address:
+            address = data['PublicAddress']
+            if address:
+                print('installing {} on {}'.format(unit_name, address))
+        # Notify status changes.
+        if status != current_status:
+            if status == 'pending':
+                print('{} deployment is pending'.format(unit_name))
+            elif status == 'installed':
+                print('{} is installed'.format(unit_name))
+            elif address and status == 'started':
+                # We can exit this loop.
+                print('{} is ready'.format(unit_name))
+                return address
+            current_status = status

=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2013-10-17 16:51:55 +0000
+++ quickstart/manage.py	2013-10-18 14:16:02 +0000
@@ -20,6 +20,7 @@
 import argparse
 import logging
 import os
+import webbrowser
 
 import quickstart
 from quickstart import (
@@ -141,5 +142,13 @@
     api_url = app.get_api_url(options.env_name)
     print('connecting to {}'.format(api_url))
     env = app.connect(api_url, options.admin_secret)
-    print('deploying Juju GUI')
+    print('requesting Juju GUI deployment')
     app.deploy_gui(env, 'juju-gui')
+    print('Juju GUI deployment request accepted')
+    address = app.watch(env, 'juju-gui')
+    url = 'https://{}'.format(address)
+    print('url: {}\npassword: {}'.format(url, options.admin_secret))
+    # XXX 2013-10-18 frankban:
+        # Add a command line option to decline opening the browser.
+    webbrowser.open(url)
+    print('done!')

=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2013-10-17 16:52:40 +0000
+++ quickstart/tests/test_app.py	2013-10-18 14:16:02 +0000
@@ -240,7 +240,7 @@
         env = mock.Mock()
         app.deploy_gui(env, 'my-gui')
         env.assert_has_calls([
-            mock.call.deploy('my-gui', 'cs:precise/juju-gui-77', to=0),
+            mock.call.deploy('my-gui', 'cs:precise/juju-gui-78', to=0),
             mock.call.expose('my-gui')
         ])
 
@@ -252,7 +252,7 @@
         with self.assert_program_exit(expected):
             app.deploy_gui(env, 'another-gui')
         env.deploy.assert_called_once_with(
-            'another-gui', 'cs:precise/juju-gui-77', to=0)
+            'another-gui', 'cs:precise/juju-gui-78', to=0)
 
     def test_other_errors(self):
         # Any other errors occurred during the process are not trapped.
@@ -262,6 +262,172 @@
         with self.assertRaises(ValueError) as context_manager:
             app.deploy_gui(env, 'juju-gui')
         env.deploy.assert_called_once_with(
-            'juju-gui', 'cs:precise/juju-gui-77', to=0)
+            'juju-gui', 'cs:precise/juju-gui-78', to=0)
         env.expose.assert_called_once_with('juju-gui')
         self.assertIs(error, context_manager.exception)
+
+
[e-mail]('__builtin__.print')
+class TestWatch(ProgramExitTestsMixin, unittest.TestCase):
+
+    address = 'unit.example.com'
+    change_pending = ('unit', 'change', {
+        'Name': 'django/42',
+        'Status': 'pending',
+        'PublicAddress': '',
+    })
+    change_reachable = ('unit', 'change', {
+        'Name': 'django/42',
+        'Status': 'pending',
+        'PublicAddress': address,
+    })
+    change_installed = ('unit', 'change', {
+        'Name': 'django/42',
+        'Status': 'installed',
+        'PublicAddress': address,
+    })
+    change_started = ('unit', 'change', {
+        'Name': 'django/42',
+        'Status': 'started',
+        'PublicAddress': address,
+    })
+    pending_call = mock.call('django/0 deployment is pending')
+    reachable_call = mock.call('installing django/0 on {}'.format(address))
+    installed_call = mock.call('django/0 is installed')
+    started_call = mock.call('django/0 is ready')
+
+    def make_env(self, changes):
+        """Create and return a patched Environment instance.
+
+        The watch_changes method of the resulting Environment object returns
+        the provided changes.
+        """
+        env = mock.Mock()
+        env.watch_changes().next.side_effect = changes
+        return env
+
+    def test_unit_life(self, mock_print):
+        # The glorious moments in the unit's life are properly highlighted.
+        env = self.make_env([
+            self.change_pending,
+            self.change_reachable,
+            self.change_installed,
+            self.change_started,
+        ])
+        address = app.watch(env, 'django')
+        self.assertEqual(self.address, address)
+        mock_print.assert_has_calls([
+            self.pending_call,
+            self.reachable_call,
+            self.installed_call,
+            self.started_call,
+        ])
+
+    def test_weird_order(self, mock_print):
+        # Strange unit evolutions are handled.
+        env = self.make_env([
+            self.change_reachable,
+            self.change_pending,
+            self.change_installed,
+            self.change_started,
+        ])
+        address = app.watch(env, 'django')
+        self.assertEqual(self.address, address)
+        mock_print.assert_has_calls([
+            self.reachable_call,
+            self.pending_call,
+            self.installed_call,
+            self.started_call,
+        ])
+
+    def test_missing_changes(self, mock_print):
+        # Only the started change is strictly required.
+        env = self.make_env([self.change_started])
+        address = app.watch(env, 'django')
+        self.assertEqual(self.address, address)
+        mock_print.assert_has_calls([
+            self.reachable_call,
+            self.started_call,
+        ])
+
+    def test_extraneous_changes(self, mock_print):
+        # Extraneous changes are properly handled.
+        env = self.make_env([
+            self.change_pending,
+            ('unit', 'change', {
+                'Name': 'django/42',
+                'Status': 'pending',
+                'PublicAddress': '',
+                'Series': 'precise',
+            }),
+            self.change_reachable,
+            self.change_installed,
+            ('unit', 'change', {
+                'Name': 'django/42',
+                'Status': 'installed',
+                'PublicAddress': self.address,
+                'Series': 'precise',
+                'MachineId': '3',
+            }),
+            self.change_started,
+        ])
+        address = app.watch(env, 'django')
+        self.assertEqual(self.address, address)
+        mock_print.assert_has_calls([
+            self.pending_call,
+            self.reachable_call,
+            self.installed_call,
+            self.started_call,
+        ])
+
+    def test_api_error(self, mock_print):
+        # A ProgramExit is raised if an error occurs in one of the API calls.
+        env = self.make_env([
+            self.change_pending,
+            self.make_env_error('next returned an error'),
+        ])
+        expected = 'bad API server response: next returned an error'
+        with self.assert_program_exit(expected):
+            app.watch(env, 'django')
+        mock_print.assert_has_calls([self.pending_call])
+
+    def test_other_errors(self, mock_print):
+        # Any other errors occurred during the process are not trapped.
+        error = ValueError('explode!')
+        env = self.make_env([self.change_installed, error])
+        with self.assertRaises(ValueError) as context_manager:
+            app.watch(env, 'django')
+        mock_print.assert_has_calls([self.reachable_call, self.installed_call])
+        self.assertIs(error, context_manager.exception)
+
+    def test_unit_removed(self, mock_print):
+        # A ProgramExit is raised if Juju removed the unit.
+        env = self.make_env([
+            self.change_pending,
+            ('unit', 'remove', {
+                'Name': 'django/42',
+                'Status': 'pending',
+                'PublicAddress': '',
+            }),
+            self.change_reachable,
+        ])
+        with self.assert_program_exit('django/0 unexpectedly removed'):
+            app.watch(env, 'django')
+        mock_print.assert_has_calls([self.pending_call])
+
+    def test_status_error(self, mock_print):
+        # A ProgramExit is raised if an the unit is found in an error state.
+        env = self.make_env([
+            self.change_pending,
+            ('unit', 'change', {
+                'Name': 'django/42',
+                'Status': 'error',
+                'StatusInfo': 'install hook failure',
+                'PublicAddress': '',
+            }),
+            self.change_reachable,
+        ])
+        expected = 'django/0 is in an error state: error: install hook failure'
+        with self.assert_program_exit(expected):
+            app.watch(env, 'django')
+        mock_print.assert_has_calls([self.pending_call])

=== modified file 'quickstart/tests/test_utils.py'
--- quickstart/tests/test_utils.py	2013-10-17 15:07:11 +0000
+++ quickstart/tests/test_utils.py	2013-10-18 14:16:02 +0000
@@ -236,3 +236,51 @@
     def test_none(self):
         # The None value is returned as is.
         self.assertIsNone(utils.utf8(None))
+
+
+class TestUnitChanges(unittest.TestCase):
+
+    unit = 'django/42'
+
+    def test_change_found(self):
+        # A relevant change is correctly found and returned.
+        change = ('unit', 'change', {'Name': self.unit, 'Status': 'pending'})
+        self.assertEqual(change, utils.unit_changes(self.unit, [change]))
+
+    def test_change_found_removed(self):
+        # A unit removal is considered a relevant change.
+        change = ('unit', 'remove', {'Name': self.unit, 'Status': 'started'})
+        self.assertEqual(change, utils.unit_changes(self.unit, [change]))
+
+    def test_not_a_unit(self):
+        # Changes to other entities are ignored.
+        change = ('service', 'change', {'Name': 'django', 'Exposed': True})
+        self.assertIsNone(utils.unit_changes(self.unit, [change]))
+
+    def test_not_a_specific_unit(self):
+        # Changes to other units are ignored.
+        change = ('unit', 'change', {'Name': 'django/0', 'Status': 'pending'})
+        self.assertIsNone(utils.unit_changes(self.unit, [change]))
+
+    def test_multiple_changes(self):
+        # A relevant change is found between multiple ones.
+        change = ('unit', 'change', {'Name': self.unit, 'Status': 'pending'})
+        changeset = [
+            ('machine', 'change', {'Id': '0', 'Status': 'started'}),
+            change,
+            ('service', 'change', {'Name': 'django', 'Exposed': True}),
+        ]
+        self.assertEqual(change, utils.unit_changes(self.unit, changeset))
+
+    def test_multiple_unit_changes(self):
+        # A changeset is not supposed to include multiple change for a single
+        # unit. The function just picks the first change.
+        data = {'Name': self.unit, 'Status': 'pending'}
+        changeset = [
+            ('unit', 'change', data),
+            ('unit', 'remove', data),
+            ('unit', 'exterminated', data),
+        ]
+        self.assertEqual(
+            ('unit', 'change', data),
+            utils.unit_changes(self.unit, changeset))

=== modified file 'quickstart/utils.py'
--- quickstart/utils.py	2013-10-17 15:07:11 +0000
+++ quickstart/utils.py	2013-10-18 14:16:02 +0000
@@ -66,8 +66,9 @@
     if env_name:
         return env_name
     # XXX 2013-10-16 frankban bug=1193244:
-        # Switch to using "juju switch --raw" in order to avoid the fragility
-        # of parsing the command output.
+        # Support the new behavior of juju-switch, currently under development:
+        # the command will just output the environment name, or exit with an
+        # error if no default environment is configured.
     # The "juju switch" command parses ~/.juju/current-environment file. If the
     # environment name is not found there, then it tries to retrieve the name
     # from the "default" section of the ~/.juju/environments.yaml file.
@@ -153,3 +154,15 @@
     if isinstance(value, unicode):
         return value.encode('utf-8')
     return value
+
+
+def unit_changes(unit_name, changeset):
+    """Parse the changeset and return the change related to the given unit.
+
+    This function is intended to be used as a processor callable for the
+    watch_changes method of quickstart.juju.Environment.
+    """
+    for change in changeset:
+        entity, _, data = change
+        if entity == 'unit' and data['Name'] == unit_name:
+            return change