Repository navigation
feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #221
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:
📝 WalkthroughWalkthroughChangesThe module now collects structured role fingerprints instead of caller-provided messages. It formats syslog fields, optionally writes JSONL records with locking and trimming, and supports check-mode results. Unit tests cover formatting, persistence, validation, and failures. Fingerprint logging
Suggested reviewers: 🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/test_sr_fingerprint.py (4)
117-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining
_format_fingerprint_key_valuebranches.The tests cover the space-quoting branch. Three branches have no coverage:
- A value that contains
", which exercisestext.replace('"', '""').- A value that contains
=, which is the third trigger character.- A partial distro pair, such as
("RedHat", ""), which returns"unknown".The escaping branch is the part of the format that is most likely to change. A test pins the chosen convention.
💚 Proposed tests
def test_get_managed_node_distro_missing(self): self.assertEqual(sr_fingerprint._get_managed_node_distro("", ""), "unknown") + def test_get_managed_node_distro_partial(self): + self.assertEqual( + sr_fingerprint._get_managed_node_distro("RedHat", ""), "unknown" + ) + self.assertEqual(sr_fingerprint._get_managed_node_distro("", "9.4"), "unknown") + + 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"') +🤖 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 117 - 140, Add unit tests in the existing fingerprint test class covering the remaining _format_fingerprint_key_value branches: assert embedded double quotes are escaped by doubling them, assert values containing “=” use the quoted representation, and assert _get_managed_node_distro("RedHat", "") returns "unknown".
352-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an end-to-end test for the successful write path.
The tests cover
_write_jsonl_logalone, the check-mode handler paths, and the write-failure path. No test runs_handle_fingerprintwithcheck_mode=Falseandwrite_log_file: Trueagainst a real path.That combination is the primary path the PR adds. A test should assert that the file receives one parseable JSON row, and that
module.loggedcontains the key=value syslog line.💚 Proposed test
def test_handle_fingerprint_writes_log_and_syslog(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", "host2"], "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(result["fingerprint"]["status"], "success") self.assertEqual(len(module.logged), 1) self.assertIn("role_name=systemd", module.logged[0]) self.assertIn("play_hosts_number=2", module.logged[0]) with open(log_path, "r") as log_fd: lines = log_fd.read().splitlines() self.assertEqual(len(lines), 1) self.assertEqual(json.loads(lines[0]), 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 352 - 381, Add an end-to-end success-path test alongside test_handle_fingerprint_write_failure_calls_fail_json that invokes _handle_fingerprint with check_mode=False and write_log_file enabled using a real temporary path. Assert the successful exit payload, one logged key=value syslog line containing role_name and host count, and exactly one parseable JSON row whose contents match the returned fingerprint; clean up the temporary log file afterward.
195-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a trim test that starts with a file already above
max_log_size.The trim tests only exercise a file that is at or below the limit. They pass both with the current delta-based trim and with a target-based trim, so they do not pin the size bound.
The uncovered case is the defect that I raised on
library/sr_fingerprint.pylines 191-217. Write many records withmax_size=0, then write one record with a smallmax_size, and assert that the resulting file size is not greater than thatmax_size.Both trim tests also assume that every record serializes to the same
line_size. That holds only because eachrole_nameis six characters. Add a comment, or assert the invariant, so a later change to the loop bound does not break the arithmetic silently.💚 Proposed regression test
def test_trim_disabled_when_zero(self):Add this test alongside the existing trim tests:
def test_trim_enforces_limit_when_file_starts_over_limit(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") # Grow the log well past the limit with trimming disabled. for _i in range(20): sr_fingerprint._write_jsonl_log(log_file, record, max_size=0) self.assertGreater(os.path.getsize(log_file), 5 * line_size) max_size = 5 * line_size sr_fingerprint._write_jsonl_log(log_file, record, max_size=max_size) self.assertLessEqual(os.path.getsize(log_file), max_size) 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 195 - 272, Add a regression test alongside the existing trim tests that writes many records with max_size=0, verifies the file exceeds the intended limit, then writes one record with a small max_size and asserts the resulting file size is at most that limit. Also document or assert the equal serialized-line-size assumption used by the existing role_name loops in test_trim_removes_oldest_lines and test_trim_multiple_lines.
133-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant line break from this assignment.
The parenthesized value is 89 characters and exceeds Black’s 88-character default line length, so this assignment should not create a wrapped Black-formatter diff.
♻️ Proposed change
def test_format_fingerprint_syslog_quotes_values_with_spaces(self): record = _sample_fingerprint_record() - record["role_path"] = ( - "/usr/share/ansible/roles/linux-system-roles.systemd extra" - ) + record["role_path"] = "/usr/share/ansible/roles/linux-system-roles.systemd extra" message = sr_fingerprint._format_fingerprint_syslog(record)🤖 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 133 - 135, In the test assignment to record["role_path"], remove the unnecessary parenthesized line break while preserving the exact string value; format it consistently with Black’s expected output.Source: Path instructions
🤖 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 `@library/sr_fingerprint.py`:
- Around line 191-217: Update the caller of _trim_log_file to pass the remaining
byte budget (max_log_size minus the incoming record size), and change
_trim_log_file to trim until the retained existing content fits that target
rather than removing only a delta. Preserve the file replacement behavior, and
update the related tests in test_sr_fingerprint.py to assert the maximum-size
contract, including already oversized files and a zero-size prior limit.
---
Nitpick comments:
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 117-140: Add unit tests in the existing fingerprint test class
covering the remaining _format_fingerprint_key_value branches: assert embedded
double quotes are escaped by doubling them, assert values containing “=” use the
quoted representation, and assert _get_managed_node_distro("RedHat", "") returns
"unknown".
- Around line 352-381: Add an end-to-end success-path test alongside
test_handle_fingerprint_write_failure_calls_fail_json that invokes
_handle_fingerprint with check_mode=False and write_log_file enabled using a
real temporary path. Assert the successful exit payload, one logged key=value
syslog line containing role_name and host count, and exactly one parseable JSON
row whose contents match the returned fingerprint; clean up the temporary log
file afterward.
- Around line 195-272: Add a regression test alongside the existing trim tests
that writes many records with max_size=0, verifies the file exceeds the intended
limit, then writes one record with a small max_size and asserts the resulting
file size is at most that limit. Also document or assert the equal
serialized-line-size assumption used by the existing role_name loops in
test_trim_removes_oldest_lines and test_trim_multiple_lines.
- Around line 133-135: In the test assignment to record["role_path"], remove the
unnecessary parenthesized line break while preserving the exact string value;
format it consistently with Black’s expected output.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0708e83d-af80-493f-912a-119088d767fb
📒 Files selected for processing (2)
library/sr_fingerprint.pytests/unit/test_sr_fingerprint.py
| def _trim_log_file(log_file, size_needed): | ||
| """Remove oldest records until the file can accommodate size_needed bytes.""" | ||
| with open(log_file, "r") as log_fd: | ||
| lines = log_fd.readlines() | ||
| size_removed = 0 | ||
| while lines and size_removed < size_needed: | ||
| size_removed += len(lines.pop(0)) | ||
| orig_stat = os.stat(log_file) | ||
| dir_name = os.path.dirname(log_file) or "." | ||
| fd, tmp_path = tempfile.mkstemp(dir=dir_name, suffix=".tmp") | ||
| try: | ||
| os.fchmod(fd, stat.S_IMODE(orig_stat.st_mode)) | ||
| try: | ||
| os.fchown(fd, orig_stat.st_uid, orig_stat.st_gid) | ||
| except OSError: | ||
| pass | ||
| with os.fdopen(fd, "w") as tmp_fd: | ||
| tmp_fd.writelines(lines) | ||
| tmp_fd.flush() | ||
| os.fsync(tmp_fd.fileno()) | ||
| os.rename(tmp_path, log_file) | ||
| except BaseException: | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: | ||
| pass | ||
| raise |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Trimming does not enforce max_log_size when the file already exceeds it.
_trim_log_file removes only size_needed bytes, which the caller sets to len(new_line). This keeps the file at its current size, and it does not reduce the file to max_size.
If the file already exceeds max_log_size, the file stays above the limit forever. Each call then removes about one line and appends one line. Two supported configurations reach this state:
- An operator lowers
max_log_sizebetween runs. - An operator runs with
max_log_size: 0first, then sets a limit later.
The documented contract is "Maximum log file size in bytes". Pass a target size instead of a delta, and trim until the file plus the new record fits.
🐛 Proposed fix: trim to a target size
-def _trim_log_file(log_file, size_needed):
- """Remove oldest records until the file can accommodate size_needed bytes."""
+def _trim_log_file(log_file, size_allowed):
+ """Remove oldest records until the retained records fit in size_allowed bytes."""
with open(log_file, "r") as log_fd:
lines = log_fd.readlines()
- size_removed = 0
- while lines and size_removed < size_needed:
- size_removed += len(lines.pop(0))
+ size_kept = sum(len(line) for line in lines)
+ while lines and size_kept > size_allowed:
+ size_kept -= len(lines.pop(0))
orig_stat = os.stat(log_file)Update the caller to pass the remaining budget:
if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0:
- _trim_log_file(log_file, len(new_line))
+ _trim_log_file(log_file, max_size - len(new_line))Note that tests/unit/test_sr_fingerprint.py asserts the current delta behavior at lines 195-272. Update those tests with the fix.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 192-192: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(log_file, "r")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 191 - 217, Update the caller of
_trim_log_file to pass the remaining byte budget (max_log_size minus the
incoming record size), and change _trim_log_file to trim until the retained
existing content fits that target rather than removing only a delta. Preserve
the file replacement behavior, and update the related tests in
test_sr_fingerprint.py to assert the maximum-size contract, including already
oversized files and a zero-size prior limit.
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>
e042a2d to
889aca7
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