Skip to content

Fix buildroot check for c8s z-stream rebuilds - #659

Merged
opohorel merged 1 commit into
packit:mainfrom
opohorel:buildroot_check
Jul 8, 2026
Merged

opohorel merged 1 commit into
packit:mainfrom
opohorel:buildroot_check

Conversation

@opohorel

@opohorel opohorel commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

c8s is EOL — builds always go to Brew, never CentOS Stream Koji. Skip the stale c8s-build tag check and only verify the Brew z-stream buildroot (rhel-8.10.0-z-build).

Assisted-by: Claude (Cursor)

@qodo-for-packit

Copy link
Copy Markdown

PR Summary by Qodo

Skip CentOS Stream buildroot checks for c8s Z-stream rebuilds

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Treat c8s as EOL: stop checking the CentOS Stream Koji buildroot tag.
• For c8s Z-stream fix versions, validate only the Brew z-stream buildroot.
• Preserve existing CS-first dual-buildroot checks for non-c8s CS branches.
Diagram

graph TD
  A["Build/PR validation"] --> B["_resolve_buildroot_checks()"] --> C["Branch type + fix_version"]
  C --> D{ "CS branch?" }
  D -->|"yes"| E{ "Z-stream (.z)?" }
  E -->|"c8s"| F["Brew: rhel-X.Y-z-build"]
  E -->|"non-c8s"| G["CS Koji tag + Brew z-build"]
  D -->|"no"| H["Brew: branch -build/-z-build"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize EOL branch capabilities/config
  • ➕ Avoids hardcoding "c8s" inside buildroot resolution logic
  • ➕ Easier to extend when other branches become EOL or change build behavior
  • ➖ Introduces additional configuration surface area
  • ➖ May be overkill if c8s is the only exception and is being phased out
2. Infer build system by querying tag existence
  • ➕ Removes the need for branch-specific conditionals
  • ➕ Automatically adapts if tags/build systems change
  • ➖ Adds network calls and failure modes to control flow
  • ➖ More complexity and potentially slower validation

Recommendation: Current approach is appropriate for a targeted, low-risk fix, but consider moving the "EOL branch" concept into a centralized capability map if more exceptions are expected. Querying tag existence is more flexible but adds complexity and operational risk.

Files changed (1) +11 / -5

Bug fix (1) +11 / -5
utils.pySpecial-case c8s to check only Brew for Z-stream buildroots +11/-5

Special-case c8s to check only Brew for Z-stream buildroots

• Updates _resolve_buildroot_checks() to treat c8s as an EOL CentOS Stream branch: for Z-stream fix versions it returns only the Brew rhel-X.Y-z-build check. Other CS branches keep the existing dual-check behavior (CS Koji tag first, then Brew z-build) and non-Z-stream CS branches continue to check only the CS Koji build tag.

ymir/common/utils.py

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 updates the buildroot check resolution logic in ymir/common/utils.py to handle the EOL CentOS 8 Stream (c8s) branch as an exception, querying only the Brew Z-stream buildroot. The reviewer noted that if target_branch is 'c8s' but is_zstream is False, the logic incorrectly falls back to checking the CentOS Stream Koji buildroot. They suggested handling 'c8s' separately to prevent this and raising an error if a valid RHEL version cannot be parsed.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread ymir/common/utils.py
Comment thread ymir/common/utils.py Outdated
rhel_branch = construct_internal_branch_name(major, minor)
checks.append((BREWHUB_URL, f"{rhel_branch}-z-build"))
return checks
if target_branch == "c8s":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have already some logic for checking the maintenance version, could we use it here instead of hardcoding?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, I've moved the get_maintenance_majors() from jira.py to version_utils.py so we can reuse the function

@qodo-for-packit

qodo-for-packit Bot commented Jul 8, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 7 rules

Grey Divider


Informational

1. Untested c8s buildroot path 🐞 Bug ⚙ Maintainability
Description
The newly added c8s-specific return path in _resolve_buildroot_checks has no direct unit test
coverage, so future refactors could silently change the buildroot selection behavior. This function
feeds check_build_in_buildroot, which is used to decide whether to postpone rebuild triage when a
dependency isn’t yet in the target buildroot.
Code

ymir/common/utils.py[R241-246]

+            if target_branch == "c8s":
+                return [(BREWHUB_URL, f"{rhel_branch}-z-build")]
+            return [
+                (CENTOS_STREAM_KOJIHUB_URL, f"{target_branch}-build"),
+                (BREWHUB_URL, f"{rhel_branch}-z-build"),
+            ]
Relevance

⭐⭐⭐ High

Team often adds/requests tests for new logic; PR #551 expanded utils tests, PR #498 sought new
unit-test coverage.

PR-#551
PR-#498

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR adds a new c8s-only return path inside _resolve_buildroot_checks, but the existing unit
tests in test_utils.py do not import or exercise
_resolve_buildroot_checks/check_build_in_buildroot. This logic is used by the triage workflow
when calling check_build_in_buildroot to decide whether to postpone rebuilds.

ymir/common/utils.py[225-250]
ymir/common/tests/unit/test_utils.py[2-20]
ymir/agents/triage_agent.py[696-736]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A new c8s-specific branch was added to `_resolve_buildroot_checks(...)`, but there is no unit test that asserts the expected (koji_url, tag) output for:
- `target_branch="c8s"` with a Z-stream `fix_version` (should return only the Brew z-stream tag)
- non-c8s CS branches with Z-stream `fix_version` (should return both CS Koji tag and Brew z-stream tag)

### Issue Context
`check_build_in_buildroot(...)` calls `_resolve_buildroot_checks(...)` and is used in triage flow to determine whether a rebuild should proceed or be postponed based on dependency presence in buildroots.

### Fix Focus Areas
- ymir/common/utils.py[225-250]
- ymir/common/tests/unit/test_utils.py[2-20]
- ymir/agents/triage_agent.py[696-736]

### What to implement
1. Add unit tests (in `ymir/common/tests/unit/test_utils.py` or a new unit test module) that call `_resolve_buildroot_checks` directly and assert exact returned lists for:
  - `("c8s", "rhel-8.10.z")` -> `[(BREWHUB_URL, "rhel-8.10.0-z-build")]`
  - `("c9s", "rhel-9.7.z")` -> `[(CENTOS_STREAM_KOJIHUB_URL, "c9s-build"), (BREWHUB_URL, "rhel-9.7.0-z-build")]`
  - (optional) non-zstream CS: `("c9s", "rhel-9.7")` -> `[(CENTOS_STREAM_KOJIHUB_URL, "c9s-build")]`
2. Keep tests hermetic (no Koji network calls); this is pure string/branch resolution logic.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread ymir/common/utils.py Outdated
c8s is EOL — builds always go to Brew, never CentOS Stream Koji.
Skip the stale c8s-build tag check and only verify the Brew
z-stream buildroot (rhel-8.10.0-z-build).

Assisted-by: Claude (Cursor)
@opohorel
opohorel requested a review from lbarcziova July 8, 2026 09:11

@lbarcziova lbarcziova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@opohorel
opohorel merged commit 9b5fdb8 into packit:main Jul 8, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants