From 22a28daa14d7ef3f06202afcef65e4f62a9ae112 Mon Sep 17 00:00:00 2001 From: Kirill Zaitsev Date: Wed, 29 Apr 2015 14:04:02 +0300 Subject: [PATCH] Only delete 'owned' packages for --exists-action update Due to the fact that fqns are no longer unique update might currently not work correctly, because it searches for package to delete by fqn. To improve this situation this patch attempts to search for the package in the current tenant and deletes it by id. In case it is not found - raise the error, cause it most likely means that the package is in another tennant and deleting it automatically is dangerous. This change relies on 'owned' parameter in api sorting, fixed in https://review.openstack.org/#/c/177796/ Change-Id: Iee04ea65f5c4937ded4e259e3c0cb64189801da2 Closes-Bug: #1448135 --- muranoclient/tests/test_shell.py | 131 +++++++++++++++++++++++++++++++ muranoclient/v1/shell.py | 26 +++++- 2 files changed, 154 insertions(+), 3 deletions(-) diff --git a/muranoclient/tests/test_shell.py b/muranoclient/tests/test_shell.py index d1aba991..0f7ac06c 100644 --- a/muranoclient/tests/test_shell.py +++ b/muranoclient/tests/test_shell.py @@ -27,6 +27,7 @@ import requests_mock import six from testtools import matchers +from muranoclient.common import exceptions as common_exceptions from muranoclient.common import utils from muranoclient.openstack.common.apiclient import exceptions import muranoclient.shell @@ -335,6 +336,136 @@ class ShellPackagesOperations(ShellTest): 'is_public': True }, {RESULT_PACKAGE: mock.ANY},) + def _test_conflict(self, + packages, from_file, raw_input_mock, + input_action, exists_action=''): + packages.create = mock.MagicMock( + side_effect=[common_exceptions.HTTPConflict("Conflict"), None]) + + packages.filter.return_value = [mock.Mock(id='test_id')] + + raw_input_mock.return_value = input_action + args = TestArgs() + args.exists_action = exists_action + with tempfile.NamedTemporaryFile() as f: + args.filename = f.name + + pkg = make_pkg({'FullName': f.name}) + from_file.return_value = utils.Package(utils.File(pkg)) + + v1_shell.do_package_import(self.client, args) + return f.name + + @mock.patch('__builtin__.raw_input') + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_package_import_conflict_skip(self, from_file, raw_input_mock): + + name = self._test_conflict( + self.client.packages, + from_file, + raw_input_mock, + 's', + ) + + self.client.packages.create.assert_called_once_with({ + 'is_public': False, + }, {name: mock.ANY},) + + @mock.patch('__builtin__.raw_input') + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_package_import_conflict_skip_ea(self, from_file, raw_input_mock): + + name = self._test_conflict( + self.client.packages, + from_file, + raw_input_mock, + '', + exists_action='s', + ) + + self.client.packages.create.assert_called_once_with({ + 'is_public': False, + }, {name: mock.ANY},) + self.assertFalse(raw_input_mock.called) + + @mock.patch('__builtin__.raw_input') + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_package_import_conflict_abort(self, from_file, raw_input_mock): + + self.assertRaises(SystemExit, self._test_conflict, + self.client.packages, + from_file, + raw_input_mock, + 'a', + ) + + self.client.packages.create.assert_called_once_with({ + 'is_public': False, + }, mock.ANY,) + + @mock.patch('__builtin__.raw_input') + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_package_import_conflict_abort_ea(self, + from_file, raw_input_mock): + + self.assertRaises(SystemExit, self._test_conflict, + self.client.packages, + from_file, + raw_input_mock, + '', + exists_action='a', + ) + + self.client.packages.create.assert_called_once_with({ + 'is_public': False, + }, mock.ANY,) + self.assertFalse(raw_input_mock.called) + + @mock.patch('__builtin__.raw_input') + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_package_import_conflict_update(self, from_file, raw_input_mock): + + name = self._test_conflict( + self.client.packages, + from_file, + raw_input_mock, + 'u', + ) + + self.client.packages.delete.assert_called_once_with('test_id') + + self.client.packages.create.assert_has_calls( + [ + mock.call({'is_public': False}, {name: mock.ANY},), + mock.call({'is_public': False}, {name: mock.ANY},) + ], any_order=True, + ) + self.assertEqual(self.client.packages.create.call_count, 2) + + @mock.patch('__builtin__.raw_input') + @mock.patch('muranoclient.common.utils.Package.from_file') + def test_package_import_conflict_update_ea(self, + from_file, raw_input_mock): + + name = self._test_conflict( + self.client.packages, + from_file, + raw_input_mock, + '', + exists_action='u', + ) + + self.client.packages.delete.assert_called_once_with('test_id') + + self.client.packages.create.assert_has_calls( + [ + mock.call({'is_public': False}, {name: mock.ANY},), + mock.call({'is_public': False}, {name: mock.ANY},) + ], any_order=True, + ) + self.assertEqual(self.client.packages.create.call_count, 2) + self.assertFalse(raw_input_mock.called) + @mock.patch('muranoclient.common.utils.Package.from_file') def test_package_import_no_categories(self, from_file): args = TestArgs() diff --git a/muranoclient/v1/shell.py b/muranoclient/v1/shell.py index b69a7cc9..f5b5ef9f 100644 --- a/muranoclient/v1/shell.py +++ b/muranoclient/v1/shell.py @@ -280,7 +280,8 @@ def _handle_package_exists(mc, data, package, exists_action): try: return mc.packages.create(data, {name: package.file()}) except common_exceptions.HTTPConflict: - print("Package with name {0} is already registered.".format(name)) + print("Importing package {0} failed. Package with the same" + " name/classes is already registered.".format(name)) allowed_results = ['s', 'u', 'a'] res = exists_action if not res: @@ -296,8 +297,23 @@ def _handle_package_exists(mc, data, package, exists_action): print("Exiting.") sys.exit() elif res == 'u': - print("Deleting package {0}".format(name)) - mc.packages.delete(name) + pkgs = list(mc.packages.filter(fqn=name, owned=True)) + if not pkgs: + msg = ( + "Got Conflict response, but couldn't find package " + "'{0}' in the current tenant.\nThis probably means " + "conflicting package is in another tenant.\n" + "Please delete it manually." + ).format(name) + raise exceptions.CommandError(msg) + elif len(pkgs) > 1: + msg = ( + "Got {0} packages with name '{1}'.\nI'm not trusting " + "myself, please delete the package manually" + ).format(len(pkgs), name) + raise exceptions.CommandError(msg) + print("Deleting package {0}({1})".format(name, pkgs[0].id)) + mc.packages.delete(pkgs[0].id) continue @@ -358,6 +374,8 @@ def do_package_import(mc, args): "images for {1}".format(e, name)) try: _handle_package_exists(mc, data, package, args.exists_action) + except exceptions.CommandError: + raise except Exception as e: print("Error {0} occurred while installing package {1}".format( e, name)) @@ -426,6 +444,8 @@ def do_bundle_import(mc, args): try: _handle_package_exists( mc, data, dep_package, args.exists_action) + except exceptions.CommandError: + raise except Exception as e: print("Error {0} occurred while " "installing package {1}".format(e, name))