Merge lp:~frankban/juju-quickstart/remove-jenv into lp:juju-quickstart

[Francesco Banconi]

Jenv files removal functionality.

Add the ability to remove jenv files
from the Juju home.

Test: `make check`.

QA:
- `juju bootstrap` an environment;
- create a new user for the environment:
  `juju user add myuser --generate -o ~/.juju/environments/myenv`;
- run quickstart in interactive mode:
  `.venv/bin/python juju-quickstart -i`;
- the "myenv" environment should be listed under
  "Other active environments": select it;
- ensure the corresponding environment description in
  the jenv detail view makes sense;
- click to remove the environment, cancel the removal in the
  confirm dialog and ensure the environment is still there;
- click the "remove" button again, confirm the deletion and
  ensure that a message is displayed and the environment
  file has been deleted. Also quickstart redirects to the
  environments index view and "myenv" is no longer there;
- destroy the environment: done, thank you!

https://codereview.appspot.com/189540043/

[Francesco Banconi (frankban)]

Reviewers: mp+246162_code.launchpad.net,

Message:
Please take a look.

Description:
Jenv files removal functionality.

Add the ability to remove jenv files
from the Juju home.

Test: `make check`.

QA:
- `juju bootstrap` an environment;
- create a new user for the environment:
   `juju user add myuser --generate -o ~/.juju/environments/myenv`;
- run quickstart in interactive mode:
   `.venv/bin/python juju-quickstart -i`;
- the "myenv" environment should be listed under
   "Other active environments": select it;
- ensure the corresponding environment description in
   the jenv detail view makes sense;
- click to remove the environment, cancel the removal in the
   confirm dialog and ensure the environment is still there;
- click the "remove" button again, confirm the deletion and
   ensure that a message is displayed and the environment
   file has been deleted. Also quickstart redirects to the
   environments index view and "myenv" is no longer there;
- destroy the environment: done, thank you!

https://code.launchpad.net/~frankban/juju-quickstart/remove-jenv/+merge/246162

(do not edit description out of merge proposal)

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

Affected files (+200, -38 lines):
   A [revision details]
   M quickstart/cli/params.py
   M quickstart/cli/views.py
   M quickstart/manage.py
   M quickstart/models/jenv.py
   M quickstart/tests/cli/test_params.py
   M quickstart/tests/cli/test_views.py
   M quickstart/tests/models/test_jenv.py
   M quickstart/tests/test_manage.py

[Martin Hilton (martin-hilton)]

LGTM No QA yet, will do so shortly

https://codereview.appspot.com/189540043/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/189540043/diff/1/quickstart/cli/views.py#newcode528
quickstart/cli/views.py:528: '\nFor this reason, it is not possible to
edit/change it.\n'
Isn't edit/change a tautology? Wouldn't "edit it." be enough.

https://codereview.appspot.com/189540043/

[Brad Crittenden (bac)]

The code LGTM but I question the need for the functionality in
quickstart. I don't feel strongly enough to abandon the work but I think
it only marginally improves the user experience at the expense of more
complication. Perhaps I'm missing a use-case that warrants the
function.

https://codereview.appspot.com/189540043/diff/1/quickstart/cli/params.py
File quickstart/cli/params.py (right):

https://codereview.appspot.com/189540043/diff/1/quickstart/cli/params.py#newcode37
quickstart/cli/params.py:37: )
Formatting is odd. I'd prefer one per line if you're doing multi-line.

https://codereview.appspot.com/189540043/

[lp:~frankban/juju-quickstart/remove-jenv]

115. By Francesco Banconi on 2015-01-12

Fix typo and improve params formatting.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/189540043/diff/1/quickstart/cli/params.py
File quickstart/cli/params.py (right):

https://codereview.appspot.com/189540043/diff/1/quickstart/cli/params.py#newcode37
quickstart/cli/params.py:37: )
On 2015/01/12 15:29:32, bac wrote:
> Formatting is odd. I'd prefer one per line if you're doing
multi-line.

