Repository: ansible/ansible
Pull request: meraki_config_template - Check for HTTP status code
##### SUMMARY
- All requests now check returned status code
- Fail if status code isn’t what is expected

Fixes #41282

##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
meraki_config_template

##### ANSIBLE VERSION
```
ansible 2.7.0.dev0 (meraki/2.7-http-code 88764165ad) last updated 2018/06/29 19:47:54 (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 ?
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)
* [lib/ansible/modules/network/meraki/meraki_admin.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/meraki/meraki_admin.py)
* [lib/ansible/modules/network/meraki/meraki_config_template.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/meraki/meraki_config_template.py)
* [lib/ansible/modules/network/meraki/meraki_device.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/meraki/meraki_device.py)
* [lib/ansible/modules/network/meraki/meraki_network.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/meraki/meraki_network.py)
* [lib/ansible/modules/network/meraki/meraki_organization.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/meraki/meraki_organization.py)
* [lib/ansible/modules/network/meraki/meraki_snmp.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/meraki/meraki_snmp.py)
* [lib/ansible/modules/network/meraki/meraki_switchport.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/meraki/meraki_switchport.py)
* [lib/ansible/modules/network/meraki/meraki_vlan.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/meraki/meraki_vlan.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 --->

Commit message: Fix blank line error

Patch to review:
diff --git a/lib/ansible/modules/network/meraki/meraki_config_template.py b/lib/ansible/modules/network/meraki/meraki_config_template.py
index cc47dc74e012f7..3083d92eb2da20 100644
--- a/lib/ansible/modules/network/meraki/meraki_config_template.py
+++ b/lib/ansible/modules/network/meraki/meraki_config_template.py
@@ -83,7 +83,10 @@
 def get_config_templates(meraki, org_id):
     path = meraki.construct_path('get_all', org_id=org_id)
     response = meraki.request(path, 'GET')
-    return response
+    if meraki.status == 200:
+        return response
+    else:
+        meraki.fail_json(msg='Unable to get configuration templates')
 
 
 def get_template_id(meraki, name, data):
@@ -111,7 +114,10 @@ def delete_template(meraki, org_id, name, data):
     path = meraki.construct_path('delete', org_id=org_id)
     path = path + '/' + template_id
     response = meraki.request(path, 'DELETE')
-    return response
+    if meraki.status == 200:
+        return response
+    else:
+        meraki.fail_json(msg='Unable to remove configuration template')
 
 
 def bind(meraki, org_name, net_name, name, data):
@@ -126,7 +132,11 @@ def bind(meraki, org_name, net_name, name, data):
         if meraki.params['auto_bind']:
             payload['autoBind'] = meraki.params['auto_bind']
         meraki.result['changed'] = True
-        return meraki.request(path, method='POST', payload=json.dumps(payload))
+        r = meraki.request(path, method='POST', payload=json.dumps(payload))
+        if meraki.status == 200:
+            return r
+        else:
+            meraki.fail_json(msg='Unable to bind configuration template to network')
 
 
 def unbind(meraki, org_name, net_name, name, data):
@@ -136,7 +146,11 @@ def unbind(meraki, org_name, net_name, name, data):
     if is_network_bound(meraki, nets, net_name, template_id) is True:
         path = meraki.construct_path('unbind', function='config_template', net_id=net_id)
         meraki.result['changed'] = True
-        return meraki.request(path, method='POST')
+        r = meraki.request(path, method='POST')
+        if meraki.status == 200:
+            return r
+        else:
+            meraki.fail_json(msg='Unable to unbind configuration template from network')
 
 
 def main():


Code review comments:
diff_hunk: @@ -126,7 +132,11 @@ def bind(meraki, org_name, net_name, name, data):
         if meraki.params['auto_bind']:
             payload['autoBind'] = meraki.params['auto_bind']
         meraki.result['changed'] = True
-        return meraki.request(path, method='POST', payload=json.dumps(payload))
+        r = meraki.request(path, method='POST', payload=json.dumps(payload))
+        if meraki.status == 200:
+            return r
+        else:
path: lib/ansible/modules/network/meraki/meraki_config_template.py
text: @user1:
Again, the else here is unneeded and unwanted.

diff_hunk: @@ -111,7 +114,10 @@ def delete_template(meraki, org_id, name, data):
     path = meraki.construct_path('delete', org_id=org_id)
     path = path + '/' + template_id
     response = meraki.request(path, 'DELETE')
-    return response
+    if meraki.status == 200:
+        return response
+    else:
path: lib/ansible/modules/network/meraki/meraki_config_template.py
text: @user1:
Again, the else here is unneeded and unwanted.

diff_hunk: @@ -83,7 +83,10 @@
 def get_config_templates(meraki, org_id):
     path = meraki.construct_path('get_all', org_id=org_id)
     response = meraki.request(path, 'GET')
-    return response
+    if meraki.status == 200:
+        return response
+    else:
path: lib/ansible/modules/network/meraki/meraki_config_template.py
text: @user1:
Again, the else here is unneeded and unwanted.

@user1:
Looking at this again, I would prefer even this:
```python
if meraki.status != 200:
    meraki.fail_json(msg='Unable to get configuration templates')

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.

diff_hunk: @@ -136,7 +146,11 @@ def unbind(meraki, org_name, net_name, name, data):
     if is_network_bound(meraki, nets, net_name, template_id) is True:
         path = meraki.construct_path('unbind', function='config_template', net_id=net_id)
         meraki.result['changed'] = True
-        return meraki.request(path, method='POST')
+        r = meraki.request(path, method='POST')
+        if meraki.status == 200:
+            return r
+        else:
path: lib/ansible/modules/network/meraki/meraki_config_template.py
text: @user1:
Again, the else here is unneeded and unwanted.