Fix acl update on single element list

[steppi91]

Fixes inconsistent behaviour in updating single element ACL list:
When using the mark_others_as_absent flag in kafka_acls.acls there is a bug when using just a single ACL in the list.

This is because of the implementation that implements the single ACL kafka_acl.acl case with transforming it to a single element ACL list, and then treating single-element lists as an edge case.

The logic is in my opinion only necessary to prevent deleting other ACLs in presence of the mark_others_as_absent when trying to update a single ACL. But this behaviour is wrong when using actual single-element ACL lists.

Proposed Changes

The least invasive change I could think of was to remove the edge case and just setting the mark_others_as_absent flag to False in case of updating a single ACL.

[StephenSorriaux]

Thank you, would you feel comfortable with adding a test for this specific case?

[steppi91]

The tests using the mark_others_as_absent flag are commented out right now, so I did not feel comfortable adding them again. And I can't run the tests locally, they always fail with a ton of cryptic error messages

[ryarnyah]

mark_others_as_absent will always be False when calling from kafka_acl module (see line 43)

[ryarnyah]

I'm not sure we need to get all ACLs when kafka_acl module is called. When you have many ACLs it might be very slow to retrieve them.

Maybe a more elegant solution might be to extract the if directly inside callers to pass it to process_module_acls? What do you think of it? @steppi91 @StephenSorriaux ?

[ryarnyah]

The tests using the mark_others_as_absent flag are commented out right now, so I did not feel comfortable adding them again. And I can't run the tests locally, they always fail with a ton of cryptic error messages

mark_others_as_absent tests are commented now because they can broke all others tests if running <=> they all use the same clusters.. we need to add a dedicated cluster (ZK + Kafka) to test it (or maybe just another non-sasl listener for others test). Don't bother with it, adding some commented test will suffice for now i think.

Tests will work on Linux Only due to docker network usage.
To run them locally you can do that (you will need docker to be installed and your user to be in docker group):

$ cd ansible-kafka-admin
$ virtualenv ./venv
$ source ./venv/bin/activate
$ pip install -r requirements.txt -r test-requirements.txt
$ export ANSIBLE_MODULE_UTILS=$PWD/module_utils
$ molecule create && molecule converge
# If your need to run it again you can directly use molecule verify to run only the tests
$ molecule verify
# If you want to test another version of Python (default in local is 2.7) you only need to define PYTHON_VERSION env var like that
$ export PYTHON_VERSION=3.9-slim

When editing code, flake8 can be a pain in the ass, don't forget to pass it on library, module_utils & tests otherwise CI build wont pass... (but it's seem that you don't have this problem :) )

$ flake8 ./library
$ flake8 ./module_utils
$ flake8 ./molecule

[steppi91]

To run them locally you can do that (you will need docker to be installed and your user to be in docker group)

I ran the tests like this before, maybe the problem is me using docker for mac.
Some of the errors I got where because the testing container couldn't reach the IP address of the kafka-container, yet when checking the docker network configuration it seemed everything was in order.
I'll add some test and comment it out.

I'm not sure we need to get all ACLs when kafka_acl module is called. When you have many ACLs it might be very slow to retrieve them.

That is a good point, I will change the code. My usecase actually is to only update the ACLs of a specific topic, which I think is not such an uncommon usecase. Here I also don't need to retrieve all the ACLs of all topics, not sure how to nicely capture that case though with the current interfaces without getting messy.

[steppi91]

Sorry for the long delay. I modded the logic so the single ACL case is handled without requesting all ACLs. And I also added a testcase and commented it out. It is basically the other commented test case with just one ACL in the test_acl_configuration, since the rest of the test logic is the same

[StephenSorriaux]

Is this still a thing since #136 ?

[arenard]

Is there any updates about this ?