Repository: ansible/ansible
Pull request: Meraki modules - Added support for checking HTTP response codes
##### SUMMARY
- All request calls now check for response code before responding
- If the response code isn't what it should be, it fails or returns nothing
- Breaking this into multiple PRs to make backporting easier
- Using status property in Meraki module utility which has the code
- Partially fixes #41282

##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
meraki
meraki_organization
meraki_snmp
meraki_network
meraki_admin

##### ANSIBLE VERSION
```
ansible 2.7.0.dev0 (meraki/util_return_code 03813a0d78) last updated 2018/06/29 19:36:14 (GMT -500)
  config file = None
  configured module search path = ['/Users/kbreit/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/kbreit/Documents/Programming/ansible/lib/ansible
  executable location = /Users/kbreit/Documents/Programming/ansible/bin/ansible
  python version = 3.5.4 (default, Feb 25 2018, 14:56:02) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)]
```

Problem statement:
Meraki modules should analyze HTTP response code for detailed success
##### SUMMARY
Meraki's API returns a HTTP code and sometimes it varies depending on the endpoint and method. The module utility looks for HTTP status codes greater than 300 for errors, but 2xx codes aren't reviewed by the module.

##### ISSUE TYPE
 - Bug report

##### COMPONENT NAME
meraki
meraki_organization
meraki_network
meraki_snmp
meraki_admin
meraki_vlan
meraki_switchport
meraki_device
meraki_config_template

##### ANSIBLE VERSION
```
ansible 2.6.0.dev0 (backport/2.6/40856 4188435156) last updated 2018/06/07 15:49:33 (GMT -500)
  config file = None
  configured module search path = ['/Users/kbreit/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/kbreit/Documents/Programming/ansible/lib/ansible
  executable location = /Users/kbreit/Documents/Programming/ansible/bin/ansible
  python version = 3.5.4 (default, Feb 25 2018, 14:56:02) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)]
```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/module_utils/network/meraki/__init__.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/network/meraki/__init__.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @Qalthos @dagwieers @ganeshrn @gdpak @gundalow @privateip @rcarrillocruz @trishnaguha
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
!assign kbreit
Only members of the Ansible GitHub organisation can be assigned. Sorry.

**Update:** Hmm, apparently not ?

Commit message: Added support for checking HTTP response codes
- All request calls now check for response code before responding
- If the response code isn't what it should be, it fails or returns nothing
- Breaking this into multiple PRs to make backporting easier
- Using status property in Meraki module utility which has the code

Patch to review:
diff --git a/lib/ansible/module_utils/network/meraki/meraki.py b/lib/ansible/module_utils/network/meraki/meraki.py
index d86f668a6b6c33..ebdb26ab876f4e 100644
--- a/lib/ansible/module_utils/network/meraki/meraki.py
+++ b/lib/ansible/module_utils/network/meraki/meraki.py
@@ -150,7 +150,11 @@ def is_update_required(self, original, proposed, optional_ignore=None):
 
     def get_orgs(self):
         """Downloads all organizations for a user."""
-        return self.request('/organizations', method='GET')
+        response = self.request('/organizations', method='GET')
+        if self.status == 200:
+            return response
+        else:
+            self.fail_json(msg='Organization lookup failed')
 
     def is_org_valid(self, data, org_name=None, org_id=None):
         """Checks whether a specific org exists and is duplicated.
@@ -195,7 +199,10 @@ def get_nets(self, org_name=None, org_id=None):
             org_id = self.get_org_id(org_name)
         path = self.construct_path('get_all', org_id=org_id, function='network')
         r = self.request(path, method='GET')
-        return r
+        if self.status == 200:
+            return r
+        else:
+            self.fail_json(msg='Network lookup failed')
 
     def get_net(self, org_name, net_name, data=None):
         """Return network information about a particular network."""
diff --git a/lib/ansible/modules/network/meraki/meraki_admin.py b/lib/ansible/modules/network/meraki/meraki_admin.py
index 172038065bd2b0..b7d66d13a5f7d7 100644
--- a/lib/ansible/modules/network/meraki/meraki_admin.py
+++ b/lib/ansible/modules/network/meraki/meraki_admin.py
@@ -153,7 +153,8 @@ def get_admins(meraki, org_id):
         ),
         method='GET'
     )
-    return admins
+    if meraki.status == 200:
+        return admins
 
 
 def get_admin_id(meraki, data, name=None, email=None):
@@ -189,10 +190,11 @@ def find_admin(meraki, data, email):
 
 def delete_admin(meraki, org_id, admin_id):
     path = meraki.construct_path('revoke', 'admin', org_id=org_id) + admin_id
-    # meraki.fail_json(msg=path)
     r = meraki.request(path,
                        method='DELETE'
                        )
+    if meraki.status == 204:
+        return r
 
 
 def network_factory(meraki, networks, nets):
@@ -209,7 +211,9 @@ def network_factory(meraki, networks, nets):
 
 def get_nets_temp(meraki, org_id):  # Function won't be needed when get_nets is added to util
     path = meraki.construct_path('get_all', function='network', org_id=org_id)
-    return meraki.request(path, method='GET')
+    response = meraki.request(path, method='GET')
+    if meraki.status == 200:
+        return response
 
 
 def create_admin(meraki, org_id, name, email):
@@ -234,8 +238,9 @@ def create_admin(meraki, org_id, name, email):
                            method='POST',
                            payload=json.dumps(payload)
                            )
-        meraki.result['changed'] = True
-        return r
+        if meraki.status == 201:
+            meraki.result['changed'] = True
+            return r
     elif is_admin_existing is not None:  # Update existing admin
         if not meraki.params['tags']:
             payload['tags'] = []
@@ -248,8 +253,9 @@ def create_admin(meraki, org_id, name, email):
                                method='PUT',
                                payload=json.dumps(payload)
                                )
-            meraki.result['changed'] = True
-            return r
+            if meraki.status == 200:
+                meraki.result['changed'] = True
+                return r
         else:
             # meraki.fail_json(msg='No update is required!!!')
             return -1
diff --git a/lib/ansible/modules/network/meraki/meraki_network.py b/lib/ansible/modules/network/meraki/meraki_network.py
index 20dd8aaf228561..7d979f37a41ea4 100644
--- a/lib/ansible/modules/network/meraki/meraki_network.py
+++ b/lib/ansible/modules/network/meraki/meraki_network.py
@@ -218,8 +218,9 @@ def main():
                                    method='POST',
                                    payload=json.dumps(payload)
                                    )
-                meraki.result['data'] = r
-                meraki.result['changed'] = True
+                if meraki.status == 201:
+                    meraki.result['data'] = r
+                    meraki.result['changed'] = True
             else:
                 net = meraki.get_net(meraki.params['org_name'], meraki.params['net_name'], data=nets)
                 proposed = payload
@@ -239,8 +240,9 @@ def main():
                     r = meraki.request(path,
                                        method='PUT',
                                        payload=json.dumps(payload))
-                    meraki.result['data'] = r
-                    meraki.result['changed'] = True
+                    if meraki.status == 200:
+                        meraki.result['data'] = r
+                        meraki.result['changed'] = True
     elif meraki.params['state'] == 'absent':
         if is_net_valid(meraki, meraki.params['net_name'], nets) is True:
             net_id = meraki.get_net_id(net_name=meraki.params['net_name'],
diff --git a/lib/ansible/modules/network/meraki/meraki_organization.py b/lib/ansible/modules/network/meraki/meraki_organization.py
index bfc9fed0dadd8d..85b082da4c3665 100644
--- a/lib/ansible/modules/network/meraki/meraki_organization.py
+++ b/lib/ansible/modules/network/meraki/meraki_organization.py
@@ -186,18 +186,24 @@ def main():
     elif meraki.params['state'] == 'present':
         if meraki.params['clone']:  # Cloning
             payload = {'name': meraki.params['org_name']}
-            meraki.result['data'] = meraki.request(meraki.construct_path('clone',
-                                                                         org_name=meraki.params['clone']
-                                                                         ),
-                                                   payload=json.dumps(payload),
-                                                   method='POST')
-            meraki.result['changed'] = True
+            response = meraki.request(meraki.construct_path('clone',
+                                                            org_name=meraki.params['clone']
+                                                            ),
+                                      payload=json.dumps(payload),
+                                      method='POST')
+            if meraki.status == 201:
+                meraki.result['data'] = response
+                meraki.result['changed'] = True
+            else:
+                meraki.fail_json(msg='Organization clone failed')
         elif not meraki.params['org_id'] and meraki.params['org_name']:  # Create new organization
             payload = {'name': meraki.params['org_name']}
-            meraki.result['data'] = meraki.request(meraki.construct_path('create'),
-                                                   method='POST',
-                                                   payload=json.dumps(payload))
-            meraki.result['changed'] = True
+            response = meraki.request(meraki.construct_path('create'),
+                                      method='POST',
+                                      payload=json.dumps(payload))
+            if meraki.status == 201:
+                meraki.result['data'] = response
+                meraki.result['changed'] = True
         elif meraki.params['org_id'] and meraki.params['org_name']:  # Update an existing organization
             payload = {'name': meraki.params['org_name'],
                        'id': meraki.params['org_id'],
@@ -208,12 +214,16 @@ def main():
                     meraki.params['org_id'],
                     orgs),
                     payload):
-                meraki.result['data'] = meraki.request(meraki.construct_path('update',
-                                                                             org_id=meraki.params['org_id']
-                                                                             ),
-                                                       method='PUT',
-                                                       payload=json.dumps(payload))
-                meraki.result['changed'] = True
+                response = meraki.request(meraki.construct_path('update',
+                                                                org_id=meraki.params['org_id']
+                                                                ),
+                                          method='PUT',
+                                          payload=json.dumps(payload))
+                if meraki.status == 200:
+                    meraki.result['data'] = response
+                    meraki.result['changed'] = True
+                else:
+                    meraki.fail_json(msg='Organization update failed')
     # in the event of a successful module execution, you will want to
     # simple AnsibleModule.exit_json(), passing the key/value results
     meraki.exit_json(**meraki.result)
diff --git a/lib/ansible/modules/network/meraki/meraki_snmp.py b/lib/ansible/modules/network/meraki/meraki_snmp.py
index 01cfaade2276f1..3487656858ffbc 100644
--- a/lib/ansible/modules/network/meraki/meraki_snmp.py
+++ b/lib/ansible/modules/network/meraki/meraki_snmp.py
@@ -125,7 +125,8 @@ def get_snmp(meraki, org_id):
     r = meraki.request(path,
                        method='GET',
                        )
-    return r
+    if meraki.status == 200:
+        return r
 
 
 def set_snmp(meraki, org_id):
@@ -169,8 +170,9 @@ def set_snmp(meraki, org_id):
         r = meraki.request(path,
                            method='PUT',
                            payload=json.dumps(payload))
-        meraki.result['changed'] = True
-        return r
+        if meraki.status == 200:
+            meraki.result['changed'] = True
+            return r
     return -1
 
 


Code review comments:
diff_hunk: @@ -195,7 +199,10 @@ def get_nets(self, org_name=None, org_id=None):
             org_id = self.get_org_id(org_name)
         path = self.construct_path('get_all', org_id=org_id, function='network')
         r = self.request(path, method='GET')
-        return r
+        if self.status == 200:
+            return r
+        else:
path: lib/ansible/module_utils/network/meraki/meraki.py
text: @user1:
So we tend to not do the else if the previous if-block returns from the function. The else is unneeded.

diff_hunk: @@ -150,7 +150,11 @@ def is_update_required(self, original, proposed, optional_ignore=None):
 
     def get_orgs(self):
         """Downloads all organizations for a user."""
-        return self.request('/organizations', method='GET')
+        response = self.request('/organizations', method='GET')
+        if self.status == 200:
+            return response
+        else:
path: lib/ansible/module_utils/network/meraki/meraki.py
text: @user1:
So we tend to not do the else if the previous if-block returns from the function. The else is unneeded.

@user1:
Looking at this again, I would prefer even this:
```python
if self.status != 200:
    self.fail_json(msg='Organization lookup failed')

return response
```

So that the default flow returns the response, except if the status is not 200. I think that's more clear, exceptions go into if-blocks, default flow is to succeed.