Skip to content

cleanup: remove unused sanitize_encrypted param from diff function - #958

Open
VedantMadane wants to merge 2 commits into
ansible:develfrom
VedantMadane:cleanup/diff-unused-param
Open

cleanup: remove unused sanitize_encrypted param from diff function#958
VedantMadane wants to merge 2 commits into
ansible:develfrom
VedantMadane:cleanup/diff-unused-param

Conversation

@VedantMadane

@VedantMadane VedantMadane commented Mar 2, 2026

Copy link
Copy Markdown

Fixes #688. This PR removes the unused \sanitize_encrypted\ parameter from the \diff\ utility and its helper _sanitize_value, and corrects the return type in the docstring.

Summary by CodeRabbit

  • Refactor

    • Diff/comparison now always detects and masks encrypted field values, and the option to disable this behavior has been removed.
    • Encryption-aware sanitization is applied consistently to removed, added, and changed diff results.
  • Documentation

    • Updated documentation to clarify that diffs return a structured diff object rather than a generic dictionary.

@coderabbitai

coderabbitai Bot commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Sanitization for encrypted model fields now always evaluates encryption status via is_encrypted_field, instance _encrypted_field_names, and the $encrypted$ prefix. The sanitize_encrypted parameter was removed from _sanitize_value and diff function signatures. The diff docstring now correctly documents returning a ModelDiff object.

Changes

Models utility

Layer / File(s) Summary
_sanitize_value signature and always-apply masking
ansible_base/lib/utils/models.py
_sanitize_value signature drops the sanitize_encrypted parameter; the conditional early return guarded by that flag is removed so encryption/sensitivity masking is always applied based on field metadata, instance _encrypted_field_names, and the $encrypted$ prefix.
diff function contract, documentation, and sanitization calls
ansible_base/lib/utils/models.py
diff(...) signature removes the sanitize_encrypted parameter and its documentation; the :return: docstring now states it returns a ModelDiff object; internal calls to _sanitize_value are updated to the new 4-argument form.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: removing the unused sanitize_encrypted parameter from the diff function.
Linked Issues check ✅ Passed The pull request addresses both objectives from issue #688: removes the unused sanitize_encrypted parameter and updates the docstring to reflect the actual ModelDiff return type.
Out of Scope Changes check ✅ Passed All changes are directly related to the linked issue #688; no out-of-scope modifications are present in the pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@VedantMadane
VedantMadane force-pushed the cleanup/diff-unused-param branch from 7eee6ad to 3702eb5 Compare March 30, 2026 13:16
@VedantMadane

Copy link
Copy Markdown
Author

Rebased onto latest devel. I notice the DVCS check requires a Jira key (AAP-XXXXX). Is there an existing ticket this cleanup could be associated with, or would a maintainer be able to create one? Happy to update the PR title with the key.

@VedantMadane
VedantMadane force-pushed the cleanup/diff-unused-param branch from ca0f8d1 to f069909 Compare April 16, 2026 07:30
@VedantMadane
VedantMadane force-pushed the cleanup/diff-unused-param branch from f069909 to 8ed7a2b Compare June 8, 2026 13:38
@VedantMadane
VedantMadane force-pushed the cleanup/diff-unused-param branch from b749404 to ce64060 Compare June 21, 2026 16:18
@VedantMadane

VedantMadane commented Jun 28, 2026

Copy link
Copy Markdown
Author

Hi @relrod @AlanCoding , following up on this PR for #688.

The DVCS (Find Jira Key) check is still failing because it needs an internal Jira key.
As an external contributor I don't have access to create tickets on issues.redhat.com.

Could a maintainer please either create or link an AAP- Jira for this cleanup (drop the unused sanitize_encrypted param + docstring fix) or point me at an existing AAP ticket I should use?

I'll update the PR title to include the key as soon as I have it. Thanks!

@VedantMadane
VedantMadane force-pushed the cleanup/diff-unused-param branch from b72dc0d to 2097ccf Compare August 9, 2026 17:42
@VedantMadane

Copy link
Copy Markdown
Author

Re: CI failure — Check the PR for DVCS integration

This check failure is Ansible/Red Hat process policy, not a defect in the code change itself.

What failed

The DVCS (Find Jira Key) workflow runs ansible/dvcs-action and requires a real Jira key matching AAP-[0-9]+ in at least one of:

Location This PR
PR title cleanup: remove unused sanitize_encrypted param from diff functionno AAP key
Branch name cleanup/diff-unused-paramno AAP key
Commit message(s) cleanup: remove unused sanitize_encrypted param from diff functionno AAP key

Log confirms: allow_no_jira: false (repo workflow does not enable the NO_JIRA escape hatch).

Skip options checked

  • NO_JIRA in title/branch/commit: only works if the workflow passes allow_no_jira: truenot enabled here.
  • Labels: no no-jira / skip-style label exists on this repo for this check.
  • Draft PR: not treated as an exemption by the action.

Code vs process

How to clear the check (when ready)

Maintainers / contributors with access to Red Hat Jira need a real AAP-##### ticket, then put that key in the PR title, branch name, or a non-merge commit message (any one is enough per the action). Please do not invent placeholder keys (e.g. AAP-0000) just to green the check.

Leaving the PR open for maintainer guidance on filing/linking a real Jira issue if this contribution should proceed under DVCS.

VedantMadane and others added 2 commits August 18, 2026 10:04
This PR removes the unused 'sanitize_encrypted' parameter from the 'diff' utility function and its helper '_sanitize_value'.

Also updated the return docstring to correctly state that the function returns a 'ModelDiff' object instead of a dictionary.
diff() always sanitizes encrypted values after removing the unused
sanitize_encrypted parameter. Remove test_diff_no_sanitize_when_disabled
which still passed that kwarg. Remaining encrypt sanitization tests cover
class-level, instance-level, and value-prefix behavior.
@VedantMadane
VedantMadane force-pushed the cleanup/diff-unused-param branch from d5e402a to c08b3e9 Compare August 18, 2026 04:34
@github-actions

Copy link
Copy Markdown

DVCS PR Check Results:

Could not find JIRA key(s) in PR title, branch name, or commit messages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ansible_base.lib.utils.models.diff takes unused sanitize_encrypted param and has outdated return docstring

1 participant