Repository: softlayer/softlayer-python
Pull request #684 (2016-02-03 06:50:35)

Check at least one upgrade option is provided.
Ensure that private option requires the cpu option.
Filter out location based prices when upgrading.
Add additional tests around guest upgrading.

Addresses #681


Patch:
diff --git a/SoftLayer/CLI/virt/upgrade.py b/SoftLayer/CLI/virt/upgrade.py
index bd17059d..419456f9 100644
--- a/SoftLayer/CLI/virt/upgrade.py
+++ b/SoftLayer/CLI/virt/upgrade.py
@@ -27,6 +27,14 @@ def cli(env, identifier, cpu, private, memory, network):
 
     vsi = SoftLayer.VSManager(env.client)
 
+    if not any([cpu, memory, network]):
+        raise exceptions.ArgumentError(
+            "Must provide [--cpu], [--memory], or [--network] to upgrade")
+
+    if private and not cpu:
+        raise exceptions.ArgumentError(
+            "Must specify [--cpu] when using [--private]")
+
     vs_id = helpers.resolve_id(vsi.resolve_ids, identifier, 'VS')
     if not (env.skip_confirmations or formatting.confirm(
             "This action will incur charges on your account. "
diff --git a/SoftLayer/fixtures/SoftLayer_Product_Package.py b/SoftLayer/fixtures/SoftLayer_Product_Package.py
index c4b7844e..1a5fbed0 100644
--- a/SoftLayer/fixtures/SoftLayer_Product_Package.py
+++ b/SoftLayer/fixtures/SoftLayer_Product_Package.py
@@ -228,6 +228,18 @@
         'description': 'Computing Instance',
         'itemCategory': {'categoryCode': 'Computing Instance'},
         'prices': [{'id': 1144,
+                    'locationGroupId': None,
+                    'categories': [{'id': 80,
+                                    'name': 'Computing Instance',
+                                    'categoryCode': 'guest_core'}]}],
+    },
+    {
+        'id': 112233,
+        'capacity': '55',
+        'description': 'Computing Instance',
+        'itemCategory': {'categoryCode': 'Computing Instance'},
+        'prices': [{'id': 332211,
+                    'locationGroupId': 1,
                     'categories': [{'id': 80,
                                     'name': 'Computing Instance',
                                     'categoryCode': 'guest_core'}]}],
diff --git a/SoftLayer/managers/vs.py b/SoftLayer/managers/vs.py
index a78bdba4..320d7c4a 100644
--- a/SoftLayer/managers/vs.py
+++ b/SoftLayer/managers/vs.py
@@ -10,6 +10,7 @@
 import socket
 import time
 
+from SoftLayer import exceptions
 from SoftLayer.managers import ordering
 from SoftLayer import utils
 # pylint: disable=no-self-use
@@ -745,22 +746,22 @@ def upgrade(self, instance_id, cpus=None, memory=None,
         """
         package_items = self._get_package_items()
         prices = []
-        if cpus:
-            prices.append({
-                'id': self._get_item_id_for_upgrade(package_items,
-                                                    'cpus',
-                                                    cpus,
-                                                    public)})
-        if memory:
-            prices.append({
-                'id': self._get_item_id_for_upgrade(package_items,
-                                                    'memory',
-                                                    memory)})
-        if nic_speed:
-            prices.append({
-                'id': self._get_item_id_for_upgrade(package_items,
-                                                    'nic_speed',
-                                                    nic_speed)})
+
+        for option, value in {'cpus': cpus,
+                              'memory': memory,
+                              'nic_speed': nic_speed}.items():
+            if not value:
+                continue
+            price_id = self._get_price_id_for_upgrade(package_items,
+                                                      option,
+                                                      value,
+                                                      public)
+            if not price_id:
+                # Every option provided is expected to have a price
+                raise exceptions.SoftLayerError(
+                    "Unable to find %s option with value %s" % (option, value))
+
+            prices.append({'id': price_id})
 
         maintenance_window = datetime.datetime.now(utils.UTC())
         order = {
@@ -783,7 +784,7 @@ def _get_package_items(self):
         mask = [
             'description',
             'capacity',
-            'prices[id,categories[name,id,categoryCode]]'
+            'prices[id,locationGroupId,categories[name,id,categoryCode]]'
         ]
         mask = "mask[%s]" % ','.join(mask)
 
@@ -793,9 +794,9 @@ def _get_package_items(self):
 
         return package_service.getItems(id=package_id, mask=mask)
 
-    def _get_item_id_for_upgrade(self, package_items, option, value,
-                                 public=True):
-        """Find the item ids for the parameters you want to upgrade to.
+    def _get_price_id_for_upgrade(self, package_items, option, value,
+                                  public=True):
+        """Find the price id for the option and value to upgrade.
 
         :param list package_items: Contains all the items related to an VS
         :param string option: Describes type of parameter to be upgraded
@@ -807,20 +808,29 @@ def _get_item_id_for_upgrade(self, package_items, option, value,
             'cpus': 'guest_core',
             'nic_speed': 'port_speed'
         }
-
+        category_code = option_category[option]
         for item in package_items:
-            categories = item['prices'][0]['categories']
-            for category in categories:
-                if not (category['categoryCode'] == option_category[option] and
-                        str(item['capacity']) == str(value)):
+            is_private = str(item['description']).startswith('Private')
+            for price in item['prices']:
+                if 'locationGroupId' in price and price['locationGroupId']:
+                    # Skip location based prices
+                    continue
+
+                if 'categories' not in price:
                     continue
-                if option == 'cpus':
-                    if public and ('Private' not in item['description']):
-                        return item['prices'][0]['id']
-                    elif not public and ('Private' in item['description']):
-                        return item['prices'][0]['id']
-                elif option == 'nic_speed':
-                    if 'Public' in item['description']:
-                        return item['prices'][0]['id']
-                else:
-                    return item['prices'][0]['id']
+
+                categories = price['categories']
+                for category in categories:
+                    if not (category['categoryCode'] == category_code
+                            and str(item['capacity']) == str(value)):
+                        continue
+                    if option == 'cpus':
+                        if public and not is_private:
+                            return price['id']
+                        elif not public and is_private:
+                            return price['id']
+                    elif option == 'nic_speed':
+                        if 'Public' in item['description']:
+                            return price['id']
+                    else:
+                        return price['id']


Test patch:
diff --git a/tests/CLI/modules/vs_tests.py b/tests/CLI/modules/vs_tests.py
index 002b1790..f4f31841 100644
--- a/tests/CLI/modules/vs_tests.py
+++ b/tests/CLI/modules/vs_tests.py
@@ -320,3 +320,35 @@ def test_dns_sync_misc_exception(self, confirm_mock):
         result = self.run_command(['vs', 'dns-sync', '-a', '100'])
         self.assertEqual(result.exit_code, 2)
         self.assertIsInstance(result.exception, exceptions.CLIAbort)
+
+    def test_upgrade_no_options(self, ):
+        result = self.run_command(['vs', 'upgrade', '100'])
+        self.assertEqual(result.exit_code, 2)
+        self.assertIsInstance(result.exception, exceptions.ArgumentError)
+
+    def test_upgrade_private_no_cpu(self):
+        result = self.run_command(['vs', 'upgrade', '100', '--private',
+                                   '--memory=1024'])
+        self.assertEqual(result.exit_code, 2)
+        self.assertIsInstance(result.exception, exceptions.ArgumentError)
+
+    @mock.patch('SoftLayer.CLI.formatting.confirm')
+    def test_upgrade_aborted(self, confirm_mock):
+        confirm_mock.return_value = False
+        result = self.run_command(['vs', 'upgrade', '100', '--cpu=1'])
+        self.assertEqual(result.exit_code, 2)
+        self.assertIsInstance(result.exception, exceptions.CLIAbort)
+
+    @mock.patch('SoftLayer.CLI.formatting.confirm')
+    def test_upgrade(self, confirm_mock):
+        confirm_mock.return_value = True
+        result = self.run_command(['vs', 'upgrade', '100', '--cpu=4',
+                                   '--memory=2048', '--network=1000'])
+        self.assertEqual(result.exit_code, 0)
+        self.assert_called_with('SoftLayer_Product_Order', 'placeOrder')
+        call = self.calls('SoftLayer_Product_Order', 'placeOrder')[0]
+        order_container = call.args[0]
+        self.assertIn({'id': 1144}, order_container['prices'])
+        self.assertIn({'id': 1133}, order_container['prices'])
+        self.assertIn({'id': 1122}, order_container['prices'])
+        self.assertEqual(order_container['virtualGuests'], [{'id': 100}])
diff --git a/tests/managers/loadbal_tests.py b/tests/managers/loadbal_tests.py
index 960fd241..f7fd6391 100644
--- a/tests/managers/loadbal_tests.py
+++ b/tests/managers/loadbal_tests.py
@@ -19,7 +19,7 @@ def set_up(self):
     def test_get_lb_pkgs(self):
         result = self.lb_mgr.get_lb_pkgs()
 
-        self.assertEqual(len(result), 12)
+        self.assertEqual(len(result), 13)
         _filter = {
             'items': {
                 'description': {
diff --git a/tests/managers/vs_tests.py b/tests/managers/vs_tests.py
index 1ad40add..a0cecb20 100644
--- a/tests/managers/vs_tests.py
+++ b/tests/managers/vs_tests.py
@@ -7,6 +7,7 @@
 import mock
 
 import SoftLayer
+from SoftLayer import exceptions
 from SoftLayer import fixtures
 from SoftLayer import testing
 
@@ -672,10 +673,16 @@ def test_upgrade_full(self):
         self.assert_called_with('SoftLayer_Product_Order', 'placeOrder')
         call = self.calls('SoftLayer_Product_Order', 'placeOrder')[0]
         order_container = call.args[0]
-        self.assertEqual(order_container['prices'],
-                         [{'id': 1144}, {'id': 1133}, {'id': 1122}])
+        self.assertIn({'id': 1144}, order_container['prices'])
+        self.assertIn({'id': 1133}, order_container['prices'])
+        self.assertIn({'id': 1122}, order_container['prices'])
         self.assertEqual(order_container['virtualGuests'], [{'id': 1}])
 
+    def test_upgrade_skips_location_based_prices(self):
+        # Test that no prices that have locationGroupId set are used
+        self.assertRaises(exceptions.SoftLayerError,
+                          self.vs.upgrade, 1, cpus=55, memory=2, public=True)
+
     def test_get_item_id_for_upgrade(self):
         item_id = 0
         package_items = self.client['Product_Package'].getItems(id=46)