Done.

https://codereview.appspot.com/189540043/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/189540043/diff/1/quickstart/cli/views.py#newcode528
quickstart/cli/views.py:528: '\nFor this reason, it is not possible to
edit/change it.\n'
On 2015/01/12 14:50:22, martin.hilton wrote:
> Isn't edit/change a tautology? Wouldn't "edit it." be enough.

Done.

https://codereview.appspot.com/189540043/

[Martin Hilton (martin-hilton)]

QA OK following instructions.

https://codereview.appspot.com/189540043/

[Francesco Banconi (frankban)]

*** Submitted:

Jenv files removal functionality.

Add the ability to remove jenv files
from the Juju home.

Test: `make check`.

QA:
- `juju bootstrap` an environment;
- create a new user for the environment:
   `juju user add myuser --generate -o ~/.juju/environments/myenv`;
- run quickstart in interactive mode:
   `.venv/bin/python juju-quickstart -i`;
- the "myenv" environment should be listed under
   "Other active environments": select it;
- ensure the corresponding environment description in
   the jenv detail view makes sense;
- click to remove the environment, cancel the removal in the
   confirm dialog and ensure the environment is still there;
- click the "remove" button again, confirm the deletion and
   ensure that a message is displayed and the environment
   file has been deleted. Also quickstart redirects to the
   environments index view and "myenv" is no longer there;
- destroy the environment: done, thank you!

R=martin.hilton, bac
CC=
https://codereview.appspot.com/189540043

https://codereview.appspot.com/189540043/

[Francesco Banconi (frankban)]

Thanks for the reviews!

https://codereview.appspot.com/189540043/

=== modified file 'quickstart/cli/params.py'
--- quickstart/cli/params.py	2015-01-07 15:17:49 +0000
+++ quickstart/cli/params.py	2015-01-12 15:39:31 +0000
@@ -30,7 +30,13 @@
 import copy
 
 
-_PARAMS = ('env_type_db', 'env_db', 'jenv_db', 'save_callable')
+_PARAMS = (
+    'env_type_db',
+    'env_db',
+    'jenv_db',
+    'save_callable',
+    'remove_jenv_callable',
+)
 
 
 class Params(namedtuple('Params', _PARAMS)):
@@ -48,4 +54,5 @@
             env_db=copy.deepcopy(self.env_db),
             jenv_db=copy.deepcopy(self.jenv_db),
             save_callable=self.save_callable,
+            remove_jenv_callable=self.remove_jenv_callable,
         )

=== modified file 'quickstart/cli/views.py'
--- quickstart/cli/views.py	2015-01-08 14:53:06 +0000
+++ quickstart/cli/views.py	2015-01-12 15:39:31 +0000
@@ -525,16 +525,26 @@
         urwid.Text([
             ('highlight', 'Imported active environment.\n'),
             'This environment is not included in your environments.yaml file.'
-            '\nFor this reason, it is not possible to edit or remove it.\n'
+            '\nFor this reason, it is not possible to edit it.\n'
             'However, you can use the link below to ',
             ('highlight', 'use Juju Quickstart'),
-            ' with this environment.',
+            ' with this environment or ',
+            ('highlight', 'remove the corresponding jenv file'),
+            '.\n'
+            'Note that removing the Juju generated environment file does not '
+            'destroy the corresponding active environment.'
         ]),
     ])
 
+    remove_callback = ui.thunk(
+        _remove_jenv, params.jenv_db, env_data, params.remove_jenv_callable,
+        app.set_message, index_view)
+    confirm_removal_callback = ui.thunk(
+        _confirm_removal, app, env_data, remove_callback)
     controls = [
         ui.MenuButton('back', ui.thunk(index_view)),
         ui.MenuButton('use', ui.thunk(_use, params.env_db, env_data)),
+        ui.MenuButton(('control alert', 'remove'), confirm_removal_callback),
     ]
     widgets.append(ui.create_controls(*controls))
     listbox = urwid.ListBox(urwid.SimpleFocusListWalker(widgets))
