Refactor resource_fields.py

Refactor the resource_fields.py file to remove some (but not all) of the
duplication that is occurring.

  * Created a class Resource to contain the fields and label values
  * Added test cases for the new Resource class
  * Use the new Resource class to hold data for Chassis, Node, and
    Ports.
  * Have logic to support a 'sort_fields' and 'sort_labels' property
    which removes fields/labels which can not be used for sorting.

Change-Id: I846517a3714b1a86ef7ed7f52e911376e1915bb5

6 files changed, 270 insertions, 136 deletions

diff --git a/ironicclient/tests/unit/v1/test_chassis_shell.py b/ironicclient/tests/unit/v1/test_chassis_shell.py
index e3d92d6..2efd3a7 100644
--- a/ironicclient/tests/unit/v1/test_chassis_shell.py
+++ b/ironicclient/tests/unit/v1/test_chassis_shell.py
@@ -21,6 +21,18 @@ import ironicclient.v1.chassis_shell as c_shell
 
 
 class ChassisShellTest(utils.BaseTestCase):
+    def _get_client_mock_args(self, chassis=None, marker=None, limit=None,
+                              sort_dir=None, sort_key=None, detail=False):
+        args = mock.MagicMock()
+        args.chassis = chassis
+        args.marker = marker
+        args.limit = limit
+        args.sort_dir = sort_dir
+        args.sort_key = sort_key
+        args.detail = detail
+
+        return args
+
     def test_chassis_show(self):
         actual = {}
         fake_print_dict = lambda data, *args, **kwargs: actual.update(data)
@@ -47,17 +59,6 @@ class ChassisShellTest(utils.BaseTestCase):
                           c_shell.do_chassis_show,
                           client_mock, args)
 
-    def _get_client_mock_args(self, marker=None, limit=None, sort_dir=None,
-                              sort_key=None, detail=False):
-        args = mock.MagicMock()
-        args.marker = marker
-        args.limit = limit
-        args.sort_dir = sort_dir
-        args.sort_key = sort_key
-        args.detail = detail
-
-        return args
-
     def test_do_chassis_list(self):
         client_mock = mock.MagicMock()
         args = self._get_client_mock_args()
@@ -109,3 +110,21 @@ class ChassisShellTest(utils.BaseTestCase):
                           c_shell.do_chassis_list,
                           client_mock, args)
         self.assertFalse(client_mock.chassis.list.called)
+
+    def test_do_chassis_node_list(self):
+        client_mock = mock.MagicMock()
+        chassis_mock = mock.MagicMock(spec_set=[])
+        args = self._get_client_mock_args(chassis=chassis_mock)
+
+        c_shell.do_chassis_node_list(client_mock, args)
+        client_mock.chassis.list_nodes.assert_called_once_with(
+            chassis_mock, detail=False)
+
+    def test_do_chassis_node_list_details(self):
+        client_mock = mock.MagicMock()
+        chassis_mock = mock.MagicMock(spec_set=[])
+        args = self._get_client_mock_args(chassis=chassis_mock, detail=True)
+
+        c_shell.do_chassis_node_list(client_mock, args)
+        client_mock.chassis.list_nodes.assert_called_once_with(
+            chassis_mock, detail=True)
diff --git a/ironicclient/tests/unit/v1/test_resource_fields.py b/ironicclient/tests/unit/v1/test_resource_fields.py
index a826b8c..4c8d259 100644
--- a/ironicclient/tests/unit/v1/test_resource_fields.py
+++ b/ironicclient/tests/unit/v1/test_resource_fields.py
@@ -17,34 +17,53 @@ import testtools
 from ironicclient.v1 import resource_fields
 
 
