Repository: ansible/ansible
Pull request: Ensure that s3_bucket versioning is correctly enabled
##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
s3_bucket

##### ANSIBLE VERSION
```
ansible 2.3.0 (devel a6cb377420) last updated 2017/02/13 10:23:20 (GMT +1000)
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
```

##### SUMMARY
If versioning is not enabled, then `get_bucket_versioning`
can return an empty dict.

If that happens, the code to enable versioning should still
run!

The logic for suspending versioning was also incorrect, so
have updated that too.

Fixes #20491

Tested with:
```
hacking/test-module -m lib/ansible/modules/cloud/amazon/s3_bucket.py -a "profile=sandbox name=test-2017-02-13-1039 versioning=no"
 hacking/test-module -m lib/ansible/modules/cloud/amazon/s3_bucket.py -a "profile=sandbox name=test-2017-02-13-1039 versioning=yes"
 hacking/test-module -m lib/ansible/modules/cloud/amazon/s3_bucket.py -a "profile=sandbox name=test-2017-02-13-1039 versioning=no"
```

and results now as expected

Problem statement:
Module s3_bucket do not enable versioning
##### ISSUE TYPE
 - Bug Report

##### COMPONENT NAME
Module s3_bucket

##### ANSIBLE VERSION
```
ansible 2.2.0.0
  config file = 
  configured module search path = Default w/o overrides

```

##### CONFIGURATION
None

##### OS / ENVIRONMENT
Ubuntu 16.04 LTS

##### SUMMARY
Setting s3_bucket versioning option to true do not enable versioning.

##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
---

- set_fact:
    aws_storage_versioning: yes

- name: Create the storage bucket
  s3_bucket:
    name: my-awesome-bucket
    region: '{{ aws_region }}'
    tags:
      environment: '{{ aws_environment }}'
      tier: storage
    versioning: '{{ aws_storage_versioning|bool }}'
    state: present

```

<!--- You can also paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
Go to AWS console and see versioning enabled.

##### ACTUAL RESULTS
Go to AWS console and see versioning IS NOT enabled.

Hints from issue discussion:
cc @wimnat
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
+1 affected by this bug

Commit message: Ensure that s3_bucket versioning is correctly enabled

If versioning is not enabled, then `get_bucket_versioning`
can return an empty dict.

If that happens, the code to enable versioning should still
run!

The logic for suspending versioning was also incorrect, so
have updated that too.

Fixes #20491

Patch to review:
diff --git a/lib/ansible/modules/cloud/amazon/s3_bucket.py b/lib/ansible/modules/cloud/amazon/s3_bucket.py
index ae6811a9566038..4032b3dc40bb6b 100755
--- a/lib/ansible/modules/cloud/amazon/s3_bucket.py
+++ b/lib/ansible/modules/cloud/amazon/s3_bucket.py
@@ -168,22 +168,21 @@ def _create_or_update_bucket(connection, module, location):
 
     # Versioning
     versioning_status = bucket.get_versioning_status()
-    if versioning_status:
-        if versioning is not None:
-            if versioning and versioning_status['Versioning'] != "Enabled":
-                try:
-                    bucket.configure_versioning(versioning)
-                    changed = True
-                    versioning_status = bucket.get_versioning_status()
-                except S3ResponseError as e:
-                    module.fail_json(msg=e.message)
-            elif not versioning and versioning_status['Versioning'] != "Enabled":
-                try:
-                    bucket.configure_versioning(versioning)
-                    changed = True
-                    versioning_status = bucket.get_versioning_status()
-                except S3ResponseError as e:
-                    module.fail_json(msg=e.message)
+    if versioning is not None:
+        if versioning and versioning_status.get('Versioning') != "Enabled":
+            try:
+                bucket.configure_versioning(versioning)
+                changed = True
+                versioning_status = bucket.get_versioning_status()
+            except S3ResponseError as e:
+                module.fail_json(msg=e.message)
+        elif not versioning and versioning_status.get('Versioning') == "Enabled":
+            try:
+                bucket.configure_versioning(versioning)
+                changed = True
+                versioning_status = bucket.get_versioning_status()
+            except S3ResponseError as e:
+                module.fail_json(msg=e.message)
 
     # Requester pays
     requester_pays_status = get_request_payment_status(bucket)


Code review comments:
diff_hunk: @@ -168,22 +168,21 @@ def _create_or_update_bucket(connection, module, location):
 
     # Versioning
     versioning_status = bucket.get_versioning_status()
-    if versioning_status:
-        if versioning is not None:
-            if versioning and versioning_status['Versioning'] != "Enabled":
-                try:
-                    bucket.configure_versioning(versioning)
-                    changed = True
-                    versioning_status = bucket.get_versioning_status()
-                except S3ResponseError as e:
-                    module.fail_json(msg=e.message)
-            elif not versioning and versioning_status['Versioning'] != "Enabled":
-                try:
-                    bucket.configure_versioning(versioning)
-                    changed = True
-                    versioning_status = bucket.get_versioning_status()
-                except S3ResponseError as e:
-                    module.fail_json(msg=e.message)
+    if versioning is not None:
+        if versioning and versioning_status.get('Versioning') != "Enabled":
+            try:
+                bucket.configure_versioning(versioning)
+                changed = True
+                versioning_status = bucket.get_versioning_status()
+            except S3ResponseError as e:
+                module.fail_json(msg=e.message)
path: lib/ansible/modules/cloud/amazon/s3_bucket.py
text: @user1:
Please add the traceback to this message with `exception=traceback.format_exc()`

@author:
This code isn't actually changing (apart from the change to indentation)

There is no documentation that exceptions should be returned when calling `fail_json`.

I can do this, but it's a surprise that I should need to.