Repository: softlayer/softlayer-python (python)

Issue:
FirewallManager.edit_dedicated_fwl_rules docs say it takes a dict, but it only accepts a list.
First off, I'm not quite clear - what is FirewallManager.edit_dedicated_fwl_rules supposed to do? I'd like to add a single rule... I'm concerned I might accidentally replace all of my rules with just one rule, which would be really bad given I have 41 existing rules.

Anyways, here's the code that I've written:

~~~
# client and ip are already defined elsewhere.

from SoftLayer import FirewallManager

fm = FirewallManager(client)

firewallID = fm.get_firewalls()[0]['networkVlanFirewall']['id']  # I only have one firewall, so this will get it fine.

rules = fm.get_dedicated_fwl_rules(firewallID)

fm.edit_dedicated_fwl_rules(firewallID, {
    'action'                   : 'permit',
    'destinationIpAddress'     : ip,
    'destinationIpCidr'        : 32,
    'destinationIpSubnetMask'  : '255.255.255.255',
    'destinationPortRangeEnd'  : 65535,
    'destinationPortRangeStart': 1,
    'orderValue'               : rules[-1]['orderValue'] + 1,
    'protocol'                 : 'tcp',
    'sourceIpAddress'          : '0.0.0.0',
    'sourceIpCidr'             : 0,
    'sourceIpSubnetMask'       : '0.0.0.0',
    'version'                  : 4
})
~~~

When I try running this, I get this stack trace:

~~~
Traceback (most recent call last):
  File "<stdin>", line 14, in <module>
    'version'                  : 4
  File "C:\Python27\lib\site-packages\SoftLayer\managers\firewall.py", line 267, in edit_dedicated_fwl_rules
    return svc.createObject(template)
  File "C:\Python27\lib\site-packages\SoftLayer\API.py", line 363, in call_handler
    return self(name, *args, **kwargs)
  File "C:\Python27\lib\site-packages\SoftLayer\API.py", line 331, in call
    return self.client.call(self.name, name, *args, **kwargs)
  File "C:\Python27\lib\site-packages\SoftLayer\API.py", line 227, in call
    return self.transport(request)
  File "C:\Python27\lib\site-packages\SoftLayer\transports.py", line 160, in __call__
    raise _ex(ex.faultCode, ex.faultString)
SoftLayer.exceptions.SoftLayerAPIError: SoftLayerAPIError(SoftLayer_Exception_Public): Each rule must have a distinct order value, '' has a duplicate.
~~~

It says that I have a duplicate rule `orderValue` of `''`, but I clearly set it to a number. This suggests to me that I wasn't actually supposed to pass a dictionary representing a rule directly in as a second argument to `edit_dedicated_fwl_rules`... but reading over the code I can't tell what I should actually pass in. I think there's either a bug here that it isn't reading my orderValue property properly, or there's a documentation defect in that I'm not sure what I should be passing in here (or maybe both).

Pull request description:
Updates edit_dedicated_fwl_rules doc block
Resolves #634

Patch:
diff --git a/SoftLayer/API.py b/SoftLayer/API.py
index a25b36be..2612d412 100644
--- a/SoftLayer/API.py
+++ b/SoftLayer/API.py
@@ -95,21 +95,11 @@ def create_client_from_env(username=None,
 
     # If we have enough information to make an auth driver, let's do it
     if auth is None and settings.get('username') and settings.get('api_key'):
-        # NOTE(kmcdonald): some transports mask other transports, so this is
-        # a way to find the 'real' one
-        real_transport = getattr(transport, 'transport', transport)
-
-        if isinstance(real_transport, transports.XmlRpcTransport):
-            auth = slauth.BasicAuthentication(
-                settings.get('username'),
-                settings.get('api_key'),
-            )
-
-        elif isinstance(real_transport, transports.RestTransport):
-            auth = slauth.BasicHTTPAuthentication(
-                settings.get('username'),
-                settings.get('api_key'),
-            )
+
+        auth = slauth.BasicAuthentication(
+            settings.get('username'),
+            settings.get('api_key'),
+        )
 
     return BaseClient(auth=auth, transport=transport)
 
diff --git a/SoftLayer/config.py b/SoftLayer/config.py
index 748c5df2..120cadab 100644
--- a/SoftLayer/config.py
+++ b/SoftLayer/config.py
@@ -19,7 +19,6 @@ def get_client_settings_args(**kwargs):
     timeout = kwargs.get('timeout')
     if timeout is not None:
         timeout = float(timeout)
-
     return {
         'endpoint_url': kwargs.get('endpoint_url'),
         'timeout': timeout,
diff --git a/SoftLayer/managers/firewall.py b/SoftLayer/managers/firewall.py
index 07449b51..4ed350b4 100644
--- a/SoftLayer/managers/firewall.py
+++ b/SoftLayer/managers/firewall.py
@@ -243,7 +243,8 @@ def edit_dedicated_fwl_rules(self, firewall_id, rules):
         """Edit the rules for dedicated firewall.
 
         :param integer firewall_id: the instance ID of the dedicated firewall
-        :param dict rules: the rules to be pushed on the firewall
+        :param list rules: the rules to be pushed on the firewall as defined by
+                           SoftLayer_Network_Firewall_Update_Request_Rule
         """
 
         mask = ('mask[networkVlan[firewallInterfaces'
@@ -255,6 +256,7 @@ def edit_dedicated_fwl_rules(self, firewall_id, rules):
         for fwl1 in network_vlan['firewallInterfaces']:
             if fwl1['name'] == 'inside':
                 continue
+
             for control_list in fwl1['firewallContextAccessControlLists']:
                 if control_list['direction'] == 'out':
                     continue
diff --git a/SoftLayer/managers/load_balancer.py b/SoftLayer/managers/load_balancer.py
index bc687e8d..29363b7d 100644
--- a/SoftLayer/managers/load_balancer.py
+++ b/SoftLayer/managers/load_balancer.py
@@ -204,7 +204,7 @@ def edit_service(self, loadbal_id, service_id, ip_address_id=None,
                 if ip_address_id is not None:
                     service['ipAddressId'] = ip_address_id
 
-        template = {'virtualServers': list(virtual_servers)}
+        template = {'virtualServers': virtual_servers}
 
         load_balancer = self.lb_svc.editObject(template, id=loadbal_id)
         return load_balancer
diff --git a/SoftLayer/transports.py b/SoftLayer/transports.py
index 9bcbb434..50b5956d 100644
--- a/SoftLayer/transports.py
+++ b/SoftLayer/transports.py
@@ -28,14 +28,6 @@
     'FixtureTransport',
 ]
 
-REST_SPECIAL_METHODS = {
-    'deleteObject': 'DELETE',
-    'createObject': 'POST',
-    'createObjects': 'POST',
-    'editObject': 'PUT',
-    'editObjects': 'PUT',
-}
-
 
 class Request(object):
     """Transport request object."""
@@ -84,14 +76,6 @@ def __init__(self):
         self.offset = None
 
 
-class SoftLayerListResult(list):
-    """A SoftLayer list result."""
-
-    def __init__(self, items, total_count):
-        self.total_count = total_count
-        super(SoftLayerListResult, self).__init__(items)
-
-
 class XmlRpcTransport(object):
     """XML-RPC transport."""
     def __init__(self,
@@ -120,8 +104,7 @@ def __call__(self, request):
             headers[header_name] = {'id': request.identifier}
 
         if request.mask is not None:
-            headers.update(_format_object_mask_xmlrpc(request.mask,
-                                                      request.service))
+            headers.update(_format_object_mask(request.mask, request.service))
 
         if request.filter is not None:
             headers['%sObjectFilter' % request.service] = request.filter
@@ -146,23 +129,18 @@ def __call__(self, request):
         LOGGER.debug(payload)
 
         try:
-            resp = requests.request('POST', url,
-                                    data=payload,
-                                    headers=request.transport_headers,
-                                    timeout=self.timeout,
-                                    verify=request.verify,
-                                    cert=request.cert,
-                                    proxies=_proxies_dict(self.proxy))
+            response = requests.request('POST', url,
+                                        data=payload,
+                                        headers=request.transport_headers,
+                                        timeout=self.timeout,
+                                        verify=request.verify,
+                                        cert=request.cert,
+                                        proxies=_proxies_dict(self.proxy))
             LOGGER.debug("=== RESPONSE ===")
-            LOGGER.debug(resp.headers)
-            LOGGER.debug(resp.content)
-            resp.raise_for_status()
-            result = utils.xmlrpc_client.loads(resp.content)[0][0]
-            if isinstance(result, list):
-                return SoftLayerListResult(
-                    result, int(resp.headers.get('softlayer-total-items', 0)))
-            else:
-                return result
+            LOGGER.debug(response.headers)
+            LOGGER.debug(response.content)
+            response.raise_for_status()
+            return utils.xmlrpc_client.loads(response.content)[0][0]
         except utils.xmlrpc_client.Fault as ex:
             # These exceptions are formed from the XML-RPC spec
             # http://xmlrpc-epi.sourceforge.net/specs/rfc.fault_codes.php
@@ -212,68 +190,26 @@ def __call__(self, request):
 
         :param request request: Request object
         """
-        request.transport_headers.setdefault('Content-Type',
-                                             'application/json')
-        request.transport_headers.setdefault('User-Agent', self.user_agent)
-
-        params = request.headers.copy()
-        if request.mask:
-            params['objectMask'] = _format_object_mask(request.mask)
-
-        if request.limit:
-            params['limit'] = request.limit
-
-        if request.offset:
-            params['offset'] = request.offset
-
-        if request.filter:
-            params['objectFilter'] = json.dumps(request.filter)
-
-        auth = None
-        if request.transport_user:
-            auth = requests.auth.HTTPBasicAuth(
-                request.transport_user,
-                request.transport_password,
-            )
-
-        method = REST_SPECIAL_METHODS.get(request.method)
-        is_special_method = True
-        if method is None:
-            is_special_method = False
-            method = 'GET'
-
-        body = {}
-        if request.args:
-            # NOTE(kmcdonald): force POST when there are arguments because
-            # the request body is ignored otherwise.
-            method = 'POST'
-            body['parameters'] = request.args
-
-        raw_body = None
-        if body:
-            raw_body = json.dumps(body)
-
         url_parts = [self.endpoint_url, request.service]
         if request.identifier is not None:
             url_parts.append(str(request.identifier))
-
-        # Special methods (createObject, editObject, etc) use the HTTP verb
-        # to determine the action on the resource
-        if request.method is not None and not is_special_method:
+        if request.method is not None:
             url_parts.append(request.method)
+        for arg in request.args:
+            url_parts.append(str(arg))
+
+        request.transport_headers.setdefault('Content-Type',
+                                             'application/json')
+        request.transport_headers.setdefault('User-Agent', self.user_agent)
 
         url = '%s.%s' % ('/'.join(url_parts), 'json')
 
         LOGGER.debug("=== REQUEST ===")
         LOGGER.info(url)
         LOGGER.debug(request.transport_headers)
-        LOGGER.debug(raw_body)
         try:
-            resp = requests.request(method, url,
-                                    auth=auth,
+            resp = requests.request('GET', url,
                                     headers=request.transport_headers,
-                                    params=params,
-                                    data=raw_body,
                                     timeout=self.timeout,
                                     verify=request.verify,
                                     cert=request.cert,
@@ -282,13 +218,7 @@ def __call__(self, request):
             LOGGER.debug(resp.headers)
             LOGGER.debug(resp.content)
             resp.raise_for_status()
-            result = json.loads(resp.content)
-
-            if isinstance(result, list):
-                return SoftLayerListResult(
-                    result, int(resp.headers.get('softlayer-total-items', 0)))
-            else:
-                return result
+            return json.loads(resp.content)
         except requests.HTTPError as ex:
             content = json.loads(ex.response.content)
             raise exceptions.SoftLayerAPIError(ex.response.status_code,
@@ -349,7 +279,7 @@ def _proxies_dict(proxy):
     return {'http': proxy, 'https': proxy}
 
 
-def _format_object_mask_xmlrpc(objectmask, service):
+def _format_object_mask(objectmask, service):
     """Format new and old style object masks into proper headers.
 
     :param objectmask: a string- or dict-based object mask
@@ -360,22 +290,10 @@ def _format_object_mask_xmlrpc(objectmask, service):
         mheader = '%sObjectMask' % service
     else:
         mheader = 'SoftLayer_ObjectMask'
-        objectmask = _format_object_mask(objectmask)
-
-    return {mheader: {'mask': objectmask}}
-
 
-def _format_object_mask(objectmask):
-    """Format the new style object mask.
+        objectmask = objectmask.strip()
+        if (not objectmask.startswith('mask') and
+                not objectmask.startswith('[')):
+            objectmask = "mask[%s]" % objectmask
 
-    This wraps the user mask with mask[USER_MASK] if it does not already
-    have one. This makes it slightly easier for users.
-
-    :param objectmask: a string-based object mask
-
-    """
-    objectmask = objectmask.strip()
-    if (not objectmask.startswith('mask') and
-            not objectmask.startswith('[')):
-        objectmask = "mask[%s]" % objectmask
-    return objectmask
+    return {mheader: {'mask': objectmask}}
diff --git a/docs/dev/index.rst b/docs/dev/index.rst
index 4814920b..a1564bff 100644
--- a/docs/dev/index.rst
+++ b/docs/dev/index.rst
@@ -75,12 +75,12 @@ versions every time you push new code.
 
 Using tox to running the tests in multiple environments can be very time
 consuming. If you wish to quickly run the tests in your own environment, you
-may do so using `py.test <http://pytest.org/>`_.  The command to do that
+may do so using `nose <https://nose.readthedocs.org>`_.  The command to do that
 is:
 
 ::
 
-  py.test tests
+  nosetests
 
 
 Documentation


Test patch:
diff --git a/tests/transport_tests.py b/tests/transport_tests.py
index 99e27d6d..c4977ff0 100644
--- a/tests/transport_tests.py
+++ b/tests/transport_tests.py
@@ -4,12 +4,10 @@
 
     :license: MIT, see LICENSE for more details.
 """
-import io
 import warnings
 
 import mock
 import requests
-import six
 
 import SoftLayer
 from SoftLayer import consts
@@ -23,8 +21,8 @@ def set_up(self):
         self.transport = transports.XmlRpcTransport(
             endpoint_url='http://something.com',
         )
-        self.response = requests.Response()
-        list_body = six.b('''<?xml version="1.0" encoding="utf-8"?>
+        self.response = mock.MagicMock()
+        self.response.content = '''<?xml version="1.0" encoding="utf-8"?>
 <params>
 <param>
  <value>
@@ -33,10 +31,7 @@ def set_up(self):
   </array>
  </value>
 </param>
-</params>''')
-        self.response.raw = io.BytesIO(list_body)
-        self.response.headers['SoftLayer-Total-Items'] = 10
-        self.response.status_code = 200
+</params>'''
 
     @mock.patch('requests.request')
     def test_call(self, request):
@@ -74,8 +69,6 @@ def test_call(self, request):
                                    cert=None,
                                    verify=True)
         self.assertEqual(resp, [])
-        self.assertIsInstance(resp, transports.SoftLayerListResult)
-        self.assertEqual(resp.total_count, 10)
 
     def test_proxy_without_protocol(self):
         req = transports.Request()
@@ -260,32 +253,21 @@ def set_up(self):
 
     @mock.patch('requests.request')
     def test_basic(self, request):
-        request().content = '[]'
-        request().headers = requests.structures.CaseInsensitiveDict({
-            'SoftLayer-Total-Items': '10',
-        })
-
+        request().content = '{}'
         req = transports.Request()
         req.service = 'SoftLayer_Service'
         req.method = 'Resource'
-        resp = self.transport(req)
 
-        self.assertEqual(resp, [])
-        self.assertIsInstance(resp, transports.SoftLayerListResult)
-        self.assertEqual(resp.total_count, 10)
+        resp = self.transport(req)
+        self.assertEqual(resp, {})
         request.assert_called_with(
             'GET', 'http://something.com/SoftLayer_Service/Resource.json',
             headers=mock.ANY,
-            auth=None,
-            data=None,
-            params={},
             verify=True,
             cert=None,
             proxies=None,
             timeout=None)
 
-    @mock.patch('requests.request')
-    def test_error(self, request):
         # Test JSON Error
         e = requests.HTTPError('error')
         e.response = mock.MagicMock()
@@ -296,9 +278,6 @@ def test_error(self, request):
         }'''
         request().raise_for_status.side_effect = e
 
-        req = transports.Request()
-        req.service = 'SoftLayer_Service'
-        req.method = 'Resource'
         self.assertRaises(SoftLayer.SoftLayerAPIError, self.transport, req)
 
     def test_proxy_without_protocol(self):
@@ -327,9 +306,6 @@ def test_valid_proxy(self, request):
             'GET', 'http://something.com/SoftLayer_Service/Resource.json',
             proxies={'https': 'http://localhost:3128',
                      'http': 'http://localhost:3128'},
-            auth=None,
-            data=None,
-            params={},
             verify=True,
             cert=None,
             timeout=mock.ANY,
@@ -351,9 +327,6 @@ def test_with_id(self, request):
             'GET',
             'http://something.com/SoftLayer_Service/2/getObject.json',
             headers=mock.ANY,
-            auth=None,
-            data=None,
-            params={},
             verify=True,
             cert=None,
             proxies=None,
@@ -370,85 +343,11 @@ def test_with_args(self, request):
 
         resp = self.transport(req)
 
-        self.assertEqual(resp, {})
-        request.assert_called_with(
-            'POST',
-            'http://something.com/SoftLayer_Service/getObject.json',
-            headers=mock.ANY,
-            auth=None,
-            data='{"parameters": ["test", 1]}',
-            params={},
-            verify=True,
-            cert=None,
-            proxies=None,
-            timeout=None)
-
-
-    @mock.patch('requests.request')
-    def test_with_filter(self, request):
-        request().content = '{}'
-
-        req = transports.Request()
-        req.service = 'SoftLayer_Service'
-        req.method = 'getObject'
-        req.filter = {'TYPE': {'attribute': {'operation': '^= prefix'}}}
-
-        resp = self.transport(req)
-
-        self.assertEqual(resp, {})
-        request.assert_called_with(
-            'GET',
-            'http://something.com/SoftLayer_Service/getObject.json',
-            params={'objectFilter':
-                    '{"TYPE": {"attribute": {"operation": "^= prefix"}}}'},
-            headers=mock.ANY,
-            auth=None,
-            data=None,
-            verify=True,
-            cert=None,
-            proxies=None,
-            timeout=None)
-
-    @mock.patch('requests.request')
-    def test_with_mask(self, request):
-        request().content = '{}'
-
-        req = transports.Request()
-        req.service = 'SoftLayer_Service'
-        req.method = 'getObject'
-        req.mask = 'id,property'
-
-        resp = self.transport(req)
-
-        self.assertEqual(resp, {})
-        request.assert_called_with(
-            'GET',
-            'http://something.com/SoftLayer_Service/getObject.json',
-            params={'objectMask': 'mask[id,property]'},
-            headers=mock.ANY,
-            auth=None,
-            data=None,
-            verify=True,
-            cert=None,
-            proxies=None,
-            timeout=None)
-
-        # Now test with mask[] prefix
-        req = transports.Request()
-        req.service = 'SoftLayer_Service'
-        req.method = 'getObject'
-        req.mask = 'mask[id,property]'
-
-        resp = self.transport(req)
-
         self.assertEqual(resp, {})
         request.assert_called_with(
             'GET',
-            'http://something.com/SoftLayer_Service/getObject.json',
-            params={'objectMask': 'mask[id,property]'},
+            'http://something.com/SoftLayer_Service/getObject/test/1.json',
             headers=mock.ANY,
-            auth=None,
-            data=None,
             verify=True,
             cert=None,
             proxies=None,