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

@@ -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},

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))