fixes and refactoring of s3_bucket

policy should now accept and handle correctly both data structures or JSON strings
removed unused tag_set var
refactored code to make conditions clearer
rebased to allow for ceph changes, left ceph update on todo list

lib/ansible/modules/extras/cloud/amazon/s3_bucket.py

@@ -110,6 +110,7 @@ EXAMPLES = '''
 
 '''
 
+import os
 import xml.etree.ElementTree as ET
 import urlparse
 
@@ -132,10 +133,7 @@ def get_request_payment_status(bucket):
     for message in root.findall('.//{http://s3.amazonaws.com/doc/2006-03-01/}Payer'):
         payer = message.text
 
-    if payer == "BucketOwner":
+    return (payer != "BucketOwner")
-        return False
-    else:
-        return True
 
 def create_tags_container(tags):
 
@@ -147,7 +145,7 @@ def create_tags_container(tags):
     tags_obj.add_tag_set(tag_set)
     return tags_obj
 
-def _create_bucket(connection, module, location):
+def _create_or_update_bucket(connection, module, location):
 
     policy = module.params.get("policy")
     name = module.params.get("name")
@@ -167,37 +165,33 @@ def _create_bucket(connection, module, location):
 
     # Versioning
     versioning_status = bucket.get_versioning_status()
-    if not versioning_status and versioning:
+    if not versioning_status:
-        try:
+        if versioning:
-            bucket.configure_versioning(versioning)
+            try:
-            changed = True
+                bucket.configure_versioning(versioning)
-            versioning_status = bucket.get_versioning_status()
+                changed = True
-        except S3ResponseError as e:
+                versioning_status = bucket.get_versioning_status()
-            module.fail_json(msg=e.message)
+            except S3ResponseError as e:
-    elif not versioning_status and not versioning:
+                module.fail_json(msg=e.message)
-        # do nothing
+    elif versioning_status['Versioning'] == "Enabled" and not versioning:
-        pass
+        bucket.configure_versioning(versioning)
-    else:
+        changed = True
-        if versioning_status['Versioning'] == "Enabled" and not versioning:
+        versioning_status = bucket.get_versioning_status()
-            bucket.configure_versioning(versioning)
+    elif ( (versioning_status['Versioning'] == "Disabled" and versioning) or (versioning_status['Versioning'] == "Suspended" and versioning) ):
-            changed = True
+        bucket.configure_versioning(versioning)
-            versioning_status = bucket.get_versioning_status()
+        changed = True
-        elif ( (versioning_status['Versioning'] == "Disabled" and versioning) or (versioning_status['Versioning'] == "Suspended" and versioning) ):
+        versioning_status = bucket.get_versioning_status()
-            bucket.configure_versioning(versioning)
-            changed = True
-            versioning_status = bucket.get_versioning_status()
 
     # Requester pays
     requester_pays_status = get_request_payment_status(bucket)
     if requester_pays_status != requester_pays:
         if requester_pays:
-            bucket.set_request_payment(payer='Requester')
+            payer='Requester'
-            changed = True
-            requester_pays_status = get_request_payment_status(bucket)
         else:
-            bucket.set_request_payment(payer='BucketOwner')
+            payer='BucketOwner'
-            changed = True
+        bucket.set_request_payment(payer=payer)
-            requester_pays_status = get_request_payment_status(bucket)
+        changed = True
+        requester_pays_status = get_request_payment_status(bucket)
 
     # Policy
     try:
@@ -208,29 +202,24 @@ def _create_bucket(connection, module, location):
         else:
             module.fail_json(msg=e.message)
 
-    if current_policy is not None and policy is not None:
+    if policy is not None:
-        if policy is not None:
+        # Deal with policy if either JSON formatted string or just data structure
-            policy = json.dumps(policy)
+        if isinstance(policy, basestring):
+            compare_policy = json.dumps(policy)
+            load_policy = policy
+        else:
+            compare_policy = policy
+            load_policy = json.loads(policy)
 
-        if json.loads(current_policy) != json.loads(policy):
+        if current_policy is None or json.loads(current_policy) != compare_policy:
             try:
-                bucket.set_policy(policy)
+                bucket.set_policy(load_policy)
                 changed = True
                 current_policy = bucket.get_policy()
             except S3ResponseError as e:
                 module.fail_json(msg=e.message)
-
+    elif current_policy is not None:
-    elif current_policy is None and policy is not None:
-        policy = json.dumps(policy)
 
-        try:
-            bucket.set_policy(policy)
-            changed = True
-            current_policy = bucket.get_policy()
-        except S3ResponseError as e:
-            module.fail_json(msg=e.message)
-    
-    elif current_policy is not None and policy is None:
         try:
             bucket.delete_policy()
             changed = True
@@ -240,15 +229,9 @@ def _create_bucket(connection, module, location):
                 current_policy = None
             else:
                 module.fail_json(msg=e.message)
-            
-    ####
-    ## Fix up json of policy so it's not escaped
-    ####
-    
     # Tags
     try:
         current_tags = bucket.get_tags()
-        tag_set = TagSet()
     except S3ResponseError as e:
         if e.error_code == "NoSuchTagSet":
             current_tags = None
@@ -307,7 +290,8 @@ def _destroy_bucket(connection, module):
 
     module.exit_json(changed=changed)
 
-def _create_bucket_ceph(connection, module, location):
+def _create_or_update_bucket_ceph(connection, module, location):
+    #TODO: add update
 
     name = module.params.get("name")
 
@@ -322,17 +306,20 @@ def _create_bucket_ceph(connection, module, location):
         except S3CreateError as e:
             module.fail_json(msg=e.message)
 
-    module.exit_json(changed=changed)
+    if bucket:
+        module.exit_json(changed=changed)
+    else:
+        module.fail_json(msg='Unable to create bucket, no error from the API')
 
 def _destroy_bucket_ceph(connection, module):
 
     _destroy_bucket(connection, module)
 
-def create_bucket(connection, module, location, flavour='aws'):
+def create_or_update_bucket(connection, module, location, flavour='aws'):
     if flavour == 'ceph':
-        _create_bucket_ceph(connection, module, location)
+        _create_or_update_bucket_ceph(connection, module, location)
     else:
-        _create_bucket(connection, module, location)
+        _create_or_update_bucket(connection, module, location)
 
 def destroy_bucket(connection, module, flavour='aws'):
     if flavour == 'ceph':
@@ -363,11 +350,11 @@ def main():
     argument_spec.update(
         dict(
             force = dict(required=False, default='no', type='bool'),
-            policy = dict(required=False, default=None),
+            policy = dict(required=False),
-            name = dict(required=True),
+            name = dict(required=True, type='str'),
             requester_pays = dict(default='no', type='bool'),
-            s3_url = dict(aliases=['S3_URL']),
+            s3_url = dict(aliases=['S3_URL'], type='str'),
-            state = dict(default='present', choices=['present', 'absent']),
+            state = dict(default='present', type='str', choices=['present', 'absent']),
             tags = dict(required=None, default={}, type='dict'),
             versioning = dict(default='no', type='bool'),
             ceph = dict(default='no', type='bool')
@@ -444,7 +431,7 @@ def main():
     state = module.params.get("state")
 
     if state == 'present':
-        create_bucket(connection, module, location, flavour=flavour)
+        create_or_update_bucket(connection, module, location)
     elif state == 'absent':
         destroy_bucket(connection, module, flavour=flavour)