Repository: softlayer/softlayer-python
Pull request #797 (2017-01-09 21:59:51)

Fix the way storage space is looked up, remove os type requirement for file storage ordering
Fix the way storage space is looked up for block and file storage ordering. Remove the OS type requirement from file storage ordering as it is not used.

Addresses #795


Patch:
diff --git a/SoftLayer/CLI/file/order.py b/SoftLayer/CLI/file/order.py
index 7f4acfc9..d13b952b 100644
--- a/SoftLayer/CLI/file/order.py
+++ b/SoftLayer/CLI/file/order.py
@@ -28,17 +28,6 @@
               help='Endurance Storage Tier (IOP per GB)'
               '  [required for storage-type endurance]',
               type=click.Choice(['0.25', '2', '4', '10']))
-@click.option('--os-type',
-              help='Operating System',
-              type=click.Choice([
-                  'HYPER_V',
-                  'LINUX',
-                  'VMWARE',
-                  'WINDOWS_2008',
-                  'WINDOWS_GPT',
-                  'WINDOWS',
-                  'XEN']),
-              required=True)
 @click.option('--location',
               help='Datacenter short name (e.g.: dal09)',
               required=True)
@@ -48,7 +37,7 @@
               'space along with endurance file storage; specifies '
               'the size (in GB) of snapshot space to order')
 @environment.pass_env
-def cli(env, storage_type, size, iops, tier, os_type,
+def cli(env, storage_type, size, iops, tier,
         location, snapshot_size):
     """Order a file storage volume."""
     file_manager = SoftLayer.FileStorageManager(env.client)
@@ -79,8 +68,7 @@ def cli(env, storage_type, size, iops, tier, os_type,
                 storage_type='performance_storage_nfs',
                 location=location,
                 size=size,
-                iops=iops,
-                os_type=os_type
+                iops=iops
             )
         except ValueError as ex:
             raise exceptions.ArgumentError(str(ex))
@@ -98,7 +86,6 @@ def cli(env, storage_type, size, iops, tier, os_type,
                 location=location,
                 size=size,
                 tier_level=float(tier),
-                os_type=os_type,
                 snapshot_size=snapshot_size
             )
         except ValueError as ex:
diff --git a/SoftLayer/managers/block.py b/SoftLayer/managers/block.py
index 90f94cb4..7a933b04 100644
--- a/SoftLayer/managers/block.py
+++ b/SoftLayer/managers/block.py
@@ -271,7 +271,7 @@ def order_block_volume(self, storage_type, location, size, os_type,
                     package,
                     'performance_storage_iscsi'
                     ),
-                storage_utils.find_performance_space_price(package, iops),
+                storage_utils.find_performance_space_price(package, size),
                 storage_utils.find_performance_iops_price(package, size, iops),
             ]
         elif storage_type == 'storage_service_enterprise':
diff --git a/SoftLayer/managers/file.py b/SoftLayer/managers/file.py
index 717d4edd..638db547 100644
--- a/SoftLayer/managers/file.py
+++ b/SoftLayer/managers/file.py
@@ -228,7 +228,7 @@ def delete_snapshot(self, snapshot_id):
         return self.client.call('Network_Storage', 'deleteObject',
                                 id=snapshot_id)
 
-    def order_file_volume(self, storage_type, location, size, os_type,
+    def order_file_volume(self, storage_type, location, size, os_type=None,
                           iops=None, tier_level=None, snapshot_size=None):
         """Places an order for a file volume.
 
@@ -236,12 +236,15 @@ def order_file_volume(self, storage_type, location, size, os_type,
                              or "storage_service_enterprise" (endurance)
         :param location: Datacenter in which to order iSCSI volume
         :param size: Size of the desired volume, in GB
-        :param os_type: OS Type to use for volume alignment, see help for list
+        :param os_type: Not used for file storage orders, leave None
         :param iops: Number of IOPs for a "Performance" order
         :param tier_level: Tier level to use for an "Endurance" order
         :param snapshot_size: The size of optional snapshot space,
         if snapshot space should also be ordered (None if not ordered)
         """
+        if os_type:
+            raise exceptions.SoftLayerError(
+                'OS type is not used on file storage orders')
 
         try:
             location_id = storage_utils.get_location_id(self, location)
@@ -259,7 +262,7 @@ def order_file_volume(self, storage_type, location, size, os_type,
                     package,
                     'performance_storage_nfs'
                     ),
-                storage_utils.find_performance_space_price(package, iops),
+                storage_utils.find_performance_space_price(package, size),
                 storage_utils.find_performance_iops_price(package, size, iops),
             ]
         elif storage_type == 'storage_service_enterprise':
