Repository: ansible/ansible
Pull request: Modifying how optional parameters are handled in rds.py. Fixes #20370
Allowing options to be set to false/no. Previously ignored unless set to true/yes.

Added a conditional for invalid parameters since the default is false instead of null for some options (e.g. force_failover, apply_immediately, upgrade).

##### ISSUE TYPE
 - Bugfix Pull Request

##### COMPONENT NAME
lib/ansible/modules/cloud/amazon/rds.py

##### ANSIBLE VERSION
```
ansible 2.3.0 (rds_param_fix f695114736) last updated 2017/01/25 10:43:30 (GMT -400)
  config file =
  configured module search path = Default w/o overrides
```

##### SUMMARY
Fixes #20370. 
The problem: if a parameter was set to False it would be ignored (preventing the user from turning an option off). For example, if the user modified their instance using `multi_zone: no` in their playbook it wouldn't be added to the parameters used (just like the optional parameters that were by default set to null or false). So I changed that to allow parameters that were not None.
This introduced a second problem since not all defaults are null. I added a couple lines to allow if the parameter was both invalid and False to be ignored since it may be the default. If there's an easy way to ensure it's the default it might be a good thing to add.
Another way we could fix that second issue is to make all the defaults null.

The command output stayed the same after the change. The result after the change now reflects that output.
Excerpt:
```
...
TASK [modify db instance] *********************************************************************
....
changed: [localhost] => {
    "changed": true,
    "instance": {
        ...
        "multi_zone": true,
        "port": 3306,
        "replication_source": null,
        "status": "available",
        ...
    },
    "invocation": {
        "module_args": {
            "apply_immediately": true,
            ...
            "multi_zone": false,
            ...
        },
        "module_name": "rds"
    }
}

PLAY RECAP ************************************************************************************
localhost                  : ok=2    changed=1    unreachable=0    failed=0
```


Problem statement:
RDS module fails to modify instance multi_az from 'yes' to 'no'
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and master branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Bug Report

##### COMPONENT NAME
rds
ansible/lib/ansible/modules/cloud/amazon/rds.py

##### ANSIBLE VERSION
<!--- Paste verbatim output from “ansible --version” between quotes below -->
```
ansible 2.2.1.0
```

##### CONFIGURATION
<!---
Mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->

##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say “N/A” for anything that is not platform-specific.
-->
Mac OS 10.12.2

##### SUMMARY
<!--- Explain the problem briefly -->
With apply_immeditely set to 'yes':
If I try and use the module to modify an instance from multi_zone: no to multi_zone: yes, it works.
BUT, if I try and use the module to modify an instance from multi_zone: yes to multi_zone: no, it says "changed" but no action is triggered. Again, apply_immediately is set to 'yes'.
##### 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.
-->
Create an RDS instance with multi_zone set to `no`.
Write an ansible play to modify said instance multi_zone to `yes`.
eg:
```
- name: modify db instance
      rds:
        command: modify
        region: us-west-2
        instance_name: test-db
        multi_zone: yes
        apply_immediately: yes
        wait: yes
```
Run play and confirm that mutli_az has been changed from `no` to `yes`. 👍 
Now change play to modify instance from multi_zone `yes` to multi_zone `no`
eg
```
- name: modify db instance
      rds:
        command: modify
        region: us-west-2
        instance_name: test-db
        multi_zone: no
        apply_immediately: yes
        wait: yes
```
Run the play and confirm that play does not throw and error of any kind, but appears to work.
Check the RDS instance and you will see that no modification has been triggered, and multi_az remains set to `yes`. 👎 

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: modify db instance
      rds:
        command: modify
        region: us-west-2
        instance_name: test-db
        multi_zone: no
        apply_immediately: yes
        wait: yes
```

<!--- You can also paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->
Expected: changed multi_zone: `yes` to multi_zone: `no`
##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->
Actual: nothing changed.
<!--- Paste verbatim command output between quotes below -->
```
Micahs-MBP:RDS mkramer$ AWS_PROFILE=qa ansible-playbook -i inventory/ec2.py rds_main.yml -e "vars=programs" -vvvv
No config file found; using defaults
 [WARNING]: Host file not found: inventory/ec2.py

 [WARNING]: provided hosts list is empty, only localhost is available

Loading callback plugin default of type stdout, v2.0 from /Library/Python/2.7/site-packages/ansible/plugins/callback/__init__.pyc

PLAYBOOK: rds_main.yml *********************************************************
1 plays in rds_main.yml

PLAY [RDS Database] ************************************************************

