From 3b10973be1ac1c976805b0f96b2ec2c92b070f1a 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 | 186 ++++++++++++++++++++++++++++--- muranoclient/v1/shell.py | 26 ++++- 2 files changed, 196 insertions(+), 16 deletions(-) diff --git a/muranoclient/tests/test_shell.py b/muranoclient/tests/test_shell.py index cfd61ab2..c06fa32c 100644 --- a/muranoclient/tests/test_shell.py +++ b/muranoclient/tests/test_shell.py @@ -17,13 +17,16 @@ import re import StringIO import sys import tempfile +import zipfile import fixtures import mock import requests import six from testtools import matchers +import yaml +from muranoclient.common import exceptions as common_exceptions from muranoclient.common import utils from muranoclient.openstack.common.apiclient import exceptions import muranoclient.shell @@ -34,6 +37,38 @@ FIXTURE_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__), 'fixture_data')) # RESULT_PACKAGE = os.path.join(FIXTURE_DIR, 'test-app.zip') + +def make_pkg(manifest_override, image_dicts=None): + manifest = { + 'Author': '', + 'Classes': {'foo': 'foo.yaml'}, + 'Description': '', + 'Format': 1.0, + 'FullName': '', + 'Name': 'Apache HTTP Server', + 'Type': 'Application'} + manifest.update(manifest_override) + file_obj = StringIO.StringIO() + zfile = zipfile.ZipFile(file_obj, "a") + zfile.writestr('manifest.yaml', yaml.dump(manifest)) + if image_dicts: + images_list = [] + default_image_spec = { + 'ContainerFormat': 'bare', + 'DiskFormat': 'qcow2', + 'Hash': 'd41d8cd98f00b204e9800998ecf8427e', + 'Name': '', + } + for image_dict in image_dicts: + image_spec = default_image_spec.copy() + image_spec.update(image_dict) + images_list.append(image_spec) + images = {'Images': images_list, } + zfile.writestr('images.lst', yaml.dump(images)) + zfile.close() + file_obj.seek(0) + return file_obj + FAKE_ENV = {'OS_USERNAME': 'username', 'OS_PASSWORD': 'password', 'OS_TENANT_NAME': 'tenant_name', @@ -49,6 +84,8 @@ class TestArgs(object): version = '' murano_repo_url = '' exists_action = '' + is_public = False + categories = [] class ShellTest(base.TestCaseShell): @@ -324,27 +361,150 @@ class ShellPackagesOperations(ShellTest): {RESULT_PACKAGE: mock.ANY}, ) - @mock.patch('muranoclient.common.utils.Package.images') - def test_package_import_no_categories(self, mock_images): - mock_images.return_value = [] + 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() + with tempfile.NamedTemporaryFile() as f: RESULT_PACKAGE = f.name + pkg = make_pkg({'FullName': RESULT_PACKAGE}) + from_file.return_value = utils.Package(utils.File(pkg)) args.filename = RESULT_PACKAGE args.categories = None args.is_public = False - result = {RESULT_PACKAGE: utils.Package.from_file( - StringIO.StringIO("123"))} - - with mock.patch( - 'muranoclient.common.utils.Package.manifest') as man_mock: - man_mock.__getitem__.side_effect = [args.filename] - with mock.patch( - 'muranoclient.common.utils.Package.requirements', - mock.Mock(side_effect=lambda *args, **kw: result)): - v1_shell.do_package_import(self.client, args) + v1_shell.do_package_import(self.client, args) self.client.packages.create.assert_called_once_with( {'is_public': False}, 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))