-
Notifications
You must be signed in to change notification settings - Fork 21
Feat: Add utility function for getting previous errata #241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a utility function, get_previous_erratum, to find a preceding erratum based on a set of criteria. The implementation correctly adds new data classes for RHEL versions and streams, which improves type safety and code clarity. However, the review identified some critical issues related to code robustness and performance. The current implementation makes unsafe assumptions about API response structures, which could lead to runtime errors if dictionaries or lists are empty. Additionally, there is a significant performance concern due to API calls being made inside a loop. The detailed comments below provide specific suggestions to address these issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark two things for advice. 😄
|
Good work figuring out the APIs and coming up with something! I'm a bit worried about the performance of your solution - there can be a lot of errata for a package and having to loop through and list all their builds could be quite slow and resource intensive. Also, having to take an arbitrary RHEL product version string and parse it into something comparable to another is hard - how does So I tried to come up with something a little different - but wow this is so complex - I spent hours digging through the erratatool API docs and sources to try and understand how everything relates. 😅 . One of the complexities is "Product Version" vs. "Release" - for RHEL 8/9/10, the are almost identical - there's a [As I understand it, the basic idea of the split is that a layered product might have a single release that ships packages that work on top of multiple versions of RHEL - so it would have separate product versions for "1.2 release of layered product for rhel 8" and "1.2 release of layered product for rhel 9"] Another thing is "ReleasedBuild" records. They are almost impossible to find in the UI, but they are maintained (externally in some fashion from erratatool) to keep track of what is shipping in each product version. So I can: $ curl -u : --negotiate 'https://errata.engineering.redhat.com/api/v1/product_versions/RHEL-10.0.Z/released_builds/glib2' | jq
{
"build": "glib2-2.80.4-4.###el10_0.6",
"errata_id": 152126,
"created_at": "2024-08-27T05:57:05Z",
"updated_at": "2025-07-14T08:38:23Z"
}the twist here is that for unreleased releases, records exist, but they are wrong. The 90% solution
Pending REL_PREP errata?The above accomplishes the original goal of finding the relevant "SHIPPED_LIVE" errata for the package, but do we want to also consider errata in REL_PREP that will be released before this erratum?
The way we would implement that:
We don't need to list the builds - we know that one exists for this package - we can just look at the release for the errata. Note: I don't think we ever need to look at previous major versions - there will always be a package for the same major version, unless we're working on a new version of RHEL, which is not in scope at the moment. |
@owtaylor Just a quick question. Do we check |
|
We dopnm
Before RHEL-10.y is released publically, both RHEL-10.y is released both Normally, that will be released. The only case I can think of where it would not be released would be someone getting an early start on y-stream ( |
108681c to
338261d
Compare
338261d to
f290d93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely getting there! a bunch of mostly style suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! Working on it!
f290d93 to
07b2a9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks quite functional at this point - the comments below are mostly just trying to figure out how to make things more readable and maintainable for the future.
supervisor/errata_utils.py
Outdated
|
|
||
| @staticmethod | ||
| def from_str(version_string: str) -> RHELVersion | None: | ||
| pattern = r"RHEL-(\d+)\.(\d+)(?:\.(\d+))?\.(Z|GA)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd end it with \.([^\d].+)$ what you have turns "Z.AUS" into "Z" because it doesn't anchor at the end, which might be intentional but is confusing. Let's say that if it returns non-null, then str(RHELVersion.from_str(s)) must be s. In fact, add an assertion to this effect at the end of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I change it to \.([^\d].*)$ because \.([^\d].+)$ can not catch the version string with one character stream (i.e. RHEL-10.1.Z).
07b2a9c to
6022926
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, needs a doc comment for the new function. Once you do that, can you trigger a re-review from Gemini with gemini review ? It might find a few things.
|
Planing to add docstring tmrw. Will triger a gemini review after that! Let's see what it'll find! |
6022926 to
042a589
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new utility function to find the previous erratum for a package, which is a valuable addition. The implementation includes new data models for RHEL versions and releases, and logic to traverse the release history.
My review has identified a few areas for improvement:
- A critical bug that could cause a runtime
TypeErrorwhen searching for the latest erratum. - A potential performance bottleneck due to multiple API calls within a loop.
- Some correctness issues related to return types and discrepancies between the implementation and the feature description.
- Minor suggestions for improving testability and robustness.
Please see the detailed comments for specific suggestions.
|
@owtaylor In the previous comment-style review, gemini mentioned we can make |
When I reviewed my thought process on this was:
I'll stand by by that thinking :-) |
7b0c884 to
5e8c097
Compare
|
@owtaylor Could you give it a quick overlook? I think it's ready to be merged! 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One editing suggestion, otherwise looks great and is ready merge.
This ultility function finds the previous errata with given errata ID (current) and package name. The output will be a errata with the closest RHEL release version. Here we pick REL_PREP over SHIPPED_ALIVE.
5e8c097 to
0ea18f6
Compare
|
Thanks for the review!! 😁 |
This utility function finds the previous errata with given errata ID (current)
and package name. The output will be a errata with the closest RHEL release
version. Here we pick REL_PREP over SHIPPED_ALIVE.