Add 'description' in share type APIs

This fix add attr 'description' in list/create share
type APIs.

Change-Id: I39705acbfca812b66d74fbd2fee228a7a85bc2d8
Closes-Bug: #1724183

manila/api/openstack/api_version_request.py

@@ -111,13 +111,14 @@ REST_API_VERSION_HISTORY = """
              manila.
     * 2.39 - Added share-type quotas.
     * 2.40 - Added share group and share group snapshot quotas.
+    * 2.41 - Added 'description' in share type create/list APIs.
 """
 
 # The minimum and maximum versions of the API supported
 # The default api version request is defined to be the
 # minimum version of the API supported.
 _MIN_API_VERSION = "2.0"
-_MAX_API_VERSION = "2.40"
+_MAX_API_VERSION = "2.41"
 DEFAULT_API_VERSION = _MIN_API_VERSION
 
 

manila/api/openstack/rest_api_version_history.rst

@@ -226,3 +226,7 @@ user documentation.
 2.40
 ----
   Added share group and share group snapshot quotas.
+
+2.41
+----
+  Added 'description' in share type create/list APIs.

manila/api/v2/share_types.py

@@ -21,6 +21,7 @@ import six
 import webob
 from webob import exc
 
+from manila.api.openstack import api_version_request as api_version
 from manila.api.openstack import wsgi
 from manila.api.views import types as views_types
 from manila.common import constants
@@ -153,13 +154,21 @@ class ShareTypesController(wsgi.Controller):
             share_type = body['volume_type']
         name = share_type.get('name')
         specs = share_type.get('extra_specs', {})
+        description = share_type.get('description')
+        if (description and req.api_version_request
+                < api_version.APIVersionRequest("2.41")):
+            msg = _("'description' key is not supported by this "
+                    "microversion. Use 2.41 or greater microversion "
+                    "to be able to use 'description' in share type.")
+            raise webob.exc.HTTPBadRequest(explanation=msg)
         is_public = share_type.get(
             'os-share-type-access:is_public',
             share_type.get('share_type_access:is_public', True),
         )
 
-        if name is None or name == "" or len(name) > 255:
+        if (name is None or name == "" or len(name) > 255
-            msg = _("Type name is not valid.")
+                or (description and len(description) > 255)):
+            msg = _("Type name or description is not valid.")
             raise webob.exc.HTTPBadRequest(explanation=msg)
 
         # Note(cknight): Set the default extra spec value for snapshot_support
@@ -172,7 +181,8 @@ class ShareTypesController(wsgi.Controller):
             required_extra_specs = (
                 share_types.get_valid_required_extra_specs(specs)
             )
-            share_types.create(context, name, specs, is_public)
+            share_types.create(context, name, specs, is_public,
+                               description=description)
             share_type = share_types.get_share_type_by_name(context, name)
             share_type['required_extra_specs'] = required_extra_specs
             req.cache_db_share_type(share_type)

manila/api/views/types.py

@@ -26,6 +26,7 @@ class ViewBuilder(common.ViewBuilder):
         "add_is_public_attr_core_api_like",
         "add_is_public_attr_extension_like",
         "add_inferred_optional_extra_specs",
+        "add_description_attr",
     ]
 
     def show(self, request, share_type, brief=False):
@@ -90,3 +91,7 @@ class ViewBuilder(common.ViewBuilder):
     def _filter_extra_specs(self, extra_specs, valid_keys):
         return {key: value for key, value in extra_specs.items()
                 if key in valid_keys}
+
+    @common.ViewBuilder.versioned_method("2.41")
+    def add_description_attr(self, context, share_type_dict, share_type):
+        share_type_dict['description'] = share_type.get('description')

manila/db/migrations/alembic/versions/27cb96d991fa_add_description_for_share_type.py

@@ -0,0 +1,50 @@
+# Copyright (c) 2017 Huawei Technologies Co., Ltd.
+# All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+"""add description for share type
+
+Revision ID: 27cb96d991fa
+Revises: 829a09b0ddd4
+Create Date: 2017-09-16 03:07:15.548947
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '27cb96d991fa'
+down_revision = '829a09b0ddd4'
+
+from alembic import op
+from oslo_log import log
+import sqlalchemy as sa
+
+LOG = log.getLogger(__name__)
+
+
+def upgrade():
+    try:
+        op.add_column(
+            'share_types',
+            sa.Column('description', sa.String(255), nullable=True))
+    except Exception:
+        LOG.error("Column share_types.description not created!")
+        raise
+
+
+def downgrade():
+    try:
+        op.drop_column('share_types', 'description')
+    except Exception:
+        LOG.error("Column share_types.description not dropped!")
+        raise

