feat: Write roles fingerprints to /var/log/sysroles.jsonl - #178
feat: Write roles fingerprints to /var/log/sysroles.jsonl#178spetrosi wants to merge 10 commits into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe module now collects structured role fingerprints, formats escaped syslog records, optionally persists bounded JSONL records, supports check mode, and includes unit coverage. ChangesStructured fingerprint logging
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 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
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: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 230-231: Define and enforce an explicit oversized-record policy in
the JSONL write flow around the max_size check and _trim_log_file: either raise
a clear module error when a serialized row exceeds max_log_size, or
intentionally allow that single row without trimming and document the exception.
Ensure missing log files do not trigger _trim_log_file with a negative or
invalid target, preserve consistent behavior for existing files, and add a
regression test covering a missing log file whose limit is smaller than one row.
- Line 195: Rename the ambiguous loop variable l to line in the while condition
and any related expressions within the surrounding logic, preserving the
existing behavior of the size calculation and line processing.
🪄 Autofix
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: e93759fa-f9c4-4a1d-88e6-499762bed81c
📒 Files selected for processing (2)
playbooks/files/library/sr_fingerprint.pyplaybooks/files/tests/unit/test_sr_fingerprint.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 231-232: Update the append-size handling around _trim_log_file to
reject any new_line larger than positive max_size, including empty logs, with a
clear error; otherwise calculate the removal amount as cur_size + len(new_line)
- max_size so only the oldest records needed for the append are removed. Catch
this error in _handle_fingerprint and add a regression test covering an empty
log with a limit smaller than one JSONL row.
🪄 Autofix
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: 420ff308-86e8-48e9-bf61-1430ce494ea3
📒 Files selected for processing (2)
playbooks/files/library/sr_fingerprint.pyplaybooks/files/tests/unit/test_sr_fingerprint.py
🚧 Files skipped from review as they are similar to previous changes (1)
- playbooks/files/tests/unit/test_sr_fingerprint.py
* 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>
* Add max_log_lines defaulting to 10000 * Apply CodeRabbit review * Change the need for large ansible_facts to smaller distribution and distribution_version
cfb3adb to
ced463e
Compare
Add test_trim_multiple_lines Use re.match for older Python compatibility
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
playbooks/files/tests/unit/test_sr_fingerprint.py (1)
202-203: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify
max_log_sizein bytes.
len()measures Python characters, not UTF-8 bytes. These tests use only ASCII values. They can pass when the implementation exceeds the byte limit for a multibyte value.Add a record with multibyte text. Use
len(serialized.encode("utf-8"))for capacity setup. Assertos.path.getsize(log_file) <= max_sizeafter trimming.Also applies to: 228-231, 253-255
🤖 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 202 - 203, Update the affected tests around _format_fingerprint_jsonl and log trimming to include a record containing multibyte UTF-8 text, calculate max_size from the serialized record using len(serialized.encode("utf-8")), and assert os.path.getsize(log_file) <= max_size after trimming. Apply the same byte-based setup and file-size assertion to the additional referenced test cases.
🤖 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 283-287: Update _format_fingerprint_key_value to escape newline,
carriage-return, and tab characters in value before determining quoting and
formatting, preserving the existing quote escaping behavior. Add tests covering
values containing newline and tab characters.
---
Nitpick comments:
In `@playbooks/files/tests/unit/test_sr_fingerprint.py`:
- Around line 202-203: Update the affected tests around
_format_fingerprint_jsonl and log trimming to include a record containing
multibyte UTF-8 text, calculate max_size from the serialized record using
len(serialized.encode("utf-8")), and assert os.path.getsize(log_file) <=
max_size after trimming. Apply the same byte-based setup and file-size assertion
to the additional referenced test cases.
🪄 Autofix
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: a224b9bb-f745-4f4f-bc30-c191785c5c15
📒 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)
- playbooks/files/tests/unit/sr_fingerprint.py
| def _format_fingerprint_key_value(field, value): | ||
| text = "" if value is None else str(value) | ||
| if any(char in text for char in ' "='): | ||
| return '%s="%s"' % (field, text.replace('"', '""')) | ||
| return "%s=%s" % (field, text) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Escape control characters in syslog fields.
_format_fingerprint_key_value leaves newline, carriage-return, and tab characters unchanged. A parameter value with these characters can create malformed or multiple syslog records. Escape control characters before joining the key-value pairs. Add tests for newline and tab values.
🤖 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 283 - 287, Update
_format_fingerprint_key_value to escape newline, carriage-return, and tab
characters in value before determining quoting and formatting, preserving the
existing quote escaping behavior. Add tests covering values containing newline
and tab characters.
|
|
||
|
|
||
| def run_module(): | ||
| from ansible.module_utils.basic import AnsibleModule |
There was a problem hiding this comment.
this might cause problems with ansible-test or flake8 because they may expect imports to be done in a certain order and in a certain location
except - pass
649b1a8 to
06b408e
Compare
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