feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #302
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 collects structured role fingerprints, formats them for syslog and JSONL, optionally persists records with locking and trimming, supports check mode, and validates write and size errors. Unit tests cover formatting, metadata, persistence, trimming, execution, and timestamps. 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 (5)
tests/unit/test_sr_fingerprint.py (4)
121-122: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the partial-distro cases.
_get_managed_node_distrousesif distribution and distribution_version, so it returns"unknown"when either value is empty. This test covers only the both-empty case. The single-empty cases are the realistic ones, because a fact-gathering failure usually drops one value and keeps the other.Adding them pins the
andsemantics. A future change toorwould otherwise pass this test while emittingRedHat-into the fingerprint.💚 Proposed additional assertions
def test_get_managed_node_distro_missing(self): self.assertEqual(sr_fingerprint._get_managed_node_distro("", ""), "unknown") + self.assertEqual( + sr_fingerprint._get_managed_node_distro("RedHat", ""), "unknown" + ) + self.assertEqual(sr_fingerprint._get_managed_node_distro("", "9.4"), "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 121 - 122, Add assertions to test_get_managed_node_distro_missing covering both partial-distro inputs: a non-empty distribution with an empty version and an empty distribution with a non-empty version. Verify each returns "unknown", preserving the current and semantics and guarding against emitting an incomplete fingerprint.
306-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the non-check-mode syslog path.
No test covers
_handle_fingerprintwithcheck_mode=Falseandwrite_log_file=False. That path callsmodule.log(log_message)at line 320 oflibrary/sr_fingerprint.pyand thenexit_json(changed=False, fingerprint=...)at line 333. It is the main runtime path for thebeginstatus, which the PR describes as syslog-only.
_FakeModule.logalready appends toself.logged, but no test asserts that attribute. The syslog emission is therefore unverified end to end.💚 Proposed additional test
def test_handle_fingerprint_logs_to_syslog_without_log_file(self): module = _FakeModule( { "status": "begin", "write_log_file": False, "max_log_size": 2000000, "role_name": "systemd", "role_path": "/usr/share/ansible/roles/linux-system-roles.systemd", "ansible_play_hosts_all": ["host1", "host2"], "distribution": "RedHat", "distribution_version": "9.4", }, check_mode=False, ) with self.assertRaises(_ExitJsonException) as ctx: sr_fingerprint._handle_fingerprint(module) self.assertEqual(len(module.logged), 1) message = module.logged[0] self.assertIn("role_name=systemd", message) self.assertIn("status=begin", message) self.assertIn("play_hosts_number=2", message) result = ctx.exception.kwargs self.assertFalse(result["changed"]) self.assertEqual(result["fingerprint"]["status"], "begin") self.assertNotIn("jsonl_row", result)🤖 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 - 326, Add a unit test alongside test_handle_fingerprint_check_mode_without_log_file covering _handle_fingerprint with check_mode=False and write_log_file=False. Assert exactly one message is appended to module.logged and includes role_name=systemd, status=begin, and play_hosts_number=2; then verify the exit result is unchanged, contains the begin fingerprint, and omits jsonl_row.
274-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a test for a record larger than
max_size.No test covers the case where a single serialized record exceeds
max_size. In that case_trim_log_filedrains every line through thewhile linesexhaustion condition, and_write_jsonl_logstill appends the oversized record. The resulting file exceedsmax_size.That behavior is reasonable, because the alternative is to drop the record. A test would pin it and document the intent for a misconfigured
max_log_size.💚 Proposed additional test
def test_record_larger_than_max_size_is_still_written(self): with tempfile.NamedTemporaryFile(delete=False, suffix=".jsonl") as tmp: log_file = tmp.name try: record = _sample_fingerprint_record() line_size = len(sr_fingerprint._format_fingerprint_jsonl(record) + "\n") sr_fingerprint._write_jsonl_log(log_file, record, max_size=line_size) # The next write cannot fit; every older record is removed. sr_fingerprint._write_jsonl_log(log_file, record, max_size=line_size // 2) with open(log_file, "r") as log_fd: lines = log_fd.read().splitlines() self.assertEqual(len(lines), 1) self.assertEqual(json.loads(lines[0]), record) finally: _cleanup_log(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 `@tests/unit/test_sr_fingerprint.py` around lines 274 - 288, Extend the test coverage in the existing fingerprint log tests with a case for _write_jsonl_log where the serialized record exceeds max_size. Verify that older entries are removed as needed but the oversized record is still written as the sole remaining line, and assert its decoded contents match the original record.
131-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun Black before committing.
tests/unit/test_sr_fingerprint.py:133-135has a parenthesized string assignment that can fit on one line. The project requires Python Black formatting, so runtox -e black,flake8and commit the formatter’s result.🤖 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, Run Black on tests/unit/test_sr_fingerprint.py, allowing the parenthesized role_path assignment in test_format_fingerprint_syslog_quotes_values_with_spaces to be collapsed onto one line. Verify the result with tox -e black,flake8.Source: Path instructions
library/sr_fingerprint.py (1)
328-331: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider widening the failure handling around the JSONL write.
The handler catches
(IOError, OSError)._trim_log_fileopens the existing log file in text mode with the locale default encoding. If the log file contains non-ASCII bytes that the locale codec rejects, Python raisesUnicodeDecodeError, which is aValueErrorand not anOSError. That exception escapes this handler and produces a raw traceback instead of a clean module failure.This module writes ASCII-only lines, because
json.dumpsdefaults toensure_ascii=True. The decode failure therefore requires an externally modified log file. If that happens, every later role run that writes fingerprints fails with a traceback.Two options: catch
ValueErrorhere as well, or read with an explicit encoding in_trim_log_file.♻️ Proposed handling for a corrupted log file
- except (IOError, OSError) as exc: + except (IOError, OSError, ValueError) as exc: module.fail_json( msg="Failed to write fingerprint log file %s: %s" % (log_file, exc) )Alternatively, make the read robust in
_trim_log_file(outside the selected range):with open(log_file, "r", encoding="utf-8", errors="replace") as log_fd: lines = log_fd.readlines()Note that the
encodingkeyword is Python 3 only. If this module must keep Python 2 support, prefer theValueErrordiff above.🤖 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 328 - 331, Widen the exception handling around the JSONL write in the fingerprint logging flow to include ValueError, so UnicodeDecodeError raised by _trim_log_file is converted into the existing module failure via module.fail_json. Preserve the current error message and IOError/OSError handling.
🤖 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 328-331: Widen the exception handling around the JSONL write in
the fingerprint logging flow to include ValueError, so UnicodeDecodeError raised
by _trim_log_file is converted into the existing module failure via
module.fail_json. Preserve the current error message and IOError/OSError
handling.
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 121-122: Add assertions to test_get_managed_node_distro_missing
covering both partial-distro inputs: a non-empty distribution with an empty
version and an empty distribution with a non-empty version. Verify each returns
"unknown", preserving the current and semantics and guarding against emitting an
incomplete fingerprint.
- Around line 306-326: Add a unit test alongside
test_handle_fingerprint_check_mode_without_log_file covering _handle_fingerprint
with check_mode=False and write_log_file=False. Assert exactly one message is
appended to module.logged and includes role_name=systemd, status=begin, and
play_hosts_number=2; then verify the exit result is unchanged, contains the
begin fingerprint, and omits jsonl_row.
- Around line 274-288: Extend the test coverage in the existing fingerprint log
tests with a case for _write_jsonl_log where the serialized record exceeds
max_size. Verify that older entries are removed as needed but the oversized
record is still written as the sole remaining line, and assert its decoded
contents match the original record.
- Around line 131-140: Run Black on tests/unit/test_sr_fingerprint.py, allowing
the parenthesized role_path assignment in
test_format_fingerprint_syslog_quotes_values_with_spaces to be collapsed onto
one line. Verify the result with tox -e black,flake8.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f15b7c7d-e77e-4489-b1a6-fef81713914f
📒 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>
ed81839 to
a2f54bf
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