@@ -542,6 +552,24 @@
     app.set_status([' \N{RIGHTWARDS ARROW OVER LEFTWARDS ARROW} navigate '])
 
 
+def _remove_jenv(
+        jenv_db, env_data, remove_jenv_callable, set_message, redirect_view):
+    """Remove the jenv file corresonding to the env_data environment.
+
+    Update the provided jenv_db and return to the given view.
+    Also output a notification using the given set_message callable.
+    """
+    env_name = env_data['name']
+    # Remove the jenv file.
+    msg = remove_jenv_callable(env_name)
+    if msg is None:
+        msg = '{} successfully removed'.format(env_name)
+        # Also remove the environments from the jenv database.
+        del jenv_db['environments'][env_name]
+    set_message(msg)
+    redirect_view()
+
+
 def env_edit(app, params, env_data):
     """Create or modify a Juju environment.
 

=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2015-01-07 14:07:22 +0000
+++ quickstart/manage.py	2015-01-12 15:39:31 +0000
@@ -218,6 +218,7 @@
         env_db=env_db,
         jenv_db=jenv_db,
         save_callable=_create_save_callable(parser, env_file),
+        remove_jenv_callable=jenv.remove,
     )
     new_env_db, env_data = views.show(views.env_index, parameters)
     if new_env_db != env_db:

=== modified file 'quickstart/models/jenv.py'
--- quickstart/models/jenv.py	2014-12-17 12:28:36 +0000
+++ quickstart/models/jenv.py	2015-01-12 15:39:31 +0000
@@ -174,6 +174,20 @@
     return db
 
 
+def remove(env_name):
+    """Remove the jenv file corresponding to the given environment name.
+
+    Return None if the removal was successful, an error message otherwise.
+    """
+    jenv_path = _get_jenv_path(env_name)
+    try:
+        os.remove(jenv_path)
+    except OSError as err:
+        msg = 'cannot remove the {} environment: {}'
+        return msg.format(env_name, bytes(err).decode('utf-8'))
+    return None
+
+
 def validate(data):
     """Validate the given YAML decoded jenv data.
 

=== modified file 'quickstart/tests/cli/test_params.py'
--- quickstart/tests/cli/test_params.py	2015-01-07 14:07:22 +0000
+++ quickstart/tests/cli/test_params.py	2015-01-12 15:39:31 +0000
@@ -33,21 +33,24 @@
         self.env_db = helpers.make_env_db()
         self.jenv_db = helpers.make_jenv_db()
         self.save_callable = lambda env_db: None
+        self.remove_jenv_callable = lambda env_db: None
         # Set up a params object used in tests.
         self.params = params.Params(
             env_type_db=self.env_type_db,
             env_db=self.env_db,
             jenv_db=self.jenv_db,
             save_callable=self.save_callable,
+            remove_jenv_callable=self.remove_jenv_callable,
         )
 
     def test_tuple(self):
         # The params object can be used as a tuple.
-        env_type_db, env_db, jenv_db, save_callable = self.params
+        env_type_db, env_db, jenv_db, save, remove = self.params
         self.assertIs(self.env_type_db, env_type_db)
         self.assertIs(self.env_db, env_db)
         self.assertIs(self.jenv_db, jenv_db)
-        self.assertIs(self.save_callable, save_callable)
+        self.assertIs(self.save_callable, save)
+        self.assertIs(self.remove_jenv_callable, remove)
 
     def test_attributes(self):
         # Parameters can be accessed as attributes.
@@ -55,6 +58,8 @@
         self.assertIs(self.env_db, self.params.env_db)
         self.assertIs(self.jenv_db, self.params.jenv_db)
         self.assertIs(self.save_callable, self.params.save_callable)
+        self.assertIs(
+            self.remove_jenv_callable, self.params.remove_jenv_callable)
 
     def test_immutable(self):
         # It is not possible to replace a stored parameter.
@@ -69,6 +74,8 @@
         self.assertIs(self.env_db, self.params.env_db)
         self.assertIs(self.jenv_db, self.params.jenv_db)
         self.assertIs(self.save_callable, self.params.save_callable)
+        self.assertIs(
+            self.remove_jenv_callable, self.params.remove_jenv_callable)
         # The new params object stores the same data.
         self.assertEqual(self.params, params)
         # But they do not refer to the same object.

=== modified file 'quickstart/tests/cli/test_views.py'
--- quickstart/tests/cli/test_views.py	2015-01-07 15:36:57 +0000
+++ quickstart/tests/cli/test_views.py	2015-01-12 15:39:31 +0000
@@ -137,6 +137,7 @@
         # Set up the base Urwid application.
         self.loop, self.app = base.setup_urwid_app()
         self.save_callable = mock.Mock()
+        self.remove_jenv_callable = mock.Mock(return_value=None)
 
     def get_widgets_in_contents(self, filter_function=None):
         """Return a list of widgets included in the app contents.
@@ -174,8 +175,45 @@
             env_db=env_db,
             jenv_db=jenv_db,
             save_callable=self.save_callable,
+            remove_jenv_callable=self.remove_jenv_callable,
         )
 
+    def click_remove_button(self, env_name):
+        """Click the remove button in an environment detail view.
+
+        Assume the view was already called.
+        Return the dialog button widgets and the original view contents.
+        """
+        original_contents = self.app.get_contents()
+        # The "remove" button is the last one.
+        remove_button = self.get_control_buttons()[-1]
+        cli_helpers.emit(remove_button)
+        # The original env detail contents have been replaced.
+        contents = self.app.get_contents()
+        self.assertIsNot(contents, original_contents)
+        # A "remove" confirmation dialog is displayed.
+        title_widget, message_widget, buttons = cli_helpers.inspect_dialog(
+            contents)
+        self.assertEqual(
+            'Remove the {} environment'.format(env_name), title_widget.text)
+        self.assertEqual('This action cannot be undone!', message_widget.text)
+        return buttons, original_contents
+
+    def cancel_removal(self, env_name):
+        """Cancel the environment deletion by clicking the "cancel" button."""
+        buttons, original_contents = self.click_remove_button(env_name)
+        # The "cancel" button is the first one in the dialog.
+        cancel_button = buttons[0]
+        cli_helpers.emit(cancel_button)
+        return original_contents
+
+    def confirm_removal(self, env_name):
+        """Confirm the environment deletion."""
+        buttons, _ = self.click_remove_button(env_name)
+        # The "confirm" button is the second one in the dialog.
+        confirm_button = buttons[1]
+        cli_helpers.emit(confirm_button)
+
 
 class TestEnvIndex(EnvViewTestsMixin, unittest.TestCase):
 
@@ -622,18 +660,7 @@
     def test_remove_button(self):
         # A confirmation dialog is displayed if the "remove" button is clicked.
         self.call_view(env_name='ec2-west')
-        original_contents = self.app.get_contents()
-        # The "remove" button is the last one.
-        remove_button = self.get_control_buttons()[-1]
-        cli_helpers.emit(remove_button)
-        # The original env detail contents have been replaced.
-        contents = self.app.get_contents()
-        self.assertIsNot(contents, original_contents)
-        # A "remove" confirmation dialog is displayed.
-        title_widget, message_widget, buttons = cli_helpers.inspect_dialog(
-            contents)
-        self.assertEqual('Remove the ec2-west environment', title_widget.text)
-        self.assertEqual('This action cannot be undone!', message_widget.text)
+        buttons, _ = self.click_remove_button('ec2-west')
         # The dialog includes the "cancel" and "confirm" buttons.
         self.assertEqual(2, len(buttons))
         captions = map(cli_helpers.get_button_caption, buttons)
@@ -642,15 +669,7 @@
     def test_remove_cancelled(self):
         # The "remove" confirmation dialog can be safely dismissed.
         self.call_view(env_name='ec2-west')
-        original_contents = self.app.get_contents()
-        # The "remove" button is the last one.
-        remove_button = self.get_control_buttons()[-1]
-        cli_helpers.emit(remove_button)
-        contents = self.app.get_contents()
-        buttons = cli_helpers.inspect_dialog(contents)[2]
-        # The "cancel" button is the first one in the dialog.
-        cancel_button = buttons[0]
-        cli_helpers.emit(cancel_button)
+        original_contents = self.cancel_removal('ec2-west')
         # The original contents have been restored.
         self.assertIs(original_contents, self.app.get_contents())
 
@@ -659,20 +678,17 @@
         # The current environment is removed if the "remove" button is clicked
         # and then the deletion is confirmed. Subsequently the application
         # switches to the index view.
-        self.call_view(env_name='ec2-west')
-        # The "remove" button is the last one.
-        remove_button = self.get_control_buttons()[-1]
-        cli_helpers.emit(remove_button)
-        contents = self.app.get_contents()
-        buttons = cli_helpers.inspect_dialog(contents)[2]
-        # The "confirm" button is the second one in the dialog.
-        confirm_button = buttons[1]
-        cli_helpers.emit(confirm_button)
+        env_name = 'ec2-west'
+        self.call_view(env_name=env_name)
+        self.confirm_removal(env_name)
+        # A message notifies the environment has been removed.
+        self.assertEqual(
+            '{} successfully removed'.format(env_name), self.app.get_message())
         # The index view has been called passing the modified env_db in params.
         self.assertTrue(mock_env_index.called)
         params = mock_env_index.call_args[0][1]
         # The new env_db no longer includes the "ec2-west" environment.
-        self.assertNotIn('ec2-west', params.env_db['environments'])
+        self.assertNotIn(env_name, params.env_db['environments'])
         # The new env_db has been saved.
         self.save_callable.assert_called_once_with(params.env_db)
 
@@ -732,11 +748,11 @@
             self.assertEqual(expected_text, widget.text)
 
     def test_view_buttons(self):
-        # The "back" and "use" buttons are displayed.
+        # The "back", "use" and "remove" buttons are displayed.
         self.call_view(env_name='ec2-west')
         buttons = self.get_control_buttons()
         captions = map(cli_helpers.get_button_caption, buttons)
-        self.assertEqual(['back', 'use'], captions)
+        self.assertEqual(['back', 'use', 'remove'], captions)
 
     @mock.patch('quickstart.cli.views.env_index')
     def test_back_button(self, mock_env_index):
@@ -759,6 +775,59 @@
         self.assertEqual(
             expected_return_value, context_manager.exception.return_value)
 
+    def test_remove_button(self):
+        # A confirmation dialog is displayed if the "remove" button is clicked.
+        self.call_view(env_name='test-jenv')
+        buttons, _ = self.click_remove_button('test-jenv')
+        # The dialog includes the "cancel" and "confirm" buttons.
+        self.assertEqual(2, len(buttons))
+        captions = map(cli_helpers.get_button_caption, buttons)
+        self.assertEqual(['cancel', 'confirm'], captions)
+
+    def test_remove_cancelled(self):
+        # The "remove" confirmation dialog can be safely dismissed.
+        self.call_view(env_name='test-jenv')
+        original_contents = self.cancel_removal('test-jenv')
+        # The original contents have been restored.
+        self.assertIs(original_contents, self.app.get_contents())
+
+    @mock.patch('quickstart.cli.views.env_index')
+    def test_remove_confirmed(self, mock_env_index):
+        # The jenv file is removed if the "remove" button is clicked and then
+        # then the deletion is confirmed. Subsequently the application switches
+        # to the index view.
+        env_name = 'test-jenv'
+        self.call_view(env_name=env_name)
+        self.confirm_removal(env_name)
+        # A message notifies the environment has been removed.
+        self.assertEqual(
+            '{} successfully removed'.format(env_name), self.app.get_message())
+        # The index view has been called passing the modified jenv_db params.
+        self.assertTrue(mock_env_index.called)
+        params = mock_env_index.call_args[0][1]
+        # The new jenv_db no longer includes the "test-jenv" environment.
+        self.assertNotIn(env_name, params.jenv_db['environments'])
+        # The corresponding jenv file has been removed.
+        self.remove_jenv_callable.assert_called_once_with(env_name)
+        self.assertEqual(
+            'test-jenv successfully removed', self.app.get_message())
+
+    @mock.patch('quickstart.cli.views.env_index')
+    def test_remove_confirmed_error(self, mock_env_index):
+        # Errors occurred while trying to remove the jenv files are notified.
+        env_name = 'test-jenv'
+        self.call_view(env_name=env_name)
+        # Simulate an error removing the jenv file.
+        self.remove_jenv_callable.return_value = 'bad wolf'
+        self.confirm_removal(env_name)
+        # The error is notified.
+        self.assertEqual('bad wolf'.format(env_name), self.app.get_message())
+        # The index view has been called passing the original jenv_db params.
+        self.assertTrue(mock_env_index.called)
+        params = mock_env_index.call_args[0][1]
+        # The jenv_db still includes the "test_jenv" environment.
+        self.assertIn(env_name, params.jenv_db['environments'])
+
 
 class TestEnvEdit(EnvViewTestsMixin, unittest.TestCase):
 

=== modified file 'quickstart/tests/models/test_jenv.py'
--- quickstart/tests/models/test_jenv.py	2014-12-17 11:34:06 +0000
+++ quickstart/tests/models/test_jenv.py	2015-01-12 15:39:31 +0000
@@ -272,6 +272,41 @@
         self.assertEqual({'environments': {}}, jenv_db)
 
 
+class TestRemove(helpers.JenvFileTestsMixin, unittest.TestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        # Prepare the jenv file contents.
+        cls.contents = yaml.safe_dump(cls.jenv_data)
+
+    def test_successful_removal(self):
+        # The jenv file is correctly removed.
+        with self.make_jenv('local', self.contents) as path:
+            error = jenv.remove('local')
+        self.assertIsNone(error)
+        self.assertFalse(os.path.exists(path))
+
+    def test_error_directory(self):
+        # An error message is returned if the jenv path points to a directory.
+        with self.make_jenv('local', self.contents) as path:
+            dirname = os.path.dirname(path)
+            os.mkdir(os.path.join(dirname, 'ec2.jenv'))
+            error = jenv.remove('ec2')
+        expected_error = (
+            'cannot remove the ec2 environment: '
+            '[Errno 21] Is a directory: ')
+        self.assertIn(expected_error, error)
+
+    def test_error_not_found(self):
+        # An error is returned if the environment cannot be found.
+        with self.make_jenv('local', self.contents):
+            error = jenv.remove('hp')
+        expected_error = (
+            'cannot remove the hp environment: '
+            '[Errno 2] No such file or directory: ')
+        self.assertIn(expected_error, error)
+
+
 class TestValidate(
         helpers.JenvFileTestsMixin, helpers.ValueErrorTestsMixin,
         unittest.TestCase):

=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py	2015-01-07 14:07:22 +0000
+++ quickstart/tests/test_manage.py	2015-01-12 15:39:31 +0000
@@ -399,6 +399,7 @@
             env_db=env_db,
             jenv_db=jenv_db,
             save_callable=mock_save_callable(),
+            remove_jenv_callable=jenv.remove,
         )
         mock_show.assert_called_once_with(views.env_index, expected_params)