Filter Konflux golang-builder search by hermetic flag#3048
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:
WalkthroughResolve network mode per image target (CLI > image > group > default), compute a hermetic boolean per target, pass it into Konflux build searches as where["hermetic"], add SQL boolean where handling, and add tests for resolver precedence and hermetic-filter behavior. ChangesNetwork Mode Resolution and Hermetic Build Filtering
🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pyartcd/pyartcd/pipelines/update_golang.py (1)
1009-1020: ⚡ Quick winNarrow the exception handling to specific exception types.
Lines 1012 and 1019 use bare
except Exception:which catches too broadly, includingKeyboardInterruptand other exceptions that shouldn't be suppressed. Consider catching more specific exceptions based on what_load_yaml_from_repoand the GitHub API can raise (e.g.,github.GithubException,yaml.YAMLError,KeyError).♻️ Proposed fix to narrow exception handling
+from github import GithubException +from yaml import YAMLError + image_config_network_mode = None try: image_config = self._load_yaml_from_repo(repo, f"images/{image_key}.yml", ref) image_config_network_mode = (image_config.get("konflux") or {}).get("network_mode") -except Exception: +except (GithubException, YAMLError, KeyError, FileNotFoundError): _LOGGER.debug(f"Could not load image config for {image_key} from ref {ref}") group_config_network_mode = None try: group_config = self._load_yaml_from_repo(repo, "group.yml", ref) group_config_network_mode = (group_config.get("konflux") or {}).get("network_mode") -except Exception: +except (GithubException, YAMLError, KeyError, FileNotFoundError): _LOGGER.debug(f"Could not load group config from ref {ref}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyartcd/pyartcd/pipelines/update_golang.py` around lines 1009 - 1020, The bare `except Exception:` blocks around calls to _load_yaml_from_repo (affecting image_config_network_mode and group_config_network_mode) are too broad; change them to catch the specific exceptions that _load_yaml_from_repo and the underlying GitHub/YAML code may raise (for example github.GithubException, yaml.YAMLError, FileNotFoundError, KeyError), and include the exception details in the _LOGGER.debug message so failures are recorded; keep the behavior of falling back to None but avoid swallowing unexpected system-level exceptions like KeyboardInterrupt or SystemExit.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pyartcd/pyartcd/pipelines/update_golang.py`:
- Around line 1009-1020: The bare `except Exception:` blocks around calls to
_load_yaml_from_repo (affecting image_config_network_mode and
group_config_network_mode) are too broad; change them to catch the specific
exceptions that _load_yaml_from_repo and the underlying GitHub/YAML code may
raise (for example github.GithubException, yaml.YAMLError, FileNotFoundError,
KeyError), and include the exception details in the _LOGGER.debug message so
failures are recorded; keep the behavior of falling back to None but avoid
swallowing unexpected system-level exceptions like KeyboardInterrupt or
SystemExit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: f633d636-78ca-4297-856b-77c92a29d900
📒 Files selected for processing (3)
artcommon/artcommonlib/konflux/konflux_db.pypyartcd/pyartcd/pipelines/update_golang.pypyartcd/tests/pipelines/test_update_golang.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyartcd/pyartcd/pipelines/update_golang.py (1)
581-600:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't collapse
internal-onlyinto the non-hermetic search bucket.Line 584 maps every non-
hermeticmode toFalse, so aninternal-onlytarget will querywhere["hermetic"] = Falseand can reuse a builder previously produced withopenegress. That skips a rebuild even though the requested network policy is stricter. Either query on a persisted fullnetwork_modefield, or conservatively disable reuse forinternal-onlyuntil Konflux records can distinguish it.Possible conservative fix
el_hermetic_map: dict[int, bool] = {} for el_v in el_nvr_map: network_mode = self._get_effective_network_mode(el_v, go_version) + if network_mode == "internal-only": + _LOGGER.info( + "Skipping Konflux builder reuse for el%s because the DB filter only distinguishes hermetic vs non-hermetic", + el_v, + ) + continue el_hermetic_map[el_v] = network_mode == "hermetic" _LOGGER.info( f"Searching for el{el_v} golang-builder with hermetic={el_hermetic_map[el_v]} " f"(network_mode={network_mode})" ) @@ - for el_v in el_nvr_map + for el_v in el_hermetic_map ) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyartcd/pyartcd/pipelines/update_golang.py` around lines 581 - 600, The current mapping in el_hermetic_map (set via _get_effective_network_mode) treats any non-"hermetic" mode as False, causing "internal-only" targets to be treated like open-egress and allowing reuse; change the search logic in the build_records gather/konflux_db.search_builds_by_fields call so it does not collapse "internal-only" into hermetic=False — either query using a persisted full network_mode field (e.g., add where["network_mode"]=network_mode) or, if that field isn't available yet, conservatively disable reuse for "internal-only" by omitting the hermetic filter for el_v when network_mode == "internal-only" (or otherwise ensure you do not set where["hermetic"]=False for internal-only) so builders with open egress are not reused for stricter internal-only requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pyartcd/pyartcd/pipelines/update_golang.py`:
- Around line 581-600: The current mapping in el_hermetic_map (set via
_get_effective_network_mode) treats any non-"hermetic" mode as False, causing
"internal-only" targets to be treated like open-egress and allowing reuse;
change the search logic in the build_records
gather/konflux_db.search_builds_by_fields call so it does not collapse
"internal-only" into hermetic=False — either query using a persisted full
network_mode field (e.g., add where["network_mode"]=network_mode) or, if that
field isn't available yet, conservatively disable reuse for "internal-only" by
omitting the hermetic filter for el_v when network_mode == "internal-only" (or
otherwise ensure you do not set where["hermetic"]=False for internal-only) so
builders with open egress are not reused for stricter internal-only requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d0fc9265-07fd-4705-88a2-6fab9882c179
📒 Files selected for processing (2)
pyartcd/pyartcd/pipelines/update_golang.pypyartcd/tests/pipelines/test_update_golang.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyartcd/tests/pipelines/test_update_golang.py
thegreyd
force-pushed
the
better-golang-builder-search
branch
from
11c50b6 to
ec2856c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@artcommon/artcommonlib/metadata.py`:
- Line 640: The code currently reads runtime_override = getattr(self.runtime,
"network_mode_override", None) which can return a MagicMock in tests; update the
logic so you only pass a real string to resolve_network_mode: after fetching
runtime_override, validate its type (e.g., isinstance(runtime_override, str))
and if it is not a str, set runtime_override = None (or omit it) before calling
resolve_network_mode; locate the usage around the attribute access on
self.runtime and the subsequent call to resolve_network_mode to apply this
guard.
In `@pyartcd/pyartcd/pipelines/update_golang.py`:
- Around line 996-999: The try/except around calling self._load_yaml_from_repo
that sets image_config should not swallow all errors silently; modify the except
to log the caught exception (including its message/stack) before falling back to
image_config = {}. Use an appropriate logger (e.g., self.logger.exception or the
module-level logging.exception) in the except block to record the error and
context (repo, path f"images/{image_key}.yml", ref) and then continue with
image_config = {} so debugging GitHub/API/YAML parse failures is possible.
- Around line 1002-1005: The current bare except around calling
self._load_yaml_from_repo(repo, "group.yml", ref) swallows all errors and
silently falls back to group_config = {}, so change the except to capture the
exception (e.g., except Exception as e) and log the failure before assigning the
empty dict; use the existing logger on the object (self.logger.exception or
self.logger.error with the exception) or the module logger to record the
repo/ref and error message/stacktrace so failures in _load_yaml_from_repo are
visible while preserving the fallback to group_config = {}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: a2b0ce90-0d6a-41f5-a090-d530399fd867
📒 Files selected for processing (4)
artcommon/artcommonlib/konflux/konflux_db.pyartcommon/artcommonlib/metadata.pypyartcd/pyartcd/pipelines/update_golang.pypyartcd/tests/pipelines/test_update_golang.py
🚧 Files skipped from review as they are similar to previous changes (1)
- artcommon/artcommonlib/konflux/konflux_db.py
When searching for existing golang-builder builds in the Konflux DB, consider the effective network mode from ocp-build-data config. If the builder is configured as hermetic, only match hermetic builds. - Extract resolve_network_mode() into artcommonlib for shared use - Add _get_effective_network_mode() to update_golang pipeline - Add hermetic field to search_builds_by_fields where clause - Fix boolean handling in search_builds_by_fields (was using String column type for all values, now uses Boolean for bool values) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
thegreyd
force-pushed
the
better-golang-builder-search
branch
from
8bb5aed to
18d3dda
Compare
|
|
||
| Precedence: cli_override > image_config > group_config > "open" | ||
| """ | ||
| cli_override = cli_override if isinstance(cli_override, str) else None |
There was a problem hiding this comment.
mmm....
The function signature says str | None. Passing a MagicMock is a test bug, not a function bug.
The right fix would be to set runtime.network_mode_override = None in those 4 tests IMHO
…isinstance guards Per review feedback, the function signature says `str | None` — passing MagicMock is a test bug, not a function bug. Mock `get_konflux_network_mode` in the 4 lockfile tests that use MagicMock configs, matching the pattern already used by the passing tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
thegreyd
force-pushed
the
better-golang-builder-search
branch
from
18d3dda to
2b76493
Compare
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyartcd/pyartcd/pipelines/update_golang.py`:
- Around line 996-1011: When loading image/group YAML fails in
_load_yaml_from_repo, don't swallow the exception into {} because that lets
resolve_network_mode(image_config_network_mode, group_config_network_mode) fall
back to "open"; instead after logging the warning re-raise the exception (or
raise a new descriptive exception) so the pipeline fails closed and avoid
matching a non-hermetic builder; update the two except blocks around
_load_yaml_from_repo (the ones that set image_config = {} and group_config = {})
to log and then raise, so resolve_network_mode receives only valid config values
or the pipeline stops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 772bb2ce-3a8d-4885-b8fd-6fb4c8d1a1bb
📒 Files selected for processing (5)
artcommon/artcommonlib/konflux/konflux_db.pyartcommon/artcommonlib/metadata.pydoozer/tests/test_image.pypyartcd/pyartcd/pipelines/update_golang.pypyartcd/tests/pipelines/test_update_golang.py
🚧 Files skipped from review as they are similar to previous changes (2)
- artcommon/artcommonlib/metadata.py
- artcommon/artcommonlib/konflux/konflux_db.py
| try: | ||
| image_config = self._load_yaml_from_repo(repo, f"images/{image_key}.yml", ref) | ||
| except Exception as e: | ||
| _LOGGER.warning(f"Failed to load image config for {image_key}: {e}") | ||
| image_config = {} | ||
| image_config_network_mode = (image_config.get("konflux") or {}).get("network_mode") | ||
|
|
||
| try: | ||
| group_config = self._load_yaml_from_repo(repo, "group.yml", ref) | ||
| except Exception as e: | ||
| _LOGGER.warning(f"Failed to load group config from {ref}: {e}") | ||
| group_config = {} | ||
| group_config_network_mode = (group_config.get("konflux") or {}).get("network_mode") | ||
|
|
||
| network_mode = resolve_network_mode(None, image_config_network_mode, group_config_network_mode) | ||
| _LOGGER.info(f"Effective network mode for el{el_v} golang-builder: {network_mode}") |
There was a problem hiding this comment.
Failing to read build-data currently fails open to non-hermetic matching.
At Line 1004 and Line 997, exceptions are converted to {}, which can make resolve_network_mode(...) fall back to "open" and query hermetic=False. That can incorrectly reuse a non-hermetic builder when hermetic config exists but could not be fetched.
🔧 Suggested fix
- try:
- image_config = self._load_yaml_from_repo(repo, f"images/{image_key}.yml", ref)
- except Exception as e:
- _LOGGER.warning(f"Failed to load image config for {image_key}: {e}")
- image_config = {}
+ try:
+ image_config = self._load_yaml_from_repo(repo, f"images/{image_key}.yml", ref)
+ except Exception as e:
+ _LOGGER.exception("Failed to load image config for %s at ref %s", image_key, ref)
+ raise RuntimeError(f"Unable to resolve network mode from images/{image_key}.yml at {ref}") from e
@@
- try:
- group_config = self._load_yaml_from_repo(repo, "group.yml", ref)
- except Exception as e:
- _LOGGER.warning(f"Failed to load group config from {ref}: {e}")
- group_config = {}
+ try:
+ group_config = self._load_yaml_from_repo(repo, "group.yml", ref)
+ except Exception as e:
+ _LOGGER.exception("Failed to load group config at ref %s", ref)
+ raise RuntimeError(f"Unable to resolve network mode from group.yml at {ref}") from e🧰 Tools
🪛 Ruff (0.15.15)
[warning] 998-998: Do not catch blind exception: Exception
(BLE001)
[warning] 1005-1005: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyartcd/pyartcd/pipelines/update_golang.py` around lines 996 - 1011, When
loading image/group YAML fails in _load_yaml_from_repo, don't swallow the
exception into {} because that lets
resolve_network_mode(image_config_network_mode, group_config_network_mode) fall
back to "open"; instead after logging the warning re-raise the exception (or
raise a new descriptive exception) so the pipeline fails closed and avoid
matching a non-hermetic builder; update the two except blocks around
_load_yaml_from_repo (the ones that set image_config = {} and group_config = {})
to log and then raise, so resolve_network_mode receives only valid config values
or the pipeline stops.
|
/lgtm |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
2 participants
Summary
_get_effective_network_mode()toUpdateGolangPipelinethat reads the network mode from ocp-build-data config (CLI override > image config > group config > default "open"), matching doozer's precedenceget_existing_builders_konflux()to filter byhermetic=True/Falsebased on the effective network mode, preventing reuse of non-hermetic builds when config requires hermetic (or vice versa)search_builds_by_fieldsto useColumn(col_name, Boolean)instead ofStringfor bool values in thewheredictTest plan
uv run pytest pyartcd/tests/pipelines/test_update_golang.py)make lint)update_golangagainst an OCP version with hermetic golang-builder config🤖 Generated with Claude Code