ipagroup: Use PARAM_MAPPING and query state support - #1427
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="README-group.md" line_range="403" />
<code_context>
`idoverrideuser` | List of user ID overrides to manage. Only usable with IPA versions 4.8.7 and up. Requires "server" context. | no
-`rename` \| `new_name` | Rename the user object to the new name string. Only usable with `state: renamed`. | no
+`rename` \| `new_name` | Rename the group object to the new name string. Only usable with `state: renamed`. | no
`action` | Work on group or member level. It can be on of `member` or `group` and defaults to `group`. | no
-`state` | The state to ensure. It can be one of `present`, `absent` or `renamed`, default: `present`. | yes
+`query_param` | The fields to query with `state: query`. Can be `ALL`, `BASE`, `PKEY_ONLY` or a list of specific field names. Only usable with `state: query`. | no
</code_context>
<issue_to_address>
**issue (typo):** Typo in the `action` description: use "one of" instead of "on of".
Please update the `action` description to say "It can be one of `member` or `group`" to correct the typo.
```suggestion
`action` | Work on group or member level. It can be one of `member` or `group` and defaults to `group`. | no
```
</issue_to_address>
### Comment 2
<location path="plugins/modules/ipagroup.py" line_range="520" />
<code_context>
+ group_params["posix"] = not nonposix
+
+
+PARAM_MAPPING = {
+ # Read-only system fields
+ "dn": {"return_only": True},
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new query handling and PARAM_MAPPING/member abstractions to separate concerns, making the control flow and metadata easier to understand and maintain.
The added `PARAM_MAPPING` / query / member-handling abstractions are powerful but pack multiple concerns into shared structures and branches, which raises cognitive load. You can keep all functionality and simplify in two focused areas:
---
### 1. Separate query control flow and conversion
`main()` now mixes query setup/validation with the present/absent/renamed flow, and `query_convert_result` embeds per‑field rules. You can move query logic into a single helper and narrow `query_convert_result` to be mostly structural, with dedicated per‑field converters.
**Before (simplified):**
```python
with ansible_module.ipa_connect(context=context):
if state == "query":
exit_args = ansible_module.execute_query(
names, query_param, group_find, query_param_settings,
convert_result=lambda res: query_convert_result(
ansible_module, res)
)
ansible_module.exit_json(changed=False, group=exit_args)
...
```
**After:**
```python
def run_group_query(ansible_module, names, query_param,
query_param_settings):
exit_args = ansible_module.execute_query(
names, query_param, group_find, query_param_settings,
convert_result=lambda res: query_convert_result(res),
)
ansible_module.exit_json(changed=False, group=exit_args)
def query_convert_result(res):
def convert_scalar(value, key):
if key == "gidnumber":
# field-specific conversion in a dedicated place
return int(value)
return to_text(value)
result = {}
for key, value in res.items():
if key.startswith("member_") or key.startswith("membermanager_"):
result[key] = [to_text(v) for v in value]
elif isinstance(value, (list, tuple)):
if len(value) == 1:
result[key] = convert_scalar(value[0], key)
else:
result[key] = [convert_scalar(v, key) for v in value]
else:
result[key] = convert_scalar(value, key)
return result
```
And in `main()`:
```python
with ansible_module.ipa_connect(context=context):
if state == "query":
run_group_query(
ansible_module, names, query_param, query_param_settings
)
# non-query path continues unchanged
```
This keeps all behavior but isolates query handling and makes per‑field conversions explicit in one helper.
---
### 2. Split parameter mapping responsibilities
`PARAM_MAPPING` currently mixes:
- API field name mapping (`api_name`)
- type info (`type`)
- query-only/read-only flags (`query`, `return_only`)
- member semantics (`member`, `lowercase`)
- module-level parameters (`module_param`)
You can reduce complexity by splitting member-related metadata into a separate mapping used only for member list computation, while keeping `PARAM_MAPPING` focused on “group attributes” (creation/update/query fields).
**Before (simplified):**
```python
PARAM_MAPPING = {
"name": {"api_name": "cn", "gen_args": False},
"description": {},
"gid": {"api_name": "gidnumber", "type": "int"},
...
"user": {"api_name": "member_user", "gen_args": False,
"lowercase": True, "member": True},
"group": {"api_name": "member_group", "gen_args": False,
"lowercase": True, "member": True},
"service": {"api_name": "member_service", "gen_args": False,
"lowercase": True, "member": True},
...
}
...
member_lists = gen_member_add_del_lists(
PARAM_MAPPING, group_params, res_find or {}, action, state
)
```
**After (conceptual split):**
```python
PARAM_MAPPING = {
# group attributes – used by gen_args_from_mapping, query_param_settings
"name": {"api_name": "cn", "gen_args": False},
"description": {},
"gid": {"api_name": "gidnumber", "type": "int"},
"nonposix": {"gen_args": False, "query": False},
"external": {"gen_args": False, "query": False},
"posix": {"gen_args": False, "query": False},
"nomembers": {"query": False},
"rename": {"gen_args": False, "query": False},
"query_param": {"module_param": True},
# no `member` / `lowercase` flags here
}
MEMBER_MAPPING = {
"user": {"api_name": "member_user", "lowercase": True},
"group": {"api_name": "member_group", "lowercase": True},
"service": {"api_name": "member_service", "lowercase": True},
"membermanager_user": {"api_name": "membermanager_user",
"lowercase": True},
"membermanager_group": {"api_name": "membermanager_group",
"lowercase": True},
"externalmember": {"api_name": "ipaexternalmember"},
"idoverrideuser": {"api_name": "member_idoverrideuser"},
}
```
Then adjust member computation to use the dedicated mapping:
```python
member_lists = gen_member_add_del_lists(
MEMBER_MAPPING, group_params, res_find or {}, action, state
)
user_add, user_del = member_lists.get("user", ([], []))
group_add, group_del = member_lists.get("group", ([], []))
service_add, service_del = member_lists.get("service", ([], []))
...
```
`build_query_param_settings` can continue to use `PARAM_MAPPING` (group attributes), while member-specific behavior is clearly separated and easier to reason about.
This keeps the generic helpers and new query feature, but reduces the number of concerns encoded in a single mapping and makes the member-related flow more readable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| `idoverrideuser` | List of user ID overrides to manage. Only usable with IPA versions 4.8.7 and up. Requires "server" context. | no | ||
| `rename` \| `new_name` | Rename the user object to the new name string. Only usable with `state: renamed`. | no | ||
| `rename` \| `new_name` | Rename the group object to the new name string. Only usable with `state: renamed`. | no | ||
| `action` | Work on group or member level. It can be on of `member` or `group` and defaults to `group`. | no |
There was a problem hiding this comment.
issue (typo): Typo in the action description: use "one of" instead of "on of".
Please update the action description to say "It can be one of member or group" to correct the typo.
Suggested change
| `action` | Work on group or member level. It can be on of `member` or `group` and defaults to `group`. | no | |
| `action` | Work on group or member level. It can be one of `member` or `group` and defaults to `group`. | no |
| group_params["posix"] = not nonposix | ||
|
|
||
|
|
||
| PARAM_MAPPING = { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the new query handling and PARAM_MAPPING/member abstractions to separate concerns, making the control flow and metadata easier to understand and maintain.
The added PARAM_MAPPING / query / member-handling abstractions are powerful but pack multiple concerns into shared structures and branches, which raises cognitive load. You can keep all functionality and simplify in two focused areas:
1. Separate query control flow and conversion
main() now mixes query setup/validation with the present/absent/renamed flow, and query_convert_result embeds per‑field rules. You can move query logic into a single helper and narrow query_convert_result to be mostly structural, with dedicated per‑field converters.
Before (simplified):
with ansible_module.ipa_connect(context=context):
if state == "query":
exit_args = ansible_module.execute_query(
names, query_param, group_find, query_param_settings,
convert_result=lambda res: query_convert_result(
ansible_module, res)
)
ansible_module.exit_json(changed=False, group=exit_args)
...After:
def run_group_query(ansible_module, names, query_param,
query_param_settings):
exit_args = ansible_module.execute_query(
names, query_param, group_find, query_param_settings,
convert_result=lambda res: query_convert_result(res),
)
ansible_module.exit_json(changed=False, group=exit_args)
def query_convert_result(res):
def convert_scalar(value, key):
if key == "gidnumber":
# field-specific conversion in a dedicated place
return int(value)
return to_text(value)
result = {}
for key, value in res.items():
if key.startswith("member_") or key.startswith("membermanager_"):
result[key] = [to_text(v) for v in value]
elif isinstance(value, (list, tuple)):
if len(value) == 1:
result[key] = convert_scalar(value[0], key)
else:
result[key] = [convert_scalar(v, key) for v in value]
else:
result[key] = convert_scalar(value, key)
return resultAnd in main():
with ansible_module.ipa_connect(context=context):
if state == "query":
run_group_query(
ansible_module, names, query_param, query_param_settings
)
# non-query path continues unchangedThis keeps all behavior but isolates query handling and makes per‑field conversions explicit in one helper.
2. Split parameter mapping responsibilities
PARAM_MAPPING currently mixes:
- API field name mapping (
api_name) - type info (
type) - query-only/read-only flags (
query,return_only) - member semantics (
member,lowercase) - module-level parameters (
module_param)
You can reduce complexity by splitting member-related metadata into a separate mapping used only for member list computation, while keeping PARAM_MAPPING focused on “group attributes” (creation/update/query fields).
Before (simplified):
PARAM_MAPPING = {
"name": {"api_name": "cn", "gen_args": False},
"description": {},
"gid": {"api_name": "gidnumber", "type": "int"},
...
"user": {"api_name": "member_user", "gen_args": False,
"lowercase": True, "member": True},
"group": {"api_name": "member_group", "gen_args": False,
"lowercase": True, "member": True},
"service": {"api_name": "member_service", "gen_args": False,
"lowercase": True, "member": True},
...
}
...
member_lists = gen_member_add_del_lists(
PARAM_MAPPING, group_params, res_find or {}, action, state
)After (conceptual split):
PARAM_MAPPING = {
# group attributes – used by gen_args_from_mapping, query_param_settings
"name": {"api_name": "cn", "gen_args": False},
"description": {},
"gid": {"api_name": "gidnumber", "type": "int"},
"nonposix": {"gen_args": False, "query": False},
"external": {"gen_args": False, "query": False},
"posix": {"gen_args": False, "query": False},
"nomembers": {"query": False},
"rename": {"gen_args": False, "query": False},
"query_param": {"module_param": True},
# no `member` / `lowercase` flags here
}
MEMBER_MAPPING = {
"user": {"api_name": "member_user", "lowercase": True},
"group": {"api_name": "member_group", "lowercase": True},
"service": {"api_name": "member_service", "lowercase": True},
"membermanager_user": {"api_name": "membermanager_user",
"lowercase": True},
"membermanager_group": {"api_name": "membermanager_group",
"lowercase": True},
"externalmember": {"api_name": "ipaexternalmember"},
"idoverrideuser": {"api_name": "member_idoverrideuser"},
}Then adjust member computation to use the dedicated mapping:
member_lists = gen_member_add_del_lists(
MEMBER_MAPPING, group_params, res_find or {}, action, state
)
user_add, user_del = member_lists.get("user", ([], []))
group_add, group_del = member_lists.get("group", ([], []))
service_add, service_del = member_lists.get("service", ([], []))
...build_query_param_settings can continue to use PARAM_MAPPING (group attributes), while member-specific behavior is clearly separated and easier to reason about.
This keeps the generic helpers and new query feature, but reduces the number of concerns encoded in a single mapping and makes the member-related flow more readable.
t-woerner
force-pushed
the
ipagroup_query
branch
from
July 14, 2026 06:06
e44ee02 to
be9f710
Compare
rjeffman
approved these changes
Jul 15, 2026
Comment on lines
+620
to
+623
| query_param=dict(type="list", elements="str", default=None, | ||
| choices=["ALL", "BASE", "PKEY_ONLY"] | ||
| + query_param_settings["ALL"], | ||
| required=False), |
Member
There was a problem hiding this comment.
I'm wondering if in the "near future" this could be part of the IPAANsibleModule contractor, rather that defined in each module. (Not today!)
varunmylaraiah
requested changes
Jul 15, 2026
Collaborator
There was a problem hiding this comment.
@t-woerner Thanks for the PR. I noticed a regression of RHEL-70023. Removing external users from a group is failing again.
The ipagroup module has been reworked to use the new PARAM_MAPPING
added to ansible_freeipa_module.
The member handling for user, group, service and membermanager has
been simplified by using gen_member_add_del_lists. The member entries
in PARAM_MAPPING are now marked with "member": True. This replaces the
manual calls to gen_add_del_lists, gen_add_list and
gen_intersection_list across separate action/state branches with a
single unified call. externalmember and idoverrideuser are still
handled manually since they need SID-based comparison.
The new query state allows to retrieve group information from IPA.
The query_param option controls which fields are returned: BASE for
essential fields, ALL for all fields, PKEY_ONLY for group names only,
or a list of specific field names.
Here is the updated documentation of the module:
README-group.md
New tests for the query state can be found at:
tests/group/test_group_query.yml
t-woerner
force-pushed
the
ipagroup_query
branch
from
July 17, 2026 10:49
be9f710 to
0e2e6a6
Compare
varunmylaraiah
approved these changes
Jul 18, 2026
varunmylaraiah
left a comment
varunmylaraiah
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. All downstream tests passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ipagroup module has been reworked to use the new PARAM_MAPPING added to ansible_freeipa_module.
The member handling for user, group, service and membermanager has been simplified by using gen_member_add_del_lists. The member entries in PARAM_MAPPING are now marked with "member": True. This replaces the manual calls to gen_add_del_lists, gen_add_list and gen_intersection_list across separate action/state branches with a single unified call. externalmember and idoverrideuser are still handled manually since they need SID-based comparison.
The new query state allows to retrieve group information from IPA. The query_param option controls which fields are returned: BASE for essential fields, ALL for all fields, PKEY_ONLY for group names only, or a list of specific field names.
Here is the updated documentation of the module:
New tests for the query state can be found at:
Summary by Sourcery
Add query support to the ipagroup module and refactor group parameter and member handling to use centralized PARAM_MAPPING.
New Features:
Enhancements:
Documentation:
Tests: