user_role: update role to take a list instead of string #324

harCamConsulting · 2025-08-05T05:21:39Z

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 #
New feature (non-breaking change which adds functionality)

Checklist:

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

codecov-commenter · 2025-08-05T05:37:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ 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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

…(breaking change?)

harCamConsulting · 2025-08-06T22:33:34Z

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

Copilot

Pull Request Overview

Updates the user_role module to accept a list of roles instead of just a single role string, enhancing functionality while maintaining backward compatibility through aliases. Adds an optional parameter to remove unlisted roles for better idempotency control.

Converts the role parameter to accept a list while maintaining backward compatibility via aliases
Adds remove_unlisted parameter to optionally remove roles not specified in the list
Updates module logic to handle multiple role operations and provides detailed output of role membership changes

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
plugins/modules/user_role.py	Updates module documentation to reflect list-based role parameter and new remove_unlisted option
plugins/modules/user_role.ps1	Implements core logic for handling multiple roles and optional removal of unlisted roles
tests/integration/targets/user_role/tasks/main.yml	Adds comprehensive test cases for new list functionality and role removal behavior
galaxy.yml	Bumps version to 2.6.2 for the new feature
changelogs/changelog.yaml	Documents the enhancement in the changelog
sqlserver	Adds symlink file (likely for development setup)

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

plugins/modules/user_role.py

plugins/modules/user_role.ps1

sqlserver

lowlydba · 2025-08-17T18:42:15Z

@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 · 2025-08-17T23:47:08Z

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

…lydba.sqlserver into add-list-to-dbroles

harCamConsulting · 2025-08-27T02:38:38Z

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 · 2025-09-05T15:50:28Z

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

…lydba.sqlserver into add-list-to-dbroles

harCamConsulting · 2025-09-09T04:34:43Z

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 · 2025-09-19T15:52:19Z

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

harCamConsulting · 2026-01-04T22:58:05Z

Just wanted to give this a bump to see if there's anything else I can do here?

lowlydba · 2026-01-08T18:52:29Z

Just wanted to give this a bump to see if there's anything else I can do here?

You're all good. I sincerely appreciate your patience and persistence on this. I finally have had some time to review, and it all looks good. I'll do a final review with Copilot, and then ship it if things are good.

Thank you @harCamConsulting !

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.

plugins/modules/user_role.py

plugins/modules/user_role.ps1

changelogs/changelog.yaml

tests/integration/targets/user_role/tasks/main.yml

plugins/modules/user_role.ps1

tests/integration/targets/user_role/tasks/main.yml

plugins/modules/user_role.ps1

plugins/modules/user_role.py

Co-authored-by: Copilot <[email protected]>

plugins/modules/user_role.ps1

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 15 comments.

Copilot · 2026-01-08T19:18:05Z

plugins/modules/user_role.ps1

+    $output = New-Object -TypeName PSCustomObject -Property $outputProps
 }
