feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #641
Conversation
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe module now creates structured role fingerprints with metadata, emits canonical syslog text, and optionally writes JSONL records with locking and size trimming. Check mode returns results without writing. Unit tests cover collection, formatting, persistence, trimming, validation, and failures. ChangesStructured fingerprint logging
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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.
🧹 Nitpick comments (3)
library/sr_fingerprint.py (1)
283-287: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueEscape newlines in syslog values.
_format_fingerprint_key_valuequotes values that contain a space,", or=. It does not escape newline characters. If a value contains a newline, the syslog record splits into two lines and downstream line-based parsers break.Consider escaping control characters, or reuse
json.dumpsfor the quoted form so the escaping matches the JSONL output.♻️ Proposed escaping change
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('"', '""')) + if any(char in text for char in ' "=') or any( + char in text for char in "\r\n\t" + ): + return "%s=%s" % (field, json.dumps(text)) return "%s=%s" % (field, text)🤖 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 `@library/sr_fingerprint.py` around lines 283 - 287, Update _format_fingerprint_key_value to escape newline characters in values before formatting them, ensuring embedded newlines cannot split syslog records. Prefer the existing JSON-compatible escaping approach if available, while preserving the current unquoted output for values that require no quoting and the current quote-doubling behavior for embedded double quotes.tests/unit/test_sr_fingerprint.py (2)
306-402: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the non-check-mode success path.
The handler tests cover check mode, write failure, and negative
max_log_size. No test covers the normal path where_handle_fingerprintcallsmodule.logand appends the record to the log file._FakeModule.loggedat line 36 collects the syslog messages but no test reads it.This path is the main behavior described in the PR objectives.
💚 Proposed additional test
def test_handle_fingerprint_logs_and_writes_record(self): with tempfile.NamedTemporaryFile(delete=False, suffix=".jsonl") as tmp: log_path = tmp.name module = _FakeModule( { "status": "success", "write_log_file": True, "log_file": log_path, "max_log_size": 2000000, "role_name": "systemd", "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", "ansible_play_hosts_all": ["host1"], "distribution": "RedHat", "distribution_version": "9.4", }, check_mode=False, ) try: with self.assertRaises(_ExitJsonException) as ctx: sr_fingerprint._handle_fingerprint(module) result = ctx.exception.kwargs self.assertFalse(result["changed"]) self.assertEqual(len(module.logged), 1) self.assertIn("status=success", module.logged[0]) with open(log_path, "r") as log_fd: parsed = json.loads(log_fd.readline()) self.assertEqual(parsed, result["fingerprint"]) finally: _cleanup_log(log_path)🤖 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 `@tests/unit/test_sr_fingerprint.py` around lines 306 - 402, Add a unit test alongside the existing _handle_fingerprint tests for the non-check-mode success path with write_log_file enabled. Assert that _handle_fingerprint exits successfully, records one syslog message in _FakeModule.logged containing the status, and writes a JSONL record matching the returned fingerprint; clean up the temporary log file afterward.
131-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the quote and
=escaping branches.
test_format_fingerprint_syslog_quotes_values_with_spacescovers only the space case. The"doubling escape in_format_fingerprint_key_valueand the=trigger stay untested. These branches define the syslog wire format for downstream parsers.Also consider a test for the
_get_ansible_versionfallback when the module object has noansible_versionattribute.💚 Proposed additional tests
def test_format_fingerprint_key_value_escapes_quotes(self): pair = sr_fingerprint._format_fingerprint_key_value("role_name", 'a"b') self.assertEqual(pair, 'role_name="a""b"') def test_format_fingerprint_key_value_quotes_equals(self): pair = sr_fingerprint._format_fingerprint_key_value("role_name", "a=b") self.assertEqual(pair, 'role_name="a=b"') def test_get_ansible_version_falls_back_to_unknown(self): class _NoVersion(object): pass self.assertEqual( sr_fingerprint._get_ansible_version(_NoVersion()), "unknown" )🤖 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 `@tests/unit/test_sr_fingerprint.py` around lines 131 - 140, Add unit tests for `_format_fingerprint_key_value` covering doubled quote escaping and values containing `=`, asserting the exact syslog pairs. Also test `_get_ansible_version` with an object lacking `ansible_version`, verifying it returns `"unknown"`.
🤖 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.
Nitpick comments:
In `@library/sr_fingerprint.py`:
- Around line 283-287: Update _format_fingerprint_key_value to escape newline
characters in values before formatting them, ensuring embedded newlines cannot
split syslog records. Prefer the existing JSON-compatible escaping approach if
available, while preserving the current unquoted output for values that require
no quoting and the current quote-doubling behavior for embedded double quotes.
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 306-402: Add a unit test alongside the existing
_handle_fingerprint tests for the non-check-mode success path with
write_log_file enabled. Assert that _handle_fingerprint exits successfully,
records one syslog message in _FakeModule.logged containing the status, and
writes a JSONL record matching the returned fingerprint; clean up the temporary
log file afterward.
- Around line 131-140: Add unit tests for `_format_fingerprint_key_value`
covering doubled quote escaping and values containing `=`, asserting the exact
syslog pairs. Also test `_get_ansible_version` with an object lacking
`ansible_version`, verifying it returns `"unknown"`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ed9d1e8-0c2f-494c-8534-56338c9e5637
📒 Files selected for processing (2)
library/sr_fingerprint.pytests/unit/test_sr_fingerprint.py
spetrosi
changed the title
Feature: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] Reason: By default logs are printed to rsyslog. This change adds a possibility to write logs to a file on the system for the downstream users. Result: For the upstream, this makes rsyslog log message more detailed. For the downstream - also writes logs to /var/log/sysroles.jsonl Signed-off-by: Sergei Petrosian <spetrosi@redhat.com>
d435171 to
d18e88b
Compare
The sr_fingerprint module was rewritten to accept structured parameters (status, role_name, role_path, etc.) instead of a free-form sr_message. Update the role tasks and tests to match the new module interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[citest] |
2 participants
Feature: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip]
Reason: By default logs are printed to rsyslog. This change adds a possibility to write logs to a file on the system for the downstream users.
Result: For the upstream, this makes rsyslog log message more detailed. For the downstream - also writes logs to /var/log/sysroles.jsonl