ART-19443: Add gen-assembly-lp and prepare-release-lp pipelines for Layered Products#2989
Open
ashwindasr wants to merge 4 commits into
Open
ART-19443: Add gen-assembly-lp and prepare-release-lp pipelines for Layered Products#2989ashwindasr wants to merge 4 commits into
ashwindasr wants to merge 4 commits into
Conversation
3 tasks
Contributor
|
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 LP assembly and release pipelines, shared FBC helpers, and supporting updates to build outputs, runtime behavior, and tests. ChangesLP release pipeline updates
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ashwindasr
marked this pull request as draft
June 3, 2026 02:02
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
ashwindasr
changed the title
![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: 1
🧹 Nitpick comments (2)
pyartcd/tests/pipelines/test_gen_assembly_lp.py (1)
216-216: 💤 Low valueConsider using
asyncio.run()instead of deprecatedget_event_loop().run_until_complete().
asyncio.get_event_loop()is deprecated since Python 3.10. While this works in tests, usingasyncio.run()is the modern approach.♻️ Suggested refactor
- result = asyncio.get_event_loop().run_until_complete(pipeline.run()) + result = asyncio.run(pipeline.run())Also applies to: 239-239
🤖 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/tests/pipelines/test_gen_assembly_lp.py` at line 216, Replace deprecated asyncio.get_event_loop().run_until_complete(...) calls with asyncio.run(...) in the test; specifically change the line assigning result from asyncio.get_event_loop().run_until_complete(pipeline.run()) to use asyncio.run(pipeline.run()) (and the similar occurrence at the later instance), so the test invokes pipeline.run() with asyncio.run for modern, non-deprecated event loop usage.pyartcd/pyartcd/pipelines/prepare_release_lp.py (1)
444-456: ⚡ Quick winAdd exception chaining for better traceability.
Line 456 raises a new
ValueErrorbut doesn't chain the originalGithubException. This loses the original traceback, making debugging harder.Proposed fix
except GithubException as e: - raise ValueError(f"Failed to fetch {filename} from {data_path} branch {branch}: {e}") + raise ValueError(f"Failed to fetch {filename} from {data_path} branch {branch}: {e}") from e🤖 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/prepare_release_lp.py` around lines 444 - 456, The except block in get_file_from_branch catches GithubException and raises a new ValueError without chaining the original exception, losing the original traceback; modify the raise to chain the original exception (use "raise ValueError(... ) from e") so the original GithubException is preserved for debugging while keeping the descriptive message about failing to fetch the file.
🤖 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/gen_assembly_lp.py`:
- Around line 205-206: The code uses an undefined Model symbol and creates an
unused releases_model, which will raise a NameError; remove the dead
instantiation or import and use Model properly. Fix by deleting the
releases_model assignment (the releases_config from
yaml.load(releases_yaml_path) is the only value used later), or if a Model is
intended, add the proper import (e.g., from artcommonlib import Model) and
replace downstream uses to reference releases_model. Ensure the lines
referencing Model and releases_model are either removed or backed by a correct
import so no NameError occurs.
---
Nitpick comments:
In `@pyartcd/pyartcd/pipelines/prepare_release_lp.py`:
- Around line 444-456: The except block in get_file_from_branch catches
GithubException and raises a new ValueError without chaining the original
exception, losing the original traceback; modify the raise to chain the original
exception (use "raise ValueError(... ) from e") so the original GithubException
is preserved for debugging while keeping the descriptive message about failing
to fetch the file.
In `@pyartcd/tests/pipelines/test_gen_assembly_lp.py`:
- Line 216: Replace deprecated asyncio.get_event_loop().run_until_complete(...)
calls with asyncio.run(...) in the test; specifically change the line assigning
result from asyncio.get_event_loop().run_until_complete(pipeline.run()) to use
asyncio.run(pipeline.run()) (and the similar occurrence at the later instance),
so the test invokes pipeline.run() with asyncio.run for modern, non-deprecated
event loop usage.
🪄 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: c053fe81-a603-4b73-b6be-48003606ee0b
📒 Files selected for processing (5)
pyartcd/pyartcd/__main__.pypyartcd/pyartcd/pipelines/gen_assembly_lp.pypyartcd/pyartcd/pipelines/prepare_release_lp.pypyartcd/tests/pipelines/test_gen_assembly_lp.pypyartcd/tests/pipelines/test_prepare_release_lp.py
2 tasks
ashwindasr
force-pushed
the
cursor-lp-asm-abcd
branch
from
June 4, 2026 17:31
61ccf62 to
f78cb2a
Compare
Merged
2 tasks
ashwindasr
force-pushed
the
cursor-lp-asm-abcd
branch
2 times, most recently
from
June 5, 2026 18:43
2fe4c1f to
efd9db5
Compare
ashwindasr
marked this pull request as ready for review
June 5, 2026 19:45
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
doozer/tests/cli/test_fbc.py (1)
151-161: ⚡ Quick winAssert the JSON
pullspecspayload, not just the mocked inputs.These updated tests feed the new PipelineRun fields, but they never verify that
run()actually emits the derived pullspecs in its JSON output. Capturingclick.echoin one happy-path case would lock down the new CLI contract and catch regressions in pullspec reuse/parsing.🤖 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 `@doozer/tests/cli/test_fbc.py` around lines 151 - 161, The test currently mocks mock_builder.build to return PipelineRun-like status results but never asserts that run() actually emits the derived JSON "pullspecs" payload; update the happy-path test to capture click.echo output from invoking run(), parse the echoed JSON, and assert the "pullspecs" field equals the expected list (e.g., the image URL and image digest derived from mock_builder.build's status results). Locate the test that calls run() and uses mock_builder.build (the block returning "test-pipelinerun" and the status results) and add an assertion comparing the parsed JSON payload's "pullspecs" to the expected values to lock down the CLI contract.
🤖 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 `@doozer/doozerlib/cli/fbc.py`:
- Around line 665-666: The code currently returns an empty pullspec string for
reused or newly-created KonfluxFbcBuildRecord, which hides errors; update the
reuse branch (where existing_fbc_build is returned) and the fresh-build branch
(the code around where a new KonfluxFbcBuildRecord is produced) to check
existing_fbc_build.image_pullspec (and the equivalent new_record.image_pullspec)
and if it's falsy and self.force is not set, raise a clear exception (e.g.,
click.ClickException or RuntimeError) asking the user to re-run with --force or
handle the missing pullspec; if self.force is true allow returning the record as
before. Ensure you adjust both the return site that currently does
"existing_fbc_build.nvr, existing_fbc_build.image_pullspec or ''" and the
corresponding return for the fresh-build path so they do not silently return an
empty string.
- Around line 679-682: When building the digest pullspec replace
image_url.split(':')[0] with logic that strips only the tag part (the last
colon) so registry ports are preserved; update the pullspec construction that
uses image_url and image_digest (variables image_url, image_digest, pullspec) to
derive the name-without-tag via image_url.rsplit(':', 1)[0] (or equivalent) and
then combine that with @{image_digest} only when both values exist.
In `@doozer/doozerlib/runtime.py`:
- Line 96: Runtime.extra_vars is typed as dict[str, str] but get_replace_vars()
and the loop that processes CLI-style KEY=VALUE strings (see iteration near
lines 398-407) expect an iterable of "KEY=VALUE" strings; change the
Runtime.extra_vars declaration and initialization in class Runtime to a list of
strings (e.g., extra_vars: list[str] = []) so callers can pass CLI-style entries
and the existing parsing logic in get_replace_vars()/the loop continues to work
without raising "Invalid --var format" errors.
In `@pyartcd/pyartcd/fbc_util.py`:
- Around line 98-128: The current logic in the operator-extraction block of
fbc_util.py assigns a per-pullspec UNKNOWN-... value when it cannot determine an
operator (using fbc_pullspec, labels, nvr and tag), which allows same-operator
FBCs to be split; instead, change both the "else" branch and the except block to
raise an exception (e.g., ValueError or RuntimeError) with a clear message
including fbc_pullspec and nvr/tag rather than assigning operator =
f"UNKNOWN-..."; keep the existing warning/error logs but follow them by raising
so caller code fails the validation when operator identity cannot be
established.
In `@pyartcd/pyartcd/pipelines/gen_assembly_lp.py`:
- Around line 219-251: _resolve_parent_operands currently only reads
members_images from the single basis_assembly and misses inherited operands;
change it to recursively walk the basis/parent chain in releases_config (use
releases_config.get('releases') to lookup ancestors), merging each ancestor's
assembly.members.images into operand_map so that descendant overrides win (child
entries override parent entries), and guard against cycles/too-deep recursion;
preserve existing logging and return operand_map as before.
In `@pyartcd/pyartcd/pipelines/prepare_release_lp.py`:
- Around line 201-233: The _load_assembly/_extract_operand_nvrs flow currently
reads only the assembly block's members.images, which misses inherited entries
from a chain of basis.assembly (LP generated delta assemblies); update
_load_assembly to resolve the full effective assembly by following the
basis.assembly chain (reading parent assemblies from releases.yml until no
basis), merge each parent's members.images into the child (preserving override
semantics), then pass that expanded assembly to _extract_operand_nvrs so it
iterates the full effective members.images list; reference the functions
_load_assembly, _extract_operand_nvrs and the basis.assembly / members.images
keys when implementing the merge.
In `@pyartcd/tests/pipelines/test_gen_assembly_lp.py`:
- Around line 293-305: The test currently patches
GenAssemblyLPPipeline._extract_nvrs_from_fbc so the single-pullspec branch is
never exercised; remove that patch and instead patch the lower-level extractor
function that _extract_nvrs_from_fbc calls (i.e., the downstream extractor used
inside GenAssemblyLPPipeline._extract_nvrs_from_fbc), then call
pipeline._extract_nvrs_from_fbc() directly and assert it returns the expected
mapping and that validate_fbc_related_images (patched as mock_validate) was not
called; ensure the test still uses the
fbc_pullspecs=["quay.io/test/fbc@sha256:single"] input and keeps
mock_validate.assert_not_called().
---
Nitpick comments:
In `@doozer/tests/cli/test_fbc.py`:
- Around line 151-161: The test currently mocks mock_builder.build to return
PipelineRun-like status results but never asserts that run() actually emits the
derived JSON "pullspecs" payload; update the happy-path test to capture
click.echo output from invoking run(), parse the echoed JSON, and assert the
"pullspecs" field equals the expected list (e.g., the image URL and image digest
derived from mock_builder.build's status results). Locate the test that calls
run() and uses mock_builder.build (the block returning "test-pipelinerun" and
the status results) and add an assertion comparing the parsed JSON payload's
"pullspecs" to the expected values to lock down the CLI contract.
🪄 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: 5e34b1e2-18b6-4321-a54d-c17d21653d6a
📒 Files selected for processing (13)
doozer/doozerlib/cli/fbc.pydoozer/doozerlib/runtime.pydoozer/doozerlib/util.pydoozer/tests/cli/test_fbc.pyelliott/elliottlib/cli/find_builds_cli.pypyartcd/pyartcd/__main__.pypyartcd/pyartcd/fbc_util.pypyartcd/pyartcd/pipelines/gen_assembly_lp.pypyartcd/pyartcd/pipelines/prepare_release_lp.pypyartcd/pyartcd/pipelines/release_from_fbc.pypyartcd/tests/pipelines/test_gen_assembly_lp.pypyartcd/tests/pipelines/test_prepare_release_lp.pypyartcd/tests/test_fbc_util.py
✅ Files skipped from review due to trivial changes (1)
- pyartcd/pyartcd/main.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (1)
doozer/tests/cli/test_fbc.py (1)
151-161: ⚡ Quick winAssert the JSON
pullspecspayload, not just the mocked inputs.These updated tests feed the new PipelineRun fields, but they never verify that
run()actually emits the derived pullspecs in its JSON output. Capturingclick.echoin one happy-path case would lock down the new CLI contract and catch regressions in pullspec reuse/parsing.🤖 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 `@doozer/tests/cli/test_fbc.py` around lines 151 - 161, The test currently mocks mock_builder.build to return PipelineRun-like status results but never asserts that run() actually emits the derived JSON "pullspecs" payload; update the happy-path test to capture click.echo output from invoking run(), parse the echoed JSON, and assert the "pullspecs" field equals the expected list (e.g., the image URL and image digest derived from mock_builder.build's status results). Locate the test that calls run() and uses mock_builder.build (the block returning "test-pipelinerun" and the status results) and add an assertion comparing the parsed JSON payload's "pullspecs" to the expected values to lock down the CLI contract.
🤖 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 `@doozer/doozerlib/cli/fbc.py`:
- Around line 665-666: The code currently returns an empty pullspec string for
reused or newly-created KonfluxFbcBuildRecord, which hides errors; update the
reuse branch (where existing_fbc_build is returned) and the fresh-build branch
(the code around where a new KonfluxFbcBuildRecord is produced) to check
existing_fbc_build.image_pullspec (and the equivalent new_record.image_pullspec)
and if it's falsy and self.force is not set, raise a clear exception (e.g.,
click.ClickException or RuntimeError) asking the user to re-run with --force or
handle the missing pullspec; if self.force is true allow returning the record as
before. Ensure you adjust both the return site that currently does
"existing_fbc_build.nvr, existing_fbc_build.image_pullspec or ''" and the
corresponding return for the fresh-build path so they do not silently return an
empty string.
- Around line 679-682: When building the digest pullspec replace
image_url.split(':')[0] with logic that strips only the tag part (the last
colon) so registry ports are preserved; update the pullspec construction that
uses image_url and image_digest (variables image_url, image_digest, pullspec) to
derive the name-without-tag via image_url.rsplit(':', 1)[0] (or equivalent) and
then combine that with @{image_digest} only when both values exist.
In `@doozer/doozerlib/runtime.py`:
- Line 96: Runtime.extra_vars is typed as dict[str, str] but get_replace_vars()
and the loop that processes CLI-style KEY=VALUE strings (see iteration near
lines 398-407) expect an iterable of "KEY=VALUE" strings; change the
Runtime.extra_vars declaration and initialization in class Runtime to a list of
strings (e.g., extra_vars: list[str] = []) so callers can pass CLI-style entries
and the existing parsing logic in get_replace_vars()/the loop continues to work
without raising "Invalid --var format" errors.
In `@pyartcd/pyartcd/fbc_util.py`:
- Around line 98-128: The current logic in the operator-extraction block of
fbc_util.py assigns a per-pullspec UNKNOWN-... value when it cannot determine an
operator (using fbc_pullspec, labels, nvr and tag), which allows same-operator
FBCs to be split; instead, change both the "else" branch and the except block to
raise an exception (e.g., ValueError or RuntimeError) with a clear message
including fbc_pullspec and nvr/tag rather than assigning operator =
f"UNKNOWN-..."; keep the existing warning/error logs but follow them by raising
so caller code fails the validation when operator identity cannot be
established.
In `@pyartcd/pyartcd/pipelines/gen_assembly_lp.py`:
- Around line 219-251: _resolve_parent_operands currently only reads
members_images from the single basis_assembly and misses inherited operands;
change it to recursively walk the basis/parent chain in releases_config (use
releases_config.get('releases') to lookup ancestors), merging each ancestor's
assembly.members.images into operand_map so that descendant overrides win (child
entries override parent entries), and guard against cycles/too-deep recursion;
preserve existing logging and return operand_map as before.
In `@pyartcd/pyartcd/pipelines/prepare_release_lp.py`:
- Around line 201-233: The _load_assembly/_extract_operand_nvrs flow currently
reads only the assembly block's members.images, which misses inherited entries
from a chain of basis.assembly (LP generated delta assemblies); update
_load_assembly to resolve the full effective assembly by following the
basis.assembly chain (reading parent assemblies from releases.yml until no
basis), merge each parent's members.images into the child (preserving override
semantics), then pass that expanded assembly to _extract_operand_nvrs so it
iterates the full effective members.images list; reference the functions
_load_assembly, _extract_operand_nvrs and the basis.assembly / members.images
keys when implementing the merge.
In `@pyartcd/tests/pipelines/test_gen_assembly_lp.py`:
- Around line 293-305: The test currently patches
GenAssemblyLPPipeline._extract_nvrs_from_fbc so the single-pullspec branch is
never exercised; remove that patch and instead patch the lower-level extractor
function that _extract_nvrs_from_fbc calls (i.e., the downstream extractor used
inside GenAssemblyLPPipeline._extract_nvrs_from_fbc), then call
pipeline._extract_nvrs_from_fbc() directly and assert it returns the expected
mapping and that validate_fbc_related_images (patched as mock_validate) was not
called; ensure the test still uses the
fbc_pullspecs=["quay.io/test/fbc@sha256:single"] input and keeps
mock_validate.assert_not_called().
---
Nitpick comments:
In `@doozer/tests/cli/test_fbc.py`:
- Around line 151-161: The test currently mocks mock_builder.build to return
PipelineRun-like status results but never asserts that run() actually emits the
derived JSON "pullspecs" payload; update the happy-path test to capture
click.echo output from invoking run(), parse the echoed JSON, and assert the
"pullspecs" field equals the expected list (e.g., the image URL and image digest
derived from mock_builder.build's status results). Locate the test that calls
run() and uses mock_builder.build (the block returning "test-pipelinerun" and
the status results) and add an assertion comparing the parsed JSON payload's
"pullspecs" to the expected values to lock down the CLI contract.
🪄 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: 5e34b1e2-18b6-4321-a54d-c17d21653d6a
📒 Files selected for processing (13)
doozer/doozerlib/cli/fbc.pydoozer/doozerlib/runtime.pydoozer/doozerlib/util.pydoozer/tests/cli/test_fbc.pyelliott/elliottlib/cli/find_builds_cli.pypyartcd/pyartcd/__main__.pypyartcd/pyartcd/fbc_util.pypyartcd/pyartcd/pipelines/gen_assembly_lp.pypyartcd/pyartcd/pipelines/prepare_release_lp.pypyartcd/pyartcd/pipelines/release_from_fbc.pypyartcd/tests/pipelines/test_gen_assembly_lp.pypyartcd/tests/pipelines/test_prepare_release_lp.pypyartcd/tests/test_fbc_util.py
✅ Files skipped from review due to trivial changes (1)
- pyartcd/pyartcd/main.py
🛑 Comments failed to post (7)
doozer/doozerlib/cli/fbc.py (2)
665-666:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't report success when the pullspec is unknown.
Both the reuse path and the fresh-build path currently collapse a missing pullspec to
"". Since this command now publishespullspecsas part of its success output, that turns an unusable result into a reported success and can break downstream LP validation in hard-to-diagnose ways. Please fail fast here, or require--forcewhen the reusedKonfluxFbcBuildRecordhas noimage_pullspec.Also applies to: 682-685
🤖 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 `@doozer/doozerlib/cli/fbc.py` around lines 665 - 666, The code currently returns an empty pullspec string for reused or newly-created KonfluxFbcBuildRecord, which hides errors; update the reuse branch (where existing_fbc_build is returned) and the fresh-build branch (the code around where a new KonfluxFbcBuildRecord is produced) to check existing_fbc_build.image_pullspec (and the equivalent new_record.image_pullspec) and if it's falsy and self.force is not set, raise a clear exception (e.g., click.ClickException or RuntimeError) asking the user to re-run with --force or handle the missing pullspec; if self.force is true allow returning the record as before. Ensure you adjust both the return site that currently does "existing_fbc_build.nvr, existing_fbc_build.image_pullspec or ''" and the corresponding return for the fresh-build path so they do not silently return an empty string.
679-682:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve registry ports when deriving the digest pullspec.
image_url.split(':')[0]truncates refs likeregistry:5000/ns/fbc:test-fbc-1.0.0-1to justregistry, so the emitted pullspec becomes invalid as soon as--image-repopoints at a registry with a port.Suggested fix
- image_url = next((r['value'] for r in results if r['name'] == 'IMAGE_URL'), None) - image_digest = next((r['value'] for r in results if r['name'] == 'IMAGE_DIGEST'), None) - pullspec = f"{image_url.split(':')[0]}@{image_digest}" if image_url and image_digest else "" + image_url = next((r['value'] for r in results if r['name'] == 'IMAGE_URL'), None) + image_digest = next((r['value'] for r in results if r['name'] == 'IMAGE_DIGEST'), None) + if image_url and image_digest: + repo, _, name_with_tag = image_url.rpartition("/") + name = name_with_tag.split(":", 1)[0] + repository = f"{repo}/{name}" if repo else name + pullspec = f"{repository}@{image_digest}" + else: + pullspec = ""🤖 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 `@doozer/doozerlib/cli/fbc.py` around lines 679 - 682, When building the digest pullspec replace image_url.split(':')[0] with logic that strips only the tag part (the last colon) so registry ports are preserved; update the pullspec construction that uses image_url and image_digest (variables image_url, image_digest, pullspec) to derive the name-without-tag via image_url.rsplit(':', 1)[0] (or equivalent) and then combine that with @{image_digest} only when both values exist.doozer/doozerlib/runtime.py (1)
96-96:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
extra_varswith the formatget_replace_vars()actually accepts.Line 96 declares
Runtime.extra_varsasdict[str, str], but Lines 398-407 iterate it as CLI-styleKEY=VALUEstrings. Any caller that follows the new annotation and passes{"FOO": "bar"}will now fail withInvalid --var format 'FOO'instead of overridingFOO.💡 Suggested fix
- self.extra_vars: dict[str, str] = {} + self.extra_vars: list[str] = []Also applies to: 398-407
🤖 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 `@doozer/doozerlib/runtime.py` at line 96, Runtime.extra_vars is typed as dict[str, str] but get_replace_vars() and the loop that processes CLI-style KEY=VALUE strings (see iteration near lines 398-407) expect an iterable of "KEY=VALUE" strings; change the Runtime.extra_vars declaration and initialization in class Runtime to a list of strings (e.g., extra_vars: list[str] = []) so callers can pass CLI-style entries and the existing parsing logic in get_replace_vars()/the loop continues to work without raising "Invalid --var format" errors.pyartcd/pyartcd/fbc_util.py (1)
98-128:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't fail open when operator identity can't be determined.
If label extraction fails for digest pullspecs, this path assigns a different
UNKNOWN-*value per pullspec, so same-operator FBCs land in separate groups and the related-image consistency check is skipped entirely. That turns a validation failure into a pass on exactly the safety gate this helper is supposed to enforce. Raise here once operator identity can't be established instead of generating unique placeholders.🧰 Tools
🪛 Ruff (0.15.15)
[warning] 126-126: 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/fbc_util.py` around lines 98 - 128, The current logic in the operator-extraction block of fbc_util.py assigns a per-pullspec UNKNOWN-... value when it cannot determine an operator (using fbc_pullspec, labels, nvr and tag), which allows same-operator FBCs to be split; instead, change both the "else" branch and the except block to raise an exception (e.g., ValueError or RuntimeError) with a clear message including fbc_pullspec and nvr/tag rather than assigning operator = f"UNKNOWN-..."; keep the existing warning/error logs but follow them by raising so caller code fails the validation when operator identity cannot be established.pyartcd/pyartcd/pipelines/gen_assembly_lp.py (1)
219-251:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftResolve the full basis chain before diffing against the parent assembly.
This only reads
members.imagesfromself.basis_assembly, but lightweight LP assemblies store delta-only overrides there. If the parent already inherits from another assembly,parent_operand_mapis incomplete:--includecan't swap inherited operands, and fresh FBC extraction will treat most inherited pins as new deltas._resolve_parent_operands()needs to recursively mergebasis.assemblyancestors into the full effective operand set first.🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 232-232: yaml.load() without SafeLoader can execute arbitrary Python code. Use yaml.safe_load() or yaml.load(..., Loader=SafeLoader) instead.
(coderabbit.deserialization.python-yaml-unsafe-load)
🤖 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/gen_assembly_lp.py` around lines 219 - 251, _resolve_parent_operands currently only reads members_images from the single basis_assembly and misses inherited operands; change it to recursively walk the basis/parent chain in releases_config (use releases_config.get('releases') to lookup ancestors), merging each ancestor's assembly.members.images into operand_map so that descendant overrides win (child entries override parent entries), and guard against cycles/too-deep recursion; preserve existing logging and return operand_map as before.pyartcd/pyartcd/pipelines/prepare_release_lp.py (1)
201-233:
⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftExpand inherited assemblies before extracting operand NVRs.
This reads only the current assembly block, but LP inherited assemblies generated by
gen-assembly-lpstore just the delta inmembers.images. On those assemblies,prepare-release-lpwill rebuild bundles/FBCs and create shipments for the override set only, silently dropping every inherited operand from the release. Resolve thebasis.assemblychain into a full effective member list before_extract_operand_nvrs()runs.🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 214-214: yaml.load() without SafeLoader can execute arbitrary Python code. Use yaml.safe_load() or yaml.load(..., Loader=SafeLoader) instead.
(coderabbit.deserialization.python-yaml-unsafe-load)
🤖 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/prepare_release_lp.py` around lines 201 - 233, The _load_assembly/_extract_operand_nvrs flow currently reads only the assembly block's members.images, which misses inherited entries from a chain of basis.assembly (LP generated delta assemblies); update _load_assembly to resolve the full effective assembly by following the basis.assembly chain (reading parent assemblies from releases.yml until no basis), merge each parent's members.images into the child (preserving override semantics), then pass that expanded assembly to _extract_operand_nvrs so it iterates the full effective members.images list; reference the functions _load_assembly, _extract_operand_nvrs and the basis.assembly / members.images keys when implementing the merge.pyartcd/tests/pipelines/test_gen_assembly_lp.py (1)
293-305:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis test never exercises the single-pullspec code path.
_extract_nvrs_from_fbc()is patched out here, so the branch under test never runs; the only assertion is that the method object is callable. A regression that starts callingvalidate_fbc_related_images()for a single FBC would still pass. Patch the downstream extractor instead and actually invokepipeline._extract_nvrs_from_fbc().🤖 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/tests/pipelines/test_gen_assembly_lp.py` around lines 293 - 305, The test currently patches GenAssemblyLPPipeline._extract_nvrs_from_fbc so the single-pullspec branch is never exercised; remove that patch and instead patch the lower-level extractor function that _extract_nvrs_from_fbc calls (i.e., the downstream extractor used inside GenAssemblyLPPipeline._extract_nvrs_from_fbc), then call pipeline._extract_nvrs_from_fbc() directly and assert it returns the expected mapping and that validate_fbc_related_images (patched as mock_validate) was not called; ensure the test still uses the fbc_pullspecs=["quay.io/test/fbc@sha256:single"] input and keeps mock_validate.assert_not_called().
ashwindasr
marked this pull request as draft
June 5, 2026 20:15
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
ashwindasr
added
the
tide/merge-method-squash
ashwindasr
force-pushed
the
cursor-lp-asm-abcd
branch
from
June 24, 2026 18:28
4197f22 to
76de4c3
Compare
Contributor
|
[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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/prepare_release_lp.py`:
- Line 660: The path normalization in prepare_release_lp.py is incorrectly using
rstrip(".git"), which strips any trailing combination of those characters
instead of only the .git suffix. Update the data_path handling in the release
preparation logic to use a suffix-only removal approach (such as removesuffix)
so repo URLs ending in characters like g, i, t, or . are not truncated
accidentally. Keep the fix localized to the data_path assignment near the
existing repository URL cleanup.
- Around line 556-559: The snapshot parsing logic in prepare_release_lp
currently assigns sorted(builds) to every Snapshot, which mixes NVRs across
Konflux applications. Update the loop over yaml.load_all(stdout) so each
Snapshot gets NVRs derived from that document’s own components or per-app build
set instead of the full builds input, using the existing Snapshot and
SnapshotSpec construction as the anchor for the fix.
🪄 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: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5c61eea3-5074-476b-8ab0-58ea0ae74926
📒 Files selected for processing (13)
doozer/doozerlib/cli/fbc.pydoozer/doozerlib/runtime.pydoozer/doozerlib/util.pydoozer/tests/cli/test_fbc.pyelliott/elliottlib/cli/find_builds_cli.pypyartcd/pyartcd/__main__.pypyartcd/pyartcd/fbc_util.pypyartcd/pyartcd/pipelines/gen_assembly_lp.pypyartcd/pyartcd/pipelines/prepare_release_lp.pypyartcd/pyartcd/pipelines/release_from_fbc.pypyartcd/tests/pipelines/test_gen_assembly_lp.pypyartcd/tests/pipelines/test_prepare_release_lp.pypyartcd/tests/test_fbc_util.py
🚧 Files skipped from review as they are similar to previous changes (7)
- pyartcd/tests/test_fbc_util.py
- pyartcd/pyartcd/main.py
- doozer/doozerlib/runtime.py
- doozer/tests/cli/test_fbc.py
- doozer/doozerlib/util.py
- pyartcd/tests/pipelines/test_gen_assembly_lp.py
- doozer/doozerlib/cli/fbc.py
ashwindasr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ashwindasr: This pull request references ART-19443 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
ashwindasr
marked this pull request as ready for review
June 24, 2026 19:48
openshift-ci
Bot
added
needs-rebase
…ucts Implements two new pipelines for ACM/MCE assembly-based releases: - gen-assembly-lp: Extracts operand NVRs from FBC images, supports --include for swapping specific builds, and --basis-assembly for lightweight inherited assemblies. Creates a PR to ocp-build-data. - prepare-release-lp: Reads a named assembly from releases.yml, checks for existing bundles before rebuilding, triggers FBC builds, and creates shipment MRs. Uses named assembly (not stream) for deterministic builds. Includes unit tests (24 passing) and CLI registration. Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
- Fix pullspec parsing to preserve registry ports (rsplit vs split) - Raise RuntimeError on unknown operator identity in FBC validation instead of silently skipping the consistency check - Fix test_extract_nvrs_skips_validation_for_single_pullspec to actually exercise the single-pullspec code path - Document single-level inheritance limitation in docstrings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
rstrip(".git") strips any trailing combination of characters in the set
{'.', 'g', 'i', 't'}, which can mangle repo URLs whose names end in
those characters. removesuffix(".git") correctly removes only the exact
suffix.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
When an operand image is swapped in an LP assembly, the existing bundle still references the old image. The pipeline was reusing these stale bundles because it only checked operator NVR existence, not operand content. Now find-builds returns each bundle's operand_nvrs, and the pipeline validates them against the assembly's pins before reusing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
ashwindasr
force-pushed
the
cursor-lp-asm-abcd
branch
from
July 14, 2026 19:55
719aecd to
2ecd07d
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
|
@ashwindasr: 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. |
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
gen-assembly-lppipeline: extracts operand NVRs from FBC images, supports--includefor swapping specific operand builds,--basis-assemblyfor lightweight inherited assemblies, and creates a PR to ocp-build-data with the assembly definition inreleases.ymlprepare-release-lppipeline: reads a named assembly, checks for existing bundles before rebuilding (only builds missing ones), triggers FBC builds, and creates shipment MRs with release notespyartcd/pyartcd/fbc_util.py(fromrelease_from_fbc.py)artcdCLI commands (artcd gen-assembly-lp,artcd prepare-release-lp)doozer beta:fbc:rebase-and-buildto return pullspecs alongside NVRs (needed for FBC consistency validation)olm_operator_nvrstoelliott find-builds --all-image-typesJSON output (needed to scope bundle builds to assembly-pinned operators)get_release_name(LP group names don't matchopenshift-X.Y)Context
ACM and MCE need the same two-phase assembly workflow that OCP has (
gen-assembly->prepare-release), adapted for FBC-driven Layered Product releases. This separates "what goes into the release" from "ship the release", enabling operand pinning, build swapping, and reproducible releases.Test Runs
Changes to existing files
All changes to existing files are scoped to LP groups only and do not affect OCP pipelines:
doozer/doozerlib/cli/fbc.py:_rebase_and_build_fbcreturns(nvr, pullspec)tuple; JSON output gainspullspecskey. Existing consumers use.get("nvrs")defensively — no breakage.doozer/doozerlib/runtime.py:strict_mode = Falsefor non-openshift-X.Ygroups. For OCP groups,is_openshift_group = Trueso the new condition isor False— zero behavioral change.doozer/doozerlib/util.py: STANDARD assembly check moved beforeisolate_major_minor_in_group. All existing callers passopenshift-X.Ygroups where major/minor are always valid — no change in behavior.elliott/elliottlib/cli/find_builds_cli.py: Addsolm_operator_nvrskey to return dict. Adding a key to a dict is backward compatible.pyartcd/pyartcd/pipelines/release_from_fbc.py: Delegatesextract_fbc_labelsandvalidate_fbc_related_imagesto sharedfbc_util.py. API surface preserved.Test plan
pytest pyartcd/tests/pipelines/test_gen_assembly_lp.py— 25 tests passpytest pyartcd/tests/pipelines/test_prepare_release_lp.py— 26 tests passpytest pyartcd/tests/test_fbc_util.py— 8 tests passpytest doozer/tests/cli/test_fbc.py— 13 tests pass (updated mocks for new return type)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes