package: keep the full name if not grub2 or shim
#1002
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with package name parsing by ensuring that only grub2 and shim package names are shortened, while others are kept in full. This is a good fix for the reported problem and includes a new unit test. My review includes a couple of suggestions for improvement.
First, the fix appears to be partial. The same incorrect parsing logic that is fixed in this PR for packages with an architecture suffix still exists for packages without one within the same function. I've recommended refactoring to apply the fix consistently.
Second, while it's great that a new test was added, I've suggested expanding it into a table-driven test to cover more scenarios, which would also help catch the inconsistency mentioned above. This would improve test coverage and maintainability.
| let (name, _) = if ["grub2", "shim"].iter().any(|p| name_str.starts_with(p)) { | ||
| name_str.split_once('-').unwrap_or((&name_str, "")) | ||
| } else { | ||
| (name_str.as_str(), "") | ||
| }; |
There was a problem hiding this comment.
This change correctly handles package names that are not grub2 or shim. However, the same incorrect parsing logic that was just fixed here still exists earlier in this function for packages that don't end with an architecture suffix (lines 106-112).
The logic pkg.split_once('-') on line 107 will incorrectly parse a package like test-bootupd-payload-1.0-1.
To ensure consistent and correct parsing, consider refactoring this function to apply the same corrected logic for both cases. It might be possible to unify the parsing logic for both branches.
On a related note, for better performance and code style, you could define ["grub2", "shim"] as a static or const slice at the module level to avoid re-creating it on each call.
HuijingHei
force-pushed
the
package-name
branch
from
5d6aa99 to
4f1e13e
Compare
When doing tests for coreos#995, find that the package `test-bootupd-payload-1.0-1.x86_64` is parsed to `test-1.0-1` that is confused, maybe should keep the full name if it is not `grub2` or `shim`.
HuijingHei
force-pushed
the
package-name
branch
from
4f1e13e to
a106821
Compare
|
I think it's what I wrote in #986 (review). When we don't have the RPM db to figure out the package name, we will have to assume that the epoch or version starts with a number to properly split the name at the beginning. Or we ask packagers to not place the files in a folder with names with dashes. It's the not case for the bootloaders we care about right now. |
|
|
||
| let (name, _) = name_str.split_once('-').unwrap_or((&name_str, "")); | ||
| // Only cut the packages name that we know | ||
| let (name, _) = if ["grub2", "shim"].iter().any(|p| name_str.starts_with(p)) { |
There was a problem hiding this comment.
I would do something like
while true
next = split.next()
if next.starts_with_number() == false
name.join("-", next)
else
break
fi
endwhile
then is next epoch or version, etc.
In other words, maybe let's use |
Agree, currently we have only grub2 & shim that will sync with upstream change. |
SGTM, and no need for this now. Thank you for the pointer. |
2 participants
When doing tests for #995, find that the package
test-bootupd-payload-1.0-1.x86_64is parsed totest-1.0-1that is confused, maybe should keep the full name if it is notgrub2orshim.