kinesis_stream: Don't mark kstreams changed when no encryption actions taken

[Tyler-2]

Fixes ansible/ansible#65928

SUMMARY

Fixes ansible/ansible#65928.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

kinesis_stream

ADDITIONAL INFORMATION

Do a few extra conditional checks when running to avoid reporting changes if the stream already matches the desired configuration.

ok: [localhost] => {
    "changed": false, 
    "invocation": {
        "module_args": {
            "aws_access_key": null, 
            "aws_config": null, 
            "aws_secret_key": null, 
            "debug_botocore_endpoint_logs": false, 
            "ec2_url": null, 
            "encryption_state": "disabled", 
            "encryption_type": "KMS", 
            "key_id": "alias/aws/kinesis", 
            "name": "tyler-test", 
            "profile": null, 
            "region": null, 
            "retention_period": 24, 
            "security_token": null, 
            "shards": 1, 
            "state": "present", 
            "tags": null, 
            "validate_certs": true, 
            "wait": true, 
            "wait_timeout": 300
        }
    }, 
    "msg": "Kinesis Stream tyler-test encryption already stopped.", 
    "success": true

[tremble]

I know the tests don't currently exist for the kinesis module, would you be willing to try and add some integration tests too?

https://docs.ansible.com/ansible/latest/dev_guide/platforms/aws_guidelines.html#integration-tests-for-aws-modules

[Tyler-2]

That seems like it would be a feature, not really related to this fix. Would prefer that be added as an issue to be taken on. I tend to make changes during work hours to support my employer's use of Ansible, not sure if I can justify adding tests!

Do agree, it's a good idea though.

[tremble]

Unfortunately I'm not familiar enough with Kinesis to be willing to +1 this without test suites. Let's see if we can get ansible/ansible#62114 cleaned up and pulled in.

[Tyler-2]

Thankyou Tremble!

[tremble]

After some digging I suspect there's something more systemic wrong with the encryption pieces. All of the return values seem to change when encryption is being manipulated.

This change on it's own looks sane (now I understand a little about the module). I'll probably poke the code more thoroughly once this change has landed.

[Tyler-2]

Any other steps I can take to get this merged?

[ansibullbot]

cc @jillr @linuxdynasty @s-hertel @wimnat
click here for bot help

[tremble]

@Tyler-2 One quick thing while we wait on the integration tests please add a changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

[Tyler-2]

Added.

[tremble]

Looks like something's a little funky with the unit tests here. Given the results of my integration testing I suspect the unit test is buggy rather than your code.

[jillr]

The unit tests for this module pass on main, but do not pass for me locally if I rebase this PR onto latest main. @Tyler-2 the unit tests here probably need to be updated for the code changes, would you be able to take a look at that please?

[tremble]

I've dug more into the unit tests. They're intrinsically flawed, they relied on find_stream returning a fixed value in check mode. (The failing test was failing because the test was bad, not the change - find_stream returned an unencrypted stream so no change should have been made)

I've fixed up various additional issues with kinesis_stream and removed the unit tests, testing is now based on integration tests instead.

[tremble]

@Tyler-2 it's taken almost a year but your fix is finally merged! I'm sorry it's taken so long. Thanks for your PR.

[Tyler-2]

Hallelujah! Thanks Tremble for really keeping on it.