user_role: update role to take a list instead of string

[harCamConsulting]

Description

Changed the roles parameter on user_role to be able to take a list. Also added a new parameter "remove_unlisted" which will default to false. This should mean the change is non-breaking for the moment, as the role list can also take a string, which will auto convert to list of one item. In later versions (perhaps a major version?) The default should probably for remove_unlisted should probably change to true to make this module be more idempotent - but it may break compatibility with previously written tasks.

How Has This Been Tested?

~~Manually tested locally - but needs testing against unit tests.~~ Have now added unit tests for new functionality, ~~but this also changed the output of module - breaking change maybe?~~ Added functionality now to give a compatibility mode where calling with using old parameters gives old style output. Using new "roles" parameter gives new output including diff.

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue) - Fixes #
• [X] New feature (non-breaking change which adds functionality)

Checklist:

• [X] I have read/followed the CONTRIBUTING document.
• [X] I have read/followed the PR Quick Start Guide
• [X] I have added tests to cover my changes or they are N/A.
• [X] I have added a changelog fragment describing the changes.
• [X] New module options/parameters include a version_added property.

[codecov-commenter]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 89.71%. Comparing base (047e0db) to head (88f5578).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #324      +/-   ##
==========================================
- Coverage   93.77%   89.71%   -4.06%     
==========================================
  Files          68       55      -13     
  Lines        2392     1536     -856     
==========================================
- Hits         2243     1378     -865     
- Misses        149      158       +9     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[harCamConsulting]

I've now added some unit tests to test the new functionality of the lists, but to properly test, I've had to change the output of the module - possibly this could be a breaking change? The existing output was just the output of the add- or the remove- command which didn't really make sense when it was possibly doing multiple adds and/or removes anyway

[lowlydba]

@harCamConsulting Thanks for opening the PR!

I am concerned this approach may cause unnecessary breaking changes later though, since its not quite the same as the add/remove/set pattern mentioned in the original issue, and that is likely the goal end state for this sort of feature. I fear it might create more difficulty to add the ability to add lists, but not remove via lists - for example.

I don't think we need to copy the exact code used in the AD collection example, just use the same parameters for the functionality - if that makes it any easier.

If we can add those as purely new parameters, we can keep the old ones but mark them as deprecated, and then leave the backwards compatibility until a new major version release.

[harCamConsulting]

Yeah, that's fair. I'll keep working on it.

[harCamConsulting]

ok - sorry for the push spam there, but I think I've got the code back to working and passing the CI checks again. Although, the "|grp1" checks don't seem to pass, but I think it was like that before anyway? I've implemented the add/remove/set functionality under the "roles" parameter versus the old role (singular) parameter but concerned that they're too closely named and may cause confusion. Happy to take direction there and rename it to something else.

I also implemented calling the module without either of the role or roles parameter to give back a list of current roles for the user (i.e. kinda like a facts module), along with a few more tests which use that.

[lowlydba]

ok - sorry for the push spam there, but I think I've got the code back to working and passing the CI checks again. Although, the "|grp1" checks don't seem to pass, but I think it was like that before anyway?

I have fixed almost all the tests (except for the dlevel Sanity, which is just a new upstream breaking change), so we should be expecting these to work. I'll have to spend some time looking at the errors (which I will try to find the time for soon) but we'll need these to be green to merge.

EDIT: All tests should be passing on the main branch now

[harCamConsulting]

ok that's better, I was handling arrays weird, which caused them to come out double nested in the output.

Still keen to hear thoughts on the naming of "role" vs "roles" possible confusion or if there might be a better name to use.

[lowlydba]

ok that's better, I was handling arrays weird, which caused them to come out double nested in the output.

Still keen to hear thoughts on the naming of "role" vs "roles" possible confusion or if there might be a better name to use.

Loving the green tests! Its on my radar to carve out time for another review of this, thanks for hanging in there