+
 try {


In non-compatibility mode when no changes occur, the code creates an output with just 'roleMembership' but no 'diff' (lines 198-204). However, when changes do occur, it includes both 'roleMembership' and 'diff'. This is good idempotent behavior. However, in the query-only mode (no role or roles specified), the code should also return the current role membership. The current logic doesn't handle the query-only case where no roles are specified to add/remove/set - it would skip to line 207 with $output potentially undefined if not in compatibility mode.

Suggested change

try {

try {

# In non-compatibility (roles dict) mode, if no output was created earlier (e.g. query-only),

# return the current role membership without a diff.

if (-not $compatibilityMode -and $null -eq $output) {

try {

$membershipObjects = Get-DbaDbRoleMember @commonParamSplat -IncludeSystemUser $true | Where-Object { $_.UserName -eq $username }

$currentRoleMembership = [array]($membershipObjects.role | Sort-Object)

if ($null -eq $currentRoleMembership) { $currentRoleMembership = @() }

}

catch {

$module.FailJson("Failure getting current role membership: $($_.Exception.Message)", $_)

}

if ($null -eq $outputProps) { $outputProps = @{} }

$outputProps.roleMembership = $currentRoleMembership

$output = New-Object -TypeName PSCustomObject -Property $outputProps

}

@harCamConsulting I think this comment makes sense, can you add this query-mode logic in?

tests/integration/targets/user_role/tasks/main.yml

plugins/modules/user_role.ps1

Copilot · 2026-01-08T19:18:06Z

plugins/modules/user_role.ps1

+# Ensure that when using the 'roles' parameter directly, at least one operation is specified.
+if (-not $compatibilityMode) {
+    $hasSet    = ($null -ne $roles['set']    -and @($roles['set']).Count    -gt 0)
+    $hasAdd    = ($null -ne $roles['add']    -and @($roles['add']).Count    -gt 0)
+    $hasRemove = ($null -ne $roles['remove'] -and @($roles['remove']).Count -gt 0)
+
+    if (-not ($hasSet -or $hasAdd -or $hasRemove)) {
+        $module.FailJson("When using the 'roles' parameter, you must specify at least one of: roles.set, roles.add, or roles.remove.")
    }
 }


The validation check at lines 84-93 will execute even when neither role nor roles parameters are provided (the "get current role membership" mode documented in the RETURN section). When a user wants to simply query the current role membership without providing either parameter, this check will incorrectly fail. The check should only apply when the roles parameter is explicitly provided by the user.

@harCamConsulting Can you add a check in line 88 to ensure $roles is defined in addition to the compat mode check?

tests/integration/targets/user_role/tasks/main.yml

plugins/modules/user_role.ps1

plugins/modules/user_role.py

plugins/modules/user_role.ps1

plugins/modules/user_role.py

Co-authored-by: Copilot <[email protected]>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Copilot · 2026-01-08T19:35:18Z

plugins/modules/user_role.ps1

+# Ensure that when using the 'roles' parameter directly, at least one operation is specified.
+if (-not $compatibilityMode) {
+    $hasSet = ($null -ne $roles['set'] -and @($roles['set']).Count -gt 0)
+    $hasAdd = ($null -ne $roles['add'] -and @($roles['add']).Count -gt 0)
+    $hasRemove = ($null -ne $roles['remove'] -and @($roles['remove']).Count -gt 0)
+
+    if (-not ($hasSet -or $hasAdd -or $hasRemove)) {
+        $module.FailJson("When using the 'roles' parameter, you must specify at least one of: roles.set, roles.add, or roles.remove.")
    }


The validation logic has multiple issues:

Line 89 checks @($roles['set']).Count -gt 0, which prevents using roles.set: [] to remove all roles (a documented feature in the tests and documentation).

The validation doesn't account for "query mode" where neither 'role' nor 'roles' operations are specified (as tested at lines 198 and 224 of the integration tests). In this case, all three $hasSet, $hasAdd, and $hasRemove would be false, causing the module to fail incorrectly.

The fix should:

Change line 89 to $hasSet = $null -ne $roles['set'] (without the Count check) to allow empty arrays

Add logic to detect and allow query mode, where the module simply returns current role membership without modification

Copilot · 2026-01-08T19:35:18Z

plugins/modules/user_role.ps1

+if ($null -ne $roles['set']) {
+    $comparison = Compare-Object $existingRoleMembership ([array]$roles['set'])
+    if ($null -eq $comparison) {
+        $rolesToAdd = @()
+        $rolesToRemove = @()
+    }
+    else {
+        $rolesToAdd = ( $comparison | Where-Object { $_.SideIndicator -eq '=>' } ).InputObject
+        $rolesToRemove = ( $comparison | Where-Object { $_.SideIndicator -eq '<=' } ).InputObject
+    }
+}
+else {
+    $comparisonAdd = Compare-Object $existingRoleMembership ([array]$roles['add'])
+    if ($null -eq $comparisonAdd) {
+        $rolesToAdd = @()
+    }
+    else {
+        $rolesToAdd = ( $comparisonAdd | Where-Object { $_.SideIndicator -eq '=>' } ).InputObject
+    }
+
+    $comparisonRemove = Compare-Object $existingRoleMembership ([array]$roles['remove']) -IncludeEqual
+    if ($null -eq $comparisonRemove) {
+        $rolesToRemove = @()
+    }
+    else {
+        $rolesToRemove = ( $comparisonRemove | Where-Object { $_.SideIndicator -eq '==' } ).InputObject
+    }
+}


The module allows specifying roles.set simultaneously with roles.add or roles.remove, but the logic at line 123 gives precedence to roles.set, effectively ignoring any add or remove specifications. This could lead to confusing behavior where users expect add/remove to be applied in addition to set.

Consider either:

Making set mutually exclusive with add/remove in the parameter spec

Documenting that set takes precedence and add/remove are ignored when set is provided

Applying add/remove operations after the set operation

@harCamConsulting This makes sense to me, any of the options given seem good

Copilot · 2026-01-08T19:35:18Z

plugins/modules/user_role.py

+  description:
+    - If called with role, then data is output from the C(Remove-DbaDbRoleMember), (Get-DbaDbRoleMember), or C(Add-DbaDbRoleMember) functions.
+    - If called with roles, then data returned is roleMembership, which is an array of roles that the user is now a member of.
+    - If called without either role or roles, then the data returned is roleMembership which is the user's current list of roles.


The documentation states that when called without either role or roles, the data returned is roleMembership with the user's current list of roles. However, the code at lines 88-95 will fail with an error message when neither parameter is provided (in non-compatibility mode). This is inconsistent with the documented behavior shown in the test cases at lines 198 and 224.

The documentation should either be updated to remove this statement, or the code should be fixed to support query-only mode.

Suggested change

- If called without either role or roles, then the data returned is roleMembership which is the user's current list of roles.

plugins/modules/user_role.ps1

plugins/modules/user_role.py

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

harCamConsulting added 3 commits August 5, 2025 05:01

added ability for user_role to take a list for roles

3fd41b9

updated changelog

8386b2a

updated documentation for new option

40e634e

harCamConsulting mentioned this pull request Aug 5, 2025

[Request] user_role module should take a list of roles, and (optionally) remove users from the roles not in the list #321

Open

harCamConsulting added 2 commits August 6, 2025 01:36

change output of module so that unit testing can test on more things …

4c41f7b

…(breaking change?)

typo in unit tests

c8f0821

lowlydba requested review from Copilot and lowlydba August 16, 2025 21:17

Copilot AI reviewed Aug 16, 2025

View reviewed changes

plugins/modules/user_role.py Outdated Show resolved Hide resolved

plugins/modules/user_role.py Outdated Show resolved Hide resolved

plugins/modules/user_role.ps1 Outdated Show resolved Hide resolved

sqlserver Outdated Show resolved Hide resolved

lowlydba added 2 commits August 16, 2025 17:20

Merge branch 'main' into add-list-to-dbroles

d9ffdad

Merge branch 'main' into add-list-to-dbroles

b3693bd

harCamConsulting and others added 13 commits August 26, 2025 03:11

refactor to implement add/remove/set functionality

62db4fa

Merge branch 'add-list-to-dbroles' of github.com:harCamConsulting/low…

3028fe7

…lydba.sqlserver into add-list-to-dbroles

Merge branch 'lowlydba:main' into add-list-to-dbroles

3a47ebc

removing random symlink

97b68c7

remove unused param, plus linting

d66a924

fix typo in docs

5c2ab58

fix typo in tests

90783b1

updating for more linting / style

ceb87e6

roleMembership typo

fca53e7

typos

dd8946d

defaults in doc to match code

832774c

change to other type of foreach

d85b0a4

alter results so it returns empty arrays instead of null

16ae9fa

lowlydba and others added 4 commits September 5, 2025 13:14

Merge branch 'main' into add-list-to-dbroles

88f5578

use proper array casts

e33a412

Merge branch 'add-list-to-dbroles' of github.com:harCamConsulting/low…

4b0dea4

…lydba.sqlserver into add-list-to-dbroles

catch null and convert to empty array on output rolemembership

0fcedae

Merge branch 'main' into add-list-to-dbroles

504b80e

lowlydba requested a review from Copilot January 8, 2026 18:53

Copilot started reviewing on behalf of lowlydba January 8, 2026 18:53 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

lowlydba and others added 4 commits January 8, 2026 14:05

Apply suggestions from code review

0fc0192

Co-authored-by: Copilot <[email protected]>

Apply suggestions from code review

d9f3485

Co-authored-by: Copilot <[email protected]>

Apply suggestions from code review

8eee960

Co-authored-by: Copilot <[email protected]>

Apply suggestions from code review

720455d

Co-authored-by: Copilot <[email protected]>

lowlydba requested a review from Copilot January 8, 2026 19:10

Copilot started reviewing on behalf of lowlydba January 8, 2026 19:10 View session

lowlydba reviewed Jan 8, 2026

View reviewed changes

plugins/modules/user_role.ps1 Outdated Show resolved Hide resolved

Copilot AI reviewed Jan 8, 2026

View reviewed changes

lowlydba reviewed Jan 8, 2026

View reviewed changes

plugins/modules/user_role.ps1 Outdated Show resolved Hide resolved

Apply suggestions from code review

29bccba

lowlydba reviewed Jan 8, 2026

View reviewed changes

plugins/modules/user_role.py Show resolved Hide resolved

lowlydba reviewed Jan 8, 2026

View reviewed changes

plugins/modules/user_role.py Show resolved Hide resolved

Apply suggestions from code review

28a3a2c

Co-authored-by: Copilot <[email protected]>

lowlydba requested a review from Copilot January 8, 2026 19:26

Copilot started reviewing on behalf of lowlydba January 8, 2026 19:26 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

-try {
+try {
+    # In non-compatibility (roles dict) mode, if no output was created earlier (e.g. query-only),
+    # return the current role membership without a diff.
+    if (-not $compatibilityMode -and $null -eq $output) {
+        try {
+            $membershipObjects = Get-DbaDbRoleMember @commonParamSplat -IncludeSystemUser $true | Where-Object { $_.UserName -eq $username }
+            $currentRoleMembership = [array]($membershipObjects.role | Sort-Object)
+            if ($null -eq $currentRoleMembership) { $currentRoleMembership = @() }
+        }
+        catch {
+            $module.FailJson("Failure getting current role membership: $($_.Exception.Message)", $_)
+        }
+        if ($null -eq $outputProps) { $outputProps = @{} }
+        $outputProps.roleMembership = $currentRoleMembership
+        $output = New-Object -TypeName PSCustomObject -Property $outputProps
+    }

Uh oh!

user_role: update role to take a list instead of string #324

Are you sure you want to change the base?

user_role: update role to take a list instead of string #324

Uh oh!

Conversation

harCamConsulting commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

codecov-commenter commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

harCamConsulting commented Aug 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lowlydba commented Aug 17, 2025

Uh oh!

harCamConsulting commented Aug 17, 2025

Uh oh!

harCamConsulting commented Aug 27, 2025

Uh oh!

lowlydba commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harCamConsulting commented Sep 9, 2025

Uh oh!

lowlydba commented Sep 19, 2025

Uh oh!

harCamConsulting commented Jan 4, 2026

Uh oh!

lowlydba commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

lowlydba Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

lowlydba Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

harCamConsulting commented Aug 5, 2025 •

edited

Loading

codecov-commenter commented Aug 5, 2025 •

edited

Loading

lowlydba commented Sep 5, 2025 •

edited

Loading

lowlydba commented Jan 8, 2026 •

edited

Loading