feat: Add configurable sudo commands and UOS/Deepin support #6241
Conversation
WalkthroughThis pull request adds conditional sudo privilege support to Avocado's sysinfo command collection. The implementation introduces a SysinfoCommand helper class that detects Linux sudo capability via can_sudo() and validates whether commands and Linux distributions match configured lists. The Command class is extended to accept sudo_commands and sudo_distros parameters, and Command.collect is updated to conditionally apply a sudo flag to process.run() without mutating the command string. Configuration options are registered in the settings layer and loaded from sysinfo.conf into the SysInfo class, which then propagates these settings to all Command instances. A template configuration file is provided with example commands and distributions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)avocado/utils/sysinfo.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
🔇 Additional comments (10)
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: 0
🧹 Nitpick comments (4)
avocado/utils/sysinfo.py (4)
175-189: Consider cachingSysinfoCommandinstance to avoid repeated config parsing.A new
SysinfoCommand()instance is created on everycollect()call, which reads and parses the configuration file each time. For batch sysinfo collection with many commands, this adds unnecessary I/O overhead.Consider making
SysinfoCommanda module-level singleton or caching the instance:+_sysinfo_command = None + +def _get_sysinfo_command(): + global _sysinfo_command + if _sysinfo_command is None: + _sysinfo_command = SysinfoCommand() + return _sysinfo_commandThen in
collect():- sysinfo_cmd = SysinfoCommand() + sysinfo_cmd = _get_sysinfo_command()
410-413: Local import is acceptable but add a brief comment.The local import of
can_sudoinside__init__works to avoid potential circular imports. A brief comment explaining why would help future readers.
435-440: Root check is redundant.The
can_sudo()function already returnsTruewhen running as root (seeprocess.pyline 71-72:if not os.getuid(): return True). The explicitgeteuid() == 0check on lines 439-440 is therefore redundant sinceself.sudo_availablewill already beTruefor root, andprocess.run(..., sudo=True)handles this correctly.If the intent is to skip the unnecessary sudo prefix for root users, this is fine—but note that
process.run()also handles this case internally.
441-446: Specify encoding and consider checkingID_LIKEfor derivative distributions.
- The file should be opened with an explicit encoding to avoid platform-dependent behavior.
- Many distributions set
ID_LIKEto indicate their parent (e.g., a UOS derivative might haveID_LIKE=debian). Checking onlyID=may miss compatible derivatives.try: - with open('/etc/os-release') as f: + with open('/etc/os-release', encoding='utf-8') as f: + os_id = None + id_like = [] for line in f: if line.startswith('ID='): - os_id = line.strip().split('=')[1].strip('"') - return os_id.lower() in self.sudo_distros + os_id = line.strip().split('=')[1].strip('"').lower() + elif line.startswith('ID_LIKE='): + id_like = line.strip().split('=')[1].strip('"').lower().split() + if os_id and os_id in self.sudo_distros: + return True + return any(d in self.sudo_distros for d in id_like)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/utils/sysinfo.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
🔇 Additional comments (3)
avocado/etc/avocado/sysinfo.conf (1)
1-5: Configuration looks reasonable.The structure is clear and extensible. The configured commands (
dmidecode,fdisk) are appropriate choices as they typically require elevated privileges to provide complete information.Consider adding a comment clarifying that only the base command name (without arguments) should be listed, since
is_sudo_cmd()extracts only the basename for matching.avocado/utils/sysinfo.py (2)
463-468: LGTM - command parsing is robust.Good use of
shlex.split()with proper exception handling for malformed commands, andos.path.basename()ensures paths like/usr/sbin/dmidecodematch correctly against the configured base names.
414-421: No changes required — relative path resolves correctly when installed.The relative path assumption is valid. MANIFEST.in includes
recursive-include avocado/etc *, and setup.py hasinclude_package_data=True, which ensures theetcdirectory structure is preserved in the installed package. The pathos.path.dirname(__file__) + ".." + "etc/avocado/sysinfo.conf"fromavocado/utils/sysinfo.pycorrectly resolves to the packaged config file both in development and post-installation.
|
@clebergnu Thanks! Merged into one submission, resubmitted PR. |
xianglongfei-8888
changed the title
clebergnu
left a comment
clebergnu
left a comment
There was a problem hiding this comment.
This proposal of selectively using sudo makes sense to me. But, I have some comments about the implementation:
- The
SysinfoCommandclass redoes the parsing of Avocado's configuration, and it does so in a way that is not exactly the same. It's better to useavocado.core.settings.settingsinstead. - A lot of this code does not fall into the "utility" category. Think of it as if that module was standalone for other people to build upon their final functionality. And with the migration of all
avocado.utils.*to the AAUtils project, this will need to be taken into account.
So, being more specific, instead of taking configuration in avocado.utils.sysinfo from the configuration file directly, most of the code should be in the sysinfo plugin, which has access to the effective settings. From these settings, pass them as (new?) parameters to avocado.utils.sysinfo.
Let me know if this makes sense to you.
Thanks!
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
9759bba to
9461cbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
avocado/utils/sysinfo.py (2)
21-29: Logger choice is fine; consider whetherINFOis the right level for per-command logsYou’ve added a module-level logger
log = logging.getLogger("avocado.sysinfo"). Sinceavocado/core/sysinfo.pyuses the same logger name, both modules now share the same logger, which is consistent.Later you log each command execution at
INFO. That may be desirable for troubleshooting, but it can be a bit noisy if sysinfo runs many commands on every job. If log volume becomes an issue, consider switching the per-command message toDEBUGand leaving higher-level sysinfo lifecycle messages atINFO.
411-458: SysinfoCommand logic is mostly correct; a few edge-case and clarity considerationsThe overall logic is good: you gate sudo on three things—sudo availability, OS being Linux, and distro ID being listed—then further restrict it to configured commands by basename.
A few specific points worth tightening or at least documenting:
Empty distro list silently disables sudo:
- If
self.sudo_distrosis empty (e.g., misconfigured or config file missing thesudo_distroskey),use_sudo()will always returnFalse, even ifsudo_commandsis populated.- That’s acceptable if intentional (sudo only opt‑in on explicit distros), but you might want to log at
DEBUGwhensudo_distrosis empty so it’s obvious why sudo is never used.Error handling for
/etc/os-release:
- On Linux you attempt to read
/etc/os-releaseand log a warning onFileNotFoundError. That makes sense, but consider also logging a brief debug when the file is present yet noID=line is found, so users can understand why sudo is disabled.Repeated I/O / parsing:
use_sudo()walks/etc/os-releaseline by line and returns as soon as it findsID=.... This is efficient per call, but is still repeated for every command. Caching the discoveredos_idat module level or as a static attribute onSysinfoCommandwould remove redundant work.Command name extraction is robust:
is_sudo_cmd()usesshlex.split()and safely handles malformed commands with atry/except, then normalizes to a lowercase basename. That’s a solid approach and avoids false positives on arguments.Overall, the behavior is consistent with the design; the suggestions above are mainly to make the sudo decision easier to debug and to avoid surprises when misconfigured lists are empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
avocado/core/sysinfo.py(3 hunks)avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/plugins/sysinfo.py(1 hunks)avocado/utils/sysinfo.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- avocado/etc/avocado/sysinfo.conf
🧰 Additional context used
🧬 Code graph analysis (3)
avocado/plugins/sysinfo.py (2)
avocado/core/utils/path.py (2)
system_wide_or_base_path(15-47)prepend_base_path(7-12)avocado/core/settings.py (3)
register_option(646-803)section(138-139)key(142-143)
avocado/core/sysinfo.py (1)
avocado/utils/sysinfo.py (1)
Command(129-202)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (52)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: Linux with Python 3.12
- GitHub Check: Linux with Python 3.11
- GitHub Check: Linux with Python 3.13
- GitHub Check: Linux with Python 3.9
- GitHub Check: Linux with Python 3.10
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: Fedora selftests
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Egg task ubuntu:24.04
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Podman Egg task
- GitHub Check: Build Package (wheel/tarball) for Python 3.12
- GitHub Check: Version task ubi:8.8
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (3)
avocado/utils/sysinfo.py (2)
138-146: Command now carries sudo configuration; object-level design looks cleanExtending
Command.__init__to acceptsudo_commandsandsudo_distrosand storing those as attributes is a reasonable way to keep sudo behavior local to the collectible, without touching callers that don’t care about sudo (they continue to use defaults).No issues with compatibility here: existing call sites that don’t pass the new arguments will still work because the parameters are appended at the end with default
None.
162-181: SysinfoCommand integration is correct, but may be slightly heavy-weight per commandThe integration of
SysinfoCommandintoCommand.collect()is logically sound:
- Environment handling and timeout behavior are preserved.
- You compute
sudo_flag = use_sudo() and is_sudo_cmd(self.cmd)and pass it through toprocess.run(..., sudo=sudo_flag).- The decision is based on both distro and command name, which matches the new configuration model.
One potential improvement:
SysinfoCommand.__init__callscan_sudo()and, viause_sudo(), also reads/etc/os-releaseon eachcollect()call. In a typical sysinfo run, this can mean reading the same file and probing sudo many times.If this becomes a hot path, consider caching the distro and sudo availability at module level (or in a small singleton) instead of recomputing them per command. For example, memoize
SysinfoCommand.use_sudo()per process, or maintain a module-level "current distro ID" and "sudo_available" flags.Behaviorally, though, this code looks correct and backward compatible.
avocado/core/sysinfo.py (1)
161-169: Consider propagating sudo settings tofail_commandsas well
self.sudo_commands/self.sudo_distrosare only passed intosysinfo.Commandfor the regularcommandscollectibles.fail_commandsare still instantiated without these parameters, meaning commands infail_commandswill not benefit from the sudo routing logic even if explicitly configured.Either align the behavior by propagating the same sudo config to
fail_commands:- for fail_cmd in self.sysinfo_files["fail_commands"]: - self.end_fail_collectibles.add( - sysinfo.Command(fail_cmd, timeout=timeout, locale=locale) - ) + for fail_cmd in self.sysinfo_files["fail_commands"]: + self.end_fail_collectibles.add( + sysinfo.Command( + fail_cmd, + timeout=timeout, + locale=locale, + sudo_commands=self.sudo_commands, + sudo_distros=self.sudo_distros, + ) + )Or add a comment explaining why
fail_commandsintentionally exclude these settings to prevent future confusion.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6241 +/- ##
==========================================
- Coverage 73.48% 73.46% -0.03%
==========================================
Files 206 206
Lines 22494 22571 +77
==========================================
+ Hits 16530 16582 +52
- Misses 5964 5989 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
9461cbc to
0887038
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
avocado/core/sysinfo.py (1)
72-106: Config options still behave as file paths, so CSV-stylesysinfo.sudo_*usage is silently ignored
sysinfo.sudo_commandsandsysinfo.sudo_distrosare read fromsettings.as_dict()intosudo_commands_conf/sudo_distros_conf, then always treated as filenames fed intoConfigParser.read(). The actual comma‑separated values come from a[sysinfo]section in those files.If a user instead follows the “comma separated list” semantics (for example:
sysinfo.sudo_commands = dmidecode,fdiskin their config), this string is passed toconfig.read()as a filename, no file is loaded, theconfig.get(..., fallback="")calls return empty strings, and bothself.sudo_commandsandself.sudo_distrosend up empty. That effectively disables the sudo logic without any warning, which is the same behavioral problem raised in the previous review.To support both “option‑as‑value” (CSV) and “option‑as‑path to INI” models, you can fall back to treating the option itself as CSV when it doesn’t point to an existing file, instead of unconditionally relying on
ConfigParser.read():- config = configparser.ConfigParser() - candidates = [sudo_commands_conf, sudo_distros_conf] - config.read([p for p in candidates if p]) - - # Retrieve the actual sudo commands and distros values from the config files, - # falling back to empty string if the keys are missing - sudo_commands_value = config.get("sysinfo", "sudo_commands", fallback="") - sudo_distros_value = config.get("sysinfo", "sudo_distros", fallback="") + config = configparser.ConfigParser() + + def _load_sudo_list(raw_value, key): + """ + If `raw_value` is a path to an INI file, read `[sysinfo] / key` + from it; otherwise, treat `raw_value` itself as a CSV list. + """ + if not raw_value: + return "" + if os.path.isfile(raw_value): + config.read(raw_value) + return config.get("sysinfo", key, fallback="") + return raw_value + + sudo_commands_value = _load_sudo_list(sudo_commands_conf, "sudo_commands") + sudo_distros_value = _load_sudo_list(sudo_distros_conf, "sudo_distros")This keeps the current “path to
sysinfo.conf” default working, while also making direct CSV configuration onsysinfo.sudo_commands/sysinfo.sudo_distrosbehave as documented instead of being ignored. You’ll also want to ensure the help text in the plugin matches this dual behavior.
🧹 Nitpick comments (2)
avocado/utils/sysinfo.py (2)
138-147: Avoid paying sudo detection cost when there is no sudo configuration
Command.collect()always instantiatesSysinfoCommand:sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd)
SysinfoCommand.__init__callscan_sudo(), anduse_sudo()reads/etc/os-release. This will run for everyCommand, even whensudo_commands/sudo_distrosare empty (the common case on non‑UOS/Deepin systems), yieldingsudo_flag == Falsebut still adding non‑trivial overhead.You can short‑circuit when no sudo configuration is present and only construct
SysinfoCommandwhen it can actually be useful:- # Determine whether to run with sudo (do not mutate the command string) - sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) - sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd) + # Determine whether to run with sudo (do not mutate the command string) + sudo_flag = False + if self.sudo_commands and self.sudo_distros: + sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) + sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd)This keeps behavior unchanged when sudo is configured, and avoids repeated
can_sudo()and distro detection calls when it is not.Also applies to: 179-183, 192-192
413-460: Clarify intended semantics for emptysudo_distros; optional caching for repeated environment checksThe Linux/distro-aware sudo decision is generally sound, but two aspects need attention:
Behavior when
sudo_distrosis empty
use_sudo()returnsTrueonly when the parsedIDfrom/etc/os-releaseis inself.sudo_distros. With an emptysudo_distros, sudo is effectively disabled everywhere. This design choice (strict whitelist vs. optional filter) should be documented explicitly in the class docstring or parameter help. Clarify whether emptysudo_distrosmeans "never use sudo for distro filtering" (whitelist) or "apply no distro filtering" (filter-only).Repeated environment checks (optional optimization)
EachSysinfoCommandinstance callscan_sudo(), anduse_sudo()rereads/etc/os-release. If instances are created frequently, consider caching these at the class level to reduce repeated system calls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
avocado/core/sysinfo.py(3 hunks)avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/plugins/sysinfo.py(1 hunks)avocado/utils/sysinfo.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- avocado/plugins/sysinfo.py
- avocado/etc/avocado/sysinfo.conf
🧰 Additional context used
🧬 Code graph analysis (2)
avocado/core/sysinfo.py (2)
avocado/core/settings.py (1)
read(303-321)avocado/utils/sysinfo.py (1)
Command(129-204)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: Linux with Python 3.12
- GitHub Check: Linux with Python 3.10
- GitHub Check: Linux with Python 3.9
- GitHub Check: Linux with Python 3.13
- GitHub Check: Linux with Python 3.11
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Podman Egg task
- GitHub Check: Fedora selftests
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task ubi:8.8
- GitHub Check: macOS with Python 3.11
- GitHub Check: Version task debian:12.4
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (2)
avocado/core/sysinfo.py (1)
175-201: Propagation of sudo configuration into Command collectibles looks correctPassing
self.sudo_commands/self.sudo_distrosinto allsysinfo.Commandinstances for start, end, and fail commands is consistent and keeps the behavior centralized inCommand.collect(). The change is backward compatible (new parameters are optional) and doesn’t affectLogfile/Daemoncollectibles.avocado/utils/sysinfo.py (1)
21-29: Sharedavocado.sysinfologger is appropriateUsing
logging.getLogger("avocado.sysinfo")here aligns this module’s logging withavocado.core.sysinfoand keeps sysinfo‑related messages grouped under a single logger name. No issues spotted.
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
0887038 to
5f3a8a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
avocado/utils/sysinfo.py (1)
179-185: Consider reusingSysinfoCommand/ caching sudo decision to avoid repeated checksRight now each
Command.collect()with sudo config creates a newSysinfoCommand, which in turn callscan_sudo()and re-parses/etc/os-release. For environments with many sysinfo commands this can become unnecessarily expensive.You could instead:
- Build a single
SysinfoCommandinstance once perSysInfoand pass it (or a precomputedsudo_flag/use_sudo()result) into eachCommand, or- Lazily cache
use_sudo()’s result insideSysinfoCommandafter the first call.Functionality is correct as-is; this is a performance/cleanliness improvement.
Also applies to: 194-195
avocado/etc/avocado/sysinfo.conf (1)
1-5: Config snippet aligns with parsing logic; consider clarifying distro IDsThe
[sysinfo]section with comma‑separatedsudo_commands/sudo_distrosmatches the CSV parsing inSysInfo.__init__and gives sane defaults for UOS/Deepin.For extra clarity to users editing this file, you might extend the
sudo_distroscomment to mention that values should match theID=field from/etc/os-release(e.g.uos,deepin), since that’s whatSysinfoCommand.use_sudo()checks.avocado/core/sysinfo.py (1)
18-19: Config loading for sudo options is flexible; watch out for multi-file edge casesThe
_load_sudo_list()helper nicely supports both:
- treating
sysinfo.sudo_commands/sysinfo.sudo_distrosas paths to INI files with a[sysinfo]section, and- treating them directly as comma‑separated lists.
Lowercasing and trimming into sets gives clean inputs to
SysinfoCommand.One minor subtlety: because a single
ConfigParserinstance is reused, calling_load_sudo_list()for two different files that both define[sysinfo](but only one of them defines a given key) can lead to values “leaking” from the first file into the second. If you expect users to ever point these two settings at different files, consider instantiating a freshConfigParserinside_load_sudo_list()so each path is independent:- config = configparser.ConfigParser() - - def _load_sudo_list(raw_value, key): + def _load_sudo_list(raw_value, key): """ If `raw_value` is a path to an INI file, read `[sysinfo] / key` from it; otherwise, treat `raw_value` itself as a CSV list. """ if not raw_value: return "" if os.path.isfile(raw_value): - config.read(raw_value) - return config.get("sysinfo", key, fallback="") + parser = configparser.ConfigParser() + parser.read(raw_value) + return parser.get("sysinfo", key, fallback="") return raw_valueNot a blocker, but this would make the behavior easier to reason about.
Also applies to: 72-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
avocado/core/sysinfo.py(4 hunks)avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/plugins/sysinfo.py(1 hunks)avocado/utils/sysinfo.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- avocado/plugins/sysinfo.py
🧰 Additional context used
🧬 Code graph analysis (2)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
avocado/core/sysinfo.py (1)
avocado/utils/sysinfo.py (1)
Command(129-206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Linux with Python 3.10
- GitHub Check: Linux with Python 3.13
- GitHub Check: Linux with Python 3.12
- GitHub Check: Linux with Python 3.11
- GitHub Check: Linux with Python 3.9
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: Version task ubuntu:24.04
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Fedora selftests
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task fedora:41
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Code Coverage (3.11)
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
🔇 Additional comments (4)
avocado/utils/sysinfo.py (3)
21-29: Logger setup looks consistent and appropriateUsing the shared
"avocado.sysinfo"logger here matchesavocado/core/sysinfo.pyand keeps sysinfo logging centralized. No issues seen.
138-147: Constructor extension keeps backward compatibilityAdding
sudo_commandsandsudo_distrosas keyword-only-ish tail parameters, and defaulting them toNone, preserves existingCommandcall sites while allowing the new behavior. The instance attributes are straightforward and safe.
415-462: HardenSysinfoCommandinitialization for cross-platform safetyOne targeted improvement would make this more robust:
- Cross‑platform safety:
__init__()unconditionally callscan_sudo(). On platforms without Unix-like UID support (e.g., Windows), ifcan_sudo()internally usesos.getuid()without guards, it will raiseAttributeError. Wrap this call defensively:- def __init__(self, sudo_commands=None, sudo_distros=None): - from avocado.utils.process import can_sudo - self.sudo_available = can_sudo() + def __init__(self, sudo_commands=None, sudo_distros=None): + # Only attempt sudo capability checks on Linux-like systems + system_name = platform.system().lower() + try: + self.sudo_available = ( + system_name == "linux" and process.can_sudo() + ) + except Exception: # be defensive against unexpected OS-specific issues + self.sudo_available = FalseThe other suggestions (avoiding local import and caching the result) are reasonable optimizations, but their necessity depends on whether
processis imported at module level and howcan_sudo()is implemented—details that should be verified against the actual codebase.avocado/core/sysinfo.py (1)
173-209: Sudo configuration is correctly threaded into collectible commandsPassing
self.sudo_commandsandself.sudo_distrosintosysinfo.Commandforcommandsandfail_commandsensures the new sudo logic is applied uniformly at start, end, and fail‑end phases. Daemon/file collectibles remain unaffected, which is sensible.This wiring looks correct and keeps the feature nicely localized in
Command.collect()/SysinfoCommand.
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
5f3a8a0 to
98b94c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
avocado/etc/avocado/sysinfo.conf (1)
1-6: Config values align with sudo logic; clarify final comment wordingThe defaults for
sudo_commandsandsudo_distrosare consistent with howSysinfoCommandparses values (CSV lists later normalized to lowercase). The last comment is a bit meta (“comment to mention that…”); consider making it a direct statement for clarity.-# sudo_distros comment to mention that values should match the ID= field from /etc/os-release (e.g. uos, deepin) +# Values of sudo_distros must match the ID= field from /etc/os-release (e.g. uos, deepin).avocado/core/sysinfo.py (1)
72-110: Sudo list loader correctly supports INI and CSV; handle missing files more explicitlyThe
_load_sudo_list()helper nicely supports both “path to INI with[sysinfo]section” and raw CSV values, and the normalization into lowercase sets looks good. One corner case is when a value is meant to be a file path (as per plugin help) but the file doesn’t exist: it falls through toreturn raw_value, which then gets split as CSV, producing set entries based on the path string instead of cleanly disabling sudo.To avoid silently misconfiguring sudo behavior when the configured file is missing or mistyped, consider treating a non-existent path as “no config” (and logging it) instead of as CSV:
def _load_sudo_list(raw_value, key): @@ - if not raw_value: - return "" - if os.path.isfile(raw_value): - config = configparser.ConfigParser() - config.read(raw_value) - return config.get("sysinfo", key, fallback="") - return raw_value + if not raw_value: + return "" + if os.path.isfile(raw_value): + cfg = configparser.ConfigParser() + cfg.read(raw_value) + return cfg.get("sysinfo", key, fallback="") + # If the value looks like a path but the file is missing, log and ignore. + if os.sep in raw_value: + log.warning( + "sudo list file for %s not found at %r; ignoring sudo configuration", + key, + raw_value, + ) + return "" + # Otherwise, treat it as a raw CSV value. + return raw_valueavocado/plugins/sysinfo.py (1)
172-189: Sudo config options are now path-based and consistent; consider clarifying sudo_distros helpRegistering
sysinfo.sudo_commands/sysinfo.sudo_distrosas file paths (withsystem_wide_or_base_pathdefaults) matches howSysInfo.__init__()loads the lists and fixes the previous value-vs-path confusion. To make distro matching behavior more discoverable, you might hint in the help text that entries correspond to/etc/os-releaseIDvalues.- help_msg = "File with list of distributions that require sudo" + help_msg = ( + "File with list of distributions (values matching ID= in /etc/os-release) " + "that require sudo" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
avocado/core/sysinfo.py(4 hunks)avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/plugins/sysinfo.py(1 hunks)avocado/utils/sysinfo.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
avocado/plugins/sysinfo.py (2)
avocado/core/utils/path.py (2)
system_wide_or_base_path(15-47)prepend_base_path(7-12)avocado/core/settings.py (3)
register_option(646-803)section(138-139)key(142-143)
avocado/core/sysinfo.py (1)
avocado/utils/sysinfo.py (1)
Command(129-206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (36)
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: Linux with Python 3.13
- GitHub Check: Linux with Python 3.12
- GitHub Check: Linux with Python 3.10
- GitHub Check: Linux with Python 3.9
- GitHub Check: Linux with Python 3.11
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Podman Egg task
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Windows with Python 3.12
- GitHub Check: Fedora selftests
- GitHub Check: Windows with Python 3.11
- GitHub Check: Windows with Python 3.9
- GitHub Check: Version task ubi:8.8
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task debian:12.4
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (4)
avocado/core/sysinfo.py (1)
181-207: Passing sudo configuration into all Command collectibles is consistentWiring
self.sudo_commandsandself.sudo_distrosinto everysysinfo.Commandcreated for normal, end, and fail commands ensures the new sudo logic is applied uniformly without changing existing call sites or Daemon/Logfile behavior. This keeps the API backward compatible while enabling the new feature.avocado/utils/sysinfo.py (3)
21-29: Adding a dedicated sysinfo logger is appropriateUsing
logging.getLogger("avocado.sysinfo")here aligns with the core sysinfo module and gives clear visibility into command execution and sudo decisions without changing behavior elsewhere.
138-148: Command’s sudo integration looks correct and remains backward compatibleExtending
Command.__init__with optionalsudo_commandsandsudo_distroskeeps existing call sites valid while enabling sudo-aware execution. Thecollect()logic correctly:
- Derives a fresh environment and normalizes timeout as before.
- Computes
sudo_flagonly when both sudo lists are non-empty.- Delegates “should this use sudo?” to
SysinfoCommandand passessudo=sudo_flagtoprocess.run()without mutatingself.cmd.- Logs whether sudo is being used per command.
This is a clean integration point for the new configuration-driven behavior.
Also applies to: 179-185, 194-195
415-463: SysinfoCommand encapsulates sudo decisions cleanly; behavior is conservative outside configured distros
SysinfoCommandneatly centralizes sudo decisions:
use_sudo()respectscan_sudo(), only applies on Linux, avoids sudo when already root, and restricts sudo usage to distros whose/etc/os-releaseIDis in the configuredsudo_distrosset.is_sudo_cmd()safely parses the first token viashlex.split, matches by basename, and ignores malformed commands without raising.This ensures that only explicitly configured commands on explicitly configured distros gain sudo, leaving other environments unaffected.
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
98b94c8 to
61e559a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
avocado/core/sysinfo.py (2)
72-111: Sudo config parsing (CSV vs INI) looks correct; consider minor UX/logging tweaksThis nicely supports both:
- direct CSV values in
sysinfo.sudo_commands/sysinfo.sudo_distros, and- pointing those options at an INI file with a
[sysinfo]section.Two small polish suggestions:
- The comment on Line 72 still calls these “paths”; they can now be either CSV values or paths. Updating the wording would avoid confusion.
- In
_load_sudo_list(), ifraw_valuelooks like a path (contains a path separator or ends in.conf) butos.path.isfile(raw_value)is false, you silently treat it as CSV. Emitting a warning (or at least a debug log) in that case would help users spot typos in file paths.Optionally, logging the final resolved
self.sudo_commands/self.sudo_distrosat debug level could also aid troubleshooting.
178-207: Wiring sudo config into Command is aligned with the plugin‑vs‑utils splitPassing
sudo_commandsandsudo_distrosfromSysInfointosysinfo.Commandkeeps all config parsing incore/sysinfoand leavesutils/sysinfowith plain runtime data, which matches the guidance to avoid config access in utils.If you want to reduce duplication, you could factor the shared kwargs into a small dict used for all three
Command(...)calls, but that’s purely cosmetic.avocado/utils/sysinfo.py (3)
138-147: Command constructor extension is backward‑compatible; consider updating the docstringExtending
Command.__init__withsudo_commands=None, sudo_distros=Nonekeeps positional and keyword callers compatible and simply stores the new config on the instance. Equality/hash continue to be based on(cmd, log_path), which is consistent with prior behavior.You might want to extend the class docstring to briefly mention the new sudo‑related parameters so the behavior is discoverable from the API docs.
179-185: Sudo flag integration is correct; consider avoiding repeated capability checksThe logic of:
- requiring both non‑empty
sudo_commandsandsudo_distros,- checking
use_sudo()for the current distro, and- then filtering by
is_sudo_cmd(self.cmd)is sound and should avoid surprising sudo usage.
Because
SysinfoCommand.__init__currently callscan_sudo(), this path will run the sudo capability check for everyCommand.collect()invocation. If the sysinfo configuration runs many commands, this could add noticeable overhead.A small optimization (optional):
- Instantiate a single
SysinfoCommandperSysInfoinstance (or even per process) and pass it intoCommand, or- Precompute a
use_sudo_for_this_distroboolean once and haveCommand.collect()only runis_sudo_cmd()per call.That keeps behavior the same while reducing redundant calls to
can_sudo()and/etc/os-releaseparsing.Also applies to: 194-194
448-463: Command name matching relies on bare program names
is_sudo_cmd()extracts the first token viashlex.split()and compares its basename (lowercased) againstself.sudo_cmds. This works well if configuration values are simple program names (dmidecode,fdisk).Just be aware that entries like
/usr/sbin/dmidecodeinsudo_commandswill not match, because the config is not normalized throughos.path.basename()the way runtime commands are. If you anticipate users specifying full paths, you might want to normalize config values similarly:- self.sudo_cmds = sudo_commands if sudo_commands else set() + self.sudo_cmds = { + os.path.basename(cmd).lower() for cmd in (sudo_commands or set()) + }Not strictly necessary if you document that only bare command names are supported, but worth considering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
avocado/core/sysinfo.py(3 hunks)avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/plugins/sysinfo.py(1 hunks)avocado/utils/sysinfo.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- avocado/etc/avocado/sysinfo.conf
- avocado/plugins/sysinfo.py
🧰 Additional context used
🧬 Code graph analysis (2)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
avocado/core/sysinfo.py (1)
avocado/utils/sysinfo.py (1)
Command(129-206)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (40)
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: Fedora selftests
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Build Package (wheel/tarball) for Python 3.11
- GitHub Check: Version task ubuntu:24.04
- GitHub Check: Version task fedora:41
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Version task fedora:40
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Windows with Python 3.12
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (2)
avocado/core/sysinfo.py (1)
18-18: ConfigParser import usage is appropriate
configparseris used only inside the new sudo config helper and is scoped correctly; no issues here.avocado/utils/sysinfo.py (1)
21-29: Logger and imports for sysinfo utilities look goodAdding
platform,logging, and a sharedlog = logging.getLogger("avocado.sysinfo")here is appropriate and keeps sysinfo‑related logging under a single namespace across core and utils.
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
61e559a to
54e17e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
avocado/utils/sysinfo.py (3)
139-148: ExtendCommanddocstring to cover sudo-related parametersThe constructor now accepts
sudo_commandsandsudo_distros, but the class docstring and parameter docs still only mentioncmd,timeout, andlocale. Documenting the new configuration makes the public API clearer to callers.class Command(Collectible): @@ - :param cmd: String with the command. - :param timeout: Timeout for command execution. - :param locale: Force LANG for sysinfo collection + :param cmd: String with the command. + :param timeout: Timeout for command execution. + :param locale: Force LANG for sysinfo collection. + :param sudo_commands: Iterable of command basenames that should run with sudo + (lowercased; e.g. {"dmidecode", "fdisk"}). + :param sudo_distros: Iterable of distro IDs (from ID= in /etc/os-release) + on which sudo should be enabled.
180-196: Sudo decision logic is safe; consider clarifying gating and log verbosityThe
sudo_flagcomputation is conservative and preserves existing behavior when sudo config is missing orcan_sudo()is false, which is good. A few minor refinements you might consider:
- At Line 182,
if self.sudo_commands and self.sudo_distros:means both lists must be non-empty; otherwise, sudo is always disabled. If you ever want “sudo for all distros” when only commands are configured, you may want to treat a missing/emptysudo_distrosas “no distro filtering” instead of “disable sudo entirely”.- Instantiating
SysinfoCommandinsidecollect()means you re-runcan_sudo()and re-open/etc/os-releaseper command execution. Given the small number of sysinfo commands this is probably fine, but if it ever becomes a hot path you could create a singleSysinfoCommandperSysInfoinstance and pass it (or its precomputedsudo_flag/distro decision) into eachCommand.- Logging every command at
INFO(Line 185) may be noisy in large jobs; if that becomes a concern,DEBUGor a more targeted log level could be an option.All of these are behavioral tweaks rather than correctness fixes, so they can be deferred if you’re happy with the current semantics.
416-465:SysinfoCommandlogic is sound; normalize sudo lists for robustnessThe overall flow in
SysinfoCommandlooks correct:
can_sudo()is only consulted on Linux, anduse_sudo()additionally skips sudo when running as root.- Distro gating via
/etc/os-releaseandID=is straightforward.is_sudo_cmd()correctly handles full paths and quoted commands viashlex.split()andos.path.basename().To make this helper more robust and self-contained across call sites, consider normalizing the configuration once in
__init__:
- Convert both
sudo_commandsandsudo_distrosto lowercase sets, so callers don’t need to remember to pre-normalize values.class SysinfoCommand: def __init__(self, sudo_commands=None, sudo_distros=None): - self.sudo_cmds = sudo_commands if sudo_commands else set() - self.sudo_distros = sudo_distros if sudo_distros else set() + self.sudo_cmds = ( + {c.lower() for c in sudo_commands} if sudo_commands else set() + ) + self.sudo_distros = ( + {d.lower() for d in sudo_distros} if sudo_distros else set() + )This keeps
is_sudo_cmd()anduse_sudo()behavior consistent even if future callers pass in non-normalized data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
avocado/core/sysinfo.py(3 hunks)avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/plugins/sysinfo.py(1 hunks)avocado/utils/sysinfo.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- avocado/etc/avocado/sysinfo.conf
- avocado/core/sysinfo.py
🧰 Additional context used
🧬 Code graph analysis (1)
avocado/plugins/sysinfo.py (2)
avocado/core/utils/path.py (2)
system_wide_or_base_path(15-47)prepend_base_path(7-12)avocado/core/settings.py (3)
register_option(646-803)section(138-139)key(142-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Linux with Python 3.11
- GitHub Check: Linux with Python 3.9
- GitHub Check: Linux with Python 3.10
- GitHub Check: Egg task debian:11.0
- GitHub Check: Fedora selftests
- GitHub Check: Fedora develop install/uninstall task
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Podman Egg task
- GitHub Check: Egg task ubuntu:22.04
- GitHub Check: Egg task ubuntu:24.04
- GitHub Check: Egg task debian:12.4
- GitHub Check: Version task fedora:41
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: Windows with Python 3.11
- GitHub Check: Windows with Python 3.13
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Static checks
- GitHub Check: macOS with Python 3.11
🔇 Additional comments (2)
avocado/plugins/sysinfo.py (1)
172-192: Sudo configuration options now match file-based semanticsRegistering
sysinfo.sudo_commandsandsysinfo.sudo_distrosasprepend_base_pathoptions with a default ofsystem_wide_or_base_path("etc/avocado/sysinfo.conf"), and describing them explicitly as “File with list of …”, is consistent with their use as configuration file paths in core sysinfo logic. This resolves the earlier ambiguity where they looked like CSV value options.avocado/utils/sysinfo.py (1)
21-29: Imports and logger setup look appropriateAdding
platform,logging, andcan_sudoplus a dedicatedlog = logging.getLogger("avocado.sysinfo")is a clean way to support the new sudo logic and logging. The module import ofcan_sudocombined with Linux-only invocation insideSysinfoCommandkeeps cross-platform behavior safe.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
avocado/utils/sysinfo.py (3)
16-37: Imports block contains unresolved merge conflict and duplicate abc importThe imports section still has conflict markers and duplicated
from abc import ABC, abstractmethod, which makes the module invalid and matches the Ruff syntax errors.You should resolve the conflict and keep only the imports actually needed for the final design (no
configparserif SysinfoCommand doesn’t read configs):-import json -import os -import shlex -import subprocess -import tempfile -<<<<<<< HEAD -import platform -import logging -======= -import configparser -import platform -import logging -from abc import ABC, abstractmethod ->>>>>>> 9759bba9... Fixed the issue in sysinfo where the dmidecode and fdisk - l commands were executed. - -from abc import ABC, abstractmethod +import json +import os +import shlex +import subprocess +import tempfile +import platform +import logging +from abc import ABC, abstractmethod @@ -DATA_SIZE = 200000 -log = logging.getLogger("avocado.sysinfo") +DATA_SIZE = 200000 +log = logging.getLogger("avocado.sysinfo")
172-208: Unresolved conflict and duplicated sudo decision path in Command.collectThe sudo decision logic in
Command.collect()is currently split by conflict markers between:
- A version that uses injected
sudo_commands/sudo_distrosand guards withif self.sudo_commands and self.sudo_distros:, and- A version that constructs
SysinfoCommand()with no arguments and lets it read configuration itself.This both breaks syntax and contradicts the architecture agreed with the reviewer (config should be read in the plugin/core layer, not in utils). You also need
sudo_flagdefined even when no sudo config is provided.Recommend resolving the conflict in favor of the injected-settings path and always initializing
sudo_flag:@@ - if int(self.timeout) <= 0: - self.timeout = None - - # Determine whether to run with sudo (do not mutate the command string) -<<<<<<< HEAD - sudo_flag = False - if self.sudo_commands and self.sudo_distros: - sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) - sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd) -======= - sysinfo_cmd = SysinfoCommand() - sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd) ->>>>>>> 9759bba9... Fixed the issue in sysinfo where the dmidecode and fdisk - l commands were executed. - log.info("Executing Command%s: %s", " (sudo)" if sudo_flag else "", self.cmd) + if int(self.timeout) <= 0: + self.timeout = None + + # Determine whether to run with sudo (do not mutate the command string) + sudo_flag = False + if self.sudo_commands and self.sudo_distros: + sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) + sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd) + + log.info("Executing Command%s: %s", " (sudo)" if sudo_flag else "", self.cmd)This keeps the decision logic centralized in
SysinfoCommandwhile respecting the “no direct config reads in utils” guideline.
216-219: Minor: exception message formatting does not interpolate{exc}Unrelated to the sudo work, but while you’re touching this file: the
CollectibleExceptionmessage here doesn’t interpolate the exception value; it concatenates a literal"{exc}"string:raise CollectibleException( f'Could not execute "{self.cmd}": ' "{exc}" ) from excThat will produce a message like
Could not execute "cmd": {exc}instead of including the real error.You can fix it with a single f-string:
- raise CollectibleException( - f'Could not execute "{self.cmd}": ' "{exc}" - ) from exc + raise CollectibleException( + f'Could not execute "{self.cmd}": {exc}' + ) from exc
🧹 Nitpick comments (2)
avocado/utils/sysinfo.py (2)
146-156: Command signature change looks compatible; ensure sudo parameters are normalized upstreamThe extended
Command.__init__signature (sudo_commands=None, sudo_distros=None) is backwards compatible because the new parameters are optional and only stored on the instance.Just make sure that call sites pass in normalized values (e.g.
setof lowercased command basenames and distro IDs) so thatSysinfoCommanddoesn’t have to guess or re-normalize them. If you already normalize in the plugin or core sysinfo layer, this is fine as-is.
456-493: use_sudo logic is reasonable but can be slightly clarifiedOnce conflicts are resolved,
use_sudo():
- Immediately returns
Falseifsudo_availableisFalse(non-Linux or no sudo).- On Linux, skips sudo when running as root.
- Checks
/etc/os-releaseand requiresIDto be inself.sudo_distros.This matches the PR intent. You might consider a tiny readability tweak on the root check and logging level, but it’s optional:
- if not self.sudo_available: - return False - system_name = platform.system().lower() - if system_name == "linux": - if hasattr(os, "geteuid") and not os.geteuid(): - return False + if not self.sudo_available: + return False + system_name = platform.system().lower() + if system_name == "linux": + if hasattr(os, "geteuid") and os.geteuid() == 0: + return False try: - with open("/etc/os-release", encoding="utf-8") as f: + with open("/etc/os-release", encoding="utf-8") as f: for line in f: if line.startswith("ID="): os_id = line.strip().split("=")[1].strip('"') return os_id.lower() in self.sudo_distros - except FileNotFoundError: - log.debug("/etc/os-release not found.") + except FileNotFoundError: + log.debug("/etc/os-release not found.") return False return FalseBehavior stays the same but the root check is more explicit and keeps logging at debug level for missing
/etc/os-release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/utils/sysinfo.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.14.8)
avocado/utils/sysinfo.py
21-21: Expected a statement
(invalid-syntax)
21-21: Expected a statement
(invalid-syntax)
21-21: Expected a statement
(invalid-syntax)
21-21: Expected a statement
(invalid-syntax)
24-24: Expected a statement
(invalid-syntax)
24-24: Expected a statement
(invalid-syntax)
24-24: Expected a statement
(invalid-syntax)
24-24: Expected a statement
(invalid-syntax)
24-25: Expected a statement
(invalid-syntax)
29-29: Expected a statement
(invalid-syntax)
29-29: Expected a statement
(invalid-syntax)
29-29: Expected a statement
(invalid-syntax)
29-29: Expected a statement
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-29: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
29-30: Expected an identifier
(invalid-syntax)
188-188: Expected a statement
(invalid-syntax)
188-188: Expected a statement
(invalid-syntax)
188-188: Expected a statement
(invalid-syntax)
188-188: Expected a statement
(invalid-syntax)
189-189: Unexpected indentation
(invalid-syntax)
193-193: Expected a statement
(invalid-syntax)
193-193: Expected a statement
(invalid-syntax)
193-193: Expected a statement
(invalid-syntax)
193-193: Expected a statement
(invalid-syntax)
193-194: Expected a statement
(invalid-syntax)
194-194: Unexpected indentation
(invalid-syntax)
196-196: Expected a statement
(invalid-syntax)
196-196: Expected a statement
(invalid-syntax)
196-196: Expected a statement
(invalid-syntax)
196-196: Expected a statement
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-196: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
196-197: Expected an identifier
(invalid-syntax)
197-197: Unexpected indentation
(invalid-syntax)
429-429: Expected an indented block after class definition
(invalid-syntax)
429-429: Expected a statement
(invalid-syntax)
429-429: Expected a statement
(invalid-syntax)
429-429: Expected a statement
(invalid-syntax)
430-430: Unexpected indentation
(invalid-syntax)
437-437: Expected a statement
(invalid-syntax)
437-437: Expected a statement
(invalid-syntax)
437-437: Expected a statement
(invalid-syntax)
437-437: Expected a statement
(invalid-syntax)
437-438: Expected a statement
(invalid-syntax)
438-438: Unexpected indentation
(invalid-syntax)
454-454: Expected a statement
(invalid-syntax)
454-454: Expected a statement
(invalid-syntax)
454-454: Expected a statement
(invalid-syntax)
454-454: Expected a statement
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-454: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
454-455: Expected an identifier
(invalid-syntax)
456-456: Unexpected indentation
(invalid-syntax)
466-466: Expected a statement
(invalid-syntax)
466-466: Expected a statement
(invalid-syntax)
466-466: Expected a statement
(invalid-syntax)
466-466: Expected a statement
(invalid-syntax)
467-467: Unexpected indentation
(invalid-syntax)
478-478: Expected a statement
(invalid-syntax)
478-478: Expected a statement
(invalid-syntax)
478-478: Expected a statement
(invalid-syntax)
478-478: Expected a statement
(invalid-syntax)
478-479: Expected a statement
(invalid-syntax)
479-479: Unexpected indentation
(invalid-syntax)
490-490: Expected a statement
(invalid-syntax)
490-490: Expected a statement
(invalid-syntax)
490-490: Expected a statement
(invalid-syntax)
490-490: Expected a statement
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-490: Simple statements must be separated by newlines or semicolons
(invalid-syntax)
490-491: Expected an identifier
(invalid-syntax)
491-491: Unexpected indentation
(invalid-syntax)
492-492: unindent does not match any outer indentation level
(invalid-syntax)
493-493: Unexpected indentation
(invalid-syntax)
495-495: unindent does not match any outer indentation level
(invalid-syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task ubi:8.8
🔇 Additional comments (1)
avocado/utils/sysinfo.py (1)
495-510: is_sudo_cmd implementation is robust and aligns with configuration model
is_sudo_cmd()splits the command safely withshlex.split(), handles parsing errors by returningFalse, normalizes to a lowercase basename, and then checks membership inself.sudo_cmds. This is defensive and exactly what you need givensudo_commandsis configured as bare command names.No changes needed here.
xianglongfei-8888
force-pushed
the
xlf_dev
branch
2 times, most recently
from
e774811 to
43cf978
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
avocado/core/sysinfo.py (1)
86-98: Local variableconfigshadows outer scope.The nested function
_load_sudo_listusesconfigas a local variable name (line 95), which could cause confusion withself.configin the enclosing scope and theconfigparsermodule import.Consider renaming for clarity:
def _load_sudo_list(raw_value, key): # pylint: disable=wrong-spelling-in-docstring """ If `raw_value` is a path to an INI file, read `[sysinfo] / key` from it; otherwise, treat `raw_value` itself as a CSV list. """ if not raw_value: return "" if os.path.isfile(raw_value): - config = configparser.ConfigParser() - config.read(raw_value) - return config.get("sysinfo", key, fallback="") + parser = configparser.ConfigParser() + parser.read(raw_value) + return parser.get("sysinfo", key, fallback="") return raw_value
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
avocado/core/sysinfo.py(3 hunks)avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/plugins/sysinfo.py(1 hunks)avocado/utils/sysinfo.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
avocado/core/sysinfo.py (1)
avocado/utils/sysinfo.py (1)
Command(130-207)
avocado/plugins/sysinfo.py (2)
avocado/core/utils/path.py (2)
system_wide_or_base_path(15-47)prepend_base_path(7-12)avocado/core/settings.py (3)
register_option(646-803)section(138-139)key(142-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: Linux with Python 3.12
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: Egg task fedora:40
- GitHub Check: Egg task fedora:41
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Podman Egg task
- GitHub Check: Fedora selftests
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: Build Package (wheel/tarball) for Python 3.11
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: Egg task debian:11.0
- GitHub Check: Build Package (wheel/tarball) for Python 3.12
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Static checks
- GitHub Check: macOS with Python 3.11
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (6)
avocado/etc/avocado/sysinfo.conf (1)
1-6: LGTM! Configuration file is well-structured.The merge conflicts flagged in the previous review have been resolved. The file now cleanly defines
sudo_commandsandsudo_distrosunder the[sysinfo]section with appropriate guidance comments.avocado/core/sysinfo.py (1)
179-207: LGTM! Sudo configuration properly propagated to Command instances.The
sudo_commandsandsudo_distrosare consistently passed to allCommandinstantiations for start, end, and fail collectibles, enabling sudo-aware command execution across the sysinfo lifecycle.avocado/plugins/sysinfo.py (1)
172-192: LGTM! Settings registration aligns with the dual-mode loading approach.The help messages now correctly describe these as "File with list of..." options, and the
_load_sudo_listhelper inavocado/core/sysinfo.pyhandles both scenarios (file path or direct CSV value), addressing the previous review concern about config model confusion.avocado/utils/sysinfo.py (3)
180-185: Clarify: Bothsudo_commandsANDsudo_distrosmust be configured for sudo to apply.The condition on line 182 requires both
sudo_commandsandsudo_distrosto be non-empty for sudo logic to engage. If a user configures onlysudo_commandswithoutsudo_distros(or vice versa), sudo will never be used.This may be intentional (requiring explicit configuration of both), but consider whether an OR condition or separate checks might be more user-friendly, especially if a user forgets to configure one of them.
Please confirm this is the intended behavior. If so, consider adding a debug log when only one is configured to help users troubleshoot:
# Determine whether to run with sudo (do not mutate the command string) sudo_flag = False if self.sudo_commands and self.sudo_distros: sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd) + elif self.sudo_commands or self.sudo_distros: + log.debug("Sudo not applied: both sudo_commands and sudo_distros must be configured") log.info("Executing Command%s: %s", " (sudo)" if sudo_flag else "", self.cmd)
416-448: LGTM! SysinfoCommand properly consumes injected configuration.The class no longer parses configuration internally, addressing the previous review concern. It correctly:
- Only checks
can_sudo()on Linux systems- Skips sudo when running as root (
geteuid() == 0)- Parses
/etc/os-releaseto match the distro ID against the configured list- Handles missing
/etc/os-releasegracefully with a debug log
450-465: LGTM! Command matching is robust.The
is_sudo_cmdmethod correctly:
- Handles empty or malformed commands via exception handling
- Extracts the base command name (handles full paths like
/usr/sbin/dmidecode)- Performs case-insensitive comparison
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
avocado/utils/sysinfo.py (1)
179-195: Performance:SysinfoCommandinstantiated on everycollect()call invokescan_sudo()repeatedly.
SysinfoCommand.__init__callscan_sudo(), which spawns a subprocess (sudo id -u) to check sudo capability. Creating a new instance on each command execution is inefficient, especially when collecting many sysinfo commands.Consider instantiating
SysinfoCommandonce inCommand.__init__or caching thecan_sudo()result:def __init__( self, cmd, timeout=-1, locale="C", sudo_commands=None, sudo_distros=None ): # pylint: disable=R0913 super().__init__(cmd) self._name = self.log_path self.cmd = cmd self.timeout = timeout self.locale = locale self.sudo_commands = sudo_commands self.sudo_distros = sudo_distros + # Pre-compute sudo helper once to avoid repeated can_sudo() calls + self._sysinfo_cmd = None + if self.sudo_commands and self.sudo_distros: + self._sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros)Then in
collect():# Determine whether to run with sudo (do not mutate the command string) sudo_flag = False - if self.sudo_commands and self.sudo_distros: - sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) - sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd) + if self._sysinfo_cmd: + sudo_flag = self._sysinfo_cmd.use_sudo() and self._sysinfo_cmd.is_sudo_cmd(self.cmd)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
avocado/core/sysinfo.py(3 hunks)avocado/utils/sysinfo.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
avocado/core/sysinfo.py (1)
avocado/utils/sysinfo.py (1)
Command(130-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (46)
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: Fedora selftests
- GitHub Check: Windows with Python 3.10
- GitHub Check: Windows with Python 3.13
- GitHub Check: Windows with Python 3.9
- GitHub Check: Windows with Python 3.12
- GitHub Check: Smokecheck on Linux with Python 3.11
- GitHub Check: Smokecheck on Linux with Python 3.10
- GitHub Check: Analyze (python)
- GitHub Check: Code Coverage (3.11)
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
🔇 Additional comments (5)
avocado/core/sysinfo.py (1)
220-249: LGTM!The
Commandinstantiations correctly passsudo_commandsandsudo_distrosto all collectible types (start, end, and end_fail), maintaining consistency with the updated API inavocado/utils/sysinfo.py.avocado/utils/sysinfo.py (4)
16-29: LGTM!The new imports (
logging,platform,can_sudo) and module-level logger are appropriate for the sudo detection functionality. Logger name"avocado.sysinfo"is consistent with the core module.
139-148: LGTM!The addition of
sudo_commandsandsudo_distrosparameters withNonedefaults maintains backward compatibility while enabling the new sudo functionality.
416-448: LGTM with minor observation.The
SysinfoCommandclass correctly:
- Accepts injected configuration (no config parsing in utils layer, addressing prior review feedback)
- Gates
can_sudo()to Linux only- Checks root status via
geteuid()before reading/etc/os-release- Handles missing
/etc/os-releasegracefully
450-465: LGTM!The
is_sudo_cmdmethod correctly parses the command usingshlex.split, extracts the base command name, and handles edge cases (empty/malformed commands) via exception handling.
| # Retrieve the configured paths for sudo commands and distros from the settings dictionary | ||
| sudo_commands_conf = self.config.get("sysinfo.sudo_commands", "") | ||
| sudo_distros_conf = self.config.get("sysinfo.sudo_distros", "") | ||
|
|
||
| if sudo_commands_conf: | ||
| log.info("sudo_commands loaded from config: %s", sudo_commands_conf) | ||
| else: | ||
| log.debug("sudo_commands config is empty or missing") | ||
|
|
||
| if sudo_distros_conf: | ||
| log.info("sudo_distros loaded from config: %s", sudo_distros_conf) | ||
| else: | ||
| log.debug("sudo_distros config is empty or missing") | ||
|
|
||
| def _load_sudo_list(raw_value, key): | ||
| # pylint: disable=wrong-spelling-in-docstring | ||
| """ | ||
| If `raw_value` is a path to an INI file, read `[sysinfo] / key` | ||
| from it; otherwise, treat `raw_value` itself as a CSV list. | ||
| """ | ||
| if not raw_value: | ||
| return "" | ||
| if os.path.isfile(raw_value): | ||
| config = configparser.ConfigParser() | ||
| config.read(raw_value) | ||
| return config.get("sysinfo", key, fallback="") | ||
| return raw_value | ||
|
|
||
| # Retrieve the actual sudo commands and distros values from the config files, | ||
| # falling back to empty string if the keys are missing | ||
| sudo_commands_value = _load_sudo_list(sudo_commands_conf, "sudo_commands") | ||
| sudo_distros_value = _load_sudo_list(sudo_distros_conf, "sudo_distros") | ||
|
|
||
| self.sudo_commands = { | ||
| cmd.strip().lower() for cmd in sudo_commands_value.split(",") if cmd.strip() | ||
| } | ||
|
|
||
| self.sudo_distros = { | ||
| dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip() | ||
| } | ||
|
|
||
| # Retrieve the configured paths for sudo commands and distros from the settings dictionary | ||
| sudo_commands_conf = self.config.get("sysinfo.sudo_commands", "") | ||
| sudo_distros_conf = self.config.get("sysinfo.sudo_distros", "") | ||
|
|
||
| if sudo_commands_conf: | ||
| log.info("sudo_commands loaded from config: %s", sudo_commands_conf) | ||
| else: | ||
| log.debug("sudo_commands config is empty or missing") | ||
|
|
||
| if sudo_distros_conf: | ||
| log.info("sudo_distros loaded from config: %s", sudo_distros_conf) | ||
| else: | ||
| log.debug("sudo_distros config is empty or missing") | ||
|
|
||
| def _load_sudo_list(raw_value, key): | ||
| # pylint: disable=wrong-spelling-in-docstring | ||
| """ | ||
| If `raw_value` is a path to an INI file, read `[sysinfo] / key` | ||
| from it; otherwise, treat `raw_value` itself as a CSV list. | ||
| """ | ||
| if not raw_value: | ||
| return "" | ||
| if os.path.isfile(raw_value): | ||
| config = configparser.ConfigParser() | ||
| config.read(raw_value) | ||
| return config.get("sysinfo", key, fallback="") | ||
| return raw_value | ||
|
|
||
| # Retrieve the actual sudo commands and distros values from the config files, | ||
| # falling back to empty string if the keys are missing | ||
| sudo_commands_value = _load_sudo_list(sudo_commands_conf, "sudo_commands") | ||
| sudo_distros_value = _load_sudo_list(sudo_distros_conf, "sudo_distros") | ||
|
|
||
| self.sudo_commands = { | ||
| cmd.strip().lower() for cmd in sudo_commands_value.split(",") if cmd.strip() | ||
| } | ||
|
|
||
| self.sudo_distros = { | ||
| dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip() | ||
| } |
There was a problem hiding this comment.
Critical: Duplicate code block must be removed.
Lines 72–111 and lines 113–152 are exact duplicates. This appears to be a merge/rebase error. The entire block—config retrieval, logging, _load_sudo_list function definition, and set comprehensions—is repeated twice, causing:
- Redundant file reads and config parsing
- Re-definition of
_load_sudo_list(shadowing the first) - Overwriting
self.sudo_commandsandself.sudo_distroswith identical values
Remove lines 113–152 entirely.
self.sudo_distros = {
dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip()
}
-
- # Retrieve the configured paths for sudo commands and distros from the settings dictionary
- sudo_commands_conf = self.config.get("sysinfo.sudo_commands", "")
- sudo_distros_conf = self.config.get("sysinfo.sudo_distros", "")
-
- if sudo_commands_conf:
- log.info("sudo_commands loaded from config: %s", sudo_commands_conf)
- else:
- log.debug("sudo_commands config is empty or missing")
-
- if sudo_distros_conf:
- log.info("sudo_distros loaded from config: %s", sudo_distros_conf)
- else:
- log.debug("sudo_distros config is empty or missing")
-
- def _load_sudo_list(raw_value, key):
- # pylint: disable=wrong-spelling-in-docstring
- """
- If `raw_value` is a path to an INI file, read `[sysinfo] / key`
- from it; otherwise, treat `raw_value` itself as a CSV list.
- """
- if not raw_value:
- return ""
- if os.path.isfile(raw_value):
- config = configparser.ConfigParser()
- config.read(raw_value)
- return config.get("sysinfo", key, fallback="")
- return raw_value
-
- # Retrieve the actual sudo commands and distros values from the config files,
- # falling back to empty string if the keys are missing
- sudo_commands_value = _load_sudo_list(sudo_commands_conf, "sudo_commands")
- sudo_distros_value = _load_sudo_list(sudo_distros_conf, "sudo_distros")
-
- self.sudo_commands = {
- cmd.strip().lower() for cmd in sudo_commands_value.split(",") if cmd.strip()
- }
-
- self.sudo_distros = {
- dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip()
- }
-
if basedir is None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Retrieve the configured paths for sudo commands and distros from the settings dictionary | |
| sudo_commands_conf = self.config.get("sysinfo.sudo_commands", "") | |
| sudo_distros_conf = self.config.get("sysinfo.sudo_distros", "") | |
| if sudo_commands_conf: | |
| log.info("sudo_commands loaded from config: %s", sudo_commands_conf) | |
| else: | |
| log.debug("sudo_commands config is empty or missing") | |
| if sudo_distros_conf: | |
| log.info("sudo_distros loaded from config: %s", sudo_distros_conf) | |
| else: | |
| log.debug("sudo_distros config is empty or missing") | |
| def _load_sudo_list(raw_value, key): | |
| # pylint: disable=wrong-spelling-in-docstring | |
| """ | |
| If `raw_value` is a path to an INI file, read `[sysinfo] / key` | |
| from it; otherwise, treat `raw_value` itself as a CSV list. | |
| """ | |
| if not raw_value: | |
| return "" | |
| if os.path.isfile(raw_value): | |
| config = configparser.ConfigParser() | |
| config.read(raw_value) | |
| return config.get("sysinfo", key, fallback="") | |
| return raw_value | |
| # Retrieve the actual sudo commands and distros values from the config files, | |
| # falling back to empty string if the keys are missing | |
| sudo_commands_value = _load_sudo_list(sudo_commands_conf, "sudo_commands") | |
| sudo_distros_value = _load_sudo_list(sudo_distros_conf, "sudo_distros") | |
| self.sudo_commands = { | |
| cmd.strip().lower() for cmd in sudo_commands_value.split(",") if cmd.strip() | |
| } | |
| self.sudo_distros = { | |
| dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip() | |
| } | |
| # Retrieve the configured paths for sudo commands and distros from the settings dictionary | |
| sudo_commands_conf = self.config.get("sysinfo.sudo_commands", "") | |
| sudo_distros_conf = self.config.get("sysinfo.sudo_distros", "") | |
| if sudo_commands_conf: | |
| log.info("sudo_commands loaded from config: %s", sudo_commands_conf) | |
| else: | |
| log.debug("sudo_commands config is empty or missing") | |
| if sudo_distros_conf: | |
| log.info("sudo_distros loaded from config: %s", sudo_distros_conf) | |
| else: | |
| log.debug("sudo_distros config is empty or missing") | |
| def _load_sudo_list(raw_value, key): | |
| # pylint: disable=wrong-spelling-in-docstring | |
| """ | |
| If `raw_value` is a path to an INI file, read `[sysinfo] / key` | |
| from it; otherwise, treat `raw_value` itself as a CSV list. | |
| """ | |
| if not raw_value: | |
| return "" | |
| if os.path.isfile(raw_value): | |
| config = configparser.ConfigParser() | |
| config.read(raw_value) | |
| return config.get("sysinfo", key, fallback="") | |
| return raw_value | |
| # Retrieve the actual sudo commands and distros values from the config files, | |
| # falling back to empty string if the keys are missing | |
| sudo_commands_value = _load_sudo_list(sudo_commands_conf, "sudo_commands") | |
| sudo_distros_value = _load_sudo_list(sudo_distros_conf, "sudo_distros") | |
| self.sudo_commands = { | |
| cmd.strip().lower() for cmd in sudo_commands_value.split(",") if cmd.strip() | |
| } | |
| self.sudo_distros = { | |
| dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip() | |
| } | |
| # Retrieve the configured paths for sudo commands and distros from the settings dictionary | |
| sudo_commands_conf = self.config.get("sysinfo.sudo_commands", "") | |
| sudo_distros_conf = self.config.get("sysinfo.sudo_distros", "") | |
| if sudo_commands_conf: | |
| log.info("sudo_commands loaded from config: %s", sudo_commands_conf) | |
| else: | |
| log.debug("sudo_commands config is empty or missing") | |
| if sudo_distros_conf: | |
| log.info("sudo_distros loaded from config: %s", sudo_distros_conf) | |
| else: | |
| log.debug("sudo_distros config is empty or missing") | |
| def _load_sudo_list(raw_value, key): | |
| # pylint: disable=wrong-spelling-in-docstring | |
| """ | |
| If `raw_value` is a path to an INI file, read `[sysinfo] / key` | |
| from it; otherwise, treat `raw_value` itself as a CSV list. | |
| """ | |
| if not raw_value: | |
| return "" | |
| if os.path.isfile(raw_value): | |
| config = configparser.ConfigParser() | |
| config.read(raw_value) | |
| return config.get("sysinfo", key, fallback="") | |
| return raw_value | |
| # Retrieve the actual sudo commands and distros values from the config files, | |
| # falling back to empty string if the keys are missing | |
| sudo_commands_value = _load_sudo_list(sudo_commands_conf, "sudo_commands") | |
| sudo_distros_value = _load_sudo_list(sudo_distros_conf, "sudo_distros") | |
| self.sudo_commands = { | |
| cmd.strip().lower() for cmd in sudo_commands_value.split(",") if cmd.strip() | |
| } | |
| self.sudo_distros = { | |
| dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip() | |
| } | |
| if basedir is None: |
🤖 Prompt for AI Agents
In avocado/core/sysinfo.py around lines 72–152 there is an exact duplicate of
the sudo config loading block (lines 72–111 and 113–152). Remove the second copy
(lines 113–152) so the config retrieval, logging, _load_sudo_list definition,
and the set comprehensions appear only once; ensure any necessary imports used
by that block remain and run tests to confirm no references rely on the removed
duplicate.
4d7fc4f to
94be2e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
avocado/core/sysinfo.py (1)
113-152: Critical: Remove this duplicate code block.This is an exact duplicate of lines 72–111 (already flagged in previous review). The entire block—config retrieval, logging,
_load_sudo_listfunction definition, and set comprehensions—appears twice, causing redundant processing and function re-definition.self.sudo_distros = { dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip() } - # Retrieve the configured paths for sudo commands and distros from the settings dictionary - sudo_commands_conf = self.config.get("sysinfo.sudo_commands", "") - sudo_distros_conf = self.config.get("sysinfo.sudo_distros", "") - - if sudo_commands_conf: - log.info("sudo_commands loaded from config: %s", sudo_commands_conf) - else: - log.debug("sudo_commands config is empty or missing") - - if sudo_distros_conf: - log.info("sudo_distros loaded from config: %s", sudo_distros_conf) - else: - log.debug("sudo_distros config is empty or missing") - - def _load_sudo_list(raw_value, key): - # pylint: disable=wrong-spelling-in-docstring - """ - If `raw_value` is a path to an INI file, read `[sysinfo] / key` - from it; otherwise, treat `raw_value` itself as a CSV list. - """ - if not raw_value: - return "" - if os.path.isfile(raw_value): - config = configparser.ConfigParser() - config.read(raw_value) - return config.get("sysinfo", key, fallback="") - return raw_value - - # Retrieve the actual sudo commands and distros values from the config files, - # falling back to empty string if the keys are missing - sudo_commands_value = _load_sudo_list(sudo_commands_conf, "sudo_commands") - sudo_distros_value = _load_sudo_list(sudo_distros_conf, "sudo_distros") - - self.sudo_commands = { - cmd.strip().lower() for cmd in sudo_commands_value.split(",") if cmd.strip() - } - - self.sudo_distros = { - dst.strip().lower() for dst in sudo_distros_value.split(",") if dst.strip() - } - if basedir is None:
🧹 Nitpick comments (1)
avocado/utils/sysinfo.py (1)
179-195: Consider caching theSysinfoCommandinstance.A new
SysinfoCommandis instantiated on everycollect()call, which redundantly checkscan_sudo()each time. Sincesudo_commandsandsudo_distrosare immutable for theCommandinstance, consider creating theSysinfoCommandonce in__init__or lazily caching it.def __init__( self, cmd, timeout=-1, locale="C", sudo_commands=None, sudo_distros=None ): # pylint: disable=R0913 super().__init__(cmd) self._name = self.log_path self.cmd = cmd self.timeout = timeout self.locale = locale self.sudo_commands = sudo_commands self.sudo_distros = sudo_distros + self._sysinfo_cmd = None + + @property + def _sudo_helper(self): + if self._sysinfo_cmd is None and self.sudo_commands and self.sudo_distros: + self._sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) + return self._sysinfo_cmdThen in
collect():- sudo_flag = False - if self.sudo_commands and self.sudo_distros: - sysinfo_cmd = SysinfoCommand(self.sudo_commands, self.sudo_distros) - sudo_flag = sysinfo_cmd.use_sudo() and sysinfo_cmd.is_sudo_cmd(self.cmd) + sudo_flag = False + if self._sudo_helper: + sudo_flag = self._sudo_helper.use_sudo() and self._sudo_helper.is_sudo_cmd(self.cmd)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
avocado/core/sysinfo.py(3 hunks)avocado/utils/sysinfo.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
avocado/core/sysinfo.py (1)
avocado/utils/sysinfo.py (1)
Command(130-207)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (46)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: Windows with Python 3.12
- GitHub Check: Windows with Python 3.9
- GitHub Check: Egg task fedora:40
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Version task fedora:41
- GitHub Check: Version task ubi:9.2
- GitHub Check: Version task fedora:40
- GitHub Check: macOS with Python 3.11
- GitHub Check: Version task debian:11.0
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (6)
avocado/utils/sysinfo.py (4)
17-29: LGTM: Imports and logger setup are appropriate.The new imports (
logging,platform,can_sudo) and the module-level logger are correctly scoped for the new sudo-aware functionality.
139-148: LGTM: Clean parameter extension.The
sudo_commandsandsudo_distrosparameters are correctly added with safe defaults.
416-448: LGTM:SysinfoCommandfollows the injected-settings pattern.The class correctly receives configuration via constructor parameters rather than reading config files directly, addressing the previous review feedback. The
use_sudo()logic properly handles root detection, platform checks, and distro matching.
450-465: LGTM: Robust command parsing.The
is_sudo_cmd()method safely parses the command string, handles edge cases with appropriate exception handling, and correctly extracts the command basename for comparison.avocado/core/sysinfo.py (2)
72-111: LGTM: Sudo configuration loading logic.The
_load_sudo_listhelper correctly handles both INI file paths and direct CSV values, providing flexibility for configuration. The logging of config presence is helpful for debugging.
220-249: LGTM: Configuration passed to Command instances.The
sudo_commandsandsudo_distrosare correctly propagated to allCommandinstantiations, ensuring consistent sudo behavior across start, end, and fail collectibles.
xianglongfei-8888
force-pushed
the
xlf_dev
branch
2 times, most recently
from
40f3299 to
d5717d9
Compare
@clebergnu Thanks you for your suggestion! It's excellent, and the refactoring has been completed. |
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
010a305 to
cb9c412
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
avocado/core/sysinfo.py (1)
86-98: Consider adding exception handling for ConfigParser operations.The
_load_sudo_listhelper provides flexible configuration by accepting either file paths or direct CSV strings. However, if the INI file exists but is malformed,parser.read()orparser.get()could raise exceptions that would bubble up uncaught.Consider wrapping the file-reading logic:
if os.path.isfile(raw_value): + try: parser = configparser.ConfigParser() parser.read(raw_value) return parser.get("sysinfo", key, fallback="") + except (configparser.Error, IOError) as e: + log.warning("Failed to read sudo config from %s: %s", raw_value, e) + return "" return raw_valueavocado/plugins/sysinfo.py (2)
172-180: Consider clarifying that the option accepts both file paths and CSV values.The help text states "File with list of commands that require sudo", but the implementation in
avocado/core/sysinfo.pyaccepts both:
- A path to an INI file with a
[sysinfo]section- A direct comma-separated list of commands
Update the help text to reflect both modes:
- help_msg = "File with list of commands that require sudo" + help_msg = ( + "Path to config file with [sysinfo] sudo_commands, " + "or comma-separated list of commands that require sudo" + )
182-192: Consider clarifying that the option accepts both file paths and CSV values.Similar to
sudo_commands, the help text describes this as a file path only, but the implementation also accepts direct CSV values.Update the help text for consistency:
help_msg = ( - "File with list of distributions (values matching ID= in /etc/os-release) " + "Path to config file with [sysinfo] sudo_distros, " + "or comma-separated list of distributions (values matching ID= in /etc/os-release) " "that require sudo" )avocado/utils/sysinfo.py (1)
431-454: Consider caching os-release parsing and broadening exception handling.The method reads and parses
/etc/os-releaseon every call. While sysinfo commands aren't run frequently, consider:
- Caching: Parse
/etc/os-releaseonce in__init__and store the result- Exception handling: Line 450 only catches
FileNotFoundError, but the file could be malformed or have permission issuesdef __init__(self, sudo_commands=None, sudo_distros=None): self.sudo_cmds = sudo_commands if sudo_commands else set() self.sudo_distros = sudo_distros if sudo_distros else set() self.sudo_available = False + self._os_id = None # Only attempt sudo capability detection on Linux, where it is relevant. if platform.system().lower() == "linux": self.sudo_available = can_sudo() + self._os_id = self._read_os_id() + + def _read_os_id(self): + """Read OS ID from /etc/os-release.""" + try: + with open("/etc/os-release", encoding="utf-8") as f: + for line in f: + if line.startswith("ID="): + return line.strip().split("=")[1].strip('"').lower() + except (FileNotFoundError, IOError, IndexError, ValueError) as e: + log.debug("Could not read OS ID from /etc/os-release: %s", e) + return None def use_sudo(self): """ Determine if 'sudo' should be used based on the system type. Returns: bool: True if 'sudo' should be used, False otherwise. """ if not self.sudo_available: return False system_name = platform.system().lower() if system_name == "linux": if hasattr(os, "geteuid") and not os.geteuid(): return False - try: - with open("/etc/os-release", encoding="utf-8") as f: - for line in f: - if line.startswith("ID="): - os_id = line.strip().split("=")[1].strip('"') - return os_id.lower() in self.sudo_distros - except FileNotFoundError: - log.debug("/etc/os-release not found.") - return False + if self._os_id: + return self._os_id in self.sudo_distros return False return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
avocado/core/sysinfo.py(3 hunks)avocado/etc/avocado/sysinfo.conf(1 hunks)avocado/plugins/sysinfo.py(1 hunks)avocado/utils/sysinfo.py(5 hunks)avocado/utils/wait.py(2 hunks)selftests/check.py(1 hunks)selftests/functional/utils/wait.py(1 hunks)selftests/unit/utils/wait.py(1 hunks)spell.ignore(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- selftests/check.py
🚧 Files skipped from review as they are similar to previous changes (1)
- avocado/etc/avocado/sysinfo.conf
🧰 Additional context used
🧬 Code graph analysis (5)
selftests/unit/utils/wait.py (1)
avocado/utils/wait.py (1)
wait_for(15-104)
selftests/functional/utils/wait.py (2)
selftests/utils.py (1)
TestCaseTmpDir(167-174)avocado/utils/wait.py (1)
wait_for(15-104)
avocado/plugins/sysinfo.py (2)
avocado/core/utils/path.py (2)
system_wide_or_base_path(15-47)prepend_base_path(7-12)avocado/core/settings.py (3)
register_option(646-803)section(138-139)key(142-143)
avocado/utils/sysinfo.py (1)
avocado/utils/process.py (2)
can_sudo(65-86)system(1023-1085)
avocado/core/sysinfo.py (1)
avocado/utils/sysinfo.py (1)
Command(130-213)
🔇 Additional comments (13)
spell.ignore (1)
813-815: LGTM!The additions are appropriate:
- "truthy" and "falsy" are standard programming terminology
- "myfile" is used in documentation examples (e.g., wait.py docstring)
avocado/utils/wait.py (2)
1-6: LGTM!The module docstring clearly explains the purpose and use cases for the wait utilities.
16-85: LGTM!The expanded documentation is comprehensive and includes excellent examples covering common use cases. The parameter descriptions are detailed and the examples demonstrate practical scenarios.
selftests/unit/utils/wait.py (1)
1-232: LGTM!Comprehensive unit test coverage for wait.wait_for including:
- Success scenarios (immediate and delayed)
- Timeout handling
- Argument passing (positional and keyword)
- Logging behavior
- Edge cases (zero timeout, large step, empty values)
- Various return types and callable objects
The test suite is well-structured and follows best practices.
selftests/functional/utils/wait.py (1)
1-54: LGTM!Functional tests provide real-world validation of wait.wait_for behavior with actual I/O operations. The tests appropriately use threading to simulate asynchronous conditions and include reasonable timing tolerances.
avocado/core/sysinfo.py (3)
72-84: LGTM!Configuration retrieval is properly implemented with informative logging for both presence and absence of settings.
100-111: LGTM!The sudo configuration values are properly parsed into normalized lowercase sets with whitespace handling.
179-208: LGTM!The sudo configuration is consistently passed to all Command instances (start_collectibles, end_collectibles, and end_fail_collectibles), ensuring uniform sudo behavior across collection phases.
avocado/utils/sysinfo.py (5)
17-17: LGTM!The necessary imports and module-level logger are properly added to support the new sudo functionality.
Also applies to: 19-19, 26-26, 29-29
139-155: LGTM!The Command class properly extends its interface to accept sudo configuration and provides a lazy-initialization pattern via the
_sudoerproperty to createSysinfoCommandinstances only when needed.
186-202: LGTM!The sudo determination logic correctly:
- Computes sudo_flag without mutating the command string
- Logs sudo usage for observability
- Passes the sudo flag to
process.run()
422-429: LGTM!The
SysinfoCommandinitialization properly:
- Accepts injected configuration (no direct config reading)
- Initializes sudo_cmds and sudo_distros as sets with defaults
- Checks sudo availability only on Linux systems
456-471: LGTM!The
is_sudo_cmdmethod properly:
- Uses
shlex.splitto parse the command correctly- Handles empty commands and parsing errors gracefully
- Normalizes the command basename to lowercase for comparison
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
cb9c412 to
eb43cc1
Compare
…were executed. Also added sysinfo.conf to RPM spec file to resolve packaging error. Signed-off-by: xianglongfei <xianglongfei@uniontech.com>
xianglongfei-8888
force-pushed
the
xlf_dev
branch
from
eb43cc1 to
b11d1aa
Compare
2 participants
1.Compatible with UOS/Deepin operating system, the system can also be extended
2.Executing the sudo command is configurable and does not affect other systems, such as "sudo-commands=dmidecode, fdisk"
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.