Update version checks for nvme_core.io_timeout on Azure#531
Update version checks for nvme_core.io_timeout on Azure#531achilleas-k wants to merge 1 commit intoosbuild:mainfrom
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The equality check against explicit strings for
host.system_info.releasemay be brittle if the value includes minor/patch versions (e.g.9.6.1), consider using a proper version comparison or a prefix/regex match to ensure all intended sub-versions are covered. - To make the special-case handling clearer and easier to maintain, consider extracting the allowed releases ("9.6", "9.7", "10.0", "10.1") into a named constant or set near the top of the file instead of hardcoding them in the conditional.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The equality check against explicit strings for `host.system_info.release` may be brittle if the value includes minor/patch versions (e.g. `9.6.1`), consider using a proper version comparison or a prefix/regex match to ensure all intended sub-versions are covered.
- To make the special-case handling clearer and easier to maintain, consider extracting the allowed releases ("9.6", "9.7", "10.0", "10.1") into a named constant or set near the top of the file instead of hardcoding them in the conditional.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
027995f to
0c41719
Compare
The command line argument was dropped on 9.8+ and 10.2+. See osbuild/images#2127 as well as RHEL-126678 and RHEL-127204.
0c41719 to
1bb81a4
Compare
|
I think given the current state of things, we're going to have to ignore CI failures here until I chip away at every change that needs to be made. Alternatively, I can make one PR that updates every test to match the current state of composer. Not sure what the repo maintainers prefer. |
|
@achilleas-k I'd usually prefer to have individual PRs for those changes but given the amount of work and the amount of PRs we will have to merge while flying blind I would lean towards a one PR approach. |
Not a complete one. But I think I compile one from the PR I have open and start chipping away. |
The command line argument was dropped on 9.8+ and 10.2+. See osbuild/images#2127 as well as RHEL-126678 and RHEL-127204.