manila/db/sqlalchemy/models.py

@@ -485,6 +485,7 @@ class ShareTypes(BASE, ManilaBase):
     id = Column(String(36), primary_key=True)
     deleted = Column(String(36), default='False')
     name = Column(String(255))
+    description = Column(String(255))
     is_public = Column(Boolean, default=True)
 
 

manila/share/share_types.py

@@ -34,7 +34,8 @@ CONF = cfg.CONF
 LOG = log.getLogger(__name__)
 
 
-def create(context, name, extra_specs=None, is_public=True, projects=None):
+def create(context, name, extra_specs=None, is_public=True,
+           projects=None, description=None):
     """Creates share types."""
     extra_specs = extra_specs or {}
     projects = projects or []
@@ -44,10 +45,10 @@ def create(context, name, extra_specs=None, is_public=True, projects=None):
         get_valid_optional_extra_specs(extra_specs)
     except exception.InvalidExtraSpec as e:
         raise exception.InvalidShareType(reason=six.text_type(e))
-
     try:
         type_ref = db.share_type_create(context,
                                         dict(name=name,
+                                             description=description,
                                              extra_specs=extra_specs,
                                              is_public=is_public),
                                         projects=projects)

manila/test.py

@@ -370,6 +370,10 @@ class TestCase(base_test.BaseTestCase):
         return (api_version.APIVersionRequest(left) >=
                 api_version.APIVersionRequest(right))
 
+    def is_microversion_lt(self, left, right):
+        return (api_version.APIVersionRequest(left) <
+                api_version.APIVersionRequest(right))
+
     def assert_notify_called(self, mock_notify, calls):
         for i in range(0, len(calls)):
             mock_call = mock_notify.call_args_list[i]

manila/tests/api/v2/test_share_types.py

@@ -45,6 +45,7 @@ def stub_share_type(id):
     return dict(
         id=id,
         name='share_type_%s' % str(id),
+        description='description_%s' % str(id),
         extra_specs=specs,
         required_extra_specs={
             constants.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS: "true",
@@ -88,12 +89,13 @@ def return_share_types_with_volumes_destroy(context, id):
     pass
 
 
-def return_share_types_create(context, name, specs, is_public):
+def return_share_types_create(context, name, specs, is_public, description):
     pass
 
 
 def make_create_body(name="test_share_1", extra_specs=None,
-                     spec_driver_handles_share_servers=True):
+                     spec_driver_handles_share_servers=True,
+                     description=None):
     if not extra_specs:
         extra_specs = {}
 
@@ -104,9 +106,11 @@ def make_create_body(name="test_share_1", extra_specs=None,
     body = {
         "share_type": {
             "name": name,
-            "extra_specs": extra_specs
+            "extra_specs": extra_specs,
         }
     }
+    if description:
+        body["share_type"].update({"description": description})
 
     return body
 
@@ -235,6 +239,8 @@ class ShareTypesAPITest(test.TestCase):
         ('2.24', 'share_type_access', False),
         ('2.27', 'share_type_access', True),
         ('2.27', 'share_type_access', False),
+        ('2.41', 'share_type_access', True),
+        ('2.41', 'share_type_access', False),
     )
     @ddt.unpack
     def test_view_builder_show(self, version, prefix, admin):
@@ -243,6 +249,7 @@ class ShareTypesAPITest(test.TestCase):
         now = timeutils.utcnow().isoformat()
         raw_share_type = dict(
             name='new_type',
+            description='description_test',
             deleted=False,
             created_at=now,
             updated_at=now,
@@ -270,8 +277,10 @@ class ShareTypesAPITest(test.TestCase):
             for extra_spec in constants.ExtraSpecs.INFERRED_OPTIONAL_MAP:
                 expected_share_type['extra_specs'][extra_spec] = (
                     constants.ExtraSpecs.INFERRED_OPTIONAL_MAP[extra_spec])
+        if self.is_microversion_ge(version, '2.41'):
+            expected_share_type['description'] = 'description_test'
 
-        self.assertDictMatch(output['share_type'], expected_share_type)
+        self.assertDictMatch(expected_share_type, output['share_type'])
 
     @ddt.data(
         ('1.0', 'os-share-type-access', True),
@@ -288,6 +297,8 @@ class ShareTypesAPITest(test.TestCase):
         ('2.24', 'share_type_access', False),
         ('2.27', 'share_type_access', True),
         ('2.27', 'share_type_access', False),
+        ('2.41', 'share_type_access', True),
+        ('2.41', 'share_type_access', False),
     )
     @ddt.unpack
     def test_view_builder_list(self, version, prefix, admin):
@@ -306,6 +317,7 @@ class ShareTypesAPITest(test.TestCase):
             raw_share_types.append(
                 dict(
                     name='new_type',
+                    description='description_test',
                     deleted=False,
                     created_at=now,
                     updated_at=now,
@@ -321,16 +333,18 @@ class ShareTypesAPITest(test.TestCase):
         output = view_builder.index(request, raw_share_types)
 
         self.assertIn('share_types', output)
+        expected_share_type = {
+            'name': 'new_type',
+            'extra_specs': extra_specs,
+            '%s:is_public' % prefix: True,
+            'required_extra_specs': {},
+        }
+        if self.is_microversion_ge(version, '2.41'):
+            expected_share_type['description'] = 'description_test'
         for i in range(0, 10):
-            expected_share_type = {
+            expected_share_type['id'] = 42 + i
-                'name': 'new_type',
+            self.assertDictMatch(expected_share_type,
-                'extra_specs': extra_specs,
+                                 output['share_types'][i])
-                '%s:is_public' % prefix: True,
-                'required_extra_specs': {},
-                'id': 42 + i,
-            }
-            self.assertDictMatch(output['share_types'][i],
-                                 expected_share_type)
 
     @ddt.data(None, True, 'true', 'false', 'all')
     def test_parse_is_public_valid(self, value):
@@ -373,37 +387,18 @@ class ShareTypesAPITest(test.TestCase):
         self.controller._delete(req, 1)
         self.assertEqual(1, len(fake_notifier.NOTIFICATIONS))
 
-    @ddt.data(make_create_body("share_type_1"),
+    @ddt.data(
-              make_create_body(spec_driver_handles_share_servers=True),
+        (make_create_body("share_type_1"), "2.24"),
-              make_create_body(spec_driver_handles_share_servers=False))
+        (make_create_body(spec_driver_handles_share_servers=True), "2.24"),
-    def test_create_2_23(self, body):
+        (make_create_body(spec_driver_handles_share_servers=False), "2.24"),
+        (make_create_body("share_type_1"), "2.23"),
+        (make_create_body(spec_driver_handles_share_servers=True), "2.23"),
+        (make_create_body(spec_driver_handles_share_servers=False), "2.23"),
+        (make_create_body(description="description_1"), "2.41"))
+    @ddt.unpack
+    def test_create(self, body, version):
 
-        req = fakes.HTTPRequest.blank('/v2/fake/types', version="2.23")
+        req = fakes.HTTPRequest.blank('/v2/fake/types', version=version)
-        self.assertEqual(0, len(fake_notifier.NOTIFICATIONS))
-
-        res_dict = self.controller.create(req, body)
-
-        self.assertEqual(1, len(fake_notifier.NOTIFICATIONS))
-        self.assertEqual(2, len(res_dict))
-        self.assertEqual('share_type_1', res_dict['share_type']['name'])
-        self.assertEqual('share_type_1', res_dict['volume_type']['name'])
-        for extra_spec in constants.ExtraSpecs.REQUIRED:
-            self.assertIn(extra_spec,
-                          res_dict['share_type']['required_extra_specs'])
-        expected_extra_specs = {
-            constants.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS: True,
-            constants.ExtraSpecs.SNAPSHOT_SUPPORT: True,
-        }
-        expected_extra_specs.update(body['share_type']['extra_specs'])
-        share_types.create.assert_called_once_with(
-            mock.ANY, body['share_type']['name'], expected_extra_specs, True)
-
-    @ddt.data(make_create_body("share_type_1"),
-              make_create_body(spec_driver_handles_share_servers=True),
-              make_create_body(spec_driver_handles_share_servers=False))
-    def test_create_2_24(self, body):
-
-        req = fakes.HTTPRequest.blank('/v2/fake/types', version="2.24")
         self.assertEqual(0, len(fake_notifier.NOTIFICATIONS))
 
         res_dict = self.controller.create(req, body)
@@ -412,15 +407,24 @@ class ShareTypesAPITest(test.TestCase):
         self.assertEqual(2, len(res_dict))
         self.assertEqual('share_type_1', res_dict['share_type']['name'])
         self.assertEqual('share_type_1', res_dict['volume_type']['name'])
+        if self.is_microversion_ge(version, '2.41'):
+            self.assertEqual(body['share_type']['description'],
+                             res_dict['share_type']['description'])
+            self.assertEqual(body['share_type']['description'],
+                             res_dict['volume_type']['description'])
         for extra_spec in constants.ExtraSpecs.REQUIRED:
             self.assertIn(extra_spec,
                           res_dict['share_type']['required_extra_specs'])
         expected_extra_specs = {
             constants.ExtraSpecs.DRIVER_HANDLES_SHARE_SERVERS: True,
         }
+        if self.is_microversion_lt(version, '2.24'):
+            expected_extra_specs[constants.ExtraSpecs.SNAPSHOT_SUPPORT] = True
         expected_extra_specs.update(body['share_type']['extra_specs'])
         share_types.create.assert_called_once_with(
-            mock.ANY, body['share_type']['name'], expected_extra_specs, True)
+            mock.ANY, body['share_type']['name'],
+            expected_extra_specs, True,
+            description=body['share_type'].get('description'))
 
     @ddt.data(None,
               make_create_body(""),
@@ -489,6 +493,7 @@ def generate_type(type_id, is_public):
     return {
         'id': type_id,
         'name': u'test',
+        'description': u'ds_test',
         'deleted': False,
         'created_at': datetime.datetime(2012, 1, 1, 1, 1, 1, 1),
         'updated_at': None,

manila/tests/db/migrations/alembic/migrations_data_checks.py

@@ -2500,3 +2500,40 @@ class FixProjectShareTypesQuotasUniqueConstraintChecks(BaseMigrationChecks):
 
     def check_downgrade(self, engine):
         pass
+
+
+@map_to_migration('27cb96d991fa')
+class NewDescriptionColumnChecks(BaseMigrationChecks):
+    st_table_name = 'share_types'
+    st_ids = ['share_type_id_fake_3_%d' % i for i in (1, 2)]
+
+    def setup_upgrade_data(self, engine):
+        # Create share type
+        share_type_data = {
+            'id': self.st_ids[0],
+            'name': 'name_1',
+        }
+        st_table = utils.load_table(self.st_table_name, engine)
+        engine.execute(st_table.insert(share_type_data))
+
+    def check_upgrade(self, engine, data):
+        st_table = utils.load_table(self.st_table_name, engine)
+        for na in engine.execute(st_table.select()):
+            self.test_case.assertTrue(hasattr(na, 'description'))
+
+        share_type_data_ds = {
+            'id': self.st_ids[1],
+            'name': 'name_1',
+            'description': 'description_1',
+        }
+        engine.execute(st_table.insert(share_type_data_ds))
+        st = engine.execute(st_table.select().where(
+            share_type_data_ds['id'] == st_table.c.id)).first()
+        self.test_case.assertEqual(
+            share_type_data_ds['description'], st['description'])
+
+    def check_downgrade(self, engine):
+        table = utils.load_table(self.st_table_name, engine)
+        db_result = engine.execute(table.select())
+        for record in db_result:
+            self.test_case.assertFalse(hasattr(record, 'description'))

manila_tempest_tests/config.py

@@ -30,7 +30,7 @@ ShareGroup = [
                help="The minimum api microversion is configured to be the "
                     "value of the minimum microversion supported by Manila."),
     cfg.StrOpt("max_api_microversion",
-               default="2.40",
+               default="2.41",
                help="The maximum api microversion is configured to be the "
                     "value of the latest microversion supported by Manila."),
     cfg.StrOpt("region",

manila_tempest_tests/services/share/v2/json/shares_client.py

@@ -820,6 +820,8 @@ class SharesV2Client(shares_client.SharesClient):
             'extra_specs': kwargs.get('extra_specs'),
             is_public_keyname: is_public,
         }
+        if kwargs.get('description'):
+            post_body['description'] = kwargs.get('description')
         post_body = json.dumps({'share_type': post_body})
         resp, body = self.post('types', post_body, version=version)
         self.expected_success(200, resp.status)

manila_tempest_tests/tests/api/admin/test_share_types.py

@@ -58,18 +58,30 @@ class ShareTypesAdminTest(base.BaseSharesAdminTest):
             self.assertIn(old_key_name, share_type)
             self.assertNotIn(new_key_name, share_type)
 
+    def _verify_description(self, expect_des, share_type, version):
+        if utils.is_microversion_ge(version, "2.41"):
+            self.assertEqual(expect_des, share_type['description'])
+        else:
+            self.assertNotIn('description', share_type)
+
     @tc.attr(base.TAG_POSITIVE, base.TAG_API)
-    @ddt.data('2.0', '2.6', '2.7')
+    @ddt.data('2.0', '2.6', '2.7',  '2.40',  '2.41')
     def test_share_type_create_get(self, version):
         self.skip_if_microversion_not_supported(version)
 
         name = data_utils.rand_name("tempest-manila")
+        description = None
+        if utils.is_microversion_ge(version, "2.41"):
+            description = "Description for share type"
         extra_specs = self.add_extra_specs_to_dict({"key": "value", })
 
         # Create share type
         st_create = self.create_share_type(
-            name, extra_specs=extra_specs, version=version)
+            name, extra_specs=extra_specs, version=version,
+            description=description)
         self.assertEqual(name, st_create['share_type']['name'])
+        self._verify_description(
+            description, st_create['share_type'], version)
         self._verify_is_public_key_name(st_create['share_type'], version)
         st_id = st_create["share_type"]["id"]
 
@@ -77,6 +89,7 @@ class ShareTypesAdminTest(base.BaseSharesAdminTest):
         get = self.shares_v2_client.get_share_type(st_id, version=version)
         self.assertEqual(name, get["share_type"]["name"])
         self.assertEqual(st_id, get["share_type"]["id"])
+        self._verify_description(description, get['share_type'], version)
         self.assertEqual(extra_specs, get["share_type"]["extra_specs"])
         self._verify_is_public_key_name(get['share_type'], version)
 
@@ -84,16 +97,20 @@ class ShareTypesAdminTest(base.BaseSharesAdminTest):
         self.assertDictMatch(get["volume_type"], get["share_type"])
 
     @tc.attr(base.TAG_POSITIVE, base.TAG_API)
-    @ddt.data('2.0', '2.6', '2.7')
+    @ddt.data('2.0', '2.6', '2.7',  '2.40',  '2.41')
     def test_share_type_create_list(self, version):
         self.skip_if_microversion_not_supported(version)
 
         name = data_utils.rand_name("tempest-manila")
+        description = None
+        if utils.is_microversion_ge(version, "2.41"):
+            description = "Description for share type"
         extra_specs = self.add_extra_specs_to_dict()
 
         # Create share type
         st_create = self.create_share_type(
-            name, extra_specs=extra_specs, version=version)
+            name, extra_specs=extra_specs, version=version,
+            description=description)
         self._verify_is_public_key_name(st_create['share_type'], version)
         st_id = st_create["share_type"]["id"]
 

manila_tempest_tests/tests/api/admin/test_share_types_negative.py

@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import ddt
 from tempest.lib.common.utils import data_utils
 from tempest.lib import exceptions as lib_exc
 from testtools import testcase as tc
@@ -20,6 +21,7 @@ from testtools import testcase as tc
 from manila_tempest_tests.tests.api import base
 
 
+@ddt.ddt
 class ShareTypesAdminNegativeTest(base.BaseSharesMixedTest):
 
     def _create_share_type(self):
@@ -48,6 +50,18 @@ class ShareTypesAdminNegativeTest(base.BaseSharesMixedTest):
                           "x" * 256,
                           client=self.admin_shares_v2_client)
 
+    @tc.attr(base.TAG_NEGATIVE, base.TAG_API)
+    @ddt.data('2.0', '2.6', '2.40')
+    def test_create_share_type_with_description_in_wrong_version(
+            self, version):
+        self.assertRaises(lib_exc.BadRequest,
+                          self.create_share_type,
+                          data_utils.rand_name("tempest_type_name"),
+                          extra_specs=self.add_extra_specs_to_dict(),
+                          description="tempest_type_description",
+                          version=version,
+                          client=self.admin_shares_v2_client)
+
     @tc.attr(base.TAG_NEGATIVE, base.TAG_API)
     def test_get_share_type_by_nonexistent_id(self):
         self.assertRaises(lib_exc.NotFound,