Add list_repo_pkgs to modules/pkgng.py - #70321
Jsollvander wants to merge 2 commits into
Conversation
|
Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here's some information that may help as you continue your Salt journey. There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar. |
twangboy
left a comment
There was a problem hiding this comment.
1. Bug in Output Parsing (ValueError: not enough values to unpack):
Lines 2380–2382:
for line in salt.utils.itertools.split(out, "\n"):
pkg, version = line.strip().rsplit("-", 1)
ret.setdefault(pkg, []).append(version)Problem: If FreeBSD's pkg search outputs an empty line, a malformed line, or if out is empty, line.strip().rsplit("-", 1) will raise a ValueError when unpacking into pkg, version. Additionally, package names or version strings could theoretically lack a -.
Fix: Guard against empty lines and ensure rsplit produced two elements:
for line in salt.utils.itertools.split(out, "\n"):
line = line.strip()
if not line or "-" not in line:
continue
pkg, version = line.rsplit("-", 1)
ret.setdefault(pkg, []).append(version)2. Inaccurate Docstring Example:
The docstring example (lines 2340–2345) shows Ubuntu/Debian version strings (4.3-14ubuntu1.1). Because pkgng is specific to FreeBSD, updating the example to reflect FreeBSD package versions (e.g., bash-5.2.21, nginx-1.26.1,2) would keep documentation accurate.
3. Missing Unit Test for Multiple Arguments / Globs:
test_list_repo_pkgs currently only tests passing a single package ("vim"). Adding a unit test that verifies passing multiple package arguments ("vim", "zsh") or empty output parsing edge cases would ensure full coverage of the execution path.
Make docstring more accurateg. Handle empty lines better and check for "-" before rsplit(). Add two more tests, one with multiple args and one for globs.
What does this PR do?
See discussion for details.
What issues does this PR fix or reference?
Fixes #70317.
Previous Behavior
New Behavior
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes
This is my first time contributing code here. Let me know if I need to fix anything.