Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The exclusion can hide genuine errors when the output also contains perl-Errno, and the new test does not detect the original behavior.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Prevents perl-Errno package names from triggering Yum error mitigation.
Changes:
- Excludes
perl-Errnofrom error detection. - Adds a regression test for the false positive.
| File | Description |
|---|---|
src/core/src/package_managers/YumPackageManager.py |
Adjusts Yum error detection. |
src/core/tests/Test_YumPackageManager.py |
Adds regression coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #393 +/- ##
==========================================
+ Coverage 95.03% 95.06% +0.03%
==========================================
Files 113 113
Lines 21994 22013 +19
==========================================
+ Hits 20901 20926 +25
+ Misses 1093 1087 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Rajasi Rane (rane-rajasi)
left a comment
There was a problem hiding this comment.
Comments inline.
| self.assertTrue(package_manager) | ||
|
|
||
| mitigation_attempted = {'called': False} | ||
| def record_call(output): |
There was a problem hiding this comment.
Define mock functions outside similar to other test structures
| package_manager = self.container.get('package_manager') | ||
| self.assertTrue(package_manager) | ||
|
|
||
| package_manager.check_known_issues_and_attempt_fix = lambda output: self.fail("perl-Errno must not enter error mitigation") |
There was a problem hiding this comment.
Review whether we use lambda functions, IIRC we use mock functions.
Rajasi Rane (rane-rajasi)
left a comment
There was a problem hiding this comment.
Comments inline.
| return ['Red Hat Enterprise Linux Server', '8', 'Ootpa'] | ||
|
|
||
| def mock_check_known_issues_and_attempt_fix_record_call(self, output): | ||
| self.check_known_issues_and_attempt_fix_called = True |
There was a problem hiding this comment.
This is not a class level variable i.e. ideally defined in init() or for the purpose of UTs, defined in setUp()
There was a problem hiding this comment.
Removed the mock since none of the added tests now uses it
|
|
||
| package_manager.check_known_issues_and_attempt_fix = backup_check_known_issues_and_attempt_fix | ||
|
|
||
| self.assertTrue(self.check_known_issues_and_attempt_fix_called) |
There was a problem hiding this comment.
Primary assert should not be on whether a mock function is called, this can be an additional assert. However, the primary one should always be on the expected outcome, say the status reported, etc
|
|
||
| self.check_known_issues_and_attempt_fix_called = False | ||
| backup_check_known_issues_and_attempt_fix = package_manager.check_known_issues_and_attempt_fix | ||
| package_manager.check_known_issues_and_attempt_fix = self.mock_check_known_issues_and_attempt_fix_record_call |
There was a problem hiding this comment.
Why mock check_known_issues_and_attempt_fix () in both of these tests? It is simply using the error message to verify against known errors and attempts a fix if a match is found. Which here will not match, so no fix is attempted. Non mocking will actually test the complete functionality
There was a problem hiding this comment.
Removed the mock usage.
|
|
||
| self.check_known_issues_and_attempt_fix_called = False | ||
| backup_check_known_issues_and_attempt_fix = package_manager.check_known_issues_and_attempt_fix | ||
| package_manager.check_known_issues_and_attempt_fix = self.mock_check_known_issues_and_attempt_fix_record_call |
There was a problem hiding this comment.
Same question on why this is mocked? There is also an existing test with the similar error (barring perl-Errno) which runs without a mock: test_auto_issue_mitigation_when_retries_are_exhausted_raise_exception_disabled()
There was a problem hiding this comment.
I didnt notice the other test. I've updated to have the same pattern and not use mock
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

What's happening: RHEL 9.8 VMs log ERROR:[YPM] Customer Environment Error: Not a known error , but patch assessment is actually succeeding ( TaskStatus=succeeded , updates reported correctly).
What customer sees:

Root cause:
YPM's error handling checks the package manager output for the strings "Error" or "Errno". One of the packages returned by the command is perl-Errno, which contains the substring "Errno". As a result, a normal check-update or list installed output is incorrectly treated as an error. The output does not match any known error pattern, so YPM logs:
Customer Environment Error: Not a known error...
This is a false positive caused by a package name and not an actual customer environment issue.
Fix : Update the Exception block to exclude the package when checking for errors.