-class ResourceFieldsTest(testtools.TestCase):
-
-    def test_chassis_fields(self):
-        self.assertEqual(
-            len(resource_fields.CHASSIS_FIELDS),
-            len(resource_fields.CHASSIS_FIELD_LABELS))
-
-    def test_chassis_list_fields(self):
-        self.assertEqual(
-            len(resource_fields.CHASSIS_LIST_FIELDS),
-            len(resource_fields.CHASSIS_LIST_FIELD_LABELS))
-
-    def test_node_fields(self):
-        self.assertEqual(
-            len(resource_fields.NODE_FIELDS),
-            len(resource_fields.NODE_FIELD_LABELS))
-
-    def test_node_list_fields(self):
-        self.assertEqual(
-            len(resource_fields.NODE_LIST_FIELDS),
-            len(resource_fields.NODE_LIST_FIELD_LABELS))
-
-    def test_port_fields(self):
-        self.assertEqual(
-            len(resource_fields.PORT_FIELDS),
-            len(resource_fields.PORT_FIELD_LABELS))
-
-    def test_port_list_fields(self):
-        self.assertEqual(
-            len(resource_fields.PORT_LIST_FIELDS),
-            len(resource_fields.PORT_LIST_FIELD_LABELS))
+class ResourceTest(testtools.TestCase):
+    def setUp(self):
+        super(ResourceTest, self).setUp()
+        self._saved_ids = resource_fields.Resource.FIELDS
+        resource_fields.Resource.FIELDS = {
+            'item1': 'ITEM1',
+            '2nd_item': 'A second item',
+            'item_3': 'Third item',
+        }
+
+    def tearDown(self):
+        super(ResourceTest, self).tearDown()
+        resource_fields.Resource.FIELDS = self._saved_ids
+
+    def test_fields_single_value(self):
+        # Make sure single value is what we expect
+        foo = resource_fields.Resource(['item1'])
+        self.assertEqual(('item1',), foo.fields)
+        self.assertEqual(('ITEM1',), foo.labels)
+        self.assertEqual(('item1',), foo.sort_fields)
+        self.assertEqual(('ITEM1',), foo.sort_labels)
+
+    def test_fields_multiple_value_order(self):
+        # Make sure order is maintained
+        foo = resource_fields.Resource(['2nd_item', 'item1'])
+        self.assertEqual(('2nd_item', 'item1'), foo.fields)
+        self.assertEqual(('A second item', 'ITEM1'), foo.labels)
+        self.assertEqual(('2nd_item', 'item1'), foo.sort_fields)
+        self.assertEqual(('A second item', 'ITEM1'), foo.sort_labels)
+
+    def test_sort_excluded(self):
+        # Test excluding of fields for sort purposes
+        foo = resource_fields.Resource(['item_3', 'item1', '2nd_item'],
+                                       sort_excluded=['item1'])
+        self.assertEqual(('item_3', '2nd_item'), foo.sort_fields)
+        self.assertEqual(('Third item', 'A second item'), foo.sort_labels)
+
+    def test_sort_excluded_unknown(self):
+        # Test sort_excluded value not in the field_ids
+        self.assertRaises(
+            ValueError,
+            resource_fields.Resource,
+            ['item_3', 'item1', '2nd_item'],
+            sort_excluded=['item1', 'foo'])
+
+    def test_unknown_field_id(self):
+        self.assertRaises(
+            KeyError,
+            resource_fields.Resource,
+            ['item1', 'unknown_id'])
diff --git a/ironicclient/v1/chassis_shell.py b/ironicclient/v1/chassis_shell.py
index e37839a..74ac813 100644
--- a/ironicclient/v1/chassis_shell.py
+++ b/ironicclient/v1/chassis_shell.py
@@ -63,13 +63,13 @@ def do_chassis_show(cc, args):
 def do_chassis_list(cc, args):
     """List the chassis."""
     if args.detail:
-        fields = res_fields.CHASSIS_FIELDS
-        field_labels = res_fields.CHASSIS_FIELD_LABELS
-        sort_fields = res_fields.CHASSIS_SORT_FIELDS
-        sort_field_labels = res_fields.CHASSIS_SORT_FIELD_LABELS
+        fields = res_fields.CHASSIS_DETAILED_RESOURCE.fields
+        field_labels = res_fields.CHASSIS_DETAILED_RESOURCE.labels
+        sort_fields = res_fields.CHASSIS_DETAILED_RESOURCE.sort_fields
+        sort_field_labels = res_fields.CHASSIS_DETAILED_RESOURCE.sort_labels
     else:
-        fields = res_fields.CHASSIS_LIST_FIELDS
-        field_labels = res_fields.CHASSIS_LIST_FIELD_LABELS
+        fields = res_fields.CHASSIS_RESOURCE.fields
+        field_labels = res_fields.CHASSIS_RESOURCE.labels
         sort_fields = fields
         sort_field_labels = field_labels
 
@@ -169,11 +169,11 @@ def do_chassis_update(cc, args):
 def do_chassis_node_list(cc, args):
     """List the nodes contained in a chassis."""
     if args.detail:
-        fields = res_fields.NODE_FIELDS
-        field_labels = res_fields.NODE_FIELD_LABELS
+        fields = res_fields.NODE_DETAILED_RESOURCE.fields
+        field_labels = res_fields.NODE_DETAILED_RESOURCE.labels
     else:
-        fields = res_fields.NODE_LIST_FIELDS
-        field_labels = res_fields.NODE_LIST_FIELD_LABELS
+        fields = res_fields.NODE_RESOURCE.fields
+        field_labels = res_fields.NODE_RESOURCE.labels
 
     params = utils.common_params_for_list(args, fields, field_labels)
 
diff --git a/ironicclient/v1/node_shell.py b/ironicclient/v1/node_shell.py
index eaf8bc2..aebe309 100644
--- a/ironicclient/v1/node_shell.py
+++ b/ironicclient/v1/node_shell.py
@@ -25,7 +25,9 @@ from ironicclient.v1 import resource_fields as res_fields
 
 
 def _print_node_show(node):
-    data = dict([(f, getattr(node, f, '')) for f in res_fields.NODE_FIELDS])
+    data = dict(
+        [(f, getattr(node, f, ''))
+         for f in res_fields.NODE_DETAILED_RESOURCE.fields])
     cliutils.print_dict(data, wrap=72)
 
 
@@ -97,13 +99,13 @@ def do_node_list(cc, args):
     params['detail'] = args.detail
 
     if args.detail:
-        fields = res_fields.NODE_FIELDS
-        field_labels = res_fields.NODE_FIELD_LABELS
-        sort_fields = res_fields.NODE_SORT_FIELDS
-        sort_field_labels = res_fields.NODE_SORT_FIELD_LABELS
+        fields = res_fields.NODE_DETAILED_RESOURCE.fields
+        field_labels = res_fields.NODE_DETAILED_RESOURCE.labels
+        sort_fields = res_fields.NODE_DETAILED_RESOURCE.sort_fields
+        sort_field_labels = res_fields.NODE_DETAILED_RESOURCE.sort_labels
     else:
-        fields = res_fields.NODE_LIST_FIELDS
-        field_labels = res_fields.NODE_LIST_FIELD_LABELS
+        fields = res_fields.NODE_RESOURCE.fields
+        field_labels = res_fields.NODE_RESOURCE.labels
         sort_fields = fields
         sort_field_labels = field_labels
 
@@ -277,11 +279,11 @@ def do_node_vendor_passthru(cc, args):
 def do_node_port_list(cc, args):
     """List the ports associated with a node."""
     if args.detail:
-        fields = res_fields.PORT_FIELDS
-        field_labels = res_fields.PORT_FIELD_LABELS
+        fields = res_fields.PORTS_DETAILED_RESOURCE.fields
+        field_labels = res_fields.PORTS_DETAILED_RESOURCE.labels
     else:
-        fields = res_fields.PORT_LIST_FIELDS
-        field_labels = res_fields.PORT_LIST_FIELD_LABELS
+        fields = res_fields.PORT_RESOURCE.fields
+        field_labels = res_fields.PORT_RESOURCE.labels
 
     params = utils.common_params_for_list(args, fields, field_labels)
 
diff --git a/ironicclient/v1/port_shell.py b/ironicclient/v1/port_shell.py
index 6d166ff..b7c2349 100644
--- a/ironicclient/v1/port_shell.py
+++ b/ironicclient/v1/port_shell.py
@@ -84,13 +84,13 @@ def do_port_list(cc, args):
         params['address'] = args.address
 
     if args.detail:
-        fields = res_fields.PORT_FIELDS
-        field_labels = res_fields.PORT_FIELD_LABELS
-        sort_fields = res_fields.PORT_SORT_FIELDS
-        sort_field_labels = res_fields.PORT_SORT_FIELD_LABELS
+        fields = res_fields.PORT_DETAILED_RESOURCE.fields
+        field_labels = res_fields.PORT_DETAILED_RESOURCE.labels
+        sort_fields = res_fields.PORT_DETAILED_RESOURCE.sort_fields
+        sort_field_labels = res_fields.PORT_DETAILED_RESOURCE.sort_labels
     else:
-        fields = res_fields.PORT_LIST_FIELDS
-        field_labels = res_fields.PORT_LIST_FIELD_LABELS
+        fields = res_fields.PORT_RESOURCE.fields
+        field_labels = res_fields.PORT_RESOURCE.labels
         sort_fields = fields
         sort_field_labels = field_labels
 
diff --git a/ironicclient/v1/resource_fields.py b/ironicclient/v1/resource_fields.py
index 63995c7..3c8e723 100644
--- a/ironicclient/v1/resource_fields.py
+++ b/ironicclient/v1/resource_fields.py
@@ -13,77 +13,171 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from ironicclient.common.i18n import _
+
+
+class Resource(object):
+    """Resource class
+
+    This class is used to manage the various fields that a resource (e.g.
+    Chassis, Node, Port) contains.  An individual field consists of a
+    'field_id' (key) and a 'label' (value).  The caller only provides the
+    'field_ids' when instantiating the object.
+
+    Ordering of the 'field_ids' will be preserved as specified by the caller.
+
+    It also provides the ability to exclude some of these fields when they are
+    being used for sorting.
+    """
+
+    FIELDS = {
+        'address': 'Address',
+        'chassis_uuid': 'Chassis UUID',
+        'console_enabled': 'Console Enabled',
+        'created_at': 'Created At',
+        'description': 'Description',
+        'driver': 'Driver',
+        'driver_info': 'Driver Info',
+        'driver_internal_info': 'Driver Internal Info',
+        'extra': 'Extra',
+        'inspection_finished_at': 'Inspection Finished At',
+        'inspection_started_at': 'Inspection Started At',
+        'instance_info': 'Instance Info',
+        'instance_uuid': 'Instance UUID',
+        'last_error': 'Last Error',
+        'maintenance': 'Maintenance',
+        'maintenance_reason': 'Maintenance Reason',
+        'name': 'Name',
+        'node_uuid': 'Node UUID',
+        'power_state': 'Power State',
+        'properties': 'Properties',
+        'provision_state': 'Provisioning State',
+        'reservation': 'Reservation',
+        'target_power_state': 'Target Power State',
+        'target_provision_state': 'Target Provision State',
+        'updated_at': 'Updated At',
+        'uuid': 'UUID',
+    }
+
+    def __init__(self, field_ids, sort_excluded=None):
+        """Create a Resource object
+
+        :param field_ids:  A list of strings that the Resource object will
+                           contain.  Each string must match an existing key in
+                           FIELDS.
+        :param sort_excluded: Optional. A list of strings that will not be used
+                              for sorting.  Must be a subset of 'field_ids'.
+
+        :raises: ValueError if sort_excluded contains value not in field_ids
+        """
+        self._fields = tuple(field_ids)
+        self._labels = tuple([self.FIELDS[x] for x in field_ids])
+        if sort_excluded is None:
+            sort_excluded = []
+        not_existing = set(sort_excluded) - set(field_ids)
+        if not_existing:
+            raise ValueError(
+                _("sort_excluded specified with value not contained in "
+                  "field_ids.  Unknown value(s): %s") % ','.join(not_existing))
+        self._sort_fields = tuple(
+            [x for x in field_ids if x not in sort_excluded])
+        self._sort_labels = tuple([self.FIELDS[x] for x in self._sort_fields])
+
+    @property
+    def fields(self):
+        return self._fields
+
+    @property
+    def labels(self):
+        return self._labels
+
+    @property
+    def sort_fields(self):
+        return self._sort_fields
+
+    @property
+    def sort_labels(self):
+        return self._sort_labels
 
-# Chassis
-
-CHASSIS_FIELDS = ['uuid', 'description', 'created_at', 'updated_at', 'extra']
-
-CHASSIS_FIELD_LABELS = ['UUID', 'Description', 'Created At', 'Updated At',
-                        'Extra']
-
-CHASSIS_SORT_FIELDS = ['uuid', 'description', 'created_at', 'updated_at']
 
-CHASSIS_SORT_FIELD_LABELS = ['UUID', 'Description', 'Created At', 'Updated At']
-
-CHASSIS_LIST_FIELDS = ['uuid', 'description']
-
-CHASSIS_LIST_FIELD_LABELS = ['UUID', 'Description']
+# Chassis
+CHASSIS_DETAILED_RESOURCE = Resource(
+    ['uuid',
+     'description',
+     'created_at',
+     'updated_at',
+     'extra',
+     ],
+    sort_excluded=['extra'])
+CHASSIS_RESOURCE = Resource(
+    ['uuid',
+     'description',
+     ])
 
 
 # Nodes
-
-NODE_FIELDS = ['chassis_uuid', 'created_at', 'console_enabled', 'driver',
-               'driver_info', 'driver_internal_info', 'extra',
-               'instance_info', 'instance_uuid', 'last_error',
-               'maintenance', 'maintenance_reason', 'power_state',
-               'properties', 'provision_state', 'reservation',
-               'target_power_state', 'target_provision_state',
-               'updated_at', 'inspection_finished_at',
-               'inspection_started_at', 'uuid', 'name']
-
-NODE_FIELD_LABELS = ['Chassis UUID', 'Created At', 'Console Enabled', 'Driver',
-                     'Driver Info', 'Driver Internal Info', 'Extra',
-                     'Instance Info', 'Instance UUID', 'Last Error',
-                     'Maintenance', 'Maintenance Reason', 'Power State',
-                     'Properties', 'Provision State', 'Reservation',
-                     'Target Power State', 'Target Provision State',
-                     'Updated At', 'Inspection Finished At',
-                     'Inspection Started At', 'UUID', 'Name']
-
-# The server cannot sort on "chassis_uuid" because it isn't a
-# column in the "nodes" database table. "chassis_id" is stored,
-# but it is internal to ironic. See bug #1443003 for more details.
-NODE_SORT_FIELDS = [x for x in NODE_FIELDS if x not in [
-                    'chassis_uuid', 'driver_info', 'driver_internal_info',
-                    'extra', 'instance_info', 'properties']]
-
-NODE_SORT_FIELD_LABELS = [x for x in NODE_FIELD_LABELS if x not in [
-                          'Chassis UUID', 'Driver Info',
-                          'Driver Internal Info', 'EXTRA',
-                          'Instance Info', 'Properties']]
-
-NODE_LIST_FIELDS = ['uuid', 'name', 'instance_uuid', 'power_state',
-                    'provision_state', 'maintenance']
-
-NODE_LIST_FIELD_LABELS = ['UUID', 'Name', 'Instance UUID', 'Power State',
-                          'Provisioning State', 'Maintenance']
-
+NODE_DETAILED_RESOURCE = Resource(
+    ['chassis_uuid',
+     'created_at',
+     'console_enabled',
+     'driver',
+     'driver_info',
+     'driver_internal_info',
+     'extra',
+     'instance_info',
+     'instance_uuid',
+     'last_error',
+     'maintenance',
+     'maintenance_reason',
+     'power_state',
+     'properties',
+     'provision_state',
+     'reservation',
+     'target_power_state',
+     'target_provision_state',
+     'updated_at',
+     'inspection_finished_at',
+     'inspection_started_at',
+     'uuid',
+     'name',
+     ],
+    sort_excluded=[
+        # The server cannot sort on "chassis_uuid" because it isn't a column in
+        # the "nodes" database table. "chassis_id" is stored, but it is
+        # internal to ironic. See bug #1443003 for more details.
+        'chassis_uuid',
+        'driver_info',
+        'driver_internal_info',
+        'extra',
+        'instance_info',
+        'properties',
+    ])
+NODE_RESOURCE = Resource(
+    ['uuid',
+     'name',
+     'instance_uuid',
+     'power_state',
+     'provision_state',
+     'maintenance',
+     ])
 
 # Ports
-
-PORT_FIELDS = ['uuid', 'address', 'created_at', 'extra', 'node_uuid',
-               'updated_at']
-
-PORT_FIELD_LABELS = ['UUID', 'Address', 'Created At', 'Extra', 'Node UUID',
-                     'Updated At']
-
-# The server cannot sort on "node_uuid" because it isn't a
-# column in the "ports" database table. "node_id" is stored,
-# but it is internal to ironic. See bug #1443003 for more details.
-PORT_SORT_FIELDS = ['uuid', 'address', 'created_at', 'updated_at']
-
-PORT_SORT_FIELD_LABELS = ['UUID', 'Address', 'Created At', 'Updated At']
-
-PORT_LIST_FIELDS = ['uuid', 'address']
-
-PORT_LIST_FIELD_LABELS = ['UUID', 'Address']
+PORT_DETAILED_RESOURCE = Resource(
+    ['uuid',
+     'address',
+     'created_at',
+     'extra',
+     'node_uuid',
+     'updated_at',
+     ],
+    sort_excluded=[
+        'extra',
+        # The server cannot sort on "node_uuid" because it isn't a column in
+        # the "ports" database table. "node_id" is stored, but it is internal
+        # to ironic. See bug #1443003 for more details.
+        'node_uuid',
+    ])
+PORT_RESOURCE = Resource(
+    ['uuid',
+     'address',
+     ])