Add --diff support to some IPA modules - #1415
Open
NavidSassan wants to merge 2 commits into
Open
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 left some high level feedback:
- The diff tracking logic for present/absent/member actions in several modules (e.g. ipagroup, ipahostgroup, ipahbacrule, ipasudorule) is quite repetitive; consider extracting common patterns into small helpers (e.g. a function that takes
_orig, add/del lists, and the resource name) to reduce duplication and make future changes less error‑prone. - In ipauser, the member diff handling is asymmetric between
state: presentandstate: absent(e.g.manager_add/manager_del,principal_add/principal_del), which makes the code harder to reason about; consider harmonizing the use of add/del lists in both directions so the diff generation follows a consistent pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The diff tracking logic for present/absent/member actions in several modules (e.g. ipagroup, ipahostgroup, ipahbacrule, ipasudorule) is quite repetitive; consider extracting common patterns into small helpers (e.g. a function that takes `_orig`, add/del lists, and the resource name) to reduce duplication and make future changes less error‑prone.
- In ipauser, the member diff handling is asymmetric between `state: present` and `state: absent` (e.g. `manager_add`/`manager_del`, `principal_add`/`principal_del`), which makes the code harder to reason about; consider harmonizing the use of add/del lists in both directions so the diff generation follows a consistent pattern.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
NavidSassan
force-pushed
the
diff_support
branch
from
March 31, 2026 13:15
9f304df to
838af72
Compare
NavidSassan
force-pushed
the
diff_support
branch
from
March 31, 2026 15:05
838af72 to
4f56330
Compare
…rs_diff Address sourcery-ai feedback on PR freeipa#1415: * Extract the per-resource member-list diff loop, which was duplicated between state=present action=member and state=absent action=member, into a single specs-driven helper `gen_members_diff(res_find, specs)` in module_utils. Each spec is a tuple of `(member_key, add_list, del_list, current_source)`, where `current_source` may be a single attribute name in the IPA response, a list of attribute names (concatenated; for IPA fields that split one logical list, e.g. memberhost_host + externalhost), or a precomputed list. * Use the new helper in ipagroup, ipahbacrule, ipahostgroup, ipasudorule and ipasudocmdgroup. Each module now declares its member specs once and reuses them across the present/<resource>, present/member and absent/member branches. * In ipauser, harmonise the member-diff handling between state=present action=user, state=present action=member and state=absent action=member: all *_add / *_del lists are now initialised to [] at the top of the loop, each branch only fills the lists it actually touches, and a single post-state-branch block calls gen_members_diff() with the same member_specs in all three cases. The diff generation no longer flips between two patterns. No behaviour change. Net diff: +228 / -351 lines across 7 files.
|
@t-woerner @rjeffman would appreciate a review when you have a slot |
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
2 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.
This PR adds
--diffsupport to the following 8 modules:ipagroupipahbacruleipahostgroupipapwpolicyipasudocmdipasudocmdgroupipasudoruleipauser(since these are the ones that we are using for our LFOps Ansible Collection right now).
This partly fixes #483.
Summary by Sourcery
Add structured diff reporting for IPA modules to support Ansible --diff output when managing IPA objects and their memberships.
New Features:
Enhancements: