Special support for golang builder images#2495
Special support for golang builder images#2495thegreyd wants to merge 10 commits intoopenshift-eng:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCollects Golang-builder NVRs when release-based build_system derivation raises ValueError, resolves their mappings via konflux then falls back to brew and returns the mapping; when none collected, existing release-suffix routing remains. Also changes artcommon group naming for Golang builder NVRs to include RHEL EL version. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant util as util.py
participant Konflux
participant Brew
Caller->>util: provide list of NVRs
util->>util: for each NVR, try derive build_system from release
util->>util: on ValueError and NVR indicates golang-builder -> collect golang_builder_nvrs
alt golang-builder NVRs exist
util->>Konflux: get_golang_container_nvrs_konflux(golang_builder_nvrs)
alt Konflux succeeds
Konflux-->>util: mapping
util-->>Caller: return mapping
else Konflux fails
util->>Brew: get_golang_container_nvrs_brew(golang_builder_nvrs)
Brew-->>util: mapping
util-->>Caller: return mapping
end
else no golang-builder NVRs
util->>util: determine build_system from release suffix for NVRs
alt build_system == "konflux"
util->>Konflux: get_golang_container_nvrs_konflux(nvrs)
Konflux-->>util: mapping
util-->>Caller: return mapping
else build_system == "brew"
util->>Brew: get_golang_container_nvrs_brew(nvrs)
Brew-->>util: mapping
util-->>Caller: return mapping
else
util-->>Caller: log unsupported/error and handle accordingly
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@elliott/elliottlib/util.py`:
- Around line 416-423: The loop that scans nvrs in get_golang_container_nvrs
currently sets build_system = 'konflux' as soon as GOLANG_BUILDER_IMAGE_NAME is
seen, which forces konflux even if a brew NVR exists; change the logic so you do
not assign the default inside the loop — instead, only record that a
golang-builder image was seen (e.g., a local flag) and continue scanning all
nvrs for brew indicators, then after the loop decide build_system = 'konflux'
only if no brew was detected; update references to GOLANG_BUILDER_IMAGE_NAME,
the nvrs iteration and the build_system variable accordingly so unit tests won't
initialize KonfluxDb prematurely.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@elliott/elliottlib/util.py`:
- Around line 438-444: The early return after handling golang_builder_nvrs
currently discards non-builder NVRs; modify the logic in the block referencing
golang_builder_nvrs so you only short-circuit and return golang_map when
build_system is None (i.e., all NVRs are golang builders). Specifically, in the
code that calls get_golang_container_nvrs_konflux(...) and falls back to
get_golang_container_nvrs_brew(...), add a check of build_system is None before
returning golang_map so that when build_system is detected the function
continues to process the full NVR list instead of exiting early.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@elliott/elliottlib/util.py`:
- Around line 426-427: The membership test and exception re-raise are wrong in
the block that checks nvr[0] against GOLANG_BUILDER_IMAGE_NAME (around the code
using variables GOLANG_BUILDER_IMAGE_NAME and nvr) — replace the conditional "if
not GOLANG_BUILDER_IMAGE_NAME in nvr[0]:" with the proper Python syntax "if
GOLANG_BUILDER_IMAGE_NAME not in nvr[0]:" and change the "raise e" re-raise to a
bare "raise" so the original traceback is preserved (locate this in util.py
where nvr and the exception variable e are used).
🧹 Nitpick comments (1)
elliott/elliottlib/util.py (1)
442-442: Consider narrowing the exception type.Catching bare
Exceptionis flagged by BLE001. If only connection/database errors fromKonfluxDbare expected, catching a more specific exception (or a tuple of expected exceptions) would avoid masking unexpected bugs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@artcommon/artcommonlib/util.py`:
- Around line 443-449: The code builds a rhel-... group by calling
isolate_el_version_in_release(nvr) without guarding for None or exceptions,
which can produce "rhel-None-…"; update the block where match is found (the
match = re.search... branch) so that you call isolate_el_version_in_release
inside a try/except and verify the returned el_v is truthy, and only return
f"rhel-{el_v}-golang-{major}.{minor}" when el_v is valid; otherwise fall back to
returning the openshift group (f"openshift-{major}.{minor}") to avoid emitting
invalid group names (references: GOLANG_BUILDER_IMAGE_NAME,
isolate_el_version_in_release, match/groups, and the f-string returns).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@thegreyd: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
ref: https://redhat-internal.slack.com/archives/C07RAB68T5G/p1771000781276789
Fixes a couple of issues