@@ -288,7 +291,6 @@ def order_file_volume(self, storage_type, location, size, os_type,
         order = {
             'complexType': complex_type,
             'packageId': package['id'],
-            'osFormatType': {'keyName': os_type},
             'prices': prices,
             'quantity': 1,
             'location': location_id,
@@ -313,6 +315,15 @@ def enable_snapshots(self, volume_id, schedule_type, retention_count,
 
         :param integer volume_id: The id of the volume
         :param string schedule_type: 'HOURLY'|'DAILY'|'WEEKLY'
+        :param integer retention_count: The number of snapshots to attempt to
+        retain in this schedule
+        :param integer minute: The minute of the hour at which HOURLY, DAILY,
+        and WEEKLY snapshots should be taken
+        :param integer hour: The hour of the day at which DAILY and WEEKLY
+        snapshots should be taken
+        :param string|integer day_of_week: The day of the week on which WEEKLY
+        snapshots should be taken, either as a string ('SUNDAY') or integer
+        ('0' is Sunday)
         :return: Returns whether successfully scheduled or not
         """
 
@@ -340,7 +351,7 @@ def order_snapshot_space(self, volume_id, capacity, tier,
                              upgrade, **kwargs):
         """Orders snapshot space for the given file volume.
 
-        :param integer volume_id: The id of the volume
+        :param integer volume_id: The ID of the volume
         :param integer capacity: The capacity to order, in GB
         :param float tier: The tier level of the file volume, in IOPS per GB
         :param boolean upgrade: Flag to indicate if this order is an upgrade
@@ -390,7 +401,7 @@ def cancel_snapshot_space(self, volume_id,
 
         :param integer volume_id: The volume ID
         :param string reason: The reason for cancellation
-        :param boolean immediate_flag: Cancel immediately or
+        :param boolean immediate: Cancel immediately or
         on anniversary date
         """
 
@@ -422,9 +433,9 @@ def cancel_snapshot_space(self, volume_id,
     def restore_from_snapshot(self, volume_id, snapshot_id):
         """Restores a specific volume from a snapshot
 
-        :param integer volume_id: The id of the volume
+        :param integer volume_id: The ID of the volume
         :param integer snapshot_id: The id of the restore point
-        :return: Returns whether succesfully restored or not
+        :return: Returns whether successfully restored or not
         """
 
         return self.client.call('Network_Storage', 'restoreFromSnapshot',
@@ -437,7 +448,7 @@ def cancel_file_volume(self, volume_id,
 
         :param integer volume_id: The volume ID
         :param string reason: The reason for cancellation
-        :param boolean immediate_flag: Cancel immediately or
+        :param boolean immediate: Cancel immediately or
         on anniversary date
         """
         file_volume = self.get_file_volume_details(
@@ -454,7 +465,7 @@ def cancel_file_volume(self, volume_id,
     def failover_to_replicant(self, volume_id, replicant_id, immediate=False):
         """Failover to a volume replicant.
 
-        :param integer volume_id: The id of the volume
+        :param integer volume_id: The ID of the volume
         :param integer replicant_id: ID of replicant to failover to
         :param boolean immediate: Flag indicating if failover is immediate
         :return: Returns whether failover was successful or not
@@ -466,8 +477,8 @@ def failover_to_replicant(self, volume_id, replicant_id, immediate=False):
     def failback_from_replicant(self, volume_id, replicant_id):
         """Failback from a volume replicant.
 
-        :param integer volume_id: The id of the volume
-        :param integer: ID of replicant to failback from
+        :param integer volume_id: The ID of the volume
+        :param integer replicant_id: ID of replicant to failback from
         :return: Returns whether failback was successful or not
         """
 
diff --git a/SoftLayer/managers/storage_utils.py b/SoftLayer/managers/storage_utils.py
index 3fe47af6..6c840a73 100644
--- a/SoftLayer/managers/storage_utils.py
+++ b/SoftLayer/managers/storage_utils.py
@@ -60,7 +60,7 @@ def populate_host_templates(host_templates,
 
 
 def get_package(manager, category_code):
-    """Returns a product packaged based on type of storage.
+    """Returns a product package based on type of storage.
 
     :param manager: The storage manager which calls this function.
     :param category_code: Category code of product package.


Test patch:
diff --git a/tests/CLI/modules/block_tests.py b/tests/CLI/modules/block_tests.py
index fbb225a0..01759881 100644
--- a/tests/CLI/modules/block_tests.py
+++ b/tests/CLI/modules/block_tests.py
@@ -217,6 +217,30 @@ def test_volume_order_order_not_placed(self, order_mock):
                          'Order could not be placed! Please verify '
                          'your options and try again.\n')
 
+    @mock.patch('SoftLayer.BlockStorageManager.order_block_volume')
+    def test_volume_order_performance_manager_error(self, order_mock):
+        order_mock.side_effect = ValueError('failure!')
+
+        result = self.run_command(['block', 'volume-order',
+                                   '--storage-type=performance', '--size=20',
+                                   '--iops=100', '--os-type=linux',
+                                   '--location=dal05'])
+
+        self.assertEqual(2, result.exit_code)
+        self.assertEqual('Argument Error: failure!', result.exception.message)
+
+    @mock.patch('SoftLayer.BlockStorageManager.order_block_volume')
+    def test_volume_order_endurance_manager_error(self, order_mock):
+        order_mock.side_effect = ValueError('failure!')
+
+        result = self.run_command(['block', 'volume-order',
+                                   '--storage-type=endurance', '--size=20',
+                                   '--tier=0.25', '--os-type=linux',
+                                   '--location=dal05'])
+
+        self.assertEqual(2, result.exit_code)
+        self.assertEqual('Argument Error: failure!', result.exception.message)
+
     def test_enable_snapshots(self):
         result = self.run_command(['block', 'snapshot-enable', '12345678',
                                    '--schedule-type=HOURLY', '--minute=10',
diff --git a/tests/CLI/modules/file_tests.py b/tests/CLI/modules/file_tests.py
index 3bd43015..0b7f8c4e 100644
--- a/tests/CLI/modules/file_tests.py
+++ b/tests/CLI/modules/file_tests.py
@@ -151,31 +151,29 @@ def test_volume_detail(self):
     def test_volume_order_performance_iops_not_given(self):
         result = self.run_command(['file', 'volume-order',
                                    '--storage-type=performance', '--size=20',
-                                   '--os-type=linux', '--location=dal05'])
+                                   '--location=dal05'])
 
         self.assertEqual(2, result.exit_code)
 
     def test_volume_order_performance_iops_out_of_range(self):
         result = self.run_command(['file', 'volume-order',
                                    '--storage-type=performance', '--size=20',
-                                   '--iops=80000', '--os-type=linux',
-                                   '--location=dal05'])
+                                   '--iops=80000', '--location=dal05'])
 
         self.assertEqual(2, result.exit_code)
 
     def test_volume_order_performance_iops_not_multiple_of_100(self):
         result = self.run_command(['file', 'volume-order',
                                    '--storage-type=performance', '--size=20',
-                                   '--iops=122', '--os-type=linux',
-                                   '--location=dal05'])
+                                   '--iops=122', '--location=dal05'])
 
         self.assertEqual(2, result.exit_code)
 
     def test_volume_order_performance_snapshot_error(self):
         result = self.run_command(['file', 'volume-order',
                                    '--storage-type=performance', '--size=20',
-                                   '--iops=100', '--os-type=linux',
-                                   '--location=dal05', '--snapshot-size=10'])
+                                   '--iops=100', '--location=dal05',
+                                   '--snapshot-size=10'])
 
         self.assertEqual(2, result.exit_code)
 
@@ -194,8 +192,7 @@ def test_volume_order_performance(self, order_mock):
 
         result = self.run_command(['file', 'volume-order',
                                    '--storage-type=performance', '--size=20',
-                                   '--iops=100', '--os-type=linux',
-                                   '--location=dal05'])
+                                   '--iops=100', '--location=dal05'])
 
         self.assert_no_fail(result)
         self.assertEqual(result.output,
@@ -206,7 +203,7 @@ def test_volume_order_performance(self, order_mock):
     def test_volume_order_endurance_tier_not_given(self):
         result = self.run_command(['file', 'volume-order',
                                    '--storage-type=endurance', '--size=20',
-                                   '--os-type=linux', '--location=dal05'])
+                                   '--location=dal05'])
 
         self.assertEqual(2, result.exit_code)
 
@@ -226,8 +223,8 @@ def test_volume_order_endurance(self, order_mock):
 
         result = self.run_command(['file', 'volume-order',
                                    '--storage-type=endurance', '--size=20',
-                                   '--tier=0.25', '--os-type=linux',
-                                   '--location=dal05', '--snapshot-size=10'])
+                                   '--tier=0.25', '--location=dal05',
+                                   '--snapshot-size=10'])
 
         self.assert_no_fail(result)
         self.assertEqual(result.output,
@@ -242,14 +239,35 @@ def test_volume_order_order_not_placed(self, order_mock):
 
         result = self.run_command(['file', 'volume-order',
                                    '--storage-type=endurance', '--size=20',
-                                   '--tier=0.25', '--os-type=linux',
-                                   '--location=dal05'])
+                                   '--tier=0.25', '--location=dal05'])
 
         self.assert_no_fail(result)
         self.assertEqual(result.output,
                          'Order could not be placed! Please verify '
                          'your options and try again.\n')
 
+    @mock.patch('SoftLayer.FileStorageManager.order_file_volume')
+    def test_volume_order_performance_manager_error(self, order_mock):
+        order_mock.side_effect = ValueError('failure!')
+
+        result = self.run_command(['file', 'volume-order',
+                                   '--storage-type=performance', '--size=20',
+                                   '--iops=100', '--location=dal05'])
+
+        self.assertEqual(2, result.exit_code)
+        self.assertEqual('Argument Error: failure!', result.exception.message)
+
+    @mock.patch('SoftLayer.FileStorageManager.order_file_volume')
+    def test_volume_order_endurance_manager_error(self, order_mock):
+        order_mock.side_effect = ValueError('failure!')
+
+        result = self.run_command(['file', 'volume-order',
+                                   '--storage-type=endurance', '--size=20',
+                                   '--tier=0.25', '--location=dal05'])
+
+        self.assertEqual(2, result.exit_code)
+        self.assertEqual('Argument Error: failure!', result.exception.message)
+
     def test_enable_snapshots(self):
         result = self.run_command(['file', 'snapshot-enable', '12345678',
                                    '--schedule-type=HOURLY', '--minute=10',
diff --git a/tests/managers/block_tests.py b/tests/managers/block_tests.py
index 091e6e66..d288ac95 100644
--- a/tests/managers/block_tests.py
+++ b/tests/managers/block_tests.py
@@ -76,6 +76,25 @@ def test_delete_snapshot(self):
             'deleteObject',
             identifier=100)
 
+    def test_order_block_volume_invalid_location(self):
+        mock = self.set_mock('SoftLayer_Location_Datacenter', 'getDatacenters')
+        mock.return_value = []
+
+        exception = self.assertRaises(
+            exceptions.SoftLayerError,
+            self.block.order_block_volume,
+            "performance_storage_iscsi",
+            "dal05",
+            100,
+            "LINUX",
+            iops=100,
+        )
+
+        self.assertEqual(str(exception), "Invalid datacenter name "
+                                         "specified. Please provide the "
+                                         "lower case short name "
+                                         "(e.g.: dal09)")
+
     def test_order_block_volume_no_package(self):
         mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
         mock.return_value = []
@@ -138,6 +157,20 @@ def test_replicant_failback(self):
             identifier=1234,
         )
 
+    def test_order_block_volume_invalid_storage_type(self):
+        mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
+        mock.return_value = [{}]
+
+        self.assertRaises(
+            exceptions.SoftLayerError,
+            self.block.order_block_volume,
+            "something_completely_different",
+            "dal05",
+            100,
+            "LINUX",
+            iops=100,
+        )
+
     def test_order_block_volume_performance(self):
         mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
         mock.return_value = [{
@@ -545,6 +578,78 @@ def test_order_snapshot_space(self):
                 },
             )
 
+    def test_order_snapshot_space_invalid_category(self):
+        mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
+        mock.return_value = [{
+            'id': 240,
+            'name': 'Endurance',
+            'items': [{
+                'capacity': '0',
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'locationGroupId': '530',
+                    }],
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'locationGroupId': '',
+                    'categories': [{
+                        'categoryCode': 'storage_block',
+                    }],
+                }],
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'locationGroupId': '',
+                    'categories': [{
+                        'categoryCode': 'storage_snapshot_space',
+                    }],
+                    'capacityRestrictionMinimum': '300',
+                }],
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'locationGroupId': '',
+                    'categories': [{
+                        'categoryCode': 'storage_snapshot_space',
+                    }],
+                    'capacityRestrictionMinimum': '100',
+                    'capacityRestrictionMaximum': '100',
+                }],
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'id': 46130,
+                    'locationGroupId': '',
+                    'categories': [{
+                        'categoryCode': 'storage_snapshot_space',
+                    }],
+                    'capacityRestrictionMinimum': '200',
+                    'capacityRestrictionMaximum': '200',
+                }],
+            }],
+        }]
+
+        billing_item_mock = self.set_mock('SoftLayer_Network_Storage',
+                                          'getObject')
+        billing_item_mock.return_value = {
+            'billingItem': {
+                'categoryCode': 'not_storage_service_enterprise'
+            }
+        }
+
+        exception = self.assertRaises(
+            exceptions.SoftLayerError,
+            self.block.order_snapshot_space,
+            100,
+            5,
+            None,
+            False
+        )
+        self.assertEqual(str(exception), "Block volume storage_type must be "
+                                         "Endurance")
+
     def test_order_block_replicant_invalid_location(self):
         self.assertRaises(
             exceptions.SoftLayerError,
diff --git a/tests/managers/file_tests.py b/tests/managers/file_tests.py
index 1f9c5617..a92ddcb4 100644
--- a/tests/managers/file_tests.py
+++ b/tests/managers/file_tests.py
@@ -177,6 +177,25 @@ def test_delete_snapshot(self):
             'deleteObject',
             identifier=100)
 
+    def test_order_file_volume_invalid_location(self):
+        mock = self.set_mock('SoftLayer_Location_Datacenter', 'getDatacenters')
+        mock.return_value = []
+
+        exception = self.assertRaises(
+            exceptions.SoftLayerError,
+            self.file.order_file_volume,
+            "performance_storage_nfs",
+            "dal05",
+            100,
+            None,
+            iops=100,
+        )
+
+        self.assertEqual(str(exception), "Invalid datacenter name "
+                                         "specified. Please provide the "
+                                         "lower case short name "
+                                         "(e.g.: dal09)")
+
     def test_order_file_volume_no_package(self):
         mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
         mock.return_value = []
@@ -187,7 +206,7 @@ def test_order_file_volume_no_package(self):
             "performance_storage_nfs",
             "dal05",
             40,
-            "LINUX",
+            None,
             iops=100,
         )
 
@@ -201,9 +220,38 @@ def test_order_file_volume_too_many_packages(self):
             "performance_storage_nfs",
             "dal05",
             40,
+            None,
+            iops=100,
+        )
+
+    def test_order_file_volume_invalid_storage_type(self):
+        mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
+        mock.return_value = [{}]
+
+        exception = self.assertRaises(
+            exceptions.SoftLayerError,
+            self.file.order_file_volume,
+            "something_completely_different",
+            "dal05",
+            100,
+            None,
+            iops=100,
+        )
+        self.assertEqual(str(exception), "File volume storage_type must be "
+                                         "either Performance or Endurance")
+
+    def test_order_file_volume_os_type_provided(self):
+        exception = self.assertRaises(
+            exceptions.SoftLayerError,
+            self.file.order_file_volume,
+            "performance_storage_nfs",
+            "dal05",
+            100,
             "LINUX",
             iops=100,
         )
+        self.assertEqual(str(exception), "OS type is not used on file "
+                                         "storage orders")
 
     def test_order_file_volume_performance(self):
         mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
@@ -246,7 +294,7 @@ def test_order_file_volume_performance(self):
             "performance_storage_nfs",
             "dal05",
             100,
-            "LINUX",
+            None,
             iops=100,
             )
 
@@ -330,7 +378,7 @@ def test_order_file_volume_endurance(self):
             "storage_service_enterprise",
             "dal05",
             100,
-            "LINUX",
+            None,
             tier_level=0.25,
             )
 
@@ -417,7 +465,7 @@ def test_order_file_volume_endurance_with_snapshot(self):
             "storage_service_enterprise",
             "dal05",
             100,
-            "LINUX",
+            None,
             tier_level=0.25,
             snapshot_size=10,
             )
@@ -467,6 +515,78 @@ def test_order_snapshot_space_too_many_packages(self):
             False,
         )
 
+    def test_order_snapshot_space_invalid_category(self):
+        mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
+        mock.return_value = [{
+            'id': 240,
+            'name': 'Endurance',
+            'items': [{
+                'capacity': '0',
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'locationGroupId': '530',
+                    }],
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'locationGroupId': '',
+                    'categories': [{
+                        'categoryCode': 'storage_file',
+                    }],
+                }],
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'locationGroupId': '',
+                    'categories': [{
+                        'categoryCode': 'storage_snapshot_space',
+                    }],
+                    'capacityRestrictionMinimum': '300',
+                }],
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'locationGroupId': '',
+                    'categories': [{
+                        'categoryCode': 'storage_snapshot_space',
+                    }],
+                    'capacityRestrictionMinimum': '100',
+                    'capacityRestrictionMaximum': '100',
+                }],
+            }, {
+                'capacity': '5',
+                'prices': [{
+                    'id': 46130,
+                    'locationGroupId': '',
+                    'categories': [{
+                        'categoryCode': 'storage_snapshot_space',
+                    }],
+                    'capacityRestrictionMinimum': '200',
+                    'capacityRestrictionMaximum': '200',
+                }],
+            }],
+        }]
+
+        billing_item_mock = self.set_mock('SoftLayer_Network_Storage',
+                                          'getObject')
+        billing_item_mock.return_value = {
+            'billingItem': {
+                'categoryCode': 'not_storage_service_enterprise'
+            }
+        }
+
+        exception = self.assertRaises(
+            exceptions.SoftLayerError,
+            self.file.order_snapshot_space,
+            100,
+            5,
+            None,
+            False,
+        )
+        self.assertEqual(str(exception), "File volume storage_type must be "
+                                         "Endurance")
+
     def test_order_snapshot_space(self):
         mock = self.set_mock('SoftLayer_Product_Package', 'getAllObjects')
         mock.return_value = [{