feat(konflux): implement base image snapshot-to-release workflow#2476
Merged
openshift-merge-bot[bot] merged 4 commits intoopenshift-eng:mainfrom Mar 16, 2026
Merged
Conversation
Add conditional snapshot-release workflow for golang base images with proper Release monitoring and comprehensive test coverage. Base images with both base_only=true and snapshot_release=true now trigger automated snapshot creation, release generation, and proper completion tracking. Key components: - BaseImageHandler orchestrates 5-step workflow (snapshot→release→monitoring) - Elliott create_release_from_snapshot function bridges snapshot and release - Fixed Release monitoring to handle Progressing→Succeeded/Failed transitions - Added snapshot_release schema property for granular workflow control - Enhanced conditional logic prevents workflow from running on regular images
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds a Konflux snapshot-to-release workflow: new BaseImageHandler orchestrates snapshot creation, release derivation/creation and polling; KonfluxImageBuilder invokes it post-build; ImageMetadata gains predicates for base and snapshot_release flags; CLI, schema, and tests updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant KIB as KonfluxImageBuilder
participant IM as ImageMetadata
participant BIH as BaseImageHandler
participant KAPI as Konflux API
KIB->>IM: is_base_image() / is_snapshot_release_enabled()
IM-->>KIB: true/false
alt proceed
KIB->>BIH: instantiate(metadata, nvr, image_pullspec, dry_run)
KIB->>BIH: process_base_image_completion()
BIH->>KAPI: create snapshot (from NVR)
KAPI-->>BIH: snapshot_name
BIH->>BIH: derive release_plan
BIH->>KAPI: create release (from snapshot, release_plan)
KAPI-->>BIH: release_name
loop poll until Released or timeout
BIH->>KAPI: get release status
KAPI-->>BIH: status (Progressing/Released/Failed)
end
BIH-->>KIB: (release_name, snapshot_name) or failure
else skip
KIB->>KIB: log skip
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
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/snapshot_cli.py (1)
289-294:⚠️ Potential issue | 🟠 MajorPotential name collision when custom name is used with multiple applications.
When
custom_nameis provided and builds span multiple Konflux applications, all snapshots will share the same name (line 294). Since Kubernetes requires unique resource names within a namespace, creating the second snapshot will fail with a conflict error.Consider appending an index even when custom_name is provided:
🐛 Proposed fix to ensure unique names
- "name": snapshot_name if self.custom_name else f"{snapshot_name}-{index}", + "name": snapshot_name if (self.custom_name and len(app_components) == 1) else f"{snapshot_name}-{index}",
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/backend/base_image_handler.py`:
- Around line 125-128: The snapshot naming can collide when multiple runs occur
within the same second; update how the timestamp is generated for
custom_snapshot_name in base_image_handler.py by using a higher-resolution UTC
timestamp (include microseconds or milliseconds) instead of
get_utc_now_formatted_str() so group_safe-{component_name}-{timestamp} becomes
unique for rapid/retry runs; modify the code that builds timestamp (or replace
get_utc_now_formatted_str) to include %f (microseconds) or an equivalent
high-resolution UTC value before constructing custom_snapshot_name that
currently uses runtime.group and component_name.
- Around line 187-202: The current dry-run still calls
create_release_from_snapshot which performs cluster interactions; to avoid that,
short-circuit when self.dry_run is true by constructing the synthetic release
name locally instead of invoking create_release_from_snapshot: detect dry-run
(self.dry_run or set elliott_runtime.dry_run), build the synthetic name using
the same naming convention used by create_release_from_snapshot, assign it to
release_name, and skip calling create_release_from_snapshot; keep the existing
ElliottRuntime setup (ElliottRuntime, elliott_runtime.group/assembly/product)
only if needed for name construction, otherwise avoid cluster-using helper calls
entirely.
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 992-1078: The handler currently re-inspects the built image for
NVR with _extract_nvr_from_build and may skip workflow; instead, change the
build() call site to pass the already-computed nvr into
_handle_base_image_completion (add a new parameter like nvr: Optional[str]),
update _handle_base_image_completion to use the passed nvr directly (skip
calling _extract_nvr_from_build) and only fall back to extraction if the passed
nvr is None, and remove or stop calling _extract_nvr_from_build where no
fallback is needed; ensure you pass the nvr into BaseImageHandler(metadata, nvr,
image_pullspec, ...) and update any callers/signature uses accordingly.
In `@elliott/elliottlib/cli/konflux_release_cli.py`:
- Around line 495-547: The CLI currently hardcodes 'ocp-art-tenant' as a
fallback namespace; after calling runtime.initialize(build_system='konflux') in
release_from_snapshot_cli, call
resolve_konflux_namespace_by_product(runtime.product, konflux_namespace), store
the result (e.g., resolved_namespace), pass resolved_namespace into
create_release_from_snapshot (namespace=resolved_namespace) and use
resolved_namespace in the LOGGER.info/green_print message instead of the literal
fallback; ensure any place that referenced konflux_namespace or used the
hardcoded 'ocp-art-tenant' uses the resolved_namespace consistently.
In `@elliott/elliottlib/cli/snapshot_cli.py`:
- Around line 203-211: The code uses custom_name directly when set, skipping
Kubernetes name validation done for auto-generated names; update the branch that
handles self.custom_name to validate or normalize it with the same rules as
normalize_group_name_for_k8s (or call that function on self.custom_name), and if
the result is empty or still invalid raise a ValueError with a clear message;
ensure snapshot_name is assigned the validated/normalized value (and retain the
timestamp suffix behavior only for auto-generated names using
get_utc_now_formatted_str()).
🧹 Nitpick comments (2)
elliott/elliottlib/cli/snapshot_cli.py (1)
105-106: Type hint should usestr | Nonefor optional parameter.The static analysis correctly flags that
custom_name: str = Noneshould becustom_name: str | None = Noneper PEP 484. However, this matches the existing pattern on line 104 (job_url: str = None), so this is consistent with current codebase style.✨ Optional fix for type hints
- job_url: str = None, - custom_name: str = None, + job_url: str | None = None, + custom_name: str | None = None,ocp-build-data-validator/validator/json_schemas/image_config.base.schema.json (1)
176-186: Consider enforcingsnapshot_releaseonly for base images.
The description says it applies only tobase_onlyimages, but the schema currently allows it everywhere. A smallif/thenguard would prevent misconfiguration.Suggested schema guard
@@ - "additionalProperties": false + "allOf": [ + { + "if": { "properties": { "snapshot_release": { "const": true } } }, + "then": { + "properties": { "base_only": { "const": true } }, + "required": ["base_only"] + } + } + ], + "additionalProperties": false
Comment on lines
+125
to
+128
| timestamp = get_utc_now_formatted_str() | ||
| group_safe = self.runtime.group.replace('openshift-', '').replace('.', '-') | ||
| custom_snapshot_name = f"{group_safe}-{component_name}-{timestamp}" | ||
|
|
Contributor
There was a problem hiding this comment.
Snapshot name can collide within the same second.
Because --name is explicit, concurrent runs or quick retries in the same second can fail with name collisions. Consider microsecond granularity to reduce this risk.
🔧 Suggested fix
- timestamp = get_utc_now_formatted_str()
+ timestamp = get_utc_now_formatted_str(microseconds=True)🤖 Prompt for AI Agents
In `@doozer/doozerlib/backend/base_image_handler.py` around lines 125 - 128, The
snapshot naming can collide when multiple runs occur within the same second;
update how the timestamp is generated for custom_snapshot_name in
base_image_handler.py by using a higher-resolution UTC timestamp (include
microseconds or milliseconds) instead of get_utc_now_formatted_str() so
group_safe-{component_name}-{timestamp} becomes unique for rapid/retry runs;
modify the code that builds timestamp (or replace get_utc_now_formatted_str) to
include %f (microseconds) or an equivalent high-resolution UTC value before
constructing custom_snapshot_name that currently uses runtime.group and
component_name.
Comment on lines
+495
to
+547
| @konflux_release_cli.command("release-from-snapshot", short_help="Create Konflux release from existing snapshot") | ||
| @click.argument("snapshot_name", metavar="SNAPSHOT_NAME") | ||
| @click.option("--release-plan", required=True, help="Konflux release plan name to use") | ||
| @click.option("--apply", is_flag=True, default=False, help="Apply the release resource immediately") | ||
| @click.option( | ||
| '--konflux-namespace', | ||
| metavar='NAMESPACE', | ||
| help='The namespace to use for Konflux cluster connections. If not provided, will be auto-detected based on group.', | ||
| ) | ||
| @click.pass_obj | ||
| @click_coroutine | ||
| async def release_from_snapshot_cli( | ||
| runtime: Runtime, snapshot_name: str, release_plan: str, apply: bool, konflux_namespace: str | ||
| ): | ||
| """ | ||
| Create a Konflux Release resource from an existing snapshot. | ||
|
|
||
| This command creates a Release resource that references an existing snapshot | ||
| and uses the specified release plan to orchestrate the release process. | ||
| The release will generate both quay.io and registry.redhat.io URLs upon completion. | ||
|
|
||
| Examples: | ||
|
|
||
| Create release from snapshot (preview mode): | ||
| elliott -g openshift-4.22 release release-from-snapshot my-snapshot --release-plan ocp-art-base-images-silent-4-22-rhel9 | ||
|
|
||
| Apply the release immediately: | ||
| elliott -g openshift-4.22 release release-from-snapshot my-snapshot --release-plan ocp-art-base-images-silent-4-22-rhel9 --apply | ||
| """ | ||
| # Initialize runtime for Konflux operations | ||
| runtime.initialize(build_system='konflux') | ||
|
|
||
| try: | ||
| release_name = await create_release_from_snapshot( | ||
| runtime=runtime, | ||
| snapshot_name=snapshot_name, | ||
| release_plan=release_plan, | ||
| namespace=konflux_namespace, | ||
| apply=apply, | ||
| ) | ||
|
|
||
| if apply: | ||
| green_print(f"✓ Successfully created release: {release_name}") | ||
| LOGGER.info( | ||
| f"Monitor release progress: kubectl get release {release_name} -n {konflux_namespace or 'ocp-art-tenant'}" | ||
| ) | ||
| else: | ||
| LOGGER.info("Preview completed - use --apply to create the release") | ||
|
|
||
| except Exception as e: | ||
| red_print(f"Failed to create release from snapshot: {e}") | ||
| LOGGER.error(f"CLI command failed: {e}") | ||
| exit(1) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and verify the file exists
find . -name "konflux_release_cli.py" -type fRepository: openshift-eng/art-tools
Length of output: 114
🏁 Script executed:
# Check the specific lines mentioned in the review
wc -l elliott/elliottlib/cli/konflux_release_cli.pyRepository: openshift-eng/art-tools
Length of output: 116
🏁 Script executed:
# Examine the code around the mentioned lines
sed -n '490,550p' elliott/elliottlib/cli/konflux_release_cli.pyRepository: openshift-eng/art-tools
Length of output: 2434
🏁 Script executed:
# Search for the suggested function resolve_konflux_namespace_by_product
rg "resolve_konflux_namespace_by_product" --type pyRepository: openshift-eng/art-tools
Length of output: 3810
🏁 Script executed:
# Search for any namespace resolution patterns in the codebase
rg "ocp-art-tenant" --type py -B 2 -A 2Repository: openshift-eng/art-tools
Length of output: 15359
🏁 Script executed:
# Look for how namespace is resolved elsewhere
rg "konflux_namespace" --type py -B 2 -A 2 | head -100Repository: openshift-eng/art-tools
Length of output: 7286
🏁 Script executed:
# Check if the function is already imported and used in konflux_release_cli.py
grep -n "resolve_konflux_namespace_by_product" elliott/elliottlib/cli/konflux_release_cli.pyRepository: openshift-eng/art-tools
Length of output: 297
🏁 Script executed:
# Get the exact lines around 510-535 to see if the fix has been applied
sed -n '510,545p' elliott/elliottlib/cli/konflux_release_cli.pyRepository: openshift-eng/art-tools
Length of output: 1538
🏁 Script executed:
# Check the function definition to understand how it works
grep -A 10 "def resolve_konflux_namespace_by_product" artcommon/artcommonlib/util.pyRepository: openshift-eng/art-tools
Length of output: 497
🏁 Script executed:
# Verify imports at the top of the konflux_release_cli.py file
head -50 elliott/elliottlib/cli/konflux_release_cli.py | grep -E "^from|^import"Repository: openshift-eng/art-tools
Length of output: 673
🏁 Script executed:
# Get the complete function to see if resolved_namespace is being used correctly
sed -n '465,547p' elliott/elliottlib/cli/konflux_release_cli.pyRepository: openshift-eng/art-tools
Length of output: 3292
Resolve the namespace before use to avoid incorrect hardcoding for non-OCP products.
The release_from_snapshot_cli function currently hardcodes ocp-art-tenant as a fallback when --konflux-namespace is not provided, which is incorrect for non-OCP products like OADP, MTA, etc. Call resolve_konflux_namespace_by_product(runtime.product, konflux_namespace) once after runtime initialization and use the resolved value consistently for both the create_release_from_snapshot call and the log message.
Suggested fix
# Initialize runtime for Konflux operations
runtime.initialize(build_system='konflux')
+ resolved_namespace = resolve_konflux_namespace_by_product(runtime.product, konflux_namespace)
try:
release_name = await create_release_from_snapshot(
runtime=runtime,
snapshot_name=snapshot_name,
release_plan=release_plan,
- namespace=konflux_namespace,
+ namespace=resolved_namespace,
apply=apply,
)
if apply:
green_print(f"✓ Successfully created release: {release_name}")
LOGGER.info(
- f"Monitor release progress: kubectl get release {release_name} -n {konflux_namespace or 'ocp-art-tenant'}"
+ f"Monitor release progress: kubectl get release {release_name} -n {resolved_namespace}"
)🧰 Tools
🪛 Ruff (0.15.0)
[warning] 544-544: Do not catch blind exception: Exception
(BLE001)
[warning] 546-546: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
In `@elliott/elliottlib/cli/konflux_release_cli.py` around lines 495 - 547, The
CLI currently hardcodes 'ocp-art-tenant' as a fallback namespace; after calling
runtime.initialize(build_system='konflux') in release_from_snapshot_cli, call
resolve_konflux_namespace_by_product(runtime.product, konflux_namespace), store
the result (e.g., resolved_namespace), pass resolved_namespace into
create_release_from_snapshot (namespace=resolved_namespace) and use
resolved_namespace in the LOGGER.info/green_print message instead of the literal
fallback; ensure any place that referenced konflux_namespace or used the
hardcoded 'ocp-art-tenant' uses the resolved_namespace consistently.
Comment on lines
+203
to
+211
| if self.custom_name: | ||
| snapshot_name = self.custom_name | ||
| else: | ||
| group_name_safe = normalize_group_name_for_k8s(self.runtime.group) | ||
| if not group_name_safe: | ||
| raise ValueError( | ||
| f"Group name '{self.runtime.group}' produces invalid normalized name for Kubernetes snapshot" | ||
| ) | ||
| snapshot_name = f"{group_name_safe}-{get_utc_now_formatted_str()}" |
Contributor
There was a problem hiding this comment.
Missing validation for custom snapshot name against Kubernetes naming rules.
When custom_name is provided, it bypasses the normalize_group_name_for_k8s validation that the auto-generated path uses. Invalid names (e.g., uppercase, underscores, exceeding 63 characters) will cause Kubernetes API errors at snapshot creation time.
Consider validating or normalizing the custom name:
🛡️ Proposed fix to validate custom_name
async def new_snapshots(self, build_records: List[KonfluxRecord]) -> list[dict]:
if self.custom_name:
- snapshot_name = self.custom_name
+ # Validate custom name against K8s naming rules
+ if len(self.custom_name) > 63:
+ raise ValueError(f"Custom snapshot name '{self.custom_name}' exceeds 63 character limit")
+ if self.custom_name != self.custom_name.lower():
+ raise ValueError(f"Custom snapshot name '{self.custom_name}' must be lowercase")
+ import re
+ if not re.match(r'^[a-z0-9]([a-z0-9-]*[a-z0-9])?$', self.custom_name):
+ raise ValueError(
+ f"Custom snapshot name '{self.custom_name}' must be a valid DNS label "
+ "(lowercase alphanumeric and hyphens, starting/ending with alphanumeric)"
+ )
+ snapshot_name = self.custom_name
else:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if self.custom_name: | |
| snapshot_name = self.custom_name | |
| else: | |
| group_name_safe = normalize_group_name_for_k8s(self.runtime.group) | |
| if not group_name_safe: | |
| raise ValueError( | |
| f"Group name '{self.runtime.group}' produces invalid normalized name for Kubernetes snapshot" | |
| ) | |
| snapshot_name = f"{group_name_safe}-{get_utc_now_formatted_str()}" | |
| if self.custom_name: | |
| # Validate custom name against K8s naming rules | |
| if len(self.custom_name) > 63: | |
| raise ValueError(f"Custom snapshot name '{self.custom_name}' exceeds 63 character limit") | |
| if self.custom_name != self.custom_name.lower(): | |
| raise ValueError(f"Custom snapshot name '{self.custom_name}' must be lowercase") | |
| import re | |
| if not re.match(r'^[a-z0-9]([a-z0-9-]*[a-z0-9])?$', self.custom_name): | |
| raise ValueError( | |
| f"Custom snapshot name '{self.custom_name}' must be a valid DNS label " | |
| "(lowercase alphanumeric and hyphens, starting/ending with alphanumeric)" | |
| ) | |
| snapshot_name = self.custom_name | |
| else: | |
| group_name_safe = normalize_group_name_for_k8s(self.runtime.group) | |
| if not group_name_safe: | |
| raise ValueError( | |
| f"Group name '{self.runtime.group}' produces invalid normalized name for Kubernetes snapshot" | |
| ) | |
| snapshot_name = f"{group_name_safe}-{get_utc_now_formatted_str()}" |
🧰 Tools
🪛 Ruff (0.15.0)
[warning] 208-210: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@elliott/elliottlib/cli/snapshot_cli.py` around lines 203 - 211, The code uses
custom_name directly when set, skipping Kubernetes name validation done for
auto-generated names; update the branch that handles self.custom_name to
validate or normalize it with the same rules as normalize_group_name_for_k8s (or
call that function on self.custom_name), and if the result is empty or still
invalid raise a ValueError with a clear message; ensure snapshot_name is
assigned the validated/normalized value (and retain the timestamp suffix
behavior only for auto-generated names using get_utc_now_formatted_str()).
ashwindasr
requested changes
Feb 23, 2026
# By D. Paolella (21) and others # Via GitHub (61) and Ashwin Das (1) * main: (111 commits) refactor oadp jenkins job names refactor oadp to layered-products (openshift-eng#2540) Do not assert RHEL version for OKD remove job reference to prepare_release deprecate prepare-release.py Specify secret Fully migrate to uv fix(doozer): add USER 0 before DNF module enablement commands Don't pin stomp and setuptools and use uv run artcd Fix 'MissingModel' object is not callable error in JIRA template rendering ART-14597: Allow multiple rpm_deliveries with the same name update-golang: Fix repo url validation (openshift-eng#2534) support DATE_TIME for 4.12-4.18 ART-14613 Only rebuild golang rpms whose trackers are open (openshift-eng#2528) OKD on canonical Add links to art-build-history Add repo_type and signing_intent to schema Fix images-health dashboard filters and unify OKD/OCP dashboard [OKD] skip coreos tags mirroring when all builds failed konflux template param for build-odap (openshift-eng#2524) ... # Conflicts: # elliott/elliottlib/cli/konflux_release_cli.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/snapshot_cli.py (1)
287-293:⚠️ Potential issue | 🟠 Major
--namecan cause duplicate Snapshot names across multiple applications.At Line 292, a single custom name is reused for every generated Snapshot. When builds span multiple applications, this creates same-namespace name collisions (
AlreadyExists) and breaks multi-snapshot creation.💡 Proposed fix
snapshot_objs: list[dict] = [] + if self.custom_name and len(app_components) > 1: + raise ValueError( + "Custom --name requires builds from a single application. " + "Omit --name or pass builds for only one application." + ) + for index, (application_name, components) in enumerate(sorted(app_components.items()), start=1): components = sorted(components, key=lambda c: c['name'])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elliott/elliottlib/cli/snapshot_cli.py` around lines 287 - 293, The loop that builds snapshot metadata reuses snapshot_name for every application when self.custom_name is True, causing name collisions; update the "name" assignment in the metadata (where snapshot_name, self.custom_name, index and app_components are used) to ensure uniqueness—e.g., if self.custom_name is True and there is more than one application in app_components, append a unique suffix such as the index or the application_name (e.g., f"{snapshot_name}-{index}" or f"{snapshot_name}-{application_name}") so each generated Snapshot has a distinct name.
♻️ Duplicate comments (2)
elliott/elliottlib/cli/konflux_release_cli.py (1)
525-537:⚠️ Potential issue | 🟡 MinorResolve namespace once in CLI and reuse it for both create + monitor output.
Current output can show a wrong namespace for non-OCP products when
--konflux-namespaceis omitted, because the monitor hint falls back to a hardcoded value instead of the resolved namespace.Suggested fix
# Initialize runtime for Konflux operations runtime.initialize(build_system='konflux') + resolved_namespace = resolve_konflux_namespace_by_product(runtime.product, konflux_namespace) try: release_name = await create_release_from_snapshot( runtime=runtime, snapshot_name=snapshot_name, release_plan=release_plan, - namespace=konflux_namespace, + namespace=resolved_namespace, apply=apply, ) if apply: green_print(f"✓ Successfully created release: {release_name}") LOGGER.info( - f"Monitor release progress: kubectl get release {release_name} -n {konflux_namespace or 'ocp-art-tenant'}" + f"Monitor release progress: kubectl get release {release_name} -n {resolved_namespace}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elliott/elliottlib/cli/konflux_release_cli.py` around lines 525 - 537, The CLI resolves the Konflux namespace inconsistently: it can pass a different namespace to create_release_from_snapshot and then log a hardcoded 'ocp-art-tenant' fallback in LOGGER.info, causing an incorrect monitor hint; update the flow to resolve the effective namespace once (e.g., compute resolved_namespace from konflux_namespace or the function that determines the default) before calling create_release_from_snapshot, pass that resolved_namespace into create_release_from_snapshot (instead of raw konflux_namespace if applicable), and then reuse the same resolved_namespace when building the monitor message in LOGGER.info/green_print so the created release and monitor command always reference the same namespace (references: create_release_from_snapshot, release_name, konflux_namespace, LOGGER.info, green_print).doozer/doozerlib/backend/konflux_image_builder.py (1)
1003-1029:⚠️ Potential issue | 🟠 MajorPass the already computed NVR instead of re-inspecting the built image.
At Line 1026, this re-extracts NVR from registry metadata and can skip the workflow if image inspection fails, even though
nvris already computed inbuild()before Line 267.Suggested fix
- async def _handle_base_image_completion(self, metadata: ImageMetadata, image_pullspec: str) -> None: + async def _handle_base_image_completion( + self, metadata: ImageMetadata, image_pullspec: str, nvr: Optional[str] = None + ) -> None: @@ - nvr = await self._extract_nvr_from_build(metadata, image_pullspec) + if not nvr: + nvr = await self._extract_nvr_from_build(image_pullspec) if not nvr: LOGGER.warning(f"Could not extract NVR for base image {metadata.distgit_key}, skipping workflow") return- await self._handle_base_image_completion(metadata, image_pullspec) + await self._handle_base_image_completion(metadata, image_pullspec, nvr=nvr)- async def _extract_nvr_from_build(self, metadata: ImageMetadata, image_pullspec: str) -> Optional[str]: + async def _extract_nvr_from_build(self, image_pullspec: str) -> Optional[str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doozer/doozerlib/backend/konflux_image_builder.py` around lines 1003 - 1029, The method _handle_base_image_completion currently re-extracts the NVR by calling _extract_nvr_from_build(image_pullspec) which can fail and skip the snapshot-release workflow; instead use the already-computed NVR from the build flow: either accept the NVR as an additional parameter to _handle_base_image_completion (propagate it from build()), or read it from metadata if metadata exposes the value (e.g., metadata.nvr or similar), and remove the _extract_nvr_from_build call and its fallback warning; keep references to metadata, image_pullspec, _handle_base_image_completion, _extract_nvr_from_build, and build() when making the change so callers are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 260-272: The post-build base-image workflow can fail but current
code sets metadata.build_status=True and record["message"/"status"] to success
before calling _handle_base_image_completion, and that handler swallows errors;
change the flow so you call await self._handle_base_image_completion(metadata,
image_pullspec) first, let it raise on error (or return an explicit success
bool), and only then set metadata.build_status=True and
record["message"]="Success"/record["status"]=0; update
_handle_base_image_completion to propagate exceptions (raise) or return failure
details instead of swallowing/logging them so callers can mark the build failed
and set an appropriate record message/status when snapshot/release fails.
In `@elliott/elliottlib/cli/konflux_release_cli.py`:
- Around line 338-364: create_release_from_snapshot validates snapshot existence
but never checks the ReleasePlan; add a call to konflux_client._get to verify
the ReleasePlan exists before constructing the release_resource. Specifically,
after verifying snapshot_name with await konflux_client._get(API_VERSION,
KIND_SNAPSHOT, snapshot_name), perform await konflux_client._get(API_VERSION,
KIND_RELEASEPLAN, release_plan) and catch exceptions.NotFoundError to raise a
RuntimeError like "ReleasePlan {release_plan} not found in namespace
{namespace}" so creation fails fast and with a clear message (use the same
API_VERSION, exception handling, and namespace variables already used).
---
Outside diff comments:
In `@elliott/elliottlib/cli/snapshot_cli.py`:
- Around line 287-293: The loop that builds snapshot metadata reuses
snapshot_name for every application when self.custom_name is True, causing name
collisions; update the "name" assignment in the metadata (where snapshot_name,
self.custom_name, index and app_components are used) to ensure uniqueness—e.g.,
if self.custom_name is True and there is more than one application in
app_components, append a unique suffix such as the index or the application_name
(e.g., f"{snapshot_name}-{index}" or f"{snapshot_name}-{application_name}") so
each generated Snapshot has a distinct name.
---
Duplicate comments:
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 1003-1029: The method _handle_base_image_completion currently
re-extracts the NVR by calling _extract_nvr_from_build(image_pullspec) which can
fail and skip the snapshot-release workflow; instead use the already-computed
NVR from the build flow: either accept the NVR as an additional parameter to
_handle_base_image_completion (propagate it from build()), or read it from
metadata if metadata exposes the value (e.g., metadata.nvr or similar), and
remove the _extract_nvr_from_build call and its fallback warning; keep
references to metadata, image_pullspec, _handle_base_image_completion,
_extract_nvr_from_build, and build() when making the change so callers are
updated consistently.
In `@elliott/elliottlib/cli/konflux_release_cli.py`:
- Around line 525-537: The CLI resolves the Konflux namespace inconsistently: it
can pass a different namespace to create_release_from_snapshot and then log a
hardcoded 'ocp-art-tenant' fallback in LOGGER.info, causing an incorrect monitor
hint; update the flow to resolve the effective namespace once (e.g., compute
resolved_namespace from konflux_namespace or the function that determines the
default) before calling create_release_from_snapshot, pass that
resolved_namespace into create_release_from_snapshot (instead of raw
konflux_namespace if applicable), and then reuse the same resolved_namespace
when building the monitor message in LOGGER.info/green_print so the created
release and monitor command always reference the same namespace (references:
create_release_from_snapshot, release_name, konflux_namespace, LOGGER.info,
green_print).
ℹ️ Review info
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 (6)
doozer/doozerlib/backend/konflux_image_builder.pydoozer/doozerlib/image.pydoozer/tests/test_image.pyelliott/elliottlib/cli/konflux_release_cli.pyelliott/elliottlib/cli/snapshot_cli.pyocp-build-data-validator/validator/json_schemas/image_config.base.schema.json
🚧 Files skipped from review as they are similar to previous changes (3)
- doozer/tests/test_image.py
- doozer/doozerlib/image.py
- ocp-build-data-validator/validator/json_schemas/image_config.base.schema.json
Comment on lines
+260
to
+272
| metadata.build_status = True | ||
| record["message"] = "Success" | ||
| record["status"] = 0 | ||
|
|
||
| # Check if this is a base image and trigger snapshot-release workflow | ||
| LOGGER.info(f"Post-build processing for {metadata.distgit_key}, image_pullspec: {image_pullspec}") | ||
| if image_pullspec: | ||
| await self._handle_base_image_completion(metadata, image_pullspec) | ||
| else: | ||
| LOGGER.warning( | ||
| f"No image pullspec available for base image workflow, skipping for {metadata.distgit_key}" | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
Base-image workflow failures are currently reported as build success.
build_status and success record fields are set before post-build processing (Lines 260-263), and _handle_base_image_completion swallows failures (Lines 1039-1044). This can mark the build successful even when snapshot/release fails.
Suggested fix
- metadata.build_status = True
- record["message"] = "Success"
- record["status"] = 0
-
# Check if this is a base image and trigger snapshot-release workflow
LOGGER.info(f"Post-build processing for {metadata.distgit_key}, image_pullspec: {image_pullspec}")
if image_pullspec:
await self._handle_base_image_completion(metadata, image_pullspec)
else:
LOGGER.warning(
f"No image pullspec available for base image workflow, skipping for {metadata.distgit_key}"
)
+ metadata.build_status = True
+ record["message"] = "Success"
+ record["status"] = 0- if result:
+ if result:
release_name, snapshot_name = result
LOGGER.info(f"✓ Base image workflow completed successfully for {metadata.distgit_key}")
LOGGER.info(f" Release: {release_name}")
LOGGER.info(f" Snapshot: {snapshot_name}")
else:
- LOGGER.warning(f"Base image workflow failed for {metadata.distgit_key}")
+ raise RuntimeError(f"Base image workflow failed for {metadata.distgit_key}")
- except Exception as e:
- LOGGER.error(f"Base image workflow error for {metadata.distgit_key}: {e}")
- LOGGER.debug(f"Base image workflow traceback: {traceback.format_exc()}")
+ except Exception:
+ LOGGER.exception(f"Base image workflow error for {metadata.distgit_key}")
+ raiseAlso applies to: 1034-1044
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doozer/doozerlib/backend/konflux_image_builder.py` around lines 260 - 272,
The post-build base-image workflow can fail but current code sets
metadata.build_status=True and record["message"/"status"] to success before
calling _handle_base_image_completion, and that handler swallows errors; change
the flow so you call await self._handle_base_image_completion(metadata,
image_pullspec) first, let it raise on error (or return an explicit success
bool), and only then set metadata.build_status=True and
record["message"]="Success"/record["status"]=0; update
_handle_base_image_completion to propagate exceptions (raise) or return failure
details instead of swallowing/logging them so callers can mark the build failed
and set an appropriate record message/status when snapshot/release fails.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
doozer/doozerlib/backend/konflux_image_builder.py (1)
260-267:⚠️ Potential issue | 🟠 MajorBase-image workflow failures can still be reported as successful builds.
Success is recorded before post-build workflow completes, and
_handle_base_image_completionswallows workflow failures. This can leavemetadata.build_statusand record status as success even when snapshot/release fails.🔧 Suggested fix
- metadata.build_status = True - record["message"] = "Success" - record["status"] = 0 - LOGGER.info(f"Post-build processing for {metadata.distgit_key}, image_pullspec: {image_pullspec}") if image_pullspec: await self._handle_base_image_completion(metadata, image_pullspec, nvr) else: LOGGER.warning( f"No image pullspec available for base image workflow, skipping for {metadata.distgit_key}" ) + metadata.build_status = True + record["message"] = "Success" + record["status"] = 0 @@ if result: release_name, snapshot_name = result LOGGER.info(f"✓ Base image workflow completed successfully for {metadata.distgit_key}") LOGGER.info(f" Release: {release_name}") LOGGER.info(f" Snapshot: {snapshot_name}") else: - LOGGER.warning(f"Base image workflow failed for {metadata.distgit_key}") + raise RuntimeError(f"Base image workflow failed for {metadata.distgit_key}") - except Exception as e: - LOGGER.error(f"Base image workflow error for {metadata.distgit_key}: {e}") - LOGGER.debug(f"Base image workflow traceback: {traceback.format_exc()}") + except Exception: + LOGGER.exception(f"Base image workflow error for {metadata.distgit_key}") + raiseAlso applies to: 1029-1039
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doozer/doozerlib/backend/konflux_image_builder.py` around lines 260 - 267, The code marks build success (metadata.build_status = True and record["status"] = 0) before running post-build steps and ignores failures from _handle_base_image_completion; change the flow so success is only recorded after post-build processing completes without error: move the metadata.build_status and record status updates to after the call to _handle_base_image_completion (and the equivalent block around the 1029-1039 area), and modify _handle_base_image_completion call handling to detect raised exceptions or non-success return values (do not swallow them) — on error set metadata.build_status = False, record["status"] and record["message"] accordingly, and log the failure via LOGGER.error; ensure any exceptions are propagated or converted to proper failure records instead of being ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doozer/tests/backend/test_base_image_handler.py`:
- Around line 25-27: The test mocks for resolve_konflux_namespace_by_product and
resolve_konflux_kubeconfig_by_product are targeting artcommonlib.util but
BaseImageHandler imports these functions into its own module namespace, so
update both tests to patch the names where they are looked up (the
base_image_handler module) instead of artcommonlib.util; change the `@patch`
decorators that currently reference
"artcommonlib.util.resolve_konflux_namespace_by_product" and
"artcommonlib.util.resolve_konflux_kubeconfig_by_product" to patch
"doozer.backend.base_image_handler.resolve_konflux_namespace_by_product" and
"doozer.backend.base_image_handler.resolve_konflux_kubeconfig_by_product" (apply
the same change to both test methods), leaving the KonfluxClient.from_kubeconfig
patch as-is and ensuring the patched names align with BaseImageHandler's
imports.
---
Duplicate comments:
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 260-267: The code marks build success (metadata.build_status =
True and record["status"] = 0) before running post-build steps and ignores
failures from _handle_base_image_completion; change the flow so success is only
recorded after post-build processing completes without error: move the
metadata.build_status and record status updates to after the call to
_handle_base_image_completion (and the equivalent block around the 1029-1039
area), and modify _handle_base_image_completion call handling to detect raised
exceptions or non-success return values (do not swallow them) — on error set
metadata.build_status = False, record["status"] and record["message"]
accordingly, and log the failure via LOGGER.error; ensure any exceptions are
propagated or converted to proper failure records instead of being ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e158e2b5-8aa5-4f9c-8f3c-bde49fec4a17
📒 Files selected for processing (4)
doozer/doozerlib/backend/base_image_handler.pydoozer/doozerlib/backend/konflux_image_builder.pydoozer/tests/backend/test_base_image_handler.pyelliott/elliottlib/cli/konflux_release_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- elliott/elliottlib/cli/konflux_release_cli.py
lgarciaaco
force-pushed
the
feat/base-image-snapshot-release
branch
from
e6018b7 to
c51d26a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
doozer/tests/backend/test_base_image_handler.py (1)
25-27:⚠️ Potential issue | 🟠 MajorPatch targets are incorrect for resolver mocks.
At Line 26/27 and Line 48/49, the decorators patch
artcommonlib.util.*, butBaseImageHandlerlooks up these names indoozerlib.backend.base_image_handler. These mocks won’t reliably intercept the calls.🔧 Proposed fix
- `@patch`("artcommonlib.util.resolve_konflux_namespace_by_product") - `@patch`("artcommonlib.util.resolve_konflux_kubeconfig_by_product") + `@patch`("doozerlib.backend.base_image_handler.resolve_konflux_namespace_by_product") + `@patch`("doozerlib.backend.base_image_handler.resolve_konflux_kubeconfig_by_product")#!/bin/bash # Verify import lookup location vs patch targets rg -n "from artcommonlib.util import|resolve_konflux_(namespace|kubeconfig)_by_product" doozer/doozerlib/backend/base_image_handler.py -C2 rg -n '@patch\(".*resolve_konflux_(namespace|kubeconfig)_by_product"\)' doozer/tests/backend/test_base_image_handler.py -C1Also applies to: 47-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doozer/tests/backend/test_base_image_handler.py` around lines 25 - 27, The test patches target artcommonlib.util but BaseImageHandler imports those functions/clients into doozerlib.backend.base_image_handler, so update the `@patch` decorators to mock the names where they are looked up: change the patch targets to doozerlib.backend.base_image_handler.resolve_konflux_namespace_by_product, doozerlib.backend.base_image_handler.resolve_konflux_kubeconfig_by_product and doozerlib.backend.base_image_handler.KonfluxClient.from_kubeconfig (matching the symbols used by BaseImageHandler) so the mocks reliably intercept calls in the test.doozer/doozerlib/backend/konflux_image_builder.py (1)
260-271:⚠️ Potential issue | 🟠 MajorBuild success is committed before base-image workflow outcome is known.
Line 260 sets successful build state before post-build workflow runs, and failures are swallowed in
_handle_base_image_completion(Line 1035 and Line 1037). This can report false-success when snapshot/release fails.🔧 Proposed fix
- metadata.build_status = True - record["message"] = "Success" - record["status"] = 0 - LOGGER.info(f"Post-build processing for {metadata.distgit_key}, image_pullspec: {image_pullspec}") if image_digest: await self._handle_base_image_completion(metadata, image_digest, nvr) else: LOGGER.warning( f"No image pullspec available for base image workflow, skipping for {metadata.distgit_key}" ) + metadata.build_status = True + record["message"] = "Success" + record["status"] = 0- if result: + if result: release_name, snapshot_name = result LOGGER.info(f"✓ Base image workflow completed successfully for {metadata.distgit_key}") LOGGER.info(f" Release: {release_name}") LOGGER.info(f" Snapshot: {snapshot_name}") else: - LOGGER.warning(f"Base image workflow failed for {metadata.distgit_key}") + raise RuntimeError(f"Base image workflow failed for {metadata.distgit_key}") - except Exception as e: - LOGGER.error(f"Base image workflow error for {metadata.distgit_key}: {e}") - LOGGER.debug(f"Base image workflow traceback: {traceback.format_exc()}") + except Exception: + LOGGER.exception(f"Base image workflow error for {metadata.distgit_key}") + raiseAlso applies to: 1029-1039
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doozer/doozerlib/backend/konflux_image_builder.py` around lines 260 - 271, The code marks success early by setting metadata.build_status = True and record["message"] = "Success" before awaiting _handle_base_image_completion; instead, move or delay setting metadata.build_status and the success record until after the await of self._handle_base_image_completion(metadata, image_digest, nvr) completes successfully, and wrap that await in a try/except so that if _handle_base_image_completion raises you set metadata.build_status = False, set record["status"] to a non-zero value and populate record["message"] with the caught error, and log the failure via LOGGER.error; keep the LOGGER.info for post-build processing but ensure outcome-based state updates occur after the try/except around _handle_base_image_completion.doozer/doozerlib/backend/base_image_handler.py (1)
124-129:⚠️ Potential issue | 🟡 MinorUse higher-resolution timestamp for snapshot name generation.
Line 124 currently uses second-level precision, so rapid retries/concurrent runs can still collide on
snapshot_name.🔧 Proposed fix
- timestamp = get_utc_now_formatted_str() + timestamp = get_utc_now_formatted_str(microseconds=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doozer/doozerlib/backend/base_image_handler.py` around lines 124 - 129, The snapshot_name uses get_utc_now_formatted_str() which yields second-level precision and can collide; update the timestamp generation in base_image_handler.py (around where snapshot_name is formed) to use a higher-resolution UTC timestamp (millisecond or microsecond precision) — e.g. use datetime.utcnow().strftime including %f or a helper that returns ms/µs — then rebuild snapshot_name = f"{group_safe}-{component_name}-{timestamp}" using that higher-res timestamp; keep the normalize_group_name_for_k8s(self.runtime.group) and ValueError logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 265-267: The call to _handle_base_image_completion(metadata,
image_digest, nvr) passes a digest-only string (e.g., "sha256:...") into the
image_pullspec parameter which expects a full pullspec; change the argument so
image_pullspec is a full repository pullspec with the digest (e.g.,
"<repository>/<name>@sha256:...") by constructing it from existing metadata
fields (such as a repo/name or original pullspec) plus "@" + image_digest or by
using metadata["image_pullspec"] if present, and pass that full pullspec into
_handle_base_image_completion instead of the raw image_digest.
---
Duplicate comments:
In `@doozer/doozerlib/backend/base_image_handler.py`:
- Around line 124-129: The snapshot_name uses get_utc_now_formatted_str() which
yields second-level precision and can collide; update the timestamp generation
in base_image_handler.py (around where snapshot_name is formed) to use a
higher-resolution UTC timestamp (millisecond or microsecond precision) — e.g.
use datetime.utcnow().strftime including %f or a helper that returns ms/µs —
then rebuild snapshot_name = f"{group_safe}-{component_name}-{timestamp}" using
that higher-res timestamp; keep the
normalize_group_name_for_k8s(self.runtime.group) and ValueError logic unchanged.
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 260-271: The code marks success early by setting
metadata.build_status = True and record["message"] = "Success" before awaiting
_handle_base_image_completion; instead, move or delay setting
metadata.build_status and the success record until after the await of
self._handle_base_image_completion(metadata, image_digest, nvr) completes
successfully, and wrap that await in a try/except so that if
_handle_base_image_completion raises you set metadata.build_status = False, set
record["status"] to a non-zero value and populate record["message"] with the
caught error, and log the failure via LOGGER.error; keep the LOGGER.info for
post-build processing but ensure outcome-based state updates occur after the
try/except around _handle_base_image_completion.
In `@doozer/tests/backend/test_base_image_handler.py`:
- Around line 25-27: The test patches target artcommonlib.util but
BaseImageHandler imports those functions/clients into
doozerlib.backend.base_image_handler, so update the `@patch` decorators to mock
the names where they are looked up: change the patch targets to
doozerlib.backend.base_image_handler.resolve_konflux_namespace_by_product,
doozerlib.backend.base_image_handler.resolve_konflux_kubeconfig_by_product and
doozerlib.backend.base_image_handler.KonfluxClient.from_kubeconfig (matching the
symbols used by BaseImageHandler) so the mocks reliably intercept calls in the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e28375a-7567-4194-b107-60b4ea9d3638
📒 Files selected for processing (4)
doozer/doozerlib/backend/base_image_handler.pydoozer/doozerlib/backend/konflux_image_builder.pydoozer/tests/backend/test_base_image_handler.pyelliott/elliottlib/cli/konflux_release_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- elliott/elliottlib/cli/konflux_release_cli.py
lgarciaaco
force-pushed
the
feat/base-image-snapshot-release
branch
from
c51d26a to
4a2296c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
doozer/doozerlib/backend/base_image_handler.py (1)
124-129:⚠️ Potential issue | 🟡 MinorSnapshot name collision risk remains for same-second retries.
Name generation still uses second-level precision; rapid retries can collide.
🔧 Suggested fix
- timestamp = get_utc_now_formatted_str() + timestamp = get_utc_now_formatted_str(microseconds=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doozer/doozerlib/backend/base_image_handler.py` around lines 124 - 129, The snapshot name generation in base_image_handler.py (where snapshot_name is built using get_utc_now_formatted_str(), normalize_group_name_for_k8s(self.runtime.group), and component_name) can collide for retries within the same second; change the construction to include higher-resolution time or a short unique suffix: either update get_utc_now_formatted_str to include milliseconds/microseconds or append a small random/UUID-derived token (e.g., a 6–8 char hex from uuid4) when building snapshot_name to guarantee uniqueness while keeping the group/component logic intact.doozer/doozerlib/backend/konflux_image_builder.py (1)
260-267:⚠️ Potential issue | 🟠 MajorBase-image workflow failure is still masked as a successful build.
metadata.build_statusand success record fields are set before base-image post-processing, and_handle_base_image_completionlogs/continues on failure. This still allows a failed snapshot/release flow to be reported as success.🔧 Suggested fix
- metadata.build_status = True - record["message"] = "Success" - record["status"] = 0 - LOGGER.info(f"Post-build processing for {metadata.distgit_key}, image_pullspec: {image_pullspec}") if image_digest: await self._handle_base_image_completion(metadata, record["image_pullspec"], nvr) else: LOGGER.warning( f"No image pullspec available for base image workflow, skipping for {metadata.distgit_key}" ) + + metadata.build_status = True + record["message"] = "Success" + record["status"] = 0- if result: + if result: release_name, snapshot_name = result LOGGER.info(f"✓ Base image workflow completed successfully for {metadata.distgit_key}") LOGGER.info(f" Release: {release_name}") LOGGER.info(f" Snapshot: {snapshot_name}") else: - LOGGER.warning(f"Base image workflow failed for {metadata.distgit_key}") + raise RuntimeError(f"Base image workflow failed for {metadata.distgit_key}") - except Exception as e: - LOGGER.error(f"Base image workflow error for {metadata.distgit_key}: {e}") - LOGGER.debug(f"Base image workflow traceback: {traceback.format_exc()}") + except Exception: + LOGGER.exception(f"Base image workflow error for {metadata.distgit_key}") + raiseAlso applies to: 1029-1039
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doozer/doozerlib/backend/konflux_image_builder.py` around lines 260 - 267, The code marks builds successful (metadata.build_status = True, record["message"]="Success", record["status"]=0) before running base-image post-processing, which lets failures in _handle_base_image_completion be masked; change the flow so you only set metadata.build_status and the success record after _handle_base_image_completion completes successfully (when image_digest is truthy), and if _handle_base_image_completion raises or returns a failure indicator catch it, set metadata.build_status = False, set record["message"] to the error/failed reason and record["status"] to a non-zero value, and ensure errors are logged and not swallowed so the snapshot/release flow correctly reports failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doozer/doozerlib/backend/base_image_handler.py`:
- Around line 161-164: The call to konflux_client.resource_url is being passed a
ResourceInstance instead of a dict; change the two usages to pass the dictionary
representation by calling .to_dict() on the ResourceInstance (e.g., when
computing snapshot_url use
konflux_client.resource_url(created_snapshot.to_dict())); update any other
similar call in this file that passes a ResourceInstance (look for
konflux_client.resource_url(created_*) usages) to use .to_dict() as well.
- Around line 264-266: The code uses the ResourceInstance returned by
self.konflux_client._get(...) (assigned to release_obj) and then does dict-style
access; change this to normalize the result first by calling .to_dict() on the
ResourceInstance (e.g., release_obj = (await
self.konflux_client._get(API_VERSION, KIND_RELEASE, release_name)).to_dict() or
assign a separate dict like release_dict = release_obj.to_dict()) and then use
release_dict.get('status', {}) and status.get('conditions', []); update any
subsequent references that expect a dict to use the converted dict variable so
dict-style access is safe.
---
Duplicate comments:
In `@doozer/doozerlib/backend/base_image_handler.py`:
- Around line 124-129: The snapshot name generation in base_image_handler.py
(where snapshot_name is built using get_utc_now_formatted_str(),
normalize_group_name_for_k8s(self.runtime.group), and component_name) can
collide for retries within the same second; change the construction to include
higher-resolution time or a short unique suffix: either update
get_utc_now_formatted_str to include milliseconds/microseconds or append a small
random/UUID-derived token (e.g., a 6–8 char hex from uuid4) when building
snapshot_name to guarantee uniqueness while keeping the group/component logic
intact.
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 260-267: The code marks builds successful (metadata.build_status =
True, record["message"]="Success", record["status"]=0) before running base-image
post-processing, which lets failures in _handle_base_image_completion be masked;
change the flow so you only set metadata.build_status and the success record
after _handle_base_image_completion completes successfully (when image_digest is
truthy), and if _handle_base_image_completion raises or returns a failure
indicator catch it, set metadata.build_status = False, set record["message"] to
the error/failed reason and record["status"] to a non-zero value, and ensure
errors are logged and not swallowed so the snapshot/release flow correctly
reports failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 198234ec-f84a-4e53-9b5a-53fcab989e93
📒 Files selected for processing (4)
doozer/doozerlib/backend/base_image_handler.pydoozer/doozerlib/backend/konflux_image_builder.pydoozer/tests/backend/test_base_image_handler.pyelliott/elliottlib/cli/konflux_release_cli.py
🚧 Files skipped from review as they are similar to previous changes (2)
- elliott/elliottlib/cli/konflux_release_cli.py
- doozer/tests/backend/test_base_image_handler.py
Comment on lines
+161
to
+164
| created_snapshot = await self.konflux_client._create(snapshot_obj) | ||
| actual_snapshot_name = created_snapshot.metadata.name | ||
| snapshot_url = self.konflux_client.resource_url(created_snapshot) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the resource_url method definition and _create method
echo "=== Checking KonfluxClient methods ==="
rg -n "def resource_url|async def _create" doozer/doozerlib/backend/konflux_client.py -A 10
echo ""
echo "=== Checking call sites in base_image_handler.py ==="
rg -n "resource_url\(" doozer/doozerlib/backend/base_image_handler.py -B 3 -A 2
echo ""
echo "=== Checking ResourceInstance class definition ==="
rg -n "class ResourceInstance" -A 20Repository: openshift-eng/art-tools
Length of output: 3134
🏁 Script executed:
#!/bin/bash
# Check if ResourceInstance has a to_dict method
echo "=== Searching for to_dict method in resource module ==="
fd "resource\." doozer/doozerlib/backend/ -type f | head -5
find doozer/doozerlib/backend/ -name "*resource*" -type f
echo ""
echo "=== Check imports in base_image_handler.py to understand ResourceInstance ==="
head -50 doozer/doozerlib/backend/base_image_handler.py | grep -E "^import|^from"Repository: openshift-eng/art-tools
Length of output: 786
🏁 Script executed:
#!/bin/bash
# Verify _create return type and check if ResourceInstance exists and has to_dict
echo "=== Full _create implementation ==="
rg -n "async def _create" doozer/doozerlib/backend/konflux_client.py -A 15 | head -30
echo ""
echo "=== Check return statement in _create ==="
rg -n "return.*_create\|return await" doozer/doozerlib/backend/konflux_client.py | grep -A 2 -B 2 "_create"
echo ""
echo "=== Search for ResourceInstance class ==="
rg -n "class ResourceInstance|\.to_dict\(" --type py | grep -i resource | head -10
echo ""
echo "=== Check if resource module exists ==="
find doozer/doozerlib/backend -name "*.py" -exec grep -l "ResourceInstance\|resource.ResourceInstance" {} \;Repository: openshift-eng/art-tools
Length of output: 2897
🏁 Script executed:
#!/bin/bash
# Check if ResourceInstance has a to_dict method by looking at resource module
echo "=== Find resource module location ==="
find doozer -name "resource.py" -o -name "resources.py" | head -5
echo ""
echo "=== Check kubernetes.dynamic imports for ResourceInstance ==="
rg -n "from kubernetes|import.*resource" doozer/doozerlib/backend/konflux_client.py | head -20
echo ""
echo "=== Verify _create full return path ==="
rg -n "async def _create" doozer/doozerlib/backend/konflux_client.py -A 20 | tail -15Repository: openshift-eng/art-tools
Length of output: 1335
🏁 Script executed:
#!/bin/bash
# Check kubernetes.dynamic.resource.ResourceInstance to_dict method
python3 << 'EOF'
try:
from kubernetes.dynamic import resource
# Check if ResourceInstance has to_dict
if hasattr(resource.ResourceInstance, 'to_dict'):
print("✓ ResourceInstance has to_dict method")
else:
print("✗ ResourceInstance does NOT have to_dict method")
# Try to inspect the class
import inspect
print("\nResourceInstance methods:")
methods = [m for m in dir(resource.ResourceInstance) if not m.startswith('_')]
for m in sorted(methods)[:20]:
print(f" - {m}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: openshift-eng/art-tools
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Examine the actual usage patterns in the codebase to confirm the pattern
echo "=== All resource_url calls in the codebase ==="
rg -n "resource_url\(" doozer/doozerlib/backend/ -B 1 -A 1
echo ""
echo "=== Specifically check base_image_handler.py for pattern ==="
rg -n "resource_url\(" doozer/doozerlib/backend/base_image_handler.py -B 2 -A 1
echo ""
echo "=== Check return type comment in _create ==="
sed -n '296,315p' doozer/doozerlib/backend/konflux_client.pyRepository: openshift-eng/art-tools
Length of output: 6105
resource_url(...) is called with the wrong object type.
KonfluxClient._create(...) returns a ResourceInstance object, while resource_url(...) expects a dictionary. All other call sites in the codebase correctly pass .to_dict(), but these two calls do not. Update both lines to pass the dictionary representation.
🔧 Suggested fix
- snapshot_url = self.konflux_client.resource_url(created_snapshot)
+ snapshot_url = self.konflux_client.resource_url(created_snapshot.to_dict())- release_url = self.konflux_client.resource_url(created_release)
+ release_url = self.konflux_client.resource_url(created_release.to_dict())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doozer/doozerlib/backend/base_image_handler.py` around lines 161 - 164, The
call to konflux_client.resource_url is being passed a ResourceInstance instead
of a dict; change the two usages to pass the dictionary representation by
calling .to_dict() on the ResourceInstance (e.g., when computing snapshot_url
use konflux_client.resource_url(created_snapshot.to_dict())); update any other
similar call in this file that passes a ResourceInstance (look for
konflux_client.resource_url(created_*) usages) to use .to_dict() as well.
Comment on lines
+264
to
+266
| release_obj = await self.konflux_client._get(API_VERSION, KIND_RELEASE, release_name) | ||
| status = release_obj.get('status', {}) | ||
| conditions = status.get('conditions', []) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check _get implementation in konflux_client.py
rg -n "async def _get" doozer/doozerlib/backend/konflux_client.py -A40 -B5Repository: openshift-eng/art-tools
Length of output: 6856
🏁 Script executed:
#!/bin/bash
# Check usages of _get with KIND_RELEASE in base_image_handler.py
rg -n "_get\(API_VERSION, KIND_RELEASE" doozer/doozerlib/backend/base_image_handler.py -A8 -B4Repository: openshift-eng/art-tools
Length of output: 1496
🏁 Script executed:
#!/bin/bash
# Check if ResourceInstance class has to_dict method and what _get actually returns
fd "konflux_client.py" -type f | head -5 | xargs head -300Repository: openshift-eng/art-tools
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Search for ResourceInstance usage patterns and imports
rg -n "ResourceInstance" doozer/doozerlib/backend/ -B2 -A2Repository: openshift-eng/art-tools
Length of output: 11533
🏁 Script executed:
#!/bin/bash
# Check if there are any type hints that show what _get returns
rg -n "-> resource\." doozer/doozerlib/backend/konflux_client.pyRepository: openshift-eng/art-tools
Length of output: 471
🏁 Script executed:
#!/bin/bash
# Check broader usage of _get return values and how they're accessed
rg -n "await.*_get\(" doozer/doozerlib/backend/base_image_handler.py -A4Repository: openshift-eng/art-tools
Length of output: 1081
🏁 Script executed:
#!/bin/bash
# Check if there are other places calling .get() on _get() results
rg -n "release_obj\.get\(" doozer/doozerlib/backend/ -B3 -A1Repository: openshift-eng/art-tools
Length of output: 614
🏁 Script executed:
#!/bin/bash
# Check what kubernetes library is being used and ResourceInstance behavior
rg -n "from kubernetes" doozer/doozerlib/backend/konflux_client.py | head -10Repository: openshift-eng/art-tools
Length of output: 245
Normalize _get() return value before dict-style access.
The code at lines 264-266 calls .get('status', {}) directly on the ResourceInstance returned by _get(), which violates the established pattern throughout the codebase. Both pipelinerun_utils.py and konflux_client.py explicitly convert ResourceInstance to dict using .to_dict() before dict-style access. Without this conversion, release polling will fail at runtime.
🔧 Suggested fix
release_obj = await self.konflux_client._get(API_VERSION, KIND_RELEASE, release_name)
- status = release_obj.get('status', {})
+ release_data = release_obj.to_dict() if hasattr(release_obj, "to_dict") else release_obj
+ status = release_data.get('status', {})
conditions = status.get('conditions', [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doozer/doozerlib/backend/base_image_handler.py` around lines 264 - 266, The
code uses the ResourceInstance returned by self.konflux_client._get(...)
(assigned to release_obj) and then does dict-style access; change this to
normalize the result first by calling .to_dict() on the ResourceInstance (e.g.,
release_obj = (await self.konflux_client._get(API_VERSION, KIND_RELEASE,
release_name)).to_dict() or assign a separate dict like release_dict =
release_obj.to_dict()) and then use release_dict.get('status', {}) and
status.get('conditions', []); update any subsequent references that expect a
dict to use the converted dict variable so dict-style access is safe.
Contributor
Author
|
Created the Snapshot and Release and failed on conforma Logs output |
lgarciaaco
force-pushed
the
feat/base-image-snapshot-release
branch
from
4a2296c to
3e4284b
Compare
… workflow Simplifies the base image completion workflow by eliminating the Elliott CLI dependency and cmd_gather_async usage. The snapshot and release creation now uses direct Konflux API calls following the same patterns as CreateSnapshotCli and CreateReleaseCli. Key changes: - Replace Elliott CLI subprocess calls with native Konflux client operations - Remove unnecessary NVR extraction since it's already computed during build - Clean up imports and remove unused Elliott integration code - Streamline error handling and logging for better diagnostics - Remove Elliott CLI command for release-from-snapshot (functionality moved to base handler) This reduces external dependencies and improves maintainability by keeping the base image workflow self-contained within the doozer codebase.
lgarciaaco
force-pushed
the
feat/base-image-snapshot-release
branch
from
3e4284b to
cca6052
Compare
ashwindasr
requested changes
Mar 8, 2026
|
|
||
| LOGGER.info(f"Post-build processing for {metadata.distgit_key}, image_pullspec: {image_pullspec}") | ||
| if image_digest: | ||
| await self._handle_base_image_completion(metadata, record["image_pullspec"], nvr) |
Contributor
There was a problem hiding this comment.
If the build is not pushed out to the base images repo due to conforma failure, that could be an issue.
I think it might be a good idea to move this feature to a new job and trigger it from ocp4-konflux. That way we can update ocp4-scan-konflux to check to ensure if the latest builds are released to the repo. We can also rerun the job if any was missed as well. What do you think?
Remove base image handling from konflux build integration and create dedicated doozer command for snapshot-to-release workflow. This enables the workflow to run as a separate job rather than embedded in build completion, improving separation of concerns and job scheduling flexibility. Changes: - Add images:release-to-base-repo command with full workflow logic - Remove _handle_base_image_completion from KonfluxImageBuilder - Remove BaseImageHandler import from build system - Clean up build completion hooks for base images
Contributor
|
@lgarciaaco: The following tests 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. |
ashwindasr
approved these changes
Mar 16, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashwindasr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
merged commit 32381c2
into
openshift-eng:main
3 of 5 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implement conditional snapshot-to-release workflow for golang base images that enhances existing Konflux build completion processes while maintaining backward compatibility. Adds base image detection, snapshot-release orchestration, and proper Release state monitoring.
Problem
Before: Only quay.io URLs are updated directly after build completion, with no snapshot/release workflow
After: Base images should trigger snapshot creation → release creation → dual URL updates (both quay.io and registry.redhat.io) in streams.yml with proper conditional control via base_only=true and snapshot_release=true configuration flags
Tests
Test job
Output