ART-14381: enforce network mode in find-builds #2372
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@fgallott: This pull request references ART-14381 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[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 |
WalkthroughCentralized konflux network-mode resolution was added to artcommon, removed from doozer, Elliott's Konflux build retrieval was made network-mode-aware (hermetic vs non-hermetic), and tests were added/updated to validate hermetic filtering and network-mode-aware behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Elliott CLI
participant Meta as MetadataBase / ImageMetadata
participant Query as Build Query Engine
participant BQ as BigQuery
CLI->>Meta: request get_konflux_network_mode(image)
Meta->>Meta: check runtime override
alt override present
Meta-->>CLI: return override
else
Meta->>Meta: check image konflux config
alt image config present
Meta-->>CLI: return image mode
else
Meta->>Meta: check group konflux config
alt group config present
Meta-->>CLI: return group mode
else
Meta-->>CLI: return "open"
end
end
end
CLI->>Query: get_latest_build(image, extra_patterns based on mode)
alt mode == "hermetic"
Query->>BQ: select ... WHERE hermetic = TRUE
else
Query->>BQ: select ... WHERE hermetic = FALSE
end
BQ-->>Query: return builds
Query-->>CLI: return latest build
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)elliott/elliottlib/cli/find_builds_cli.py (2)
artcommon/tests/test_konflux_db.py (1)
artcommon/artcommonlib/metadata.py (3)
🪛 Ruff (0.14.11)artcommon/artcommonlib/metadata.py587-587: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
artcommon/artcommonlib/metadata.py (1)
428-467: Fix pinned Konflux lookups whenel_targetis passed as a string (e.g."el8").
get_latest_konflux_buildnow acceptsint | str | None, but the pinned path forwards that toget_pinned_konflux_build, which will mis-handle"el8"("elel8"key) and brew-tag strings.Proposed diff
@@ - async def get_pinned_konflux_build(self, el_target: int): + async def get_pinned_konflux_build(self, el_target: int | str | None): @@ - if self.meta_type == 'rpm': - if el_target is None: + if self.meta_type == 'rpm': + if el_target is None: raise ValueError( f'Expected el_target to be set when querying a pinned RPM component {self.distgit_key}' ) - is_nvr = is_config[f'el{el_target}'] + if isinstance(el_target, str): + if el_target.startswith("el") and el_target[2:].isdigit(): + el_target = int(el_target[2:]) + elif el_target.isdigit(): + el_target = int(el_target) + else: + inferred = isolate_el_version_in_brew_tag(el_target) + if inferred: + el_target = inferred + if not isinstance(el_target, int): + raise ValueError(f"Invalid el_target for pinned konflux build: {el_target}") + is_nvr = is_config[f'el{el_target}']
🤖 Fix all issues with AI agents
In `@artcommon/artcommonlib/metadata.py`:
- Around line 177-189: The parameters exclude_large_columns and max_window_days
on get_latest_brew_build are intentionally unused for the Brew path and must be
marked as used to silence Ruff ARG002 without changing the API; inside
get_latest_brew_build (function name reference) explicitly consume them (for
example by assigning them to a throwaway variable or deleting them) and add a
short comment stating they are intentionally unused for Brew builds so CI won't
flag them.
- Around line 428-440: The parameter extra_patterns in get_latest_konflux_build
uses a mutable default (dict) which is unsafe; change the signature to use
extra_patterns: dict | None = None (or Optional[dict]) and inside
get_latest_konflux_build initialize it to an empty dict when None (e.g., if
extra_patterns is None: extra_patterns = {}), updating any type
hints/annotations and callers as needed to avoid mutating a shared default.
In `@elliott/elliottlib/cli/find_builds_cli.py`:
- Around line 754-755: The raised ElliottFatalError message in
find_builds_cli.py is too long and triggers Ruff TRY003; shorten the exception
message (e.g., "Failed to find Konflux builds") and move the detailed numeric
context into a prior log statement (use the module/logger used in this file to
log len(image_metas) and len(records) before raising). Update the raise of
ElliottFatalError and add a concise logging call referencing image_metas and
records so the exception text stays brief.
- Around line 706-726: Add a short clarifying comment in
_get_latest_build_by_network_mode_config explaining that configured_network_mode
values "internal-only" and "open" are both queried as hermetic=False because
Konflux only stores a hermetic boolean; reference
ImageMetadata.get_konflux_network_mode and the get_latest_build call
(extra_patterns={"hermetic": is_hermetic}) and note that the internal-only vs
open distinction is enforced at runtime (e.g., NO_PROXY in rebaser.py), not at
build selection time.
🧹 Nitpick comments (5)
elliott/elliottlib/cli/find_builds_cli.py (2)
706-727: Make_get_latest_build_by_network_mode_configexplicitly async + fix Konflux record typings.
The helper currently returns an awaitable while being a normaldef, andfind_builds_konfluxannotates records aslist[dict]even though later code treats them asKonflux*Recordobjects. This is easy to misread and undermines type checking.Proposed diff
@@ -def _get_latest_build_by_network_mode_config(image: ImageMetadata): +async def _get_latest_build_by_network_mode_config(image: ImageMetadata) -> KonfluxBuildRecord | None: @@ - return image.get_latest_build( + return await image.get_latest_build( el_target=image.branch_el_target(), exclude_large_columns=True, extra_patterns={"hermetic": is_hermetic} ) @@ -async def find_builds_konflux(runtime, payload) -> list[dict]: +async def find_builds_konflux(runtime: Runtime, payload: bool) -> list[KonfluxBuildRecord]: @@ - tasks = [_get_latest_build_by_network_mode_config(image) for image in image_metas] - records: list[dict] = [r for r in await asyncio.gather(*tasks) if r is not None] + tasks = [_get_latest_build_by_network_mode_config(image) for image in image_metas] + records: list[KonfluxBuildRecord] = [r for r in await asyncio.gather(*tasks) if r is not None]Also applies to: 729-755
800-803: Minor: simplify the gather call.
asyncio.gather(*[task for task in tasks])can beasyncio.gather(*tasks).elliott/tests/test_find_builds_cli.py (1)
64-81: Add a “not called” assertion for the filtered-out image meta.
This makes the payload filtering behavior harder to regress.Proposed diff
@@ image_meta_1.get_latest_build.assert_called_once_with( el_target="el8", exclude_large_columns=True, extra_patterns={"hermetic": True} ) + image_meta_2.get_latest_build.assert_not_called()artcommon/artcommonlib/metadata.py (1)
575-588: Validateruntime.network_mode_overrideand harden config access inget_konflux_network_mode.
Override currently bypasses validation; also, direct.konflux.get(...)assumes the intermediate nodes always exist.Proposed diff
@@ def get_konflux_network_mode(self) -> str: - runtime_override = getattr(self.runtime, 'network_mode_override', None) - if runtime_override: - return runtime_override + runtime_override = getattr(self.runtime, 'network_mode_override', None) - group_config_network_mode = self.runtime.group_config.konflux.get("network_mode") - image_config_network_mode = self.config.konflux.get("network_mode") + group_konflux = getattr(self.runtime.group_config, "konflux", None) + image_konflux = getattr(self.config, "konflux", None) + group_config_network_mode = group_konflux.get("network_mode") if group_konflux else None + image_config_network_mode = image_konflux.get("network_mode") if image_konflux else None - network_mode = image_config_network_mode or group_config_network_mode or "open" + network_mode = runtime_override or image_config_network_mode or group_config_network_mode or "open" valid_network_modes = ["hermetic", "internal-only", "open"] if network_mode not in valid_network_modes: raise ValueError(f"Invalid network mode; {network_mode}. Valid modes: {valid_network_modes}") return network_modeartcommon/tests/test_konflux_db.py (1)
483-522: Make hermetic SQL assertion more resilient to boolean rendering variations.
SQLAlchemy may render boolean literals differently across versions and dialects (true/false, TRUE/FALSE, 1/0). The current assertion checking only lowercase "true"/"false" could fail with SQLAlchemy updates. Consider checking for multiple possible boolean representations.Suggested refactor
- test_cases = [ - (True, "true"), - (False, "false"), - ] + test_cases = [ + (True, {"true", "1"}), + (False, {"false", "0"}), + ] - for hermetic_value, expected_sql_fragment in test_cases: + for hermetic_value, expected_literals in test_cases: for clause in where_clauses: clause_str = str(clause.compile(compile_kwargs={"literal_binds": True})) - if "hermetic" in clause_str.lower() and expected_sql_fragment in clause_str.lower(): + clause_lower = clause_str.lower() + if "hermetic" in clause_lower and any(lit in clause_lower for lit in expected_literals): hermetic_clause_found = True break
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
artcommon/artcommonlib/metadata.pyartcommon/tests/test_konflux_db.pydoozer/doozerlib/metadata.pyelliott/elliottlib/cli/find_builds_cli.pyelliott/tests/test_find_builds_cli.py
💤 Files with no reviewable changes (1)
- doozer/doozerlib/metadata.py
🧰 Additional context used
🧬 Code graph analysis (3)
elliott/tests/test_find_builds_cli.py (3)
artcommon/artcommonlib/metadata.py (3)
get_konflux_network_mode(575-588)get_latest_build(533-548)branch_el_target(102-110)elliott/elliottlib/imagecfg.py (4)
base_only(21-26)is_release(29-30)is_payload(33-34)is_olm_operator(37-38)elliott/elliottlib/cli/find_builds_cli.py (1)
find_builds_konflux(729-755)
artcommon/tests/test_konflux_db.py (1)
artcommon/artcommonlib/konflux/konflux_db.py (2)
get_by_name(235-328)get_latest_build(895-1061)
elliott/elliottlib/cli/find_builds_cli.py (3)
artcommon/artcommonlib/konflux/konflux_build_record.py (2)
KonfluxBuildRecord(231-319)KonfluxBundleBuildRecord(322-385)artcommon/artcommonlib/metadata.py (2)
get_konflux_network_mode(575-588)get_latest_build(533-548)artcommon/artcommonlib/konflux/konflux_db.py (1)
get_latest_build(895-1061)
🪛 Ruff (0.14.11)
elliott/elliottlib/cli/find_builds_cli.py
754-754: Avoid specifying long messages outside the exception class
(TRY003)
artcommon/artcommonlib/metadata.py
187-187: Unused method argument: exclude_large_columns
(ARG002)
188-188: Unused method argument: max_window_days
(ARG002)
436-436: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
587-587: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (1)
elliott/tests/test_find_builds_cli.py (1)
109-138: Tests cover hermetic vs non-hermetic selection nicely.
Theextra_patternsassertions line up with the new network-mode-aware selection logic.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
fgallott
force-pushed
the
art-14381-enforce-network-mode
branch
from
f184c52 to
851f4cf
Compare
|
@fgallott: This pull request references ART-14381 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
| else: | ||
| raise ValueError(f'Invalid value for --build-system: {self.runtime.build_system}') | ||
|
|
||
| def get_konflux_network_mode(self) -> str: |
There was a problem hiding this comment.
There is already a method doing this:
art-tools/doozer/doozerlib/metadata.py
Line 642 in 22e53c0
There was a problem hiding this comment.
ahh I see you removed it later 👍
fgallott
force-pushed
the
art-14381-enforce-network-mode
branch
from
851f4cf to
b37ee1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
elliott/elliottlib/cli/find_builds_cli.py (1)
733-757: Return type annotation should belist[KonfluxBuildRecord]for clarity.While
KonfluxBuildRecordimplements__getitem__to support dict-like access (e.g.,record['nvr']), the return type annotation-> list[dict]is misleading for type checkers and IDE autocompletion. Developers would expect plain dictionaries rather thanKonfluxBuildRecordobjects. Update the return type to-> list[KonfluxBuildRecord]to accurately reflect what the function returns and improve type safety.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
artcommon/artcommonlib/metadata.pyartcommon/tests/test_konflux_db.pydoozer/doozerlib/metadata.pyelliott/elliottlib/cli/find_builds_cli.pyelliott/tests/test_find_builds_cli.py
💤 Files with no reviewable changes (1)
- doozer/doozerlib/metadata.py
🚧 Files skipped from review as they are similar to previous changes (1)
- elliott/tests/test_find_builds_cli.py
🧰 Additional context used
🧬 Code graph analysis (2)
artcommon/tests/test_konflux_db.py (1)
artcommon/artcommonlib/konflux/konflux_db.py (2)
get_by_name(235-328)get_latest_build(895-1061)
artcommon/artcommonlib/metadata.py (3)
doozer/doozerlib/runtime.py (2)
group_config(234-235)group_config(238-239)artcommon/artcommonlib/runtime.py (1)
group_config(67-68)elliott/elliottlib/runtime.py (2)
group_config(128-129)group_config(132-133)
🪛 Ruff (0.14.11)
artcommon/artcommonlib/metadata.py
588-588: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
artcommon/artcommonlib/metadata.py (1)
576-589: LGTM! Clean implementation of network mode resolution.The precedence chain (runtime override → image config → group config → default "open") is clear and correct. The validation against the allowed modes provides good guardrails.
One minor observation: the static analysis flagged TRY003 for the long exception message on line 588, but this is a style preference and the message provides useful debugging context.
artcommon/tests/test_konflux_db.py (1)
483-521: Good test coverage for hermetic filtering.The test correctly validates that the
hermeticflag passed viaextra_patternsresults in the appropriate SQL clause. The use ofsubTestfor parameterized testing and mock resets per iteration is appropriate.Minor suggestion: consider adding a test case for when
hermeticis not specified inextra_patternsto verify it doesn't accidentally filter by hermetic status.elliott/elliottlib/cli/find_builds_cli.py (2)
710-730: Well-structured helper for network-mode-aware build retrieval.The helper correctly:
- Resolves the image's configured network mode via the new centralized method
- Maps hermetic mode to
hermetic=Trueand both internal-only/open modes tohermetic=False- Passes the filter through
extra_patternsto the build queryThe docstring clearly explains the behavior and precedence, which addresses the previous review feedback about clarifying the internal-only vs open distinction.
760-802: Correct integration of network-mode-aware retrieval.The function properly uses
_get_latest_build_by_network_mode_configfor each image (line 802), ensuring network mode is respected across all image types. The categorization logic for payload/non-payload and OLM handling remains intact.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@fgallott: This pull request references ART-14381 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
fgallott
force-pushed
the
art-14381-enforce-network-mode
branch
from
b37ee1a to
208d3aa
Compare
fgallott
force-pushed
the
art-14381-enforce-network-mode
branch
from
208d3aa to
1618e88
Compare
|
@fgallott: 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. |
3 participants
Summary
Enforces network mode filtering when querying Konflux builds in Elliott's find-builds command. This prevents accidentally shipping non-hermetic builds for images configured to use hermetic mode.
Changes
_get_latest_build_by_network_mode_config()that queries builds based on configured network mode (hermetic vs non-hermetic)get_konflux_network_mode()fromdoozertoartcommonfor shared use across toolsMoved to #2373
List[str] → list[str],Optional[X] → X | None)extra_patterns: dict = {} → extra_patterns: dict | None = None)