feat: Write roles fingerprints to /var/log/sysroles.jsonl - #178
Conversation
* Extend the sr_fingerprint module to write syslog to /var/log/sysroles.jsonl in addition to writing them to syslog. * Add unit test for sr_fingerprint.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 24 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe module now collects role fingerprints, formats syslog key-value records, optionally persists bounded JSONL records, supports check mode, and includes comprehensive unit coverage. ChangesStructured fingerprint logging
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
playbooks/files/library/sr_fingerprint.py (2)
211-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider making
run_moduleeasier to unit test.
run_modulebuildsAnsibleModuleinternally, so the write-failure path (fail_jsonon Lines 247-251) and the check-mode preview branch (Lines 230-239) are not exercised bytest_sr_fingerprint.py, only the extracted helper functions are. Accepting an injected module instance (or splitting the check-mode/write logic into a testable helper) would let tests cover thefail_jsonpath, which is the module's main failure mode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playbooks/files/library/sr_fingerprint.py` around lines 211 - 256, Refactor run_module so its AnsibleModule dependency can be injected, or extract the check-mode and JSONL write handling into a separately callable helper. Preserve the existing check-mode preview behavior and ensure tests can exercise the _write_jsonl_log failure path through module.fail_json.
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the actual return values.
RETURNis an empty placeholder, but the module now returnsfingerprint, and conditionallymessage,jsonl_row, andlog_file(Lines 231-239, 256). Document these fields soansible-docand downstream consumers ofregister:output can see the real return contract.♻️ Suggested RETURN documentation
-RETURN = r""" # """ +RETURN = r""" +fingerprint: + description: The canonical fingerprint record that was logged. + type: dict + returned: always +message: + description: Human-readable preview of the syslog line (check mode only). + type: str + returned: when check mode is enabled +jsonl_row: + description: The JSON line that would be (or was) appended to the log file. + type: str + returned: when write_log_file is true +log_file: + description: Path to the JSONL log file. + type: str + returned: when write_log_file is true +"""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playbooks/files/library/sr_fingerprint.py` at line 84, Replace the placeholder RETURN documentation in the sr_fingerprint module with a complete description of the actual return contract: always document fingerprint, and document the conditional message, jsonl_row, and log_file fields, including when each is produced, so ansible-doc and registered results expose these values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inventory/group_vars/active_roles.yml`:
- Line 12: Update the entry in present_files to reference
tests/unit/test_sr_fingerprint.py instead of the .yml path, matching the actual
added test file and preserving the exact path expected by file-sync automation.
In `@playbooks/files/library/sr_fingerprint.py`:
- Around line 175-188: Preserve the actual distribution values returned by
`_collect_fingerprint_record` and its `managed_node_distro` field. Do not mark
the entire `ansible_facts` input as `no_log=True`; instead, pass only the fields
consumed by `_get_managed_node_distro` as separate parameters while retaining
the existing record-building flow.
---
Nitpick comments:
In `@playbooks/files/library/sr_fingerprint.py`:
- Around line 211-256: Refactor run_module so its AnsibleModule dependency can
be injected, or extract the check-mode and JSONL write handling into a
separately callable helper. Preserve the existing check-mode preview behavior
and ensure tests can exercise the _write_jsonl_log failure path through
module.fail_json.
- Line 84: Replace the placeholder RETURN documentation in the sr_fingerprint
module with a complete description of the actual return contract: always
document fingerprint, and document the conditional message, jsonl_row, and
log_file fields, including when each is produced, so ansible-doc and registered
results expose these values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2eb2986c-d66c-4fdc-9d50-f2c7b1cde1aa
📒 Files selected for processing (3)
inventory/group_vars/active_roles.ymlplaybooks/files/library/sr_fingerprint.pyplaybooks/files/tests/unit/test_sr_fingerprint.py
* Add max_log_lines defaulting to 10000 * Apply CodeRabbit review * Change the need for large ansible_facts to smaller distribution and distribution_version
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
playbooks/files/tests/unit/test_sr_fingerprint.py (2)
251-271: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid the hardcoded
/tmppath to prevent a lint failure.Lines 256 and 269 use the literal
"/tmp/test.jsonl". Static analysis flags this as CWE-377 (Ruff S108, ast-grephardcoded-tmp-file) at the error level. The test never opens this file, becausecheck_mode=Truemakes_handle_fingerprintreturn before the write branch runs. Even so, the literal path still triggers the lint rule and can fail the pipeline.Derive the path with
tempfile.gettempdir()instead of a literal/tmpstring.🔧 Proposed fix
def test_handle_fingerprint_check_mode_with_log_file(self): + log_file = os.path.join(tempfile.gettempdir(), "test.jsonl") module = _FakeModule( { "status": "success", "write_log_file": True, - "log_file": "/tmp/test.jsonl", + "log_file": log_file, "role_name": "systemd", "role_path": "/usr/share/ansible/roles/systemd", "ansible_play_hosts_all": ["host1"], "distribution": "RedHat", "distribution_version": "9.4", }, check_mode=True, ) with self.assertRaises(_ExitJsonException) as ctx: sr_fingerprint._handle_fingerprint(module) result = ctx.exception.kwargs self.assertIn("jsonl_row", result) - self.assertEqual(result["log_file"], "/tmp/test.jsonl") + self.assertEqual(result["log_file"], log_file)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playbooks/files/tests/unit/test_sr_fingerprint.py` around lines 251 - 271, Replace the hardcoded “/tmp/test.jsonl” values in test_handle_fingerprint_check_mode_with_log_file with a path derived from tempfile.gettempdir(), and use the same derived path for both the fake module input and the assertion. Preserve the existing test behavior and add or reuse the tempfile import as needed.Source: Linters/SAST tools
273-291: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the fingerprint write-failure test independent from non-root execution.
This test uses
/nonexistent/deep/pathand expects directory creation or file opening to fail. As root, the write can succeed, so the expected failure path is not covered. Mock the failure, such asbuiltins.openor_ensure_parent_dir, to make the assertion deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playbooks/files/tests/unit/test_sr_fingerprint.py` around lines 273 - 291, Update test_handle_fingerprint_write_failure_calls_fail_json to mock the file-writing failure deterministically, using builtins.open or _ensure_parent_dir, instead of relying on the /nonexistent/deep/path filesystem behavior. Preserve the existing _FailJsonException assertion and failure-message validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playbooks/files/library/sr_fingerprint.py`:
- Around line 186-200: Update _write_jsonl_log and _trim_log_file to hold a
cross-process lock across the append and optional trim operations. Perform
trimming by writing retained lines to a temporary file in the log’s directory,
flushing and closing it, then atomically replacing the original log; ensure
concurrent writers cannot overwrite rows added after their snapshot.
- Line 299: Validate max_log_lines before fingerprint handling so negative
values are rejected, while preserving 0 as the documented value that disables
trimming and positive values as valid limits.
---
Nitpick comments:
In `@playbooks/files/tests/unit/test_sr_fingerprint.py`:
- Around line 251-271: Replace the hardcoded “/tmp/test.jsonl” values in
test_handle_fingerprint_check_mode_with_log_file with a path derived from
tempfile.gettempdir(), and use the same derived path for both the fake module
input and the assertion. Preserve the existing test behavior and add or reuse
the tempfile import as needed.
- Around line 273-291: Update
test_handle_fingerprint_write_failure_calls_fail_json to mock the file-writing
failure deterministically, using builtins.open or _ensure_parent_dir, instead of
relying on the /nonexistent/deep/path filesystem behavior. Preserve the existing
_FailJsonException assertion and failure-message validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44120bfa-fa79-4e4c-ab39-285f4c54179c
📒 Files selected for processing (4)
inventory/group_vars/active_roles.ymlplaybooks/files/library/sr_fingerprint.pyplaybooks/files/tests/unit/sr_fingerprint.pyplaybooks/files/tests/unit/test_sr_fingerprint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- inventory/group_vars/active_roles.yml
|
I think using a max size in bytes rather than a max number of records might be easier to implement: |
| ) | ||
| record = sr_fingerprint._collect_fingerprint_record(module, "begin") | ||
| self.assertEqual(record["role_name"], "systemd") | ||
| self.assertEqual(record["role_path"], "/usr/share/ansible/roles/systemd") |
There was a problem hiding this comment.
| self.assertEqual(record["role_path"], "/usr/share/ansible/roles/systemd") | |
| self.assertEqual(record["role_path"], "/usr/share/ansible/roles/linux-system-roles.systemd") |
| module = _FakeModule( | ||
| { | ||
| "role_name": "systemd", | ||
| "role_path": "/usr/share/ansible/roles/systemd", |
There was a problem hiding this comment.
| "role_path": "/usr/share/ansible/roles/systemd", | |
| "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", |
| self.assertEqual( | ||
| message, | ||
| "date=2026-06-10T12:00:00+00:00 role_name=systemd " | ||
| "role_path=/usr/share/ansible/roles/systemd status=begin " |
There was a problem hiding this comment.
| "role_path=/usr/share/ansible/roles/systemd status=begin " | |
| "role_path=/usr/share/ansible/roles/linux-system-roles.systemd status=begin " |
| return { | ||
| "date": "2026-06-10T12:00:00+00:00", | ||
| "role_name": "systemd", | ||
| "role_path": "/usr/share/ansible/roles/systemd", |
There was a problem hiding this comment.
| "role_path": "/usr/share/ansible/roles/systemd", | |
| "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", |
|
|
||
| def test_format_fingerprint_syslog_quotes_values_with_spaces(self): | ||
| record = _sample_fingerprint_record() | ||
| record["role_path"] = "/usr/share/ansible/roles/systemd extra" |
There was a problem hiding this comment.
| record["role_path"] = "/usr/share/ansible/roles/systemd extra" | |
| record["role_path"] = "/usr/share/ansible/roles/linux-system-roles.systemd extra" |
| record = _sample_fingerprint_record() | ||
| record["role_path"] = "/usr/share/ansible/roles/systemd extra" | ||
| message = sr_fingerprint._format_fingerprint_syslog(record) | ||
| self.assertIn('role_path="/usr/share/ansible/roles/systemd extra"', message) |
There was a problem hiding this comment.
| self.assertIn('role_path="/usr/share/ansible/roles/systemd extra"', message) | |
| self.assertIn('role_path="/usr/share/ansible/roles/linux-system-roles.systemd extra"', message) |
| "write_log_file": False, | ||
| "max_log_lines": 10000, | ||
| "role_name": "systemd", | ||
| "role_path": "/usr/share/ansible/roles/systemd", |
There was a problem hiding this comment.
| "role_path": "/usr/share/ansible/roles/systemd", | |
| "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", |
| "log_file": log_path, | ||
| "max_log_lines": 10000, | ||
| "role_name": "systemd", | ||
| "role_path": "/usr/share/ansible/roles/systemd", |
There was a problem hiding this comment.
| "role_path": "/usr/share/ansible/roles/systemd", | |
| "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", |
| "log_file": log_path, | ||
| "max_log_lines": 10000, | ||
| "role_name": "systemd", | ||
| "role_path": "/usr/share/ansible/roles/systemd", |
There was a problem hiding this comment.
| "role_path": "/usr/share/ansible/roles/systemd", | |
| "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", |
| "write_log_file": False, | ||
| "max_log_lines": -1, | ||
| "role_name": "systemd", | ||
| "role_path": "/usr/share/ansible/roles/systemd", |
There was a problem hiding this comment.
| "role_path": "/usr/share/ansible/roles/systemd", | |
| "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", |
2 participants
Extend the sr_fingerprint module to write syslog to /var/log/sysroles.jsonl in addition to writing them to syslog.
Add unit test for sr_fingerprint.py
Summary by CodeRabbit