Conversation
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1079 +/- ##
==========================================
- Coverage 79.27% 79.26% -0.01%
==========================================
Files 348 348
Lines 31127 31127
==========================================
- Hits 24676 24673 -3
- Misses 6451 6454 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| domain = self._hosts_dict["hostname", "localhost"] | ||
| domain = self._hosts_dict.get("hostname", "localhost") |
There was a problem hiding this comment.
Hah, that looked like a fun bug.
There was a problem hiding this comment.
Just because we're here anyway, could you add a unit test that would cover this scenario?
There was a problem hiding this comment.
Could you ask the author of this piece of code instead (iirc @Horofic)? Not sure what the original purpose is of setting domain to "localhost".
There was a problem hiding this comment.
Oof.
I've committed some small changes in 43505c6 and added a simple test. The second parameterised test case was my original intent. Which I believe, mimicked Debian, but not 100% clear on that anymore. This case intentionally fails now, feel free to implement it or take it in the direction you believe is more correct!
There was a problem hiding this comment.
When the content of /etc/hosts contains 127.0.0.1 mydomain (and no /etc/hostname is found) in my opinion the parsed hostname should be mydomain. In this limited context we have no way to safely determine the difference between a hostname and domain.
There was a problem hiding this comment.
Very fair. I adjusts the test case in 9394c1c and added some additional test cases just to be sure. Please see if you agree with them. I also adjusted some other tests to take into account some of the localhost behaviour.
There was a problem hiding this comment.
Thanks. I think leaving the domain to None makes more sense instead of assuming localhost.
There was a problem hiding this comment.
Ill change the behaviour back later today.
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
Schamper
left a comment
Schamper
left a comment
There was a problem hiding this comment.
Would you mind taking a look at this? #1079 (comment)
There was a problem hiding this comment.
Pull Request Overview
This PR improves OS detection for RedHat, CentOS, Fedora, OpenBSD, and FreeBSD while refactoring hostname parsing to return a tuple instead of a dictionary. Key changes include updating tests to accommodate the new hostname parsing format, adding new tests for various OS detections, and refactoring detection and parsing logic within the Unix OS plugins.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/plugins/os/unix/test__os.py | Updated hostname parsing assertions to match tuple return types. |
| tests/plugins/os/unix/linux/redhat/test__os.py | Added tests to verify detection of RedHat and related releases. |
| tests/plugins/os/unix/bsd/openbsd/test__os.py | Added tests for OpenBSD detection. |
| tests/plugins/os/unix/bsd/freebsd/test__os.py | Added tests for FreeBSD detection. |
| dissect/target/plugins/os/unix/locale.py | Changed language collection from a list to a set to avoid duplicates. |
| dissect/target/plugins/os/unix/linux/redhat/_os.py | Refactored detection signature and added future import. |
| dissect/target/plugins/os/unix/bsd/openbsd/_os.py | Updated detection logic and refactored hostname and domain properties. |
| dissect/target/plugins/os/unix/bsd/freebsd/_os.py | Updated detection logic using a generator for file existence checks. |
| dissect/target/plugins/os/unix/_os.py | Refactored _parse_hostname_string to return a tuple and improved hosts parsing logic. |
Comments suppressed due to low confidence (1)
dissect/target/plugins/os/unix/_os.py:189
- [nitpick] The variable name 'callable' shadows the built-in function 'callable'. Consider renaming it (for example, to 'parser') to improve code clarity.
for path, callable in paths:
This PR improves RedHat, CentOS and Fedora OS detection.