Repository: softlayer/softlayer-python

Issue:
problem with hourly bare metal servers immediate cancelation in CLI and manager
slcli server cancel --immediate <server id> does not produce an error, but cancels server on its monthly anniversary. The cause of the problem is "old" logic in cancel_hardware method of harware.py. In SL API 4.0 harware.py place_order now only supports the fast server provisioning package (see version 4 notes: #469), but cancel_hardware is still looking for a 'bareMetalInstanceFlag'  and using Billing_Item cancelService. It should use Billing_Item cancelItem instead 
 

Discussion:

Patch:
diff --git a/SoftLayer/CLI/server/detail.py b/SoftLayer/CLI/server/detail.py
index 61eeb43e..943b694b 100644
--- a/SoftLayer/CLI/server/detail.py
+++ b/SoftLayer/CLI/server/detail.py
@@ -35,7 +35,7 @@ def cli(env, identifier, passwords, price):
     result = utils.NestedDict(result)
 
     table.add_row(['id', result['id']])
-    table.add_row(['guid', result['globalIdentifier']] or formatting.blank())
+    table.add_row(['guid', result['globalIdentifier'] or formatting.blank()])
     table.add_row(['hostname', result['hostname']])
     table.add_row(['domain', result['domain']])
     table.add_row(['fqdn', result['fullyQualifiedDomainName']])
diff --git a/SoftLayer/managers/hardware.py b/SoftLayer/managers/hardware.py
index 1a1e350f..70aea1ec 100644
--- a/SoftLayer/managers/hardware.py
+++ b/SoftLayer/managers/hardware.py
@@ -47,51 +47,22 @@ def cancel_hardware(self, hardware_id, reason='unneeded', comment='',
         :param string comment: An optional comment to include with the
                                cancellation.
         """
-        # Check to see if this is actually a pre-configured server (BMC). They
-        # require a different cancellation call.
-        server = self.get_hardware(hardware_id,
-                                   mask='id,bareMetalInstanceFlag')
-
-        if server.get('bareMetalInstanceFlag'):
-            return self.cancel_metal(hardware_id, immediate)
 
+        # Get cancel reason
         reasons = self.get_cancellation_reasons()
-        cancel_reason = reasons['unneeded']
-
-        if reason in reasons:
-            cancel_reason = reasons[reason]
-
-        # Arguments per SLDN:
-        # attachmentId - Hardware ID
-        # Reason
-        # content - Comment about the cancellation
-        # cancelAssociatedItems
-        # attachmentType - Only option is HARDWARE
-        return self.client['Ticket'].createCancelServerTicket(hardware_id,
-                                                              cancel_reason,
-                                                              comment,
-                                                              True,
-                                                              'HARDWARE')
-
-    def cancel_metal(self, hardware_id, immediate=False):
-        """Cancels the specified bare metal instance.
-
-        :param int id: The ID of the bare metal instance to be cancelled.
-        :param bool immediate: If true, the bare metal instance will be
-                               cancelled immediately. Otherwise, it will be
-                               scheduled to cancel on the anniversary date.
-        """
+        cancel_reason = reasons.get(reason, reasons['unneeded'])
+
         hw_billing = self.get_hardware(hardware_id,
                                        mask='mask[id, billingItem.id]')
+        if 'billingItem' not in hw_billing:
+            raise SoftLayer.SoftLayerError(
+                "No billing item found for hardware")
 
         billing_id = hw_billing['billingItem']['id']
 
-        billing_item = self.client['Billing_Item']
-
-        if immediate:
-            return billing_item.cancelService(id=billing_id)
-        else:
-            return billing_item.cancelServiceOnAnniversaryDate(id=billing_id)
+        return self.client.call('Billing_Item', 'cancelItem',
+                                immediate, False, cancel_reason, comment,
+                                id=billing_id)
 
     def list_hardware(self, tags=None, cpus=None, memory=None, hostname=None,
                       domain=None, datacenter=None, nic_speed=None,


Test patch:
diff --git a/SoftLayer/tests/managers/hardware_tests.py b/SoftLayer/tests/managers/hardware_tests.py
index 283598c9..79aa5025 100644
--- a/SoftLayer/tests/managers/hardware_tests.py
+++ b/SoftLayer/tests/managers/hardware_tests.py
@@ -243,57 +243,53 @@ def test_place_order(self, create_dict):
         self.assert_called_with('SoftLayer_Product_Order', 'placeOrder',
                                 args=({'test': 1, 'verify': 1},))
 
-    def test_cancel_metal_immediately(self):
-
-        result = self.hardware.cancel_metal(6327, immediate=True)
-
-        self.assertEqual(result, True)
-        self.assert_called_with('SoftLayer_Billing_Item', 'cancelService',
-                                identifier=6327)
-
-    def test_cancel_metal_on_anniversary(self):
-
-        result = self.hardware.cancel_metal(6327, False)
-
-        self.assertEqual(result, True)
-        self.assert_called_with('SoftLayer_Billing_Item',
-                                'cancelServiceOnAnniversaryDate',
-                                identifier=6327)
-
     def test_cancel_hardware_without_reason(self):
         mock = self.set_mock('SoftLayer_Hardware_Server', 'getObject')
-        mock.return_value = {'id': 987, 'bareMetalInstanceFlag': False}
+        mock.return_value = {'id': 987, 'billingItem': {'id': 1234}}
 
         result = self.hardware.cancel_hardware(987)
 
-        self.assertEqual(result,
-                         fixtures.SoftLayer_Ticket.createCancelServerTicket)
+        self.assertEqual(result, True)
         reasons = self.hardware.get_cancellation_reasons()
-        args = (987, reasons['unneeded'], '', True, 'HARDWARE')
-        self.assert_called_with('SoftLayer_Ticket', 'createCancelServerTicket',
+        args = (False, False, reasons['unneeded'], '')
+        self.assert_called_with('SoftLayer_Billing_Item', 'cancelItem',
+                                identifier=1234,
                                 args=args)
 
     def test_cancel_hardware_with_reason_and_comment(self):
         mock = self.set_mock('SoftLayer_Hardware_Server', 'getObject')
-        mock.return_value = {'id': 987, 'bareMetalInstanceFlag': False}
+        mock.return_value = {'id': 987, 'billingItem': {'id': 1234}}
 
-        result = self.hardware.cancel_hardware(987, 'sales', 'Test Comment')
+        result = self.hardware.cancel_hardware(6327,
+                                               reason='sales',
+                                               comment='Test Comment')
 
-        self.assertEqual(result,
-                         fixtures.SoftLayer_Ticket.createCancelServerTicket)
+        self.assertEqual(result, True)
         reasons = self.hardware.get_cancellation_reasons()
-        args = (987, reasons['sales'], 'Test Comment', True, 'HARDWARE')
-        self.assert_called_with('SoftLayer_Ticket', 'createCancelServerTicket',
+        args = (False, False, reasons['sales'], 'Test Comment')
+        self.assert_called_with('SoftLayer_Billing_Item', 'cancelItem',
+                                identifier=1234,
                                 args=args)
 
-    def test_cancel_hardware_on_bmc(self):
+    def test_cancel_hardware(self):
 
         result = self.hardware.cancel_hardware(6327)
 
         self.assertEqual(result, True)
         self.assert_called_with('SoftLayer_Billing_Item',
-                                'cancelServiceOnAnniversaryDate',
-                                identifier=6327)
+                                'cancelItem',
+                                identifier=6327,
+                                args=(False, False, 'No longer needed', ''))
+
+    def test_cancel_hardware_no_billing_item(self):
+        mock = self.set_mock('SoftLayer_Hardware_Server', 'getObject')
+        mock.return_value = {'id': 987}
+
+        ex = self.assertRaises(SoftLayer.SoftLayerError,
+                               self.hardware.cancel_hardware,
+                               6327)
+        self.assertEqual("No billing item found for hardware",
+                         str(ex))
 
     def test_change_port_speed_public(self):
         self.hardware.change_port_speed(2, True, 100)