manila list --all-tenants should display projectID

Both 'manila list --all-tenants' and 'manila list --public'
should show column Project ID. Project/Tenant ownership of
shares is vital information, e.g. when working with project
quota allocations.

'manila list' doesn't show public shares of other projects.
Without '--all-tenants' and without '--public' the result
is scoped to the project/tenant of the user issuing the command.
So, it would be redundant to display a column for this.

Same fix has been put for snapshot-list --all-tenants too.

DocImpact
Change-Id: I7afae542c391041d48c28e268ab59c83447bb940
Closes-Bug: #1570085

manilaclient/api_versions.py

@@ -27,7 +27,7 @@ from manilaclient import utils
 
 LOG = logging.getLogger(__name__)
 
-MAX_VERSION = '2.15'
+MAX_VERSION = '2.17'
 MIN_VERSION = '2.0'
 DEPRECATED_VERSION = '1.0'
 _VERSIONED_METHOD_MAP = {}

manilaclient/tests/functional/client.py

@@ -543,7 +543,7 @@ class ManilaCLIClient(base.CLIClient):
         return self.manila(cmd, microversion=microversion)
 
     def list_shares(self, all_tenants=False, filters=None, columns=None,
-                    microversion=None):
+                    is_public=False, microversion=None):
         """List shares.
 
         :param all_tenants: bool -- whether to list shares that belong
@@ -557,10 +557,14 @@ class ManilaCLIClient(base.CLIClient):
             will be transformed to filter parameter "--project-id=foo"
         :param columns: comma separated string of columns.
             Example, "--columns Name,Size"
+        :param is_public: bool -- should list public shares or not.
+            Default is False.
         """
         cmd = 'list '
         if all_tenants:
             cmd += '--all-tenants '
+        if is_public:
+            cmd += '--public '
         if filters and isinstance(filters, dict):
             for k, v in filters.items():
                 cmd += '%(k)s=%(v)s ' % {

manilaclient/tests/functional/test_shares_listing.py

@@ -109,6 +109,19 @@ class SharesListReadWriteTest(base.BaseTestCase):
         cls.public_description = data_utils.rand_name(
             'autotest_public_share_description')
 
+        cls.admin_private_name = data_utils.rand_name(
+            'autotest_admin_private_share_name')
+        cls.admin_private_description = data_utils.rand_name(
+            'autotest_admin_private_share_description')
+
+        cls.admin_private_share = cls.create_share(
+            name=cls.admin_private_name,
+            description=cls.admin_private_description,
+            public=False,
+            cleanup_in_class=True,
+            client=None,
+            wait_for_creation=False)
+
         cls.private_share = cls.create_share(
             name=cls.private_name,
             description=cls.private_description,
@@ -124,7 +137,8 @@ class SharesListReadWriteTest(base.BaseTestCase):
             client=cls.get_user_client(),
             cleanup_in_class=True)
 
-        for share_id in (cls.private_share['id'], cls.public_share['id']):
+        for share_id in (cls.private_share['id'], cls.public_share['id'],
+                         cls.admin_private_share['id']):
             cls.get_admin_client().wait_for_share_status(share_id, 'available')
 
     def _list_shares(self, filters=None):
@@ -153,11 +167,34 @@ class SharesListReadWriteTest(base.BaseTestCase):
     def test_list_shares(self):
         self._list_shares()
 
-    def test_list_shares_for_all_tenants(self):
+    @ddt.data(1, 0)
-        shares = self.user_client.list_shares(True)
+    def test_list_shares_for_all_tenants(self, all_tenants):
-        self.assertTrue(len(shares) > 1)
+        shares = self.admin_client.list_shares(all_tenants=all_tenants)
-        for s_id in (self.private_share['id'], self.public_share['id']):
+        self.assertTrue(len(shares) >= 1)
+
+        if all_tenants:
+            self.assertTrue(all('Project ID' in s for s in shares))
+            for s_id in (self.private_share['id'], self.public_share['id'],
+                         self.admin_private_share['id']):
                 self.assertTrue(any(s_id == s['ID'] for s in shares))
+        else:
+            self.assertTrue(all('Project ID' not in s for s in shares))
+            self.assertTrue(any(self.admin_private_share['id'] == s['ID']
+                                for s in shares))
+            if self.private_share['project_id'] != (
+                    self.admin_private_share['project_id']):
+                for s_id in (
+                        self.private_share['id'], self.public_share['id']):
+                    self.assertFalse(any(s_id == s['ID'] for s in shares))
+
+    @ddt.data(True, False)
+    def test_list_shares_with_public(self, public):
+        shares = self.user_client.list_shares(is_public=public)
+        self.assertTrue(len(shares) > 1)
+        if public:
+            self.assertTrue(all('Project ID' in s for s in shares))
+        else:
+            self.assertTrue(all('Project ID' not in s for s in shares))
 
     def test_list_shares_by_name(self):
         shares = self.user_client.list_shares(
@@ -180,8 +217,8 @@ class SharesListReadWriteTest(base.BaseTestCase):
         self._list_shares({'status': 'available'})
 
     def test_list_shares_by_project_id(self):
-        project_id = self.admin_client.get_project_id(
+        project_id = self.user_client.get_project_id(
-            self.admin_client.tenant_name)
+            self.user_client.tenant_name)
         self._list_shares({'project_id': project_id})
 
     @testtools.skipUnless(

manilaclient/tests/unit/v2/test_shell.py

@@ -147,9 +147,30 @@ class ShellTest(test_utils.TestCase):
             self.run_command('list --name' + separator + '1234')
             self.assert_called('GET', '/shares/detail?name=1234')
 
+    @mock.patch.object(cliutils, 'print_list', mock.Mock())
     def test_list_all_tenants_only_key(self):
         self.run_command('list --all-tenants')
         self.assert_called('GET', '/shares/detail?all_tenants=1')
+        cliutils.print_list.assert_called_once_with(
+            mock.ANY,
+            ['ID', 'Name', 'Size', 'Share Proto', 'Status', 'Is Public',
+             'Share Type Name', 'Host', 'Availability Zone', 'Project ID'])
+
+    @mock.patch.object(cliutils, 'print_list', mock.Mock())
+    def test_list_select_column_and_all_tenants(self):
+        self.run_command('list --columns ID,Name --all-tenants')
+        self.assert_called('GET', '/shares/detail?all_tenants=1')
+        cliutils.print_list.assert_called_once_with(
+            mock.ANY,
+            ['Id', 'Name'])
+
+    @mock.patch.object(cliutils, 'print_list', mock.Mock())
+    def test_list_select_column_and_public(self):
+        self.run_command('list --columns ID,Name --public')
+        self.assert_called('GET', '/shares/detail?is_public=True')
+        cliutils.print_list.assert_called_once_with(
+            mock.ANY,
+            ['Id', 'Name'])
 
     def test_list_all_tenants_key_and_value_1(self):
         for separator in self.separators:
@@ -452,7 +473,8 @@ class ShellTest(test_utils.TestCase):
             'Is Public',
             'Share Type Name',
             'Host',
-            'Availability Zone'
+            'Availability Zone',
+            'Project ID'
         ]
         self.run_command('list --public')
         self.assert_called('GET', '/shares/detail?is_public=True')
@@ -609,9 +631,13 @@ class ShellTest(test_utils.TestCase):
             mock.ANY,
             ['Id', 'Name'])
 
+    @mock.patch.object(cliutils, 'print_list', mock.Mock())
     def test_list_snapshots_all_tenants_only_key(self):
         self.run_command('snapshot-list --all-tenants')
         self.assert_called('GET', '/snapshots/detail?all_tenants=1')
+        cliutils.print_list.assert_called_once_with(
+            mock.ANY,
+            ['ID', 'Share ID', 'Status', 'Name', 'Share Size', 'Project ID'])
 
     def test_list_snapshots_all_tenants_key_and_value_1(self):
         for separator in self.separators:

manilaclient/v2/shell.py

@@ -1281,16 +1281,18 @@ def do_access_list(cs, args):
 def do_list(cs, args):
     """List NAS shares with filters."""
 
+    columns = args.columns
+    all_tenants = int(os.environ.get("ALL_TENANTS", args.all_tenants))
+    if columns is not None:
+        list_of_keys = _split_columns(columns=columns)
+    else:
         list_of_keys = [
             'ID', 'Name', 'Size', 'Share Proto', 'Status', 'Is Public',
             'Share Type Name', 'Host', 'Availability Zone'
         ]
+        if all_tenants or args.public:
+            list_of_keys.append('Project ID')
 
-    columns = args.columns
-    if columns is not None:
-        list_of_keys = _split_columns(columns=columns)
-
-    all_tenants = int(os.environ.get("ALL_TENANTS", args.all_tenants))
     empty_obj = type('Empty', (object,), {'id': None})
     share_type = (_find_share_type(cs, args.share_type)
                   if args.share_type else empty_obj)
@@ -1565,14 +1567,16 @@ def do_share_instance_export_location_show(cs, args):
          'e.g. --columns "id,name"')
 def do_snapshot_list(cs, args):
     """List all the snapshots."""
+    all_tenants = int(os.environ.get("ALL_TENANTS", args.all_tenants))
+    if args.columns is not None:
+        list_of_keys = _split_columns(columns=args.columns)
+    else:
         list_of_keys = [
             'ID', 'Share ID', 'Status', 'Name', 'Share Size',
         ]
+        if all_tenants:
+            list_of_keys.append('Project ID')
 
-    if args.columns is not None:
-        list_of_keys = _split_columns(columns=args.columns)
-
-    all_tenants = int(os.environ.get("ALL_TENANTS", args.all_tenants))
     empty_obj = type('Empty', (object,), {'id': None})
     share = _find_share(cs, args.share_id) if args.share_id else empty_obj
     search_opts = {

releasenotes/notes/bug_1570085_fix-905786b797379309.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - Both 'manila list --all-tenants' and 'manila list --public'
+    will show column Project ID. Same is the change in behavior
+    of snapshot-list --all-tenants.