feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #325
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 with host and distribution metadata. It supports deterministic syslog output, optional JSONL logging, file-size trimming, locking, validation, and check-mode reporting. Role 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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/unit/test_sr_fingerprint.py (4)
328-350: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the
_handle_fingerprintcoverage.Two gaps exist in this group of tests.
First,
test_handle_fingerprint_check_mode_with_log_filedoes not assert that check mode writes nothing. That is the core guarantee of the check-mode path, so assert thatlog_pathdoes not exist after the call.Second, no test covers the normal execution path.
_FakeModule.loggedat Line 36 collects the syslog output, but no test reads it. The syslog record is the primary output of this module. Add a test that runs_handle_fingerprintwithcheck_mode=Falseandwrite_log_file=False, then asserts the logged line equals_format_fingerprint_syslog(...)of the returned fingerprint.💚 Proposed additions
parsed = json.loads(result["jsonl_row"]) self.assertEqual(parsed["role_name"], "systemd") + self.assertFalse(os.path.exists(log_path)) + + def test_handle_fingerprint_logs_syslog_line(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", "host2"], + "distribution": "RedHat", + "distribution_version": "9.4", + }, + check_mode=False, + ) + with self.assertRaises(_ExitJsonException) as ctx: + sr_fingerprint._handle_fingerprint(module) + result = ctx.exception.kwargs + self.assertEqual(len(module.logged), 1) + self.assertEqual( + module.logged[0], + sr_fingerprint._format_fingerprint_syslog(result["fingerprint"]), + ) + self.assertIn("status=success", module.logged[0]) + self.assertIn("play_hosts_number=2", module.logged[0])🤖 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 328 - 350, Strengthen the _handle_fingerprint tests by asserting test_handle_fingerprint_check_mode_with_log_file leaves log_path nonexistent after execution. Add a normal-mode test using check_mode=False and write_log_file=False, then verify _FakeModule.logged contains a line equal to _format_fingerprint_syslog(...) for the returned fingerprint.
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
shutil.rmtreeso cleanup cannot mask a test failure.If
_write_jsonl_lograises before it createssubdir,os.listdir(subdir)raisesFileNotFoundErrorinside thefinallyblock. That second exception replaces the original failure, so the report hides the real cause.shutil.rmtreehandles the nested tree in one call and tolerates a partially created directory.♻️ Proposed cleanup fix
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 the import:
import re +import shutil import tempfile🤖 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 172 - 177, Replace the manual cleanup loop and separate os.rmdir calls in the finally block of the test with shutil.rmtree on tmpdir, and add the required shutil import. Ensure cleanup tolerates a missing or partially created subdir without masking the original test failure.
133-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRun Black on the wrapped assignments.
Lines 133-135 wrap a single string literal in redundant parentheses. Black removes such parentheses when the joined line fits in the line limit. The same pattern appears at Lines 206-208 and Lines 246-248. Run the formatter so CI does not reject the diff.
As per path instructions: "Must follow PEP 8 and be formatted with Python Black" and "Run
tox -e black,flake8before committing".#!/bin/bash # Description: Report Black formatting differences for the changed Python files. set -euo pipefail pip install --quiet black >/dev/null 2>&1 || true black --check --diff library/sr_fingerprint.py tests/unit/test_sr_fingerprint.py || 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 `@tests/unit/test_sr_fingerprint.py` around lines 133 - 135, Run Black on tests/unit/test_sr_fingerprint.py and apply its formatting to the wrapped assignments, including the assignments around record["role_path"] and the matching occurrences near the other noted locations. Remove redundant parentheses where the string literal fits on one line, then ensure the file passes Black and flake8.Source: Path instructions
131-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the quote and equals-sign escaping branch.
test_format_fingerprint_syslog_quotes_values_with_spacescovers only the space case. Thetext.replace('"', '""')branch in_format_fingerprint_key_valuestays untested, as does the=trigger. Downstream parsers depend on that escaping contract, so pin it.💚 Proposed additional tests
+ def test_format_fingerprint_syslog_escapes_quotes(self): + record = _sample_fingerprint_record() + record["role_path"] = '/tmp/we"ird' + message = sr_fingerprint._format_fingerprint_syslog(record) + self.assertIn('role_path="/tmp/we""ird"', message) + + def test_format_fingerprint_syslog_quotes_values_with_equals(self): + record = _sample_fingerprint_record() + record["role_path"] = "/tmp/a=b" + message = sr_fingerprint._format_fingerprint_syslog(record) + self.assertIn('role_path="/tmp/a=b"', message) + + def test_get_managed_node_distro_partial(self): + self.assertEqual(sr_fingerprint._get_managed_node_distro("Fedora", ""), "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, Extend test_format_fingerprint_syslog_quotes_values_with_spaces to include a fingerprint value containing a double quote and an equals sign, then assert the formatted output uses the expected doubled-quote escaping and quoted representation. Ensure the test exercises both the text.replace('"', '""') branch and the equals-sign trigger in _format_fingerprint_key_value.
🤖 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-197: Update _trim_log_file and its caller to accept max_size,
and trim based on the current file size so existing oversized content is removed
until the remaining log plus the pending new line fits within max_size. Preserve
removal of oldest records and ensure the caller passes the configured limit
rather than only the new line’s size.
In `@tests/unit/test_sr_fingerprint.py`:
- Line 17: Update the import in test_sr_fingerprint.py and the corresponding
package layout so sr_fingerprint is imported through its package-qualified path
rather than as a bare top-level module. Ensure the package is discoverable
through the repository’s normal configuration without modifying PYTHONPATH or
injecting library/ into sys.path.
---
Nitpick comments:
In `@tests/unit/test_sr_fingerprint.py`:
- Around line 328-350: Strengthen the _handle_fingerprint tests by asserting
test_handle_fingerprint_check_mode_with_log_file leaves log_path nonexistent
after execution. Add a normal-mode test using check_mode=False and
write_log_file=False, then verify _FakeModule.logged contains a line equal to
_format_fingerprint_syslog(...) for the returned fingerprint.
- Around line 172-177: Replace the manual cleanup loop and separate os.rmdir
calls in the finally block of the test with shutil.rmtree on tmpdir, and add the
required shutil import. Ensure cleanup tolerates a missing or partially created
subdir without masking the original test failure.
- Around line 133-135: Run Black on tests/unit/test_sr_fingerprint.py and apply
its formatting to the wrapped assignments, including the assignments around
record["role_path"] and the matching occurrences near the other noted locations.
Remove redundant parentheses where the string literal fits on one line, then
ensure the file passes Black and flake8.
- Around line 131-140: Extend
test_format_fingerprint_syslog_quotes_values_with_spaces to include a
fingerprint value containing a double quote and an equals sign, then assert the
formatted output uses the expected doubled-quote escaping and quoted
representation. Ensure the test exercises both the text.replace('"', '""')
branch and the equals-sign trigger in _format_fingerprint_key_value.
🪄 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: c663ac99-6fdf-43ee-8bf1-89a5765ac517
📒 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)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim frees only one line worth of bytes, so the file can stay above max_log_size.
_trim_log_file stops as soon as size_removed >= size_needed, where size_needed is the length of the single new line. The caller invokes it whenever cur_size + len(new_line) > max_size. If cur_size is already greater than max_size, removing only len(new_line) bytes leaves the file above the limit on every subsequent write. The file then never returns to the documented cap.
This happens when an operator lowers max_log_size between runs, or when the file grew under a previous larger limit. Pass max_size into the trim function and remove lines until the remaining content plus the new line fits.
🐛 Proposed fix to converge on the configured limit
-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_needed, max_size=0):
+ """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))
+ remaining = sum(len(line) for line in lines)
+ if max_size > 0:
+ target = max_size - size_needed
+ else:
+ target = remaining - size_needed
+ while lines and remaining > target:
+ remaining -= len(lines.pop(0))Update the call site:
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, len(new_line), max_size=max_size)🧰 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 - 197, Update _trim_log_file and
its caller to accept max_size, and trim based on the current file size so
existing oversized content is removed until the remaining log plus the pending
new line fits within max_size. Preserve removal of oldest records and ensure the
caller passes the configured limit rather than only the new line’s size.
| import tempfile | ||
| import unittest | ||
|
|
||
| import sr_fingerprint |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how the unit tests resolve the library/ import path.
set -euo pipefail
fd -t f 'conftest.py'
fd -t f 'tox.ini|pytest.ini|setup.cfg|pyproject.toml' -d 2
rg -n -C3 'PYTHONPATH|sys\.path|rootdir|testpaths|library' --glob 'tox.ini' --glob 'conftest.py' --glob 'pytest.ini' --glob 'setup.cfg' --glob 'pyproject.toml'Repository: linux-system-roles/kernel_settings
Length of output: 180
Prefer importing sr_fingerprint as a package.
tests/unit/test_sr_fingerprint.py imports library/sr_fingerprint.py as a bare top-level module, but the tracked config does not set test PYTHONPATH or inject library/ into sys.path. Change the package layout/import path instead of relying on runner-specific path setup.
🤖 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` at line 17, Update the import in
test_sr_fingerprint.py and the corresponding package layout so sr_fingerprint is
imported through its package-qualified path rather than as a bare top-level
module. Ensure the package is discoverable through the repository’s normal
configuration without modifying PYTHONPATH or injecting library/ into sys.path.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| os.fchmod(fd, stat.S_IMODE(orig_stat.st_mode)) | ||
| try: | ||
| os.fchown(fd, orig_stat.st_uid, orig_stat.st_gid) | ||
| except OSError: |
| except BaseException: | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: |
| for path in (log_file, log_file + ".lock"): | ||
| try: | ||
| os.unlink(path) | ||
| except OSError: |
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>
083004c to
0535194
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] |
3 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