feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #137
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, emits canonical syslog records, and optionally writes locked JSONL records with size trimming. It adds check-mode reporting, argument validation, write-error handling, and comprehensive unit tests. ChangesRole 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.
🧹 Nitpick comments (6)
library/sr_fingerprint.py (3)
322-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the validated local variable.
Line 300 already reads
max_log_sizefrommodule.params. Line 326 reads the same key again. Pass the local variable to keep one source for the validated value.♻️ Optional cleanup
- _write_jsonl_log( - log_file, fingerprint_record, module.params["max_log_size"] - ) + _write_jsonl_log(log_file, fingerprint_record, max_log_size)🤖 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 322 - 327, Update the _write_jsonl_log call in the write_log_file branch to pass the existing validated max_log_size local variable instead of rereading module.params["max_log_size"].
191-232: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTrim only frees the size of the new record.
_write_jsonl_logcalls_trim_log_file(log_file, len(new_line))._trim_log_filestops as soon as it removeslen(new_line)bytes. Ifcur_sizeis already abovemax_size, for example after an operator lowersmax_log_size, the file stays above the limit for many writes. Trimming to a target size instead converges in one pass.♻️ Optional: trim to the target size
- if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0: - _trim_log_file(log_file, len(new_line)) + if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0: + _trim_log_file(log_file, cur_size + len(new_line) - max_size)This changes the expectations in
tests/unit/test_sr_fingerprint.pyonly when the file starts above the limit.🤖 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 - 232, Update _trim_log_file and its call from _write_jsonl_log so trimming removes enough oldest records to bring the log within max_size after the new record is written, rather than removing only len(new_line) bytes. Pass or calculate the required excess based on the current size, max_size, and new record length, while preserving the existing behavior for files already within the limit.
283-287: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueQuote escaping uses the doubled-quote convention.
_format_fingerprint_key_valueescapes"as"". Mostkey=valuelog parsers, including the rsyslog and Logstashkvstyle used for this format, expect a backslash escape (\"). Arole_pathor distribution string that contains a quote then parses incorrectly downstream. Confirm which convention the downstream consumers of/var/log/sysroles.jsonland the syslog records use.♻️ Optional: use backslash escaping
- if any(char in text for char in ' "='): - return '%s="%s"' % (field, text.replace('"', '""')) + if any(char in text for char in ' "=\\'): + escaped = text.replace("\\", "\\\\").replace('"', '\\"') + return '%s="%s"' % (field, escaped)If you change this, update
test_format_fingerprint_syslog_quotes_values_with_spacesintests/unit/test_sr_fingerprint.py.🤖 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, The _format_fingerprint_key_value function uses doubled quotes, which downstream kv parsers may misinterpret. Confirm the escaping convention used by /var/log/sysroles.jsonl and syslog consumers, then use backslash escaping for embedded quotes if required; update test_format_fingerprint_syslog_quotes_values_with_spaces to match the confirmed behavior.tests/unit/test_sr_fingerprint.py (3)
161-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCleanup can mask the original failure.
If
_write_jsonl_lograises,subdirdoes not exist.os.listdir(subdir)in thefinallyblock then raises and hides the real error.shutil.rmtree(tmpdir)removes the tree and the lock sidecar in one call.♻️ Optional cleanup
finally: - subdir = os.path.dirname(log_file) - for name in os.listdir(subdir): - os.unlink(os.path.join(subdir, name)) - os.rmdir(subdir) - os.rmdir(tmpdir) + shutil.rmtree(tmpdir, ignore_errors=True)Add
import shutilto the imports.🤖 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 161 - 177, Update test_write_jsonl_log_creates_parent_dir cleanup to import shutil and replace the manual subdir/file removal with shutil.rmtree(tmpdir), ensuring cleanup succeeds even when _write_jsonl_log fails before creating the parent directory.
131-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
=and"escape branches.This test covers only a value that contains a space.
_format_fingerprint_key_valuealso quotes values that contain=or", and it doubles an embedded". That escape rule has no test, so a change to the convention passes silently.🧪 Suggested extra test
def test_format_fingerprint_syslog_escapes_quotes_and_equals(self): record = _sample_fingerprint_record() record["role_name"] = 'ro"le=x' message = sr_fingerprint._format_fingerprint_syslog(record) self.assertIn('role_name="ro""le=x"', message)🤖 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 a test alongside test_format_fingerprint_syslog_quotes_values_with_spaces that sets role_name to a value containing both an embedded double quote and an equals sign, then asserts _format_fingerprint_syslog outputs the value quoted with the embedded quote doubled, covering both escaping branches.
306-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a handler test for the non-check-mode success path.
The handler tests cover check mode, write failure, and argument validation. No test asserts that
_handle_fingerprintcallsmodule.logand returns thefingerprintvalue whencheck_modeis false. That path is the main behavior of the module._FakeModule.loggedalready records the calls.🧪 Suggested extra test
def test_handle_fingerprint_logs_to_syslog(self): module = _FakeModule( { "status": "success", "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"], "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) self.assertIn("status=success", module.logged[0]) self.assertEqual(ctx.exception.kwargs["fingerprint"]["status"], "success")🤖 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 - 351, Add a non-check-mode success test alongside the existing _handle_fingerprint tests, using _FakeModule with write_log_file disabled and status success. Invoke _handle_fingerprint, assert exactly one module.log call containing status=success, and verify the raised result includes fingerprint["status"] equal to success.
🤖 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 322-327: Update the _write_jsonl_log call in the write_log_file
branch to pass the existing validated max_log_size local variable instead of
rereading module.params["max_log_size"].
- Around line 191-232: Update _trim_log_file and its call from _write_jsonl_log
so trimming removes enough oldest records to bring the log within max_size after
the new record is written, rather than removing only len(new_line) bytes. Pass
or calculate the required excess based on the current size, max_size, and new
record length, while preserving the existing behavior for files already within
the limit.
- Around line 283-287: The _format_fingerprint_key_value function uses doubled
quotes, which downstream kv parsers may misinterpret. Confirm the escaping
convention used by /var/log/sysroles.jsonl and syslog consumers, then use
backslash escaping for embedded quotes if required; update
test_format_fingerprint_syslog_quotes_values_with_spaces to match the confirmed
behavior.
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 161-177: Update test_write_jsonl_log_creates_parent_dir cleanup to
import shutil and replace the manual subdir/file removal with
shutil.rmtree(tmpdir), ensuring cleanup succeeds even when _write_jsonl_log
fails before creating the parent directory.
- Around line 131-140: Add a test alongside
test_format_fingerprint_syslog_quotes_values_with_spaces that sets role_name to
a value containing both an embedded double quote and an equals sign, then
asserts _format_fingerprint_syslog outputs the value quoted with the embedded
quote doubled, covering both escaping branches.
- Around line 306-351: Add a non-check-mode success test alongside the existing
_handle_fingerprint tests, using _FakeModule with write_log_file disabled and
status success. Invoke _handle_fingerprint, assert exactly one module.log call
containing status=success, and verify the raised result includes
fingerprint["status"] equal to success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d80b59a5-34ce-4d08-bd66-0466e83c0ede
📒 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>
1937b96 to
f4fd91e
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] |
The fingerprint commit inadvertently replaced this file with a minimal template, losing role-specific test dependencies. Restore the original. 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