TASK [setup] *******************************************************************
Using module file /Library/Python/2.7/site-packages/ansible/modules/core/system/setup.py
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: mkramer
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo ~/.ansible/tmp/ansible-tmp-1484695047.43-249961633635893 `" && echo ansible-tmp-1484695047.43-249961633635893="` echo ~/.ansible/tmp/ansible-tmp-1484695047.43-249961633635893 `" ) && sleep 0'
<127.0.0.1> PUT /var/folders/0w/h513fdx52h77f2gyfmhhczrh0000gn/T/tmpd364EL TO /Users/mkramer/.ansible/tmp/ansible-tmp-1484695047.43-249961633635893/setup.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /Users/mkramer/.ansible/tmp/ansible-tmp-1484695047.43-249961633635893/ /Users/mkramer/.ansible/tmp/ansible-tmp-1484695047.43-249961633635893/setup.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/bin/python /Users/mkramer/.ansible/tmp/ansible-tmp-1484695047.43-249961633635893/setup.py; rm -rf "/Users/mkramer/.ansible/tmp/ansible-tmp-1484695047.43-249961633635893/" > /dev/null 2>&1 && sleep 0'
ok: [localhost]

TASK [modify db instance] ******************************************************
task path: /Users/mkramer/github/infrastructure/ansible/RDS/rds_main.yml:71
Using module file /Library/Python/2.7/site-packages/ansible/modules/core/cloud/amazon/rds.py
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: mkramer
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo ~/.ansible/tmp/ansible-tmp-1484695048.0-128080980636571 `" && echo ansible-tmp-1484695048.0-128080980636571="` echo ~/.ansible/tmp/ansible-tmp-1484695048.0-128080980636571 `" ) && sleep 0'
<127.0.0.1> PUT /var/folders/0w/h513fdx52h77f2gyfmhhczrh0000gn/T/tmpWCn0PP TO /Users/mkramer/.ansible/tmp/ansible-tmp-1484695048.0-128080980636571/rds.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /Users/mkramer/.ansible/tmp/ansible-tmp-1484695048.0-128080980636571/ /Users/mkramer/.ansible/tmp/ansible-tmp-1484695048.0-128080980636571/rds.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/bin/python /Users/mkramer/.ansible/tmp/ansible-tmp-1484695048.0-128080980636571/rds.py; rm -rf "/Users/mkramer/.ansible/tmp/ansible-tmp-1484695048.0-128080980636571/" > /dev/null 2>&1 && sleep 0'
changed: [localhost] => {
    "changed": true, 
    "instance": {
        "availability_zone": "us-west-2c", 
        "backup_retention": 7, 
        "create_time": 1484609755.023, 
        "endpoint": "programs-qa-blu.cbwgj0moesb4.us-west-2.rds.amazonaws.com", 
        "id": "programs-qa-blu", 
        "instance_type": "db.t2.micro", 
        "iops": null, 
        "maintenance_window": "fri:08:20-fri:08:50", 
        "multi_zone": true, 
        "port": 5432, 
        "replication_source": null, 
        "status": "available", 
        "username": "programsUser", 
        "vpc_security_groups": "sg-2620c15f"
    }, 
    "invocation": {
        "module_args": {
            "apply_immediately": true, 
            "aws_access_key": null, 
            "aws_secret_key": null, 
            "backup_retention": null, 
            "backup_window": null, 
            "character_set_name": null, 
            "command": "modify", 
            "db_engine": null, 
            "db_name": null, 
            "ec2_url": null, 
            "engine_version": null, 
            "force_failover": false, 
            "instance_name": "programs-qa-blu", 
            "instance_type": null, 
            "iops": null, 
            "license_model": null, 
            "maint_window": null, 
            "multi_zone": false, 
            "new_instance_name": null, 
            "option_group": null, 
            "parameter_group": null, 
            "password": null, 
            "port": null, 
            "profile": null, 
            "publicly_accessible": null, 
            "region": "us-west-2", 
            "security_groups": null, 
            "security_token": null, 
            "size": null, 
            "snapshot": null, 
            "source_instance": null, 
            "subnet": null, 
            "tags": null, 
            "upgrade": false, 
            "username": null, 
            "validate_certs": true, 
            "vpc_security_groups": null, 
            "wait": true, 
            "wait_timeout": 300, 
            "zone": null
        }, 
        "module_name": "rds"
    }
}

PLAY RECAP *********************************************************************
localhost                  : ok=2    changed=1    unreachable=0    failed=0   

Micahs-MBP:RDS mkramer$ 
```

Hints from issue discussion:
cc @bpennypacker @willthames
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Modifying how optional parameters are handled in rds.py. Fixes #20370

Allowing options to be set to false/no. Previously ignored unless set to true/yes.

Added a conditional for invalid parameters since the default is false instead of null for some options (e.g. force_failover, apply_immediately, upgrade).

Patch to review:
diff --git a/lib/ansible/modules/cloud/amazon/rds.py b/lib/ansible/modules/cloud/amazon/rds.py
index d12593f3201d3a..8b43ed0785ded6 100644
--- a/lib/ansible/modules/cloud/amazon/rds.py
+++ b/lib/ansible/modules/cloud/amazon/rds.py
@@ -1001,11 +1001,14 @@ def validate_parameters(required_vars, valid_vars, module):
 
     params = {}
     for (k, v) in optional_params.items():
-        if module.params.get(k) and k not in required_vars:
+        if module.params.get(k) in [True, False] and k not in required_vars:
             if k in valid_vars:
                 params[v] = module.params[k]
             else:
-                module.fail_json(msg="Parameter %s is not valid for %s command" % (k, command))
+                if module.params.get(k) == False:
+                    pass
+                else:
+                    module.fail_json(msg="Parameter %s is not valid for %s command" % (k, command))
 
     if module.params.get('security_groups'):
         params[sec_group] = module.params.get('security_groups').split(',')


Code review comments:
diff_hunk: @@ -1001,11 +1001,14 @@ def validate_parameters(required_vars, valid_vars, module):
 
     params = {}
     for (k, v) in optional_params.items():
-        if module.params.get(k) and k not in required_vars:
+        if module.params.get(k) in [True, False] and k not in required_vars:
path: lib/ansible/modules/cloud/amazon/rds.py
text: @user1:
I think this should instead be `is not None`, since there are optional parameters that are neither true or false, such as `iops`, `size`, and `license_model`.