Revert "[ART-10321] feat(konflux): Add support for canonical_builders_from_upstream" #2334
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 |
WalkthroughThe changes refactor upstream-aware matching logic for canonical builders by relocating version determination and configuration merging responsibilities from the Image class to the ImageDistGitRepo class. The rebaser is updated to gate upstream resolution with a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
doozer/doozerlib/distgit.py (1)
1886-1909: Consider explicit check for empty parent_images.Accessing
parent_images[-1]at line 1892 will raiseIndexErrorif the Dockerfile has no FROM statements. While the broadexcept Exceptionhandler catches this, the warning message at line 1907 would be misleading ("Could not determine rhel version") rather than indicating a malformed Dockerfile.Also, regarding the static analysis hint on line 1901: the broad exception catch is intentional here since failure to determine RHEL version is non-fatal. However, consider catching more specific exceptions for better diagnostics.
🔎 Optional: Add explicit check and narrow exception types
with open(df_path) as f: dfp = DockerfileParser(fileobj=f) parent_images = dfp.parent_images + if not parent_images: + self.logger.warning('No parent images found in upstream Dockerfile for %s', self.name) + return None + # We will infer the rhel version from the last build layer in the upstream Dockerfile last_layer_pullspec = parent_images[-1]
📜 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)
doozer/doozerlib/backend/rebaser.pydoozer/doozerlib/distgit.pydoozer/doozerlib/image.pydoozer/tests/test_distgit/test_image_distgit/test_image_distgit.pydoozer/tests/test_image.py
💤 Files with no reviewable changes (1)
- doozer/tests/test_image.py
🧰 Additional context used
🧬 Code graph analysis (3)
doozer/doozerlib/image.py (1)
doozer/doozerlib/rpmcfg.py (1)
clone_source(53-93)
doozer/tests/test_distgit/test_image_distgit/test_image_distgit.py (2)
artcommon/artcommonlib/model.py (1)
Model(116-155)doozer/doozerlib/distgit.py (1)
_update_image_config(1911-1957)
doozer/doozerlib/backend/rebaser.py (4)
doozer/doozerlib/runtime.py (1)
resolve_stream(1135-1158)artcommon/artcommonlib/variants.py (1)
BuildVariant(4-6)doozer/doozerlib/distgit.py (1)
_resolve_image_from_upstream_parent(1654-1707)doozer/doozerlib/util.py (2)
oc_image_info_for_arch_async__caching(736-753)extract_version_fields(218-228)
🪛 Ruff (0.14.10)
doozer/doozerlib/distgit.py
1901-1901: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (2)
- GitHub Check: tests
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
doozer/doozerlib/backend/rebaser.py (4)
92-92: Acknowledge the placeholder for disabled functionality.The
should_match_upstreamis hardcoded toFalsewith a FIXME. This effectively disables the upstream matching feature as part of the revert, which aligns with the PR objective.
476-504: Refactored stream parent resolution with gated upstream matching.The logic correctly:
- Resolves the stream image with brew registry URL transformations
- Returns early for OKD variant
- Gates the upstream resolution behind
should_match_upstreamflag (currently alwaysFalse)Since
should_match_upstreamis hardcoded toFalse, lines 498-504 are effectively dead code, but this is intentional for the revert.
1012-1020: RHEL version assertion gated byshould_match_upstream.The condition was changed from
metadata.canonical_builders_enabledtoself.should_match_upstream. Sinceshould_match_upstreamis alwaysFalse, this assertion block will never execute, which is the intended behavior for the revert.
1909-1910: Critical bug:awaiton coroutine followed by immediate dict access.Line 1910 attempts to
awaitthe coroutine and then immediately index into the result on the same line. This is syntactically incorrect in Python - theawaitexpression must be parenthesized before indexing, otherwise it tries to index the coroutine object before awaiting it.🔎 Proposed fix
- self._logger.debug('Retrieving image info for image %s', original_parent) - labels = await util.oc_image_info_for_arch_async__caching(original_parent)['config']['config']['Labels'] + self._logger.debug('Retrieving image info for image %s', original_parent) + image_info = await util.oc_image_info_for_arch_async__caching(original_parent) + labels = image_info['config']['config']['Labels']Likely an incorrect or invalid review comment.
doozer/tests/test_distgit/test_image_distgit/test_image_distgit.py (1)
661-688: Test covers both matching and non-matchingalternative_upstreamscenarios.The test validates
_update_image_configbehavior:
- When
when: 'el8'matchesupstream_intended_el_version = '8'→ config is updated,should_match_upstreamstaysTrue- When
when: 'el7'doesn't matchupstream_intended_el_version = 8→ config unchanged,should_match_upstreamset toFalseThe test also verifies type coercion works correctly by using string
'8'in one case and integer8in another.doozer/doozerlib/image.py (2)
52-53: Simplified initialization removes upstream-merge pathway.The revert removes the complex
_apply_alternative_upstream_configlogic that was previously called during__init__. Now, whenclone_source=True, it simply resolves the source without any upstream version detection or config merging.This aligns with the revert objective - the upstream-aware logic is now disabled/removed.
664-679:canonical_builders_enabledproperty is still actively used in the codebase.The property correctly implements the configuration hierarchy (image config → group config) and is used at
doozer/doozerlib/distgit.py:519in a conditional check. Since the initialization logic that relied on this property was removed, it now serves only to evaluate the configuration state without triggering side effects. This is consistent with the revert approach.doozer/doozerlib/distgit.py (4)
509-530: Initialization logic looks correct.The conditional flow properly handles:
- Distgit-only images (no source) - version attributes remain
None_update_image_config()is safely called unconditionally since it handlesNoneupstream version by returning earlyThe inline comment block (lines 509-514) provides good context for the rationale.
1842-1852: Stream resolution logic is sound.The gating on
should_match_upstreamcorrectly:
- Returns typical stream resolution when not matching upstream
- Attempts upstream parent resolution with fallback to stream.image
1854-1865: LGTM.The fallback to group config branch when distgit branch is undefined is appropriate. The
Optional[int]return type correctly accounts for cases whereisolate_rhel_major_from_distgit_branchcannot extract a version.
2232-2240: LGTM - RHEL version assertion is a good safety check.The runtime assertion ensures the final image's RHEL version matches ART's configuration when matching upstream. This prevents misconfigurations from producing images built on the wrong RHEL base.
| else: | ||
| # We found an alternative_upstream config stanza. We can match upstream | ||
| self.should_match_upstream = True | ||
| # Distgit branch must be changed to track the alternative one | ||
| self.branch = self.config.distgit.branch | ||
| # Also update metadata config | ||
| self.metadata.config = self.config | ||
| self.metadata.targets = self.metadata.determine_targets() | ||
|
|
There was a problem hiding this comment.
Potential issue: distgit.branch may be Missing after config merge.
At line 1954, self.branch = self.config.distgit.branch is assigned directly without checking if it's Missing. While alternative_upstream configs should logically include a distgit.branch override (since they target different RHEL versions), there's no enforcement.
Compare with _determine_art_rhel_version() (lines 1861-1864) which handles the Missing case:
if branch is Missing:
self.logger.warning(...)
branch = self.runtime.group_config.branch🔎 Suggested defensive check
else:
# We found an alternative_upstream config stanza. We can match upstream
self.should_match_upstream = True
# Distgit branch must be changed to track the alternative one
- self.branch = self.config.distgit.branch
+ if self.config.distgit.branch is Missing:
+ raise ValueError(
+ f"alternative_upstream config for {self.name} matched but does not define distgit.branch"
+ )
+ self.branch = self.config.distgit.branch
# Also update metadata config
self.metadata.config = self.config
self.metadata.targets = self.metadata.determine_targets()🤖 Prompt for AI Agents
In doozer/doozerlib/distgit.py around lines 1950 to 1958, the assignment
self.branch = self.config.distgit.branch should defensively handle the case
where config.distgit.branch is Missing; check if branch is Missing, log a
warning, and fall back to self.runtime.group_config.branch (or other appropriate
default) before setting self.branch, then proceed to update metadata.config and
metadata.targets as before.
1 participant
Reverts #2220