ART-21775: add doozer beta:release-payload:rebase-and-build command - #3218
ART-21775: add doozer beta:release-payload:rebase-and-build command#3218ashwindasr wants to merge 4 commits into
Conversation
Adds a new doozer CLI command that generates release payload manifests via `oc adm release new --to-dir`, writes a Dockerfile layering those manifests onto the cluster-version-operator image, pushes the result to openshift-priv/ocp-release-payloads, and triggers a Konflux build of the release payload image. Reuses BuildRepo for git operations and KonfluxClient for Application/Component/PipelineRun management. Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
Skipping CI for Draft Pull Request. |
|
@ashwindasr: This pull request references ART-21775 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesThe PR adds a CLI command that generates OpenShift release payload manifests, commits them to a source repository, and optionally starts and monitors a multi-architecture Konflux build. It also adds constants, command registration, configuration options, error handling, and tests. Release payload workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ReleasePayloadRebaseAndBuildCli
participant OpenShiftCLI
participant GitRepository
participant Konflux
User->>ReleasePayloadRebaseAndBuildCli: invoke release_payload_rebase_and_build
ReleasePayloadRebaseAndBuildCli->>OpenShiftCLI: generate and validate release manifests
OpenShiftCLI-->>ReleasePayloadRebaseAndBuildCli: return image-references and CVO pullspec
ReleasePayloadRebaseAndBuildCli->>GitRepository: write and commit payload sources
GitRepository-->>ReleasePayloadRebaseAndBuildCli: return committed repository
ReleasePayloadRebaseAndBuildCli->>Konflux: start multi-architecture PipelineRun
Konflux-->>ReleasePayloadRebaseAndBuildCli: return build outcome
ReleasePayloadRebaseAndBuildCli-->>User: emit JSON or human-readable result
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
doozer/doozerlib/cli/release_payload.py (3)
207-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a domain exception over
IOErrorfor this precondition.The missing commit is a programming or workflow error, not an I/O failure.
DoozerFatalErroris already imported and is used for the other fatal conditions in this file. A change also requires an update totest_build_raises_without_commitindoozer/tests/cli/test_release_payload.py.♻️ Proposed change
if not build_repo.commit_hash: - raise IOError("Release payload repository must have a commit to build. Did you rebase?") + raise DoozerFatalError("Release payload repository must have a commit to build. Did you rebase?")🤖 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/release_payload.py` around lines 207 - 208, Replace the IOError raised by the missing-commit precondition in the release payload build flow with the already imported DoozerFatalError, preserving the existing message. Update test_build_raises_without_commit to expect DoozerFatalError instead of IOError.
273-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
assertwith an explicit check.Python removes
assertstatements when the interpreter runs with-O. Ifgroup_configisNone, the failure then moves to a laterAttributeError. Raise an explicit error instead.♻️ Proposed change
- assert runtime.group_config is not None, "group_config is not loaded; Doozer bug?" + if runtime.group_config is None: + raise DoozerFatalError("group_config is not loaded; Doozer bug?")🤖 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/release_payload.py` at line 273, Replace the assert guarding runtime.group_config with an explicit None check that raises an appropriate error using the existing diagnostic message, ensuring the failure occurs even when Python runs with optimizations.
485-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the exception before you exit for JSON output.
The
--output jsonbranch replaces the exception withstr(e)and exits. The traceback is then lost. Log the exception first so failures stay diagnosable in CI.Note: the static analysis hint that recommends
jsonifytargets Flask responses. It does not apply to this Click command.♻️ Proposed change
except Exception as e: if output == 'json': + LOGGER.exception("Release payload rebase and build failed") click.echo(json.dumps({"error": str(e)}, indent=2)) sys.exit(1) raise🤖 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/release_payload.py` around lines 485 - 491, Update the exception handler around cli_obj.run so the JSON-output branch logs the caught exception, including traceback details, before emitting the JSON error and exiting. Preserve the existing non-JSON behavior that re-raises the exception, and do not replace Click output handling with Flask-specific jsonify.Source: Linters/SAST tools
doozer/tests/cli/test_release_payload.py (3)
144-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the negative assertion.
assertNotIn("--from-image-stream=4.21-konflux-art-latest", cmd)passes even if the command contains a different--from-image-streamvalue. Assert that no--from-image-streamargument exists at all.♻️ Proposed change
cmd = mock_cmd_assert_async.call_args.args[0] self.assertIn("--from-release=registry.example.com/ocp/release:4.21.0", cmd) - self.assertNotIn("--from-image-stream=4.21-konflux-art-latest", cmd) + self.assertFalse([arg for arg in cmd if str(arg).startswith("--from-image-stream")]) + self.assertNotIn("--reference-mode=source", cmd)🤖 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_release_payload.py` around lines 144 - 146, Strengthen the assertions around cmd in the release payload test by verifying that no argument with the --from-image-stream option exists, regardless of its value; retain the existing assertion for the expected --from-release argument.
340-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the hardcoded
/tmppath in the mock attribute.Ruff reports S108 on this line. The value is only used in a log message, so a neutral placeholder removes the finding without changing test behavior.
♻️ Proposed change
- self.build_repo.local_dir = "/tmp/release-payload" + self.build_repo.local_dir = Path(tempfile.gettempdir(), "release-payload")🤖 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_release_payload.py` at line 340, Replace the hardcoded /tmp/release-payload value assigned to self.build_repo.local_dir with a neutral non-filesystem placeholder, preserving its use in the log message and the test’s behavior.Source: Linters/SAST tools
343-442: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a test for the Click command wrapper.
The tests cover
ReleasePayloadRebaseAndBuildCliwell. They do not coverrelease_payload_rebase_and_build. ACliRunnertest would confirm theKONFLUX_SA_KUBECONFIGfallback, the--output jsonpayload, and the exit code 1 on failure.🤖 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_release_payload.py` around lines 343 - 442, Add a CliRunner-based test for the release_payload_rebase_and_build command wrapper, covering KONFLUX_SA_KUBECONFIG fallback behavior, JSON output via --output json, and exit code 1 when the command fails. Reuse the existing ReleasePayloadRebaseAndBuildCli test fixtures and mock the underlying execution so the test verifies wrapper behavior without performing real rebases or builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@doozer/doozerlib/cli/release_payload.py`:
- Around line 207-208: Replace the IOError raised by the missing-commit
precondition in the release payload build flow with the already imported
DoozerFatalError, preserving the existing message. Update
test_build_raises_without_commit to expect DoozerFatalError instead of IOError.
- Line 273: Replace the assert guarding runtime.group_config with an explicit
None check that raises an appropriate error using the existing diagnostic
message, ensuring the failure occurs even when Python runs with optimizations.
- Around line 485-491: Update the exception handler around cli_obj.run so the
JSON-output branch logs the caught exception, including traceback details,
before emitting the JSON error and exiting. Preserve the existing non-JSON
behavior that re-raises the exception, and do not replace Click output handling
with Flask-specific jsonify.
In `@doozer/tests/cli/test_release_payload.py`:
- Around line 144-146: Strengthen the assertions around cmd in the release
payload test by verifying that no argument with the --from-image-stream option
exists, regardless of its value; retain the existing assertion for the expected
--from-release argument.
- Line 340: Replace the hardcoded /tmp/release-payload value assigned to
self.build_repo.local_dir with a neutral non-filesystem placeholder, preserving
its use in the log message and the test’s behavior.
- Around line 343-442: Add a CliRunner-based test for the
release_payload_rebase_and_build command wrapper, covering KONFLUX_SA_KUBECONFIG
fallback behavior, JSON output via --output json, and exit code 1 when the
command fails. Reuse the existing ReleasePayloadRebaseAndBuildCli test fixtures
and mock the underlying execution so the test verifies wrapper behavior without
performing real rebases or builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9281ac1-9e04-4ccc-bf9d-1c90268e3e1c
📒 Files selected for processing (4)
doozer/doozerlib/cli/__main__.pydoozer/doozerlib/cli/release_payload.pydoozer/doozerlib/constants.pydoozer/tests/cli/test_release_payload.py
Use a single shared Application (release-payloads) for all release payload builds, and scope the per-group Component name (release-payload-<group>) underneath it, rather than having a separate Application per group. Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doozer/doozerlib/cli/release_payload.py`:
- Around line 100-107: Update get_component_name so distinct group names
containing "." and "_" cannot produce the same Component name; either validate
and reject ambiguous group names before formatting or encode these separators
bijectively while preserving the existing release-payload prefix and valid-name
requirements.
🪄 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: 21648ec5-0e61-4b6d-ad51-125c66146342
📒 Files selected for processing (2)
doozer/doozerlib/cli/release_payload.pydoozer/tests/cli/test_release_payload.py
🚧 Files skipped from review as they are similar to previous changes (1)
- doozer/tests/cli/test_release_payload.py
| def get_component_name(group: str) -> str: | ||
| """Konflux Component name for a group's release payload builds. | ||
|
|
||
| There is a single Component per group underneath the shared `release-payloads` | ||
| Application (Konflux builds all architectures as one multi-arch manifest list from | ||
| a single PipelineRun per group/assembly), e.g. `release-payload-openshift-4-21`. | ||
| """ | ||
| return f"release-payload-{group}".replace(".", "-").replace("_", "-") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'\b(get_component_name|construct_dest_branch|ensure_component|start_pipeline_run_for_image_build|revision|commit_sha|git_url)\b' \
--glob '*.py' .Repository: openshift-eng/art-tools
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- release_payload.py outline ---'
ast-grep outline doozer/doozerlib/cli/release_payload.py
printf '%s\n' '--- relevant implementation ---'
sed -n '1,260p' doozer/doozerlib/cli/release_payload.py
printf '%s\n' '--- exact call sites and definitions ---'
rg -n -C 8 \
'def get_component_name|get_component_name\(|def construct_dest_branch|construct_dest_branch\(|def ensure_component|ensure_component\(|def start_pipeline_run_for_image_build|start_pipeline_run_for_image_build\(' \
doozer/doozerlib doozer/testsRepository: openshift-eng/art-tools
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- branch construction ---'
rg -n -C 10 'def construct_dest_branch|construct_dest_branch\(' doozer/doozerlib/backend/rebaser.py doozer/doozerlib doozer/tests \
| head -n 220
printf '%s\n' '--- Konflux component and PipelineRun implementations ---'
rg -n -C 14 'async def ensure_component|def ensure_component|async def start_pipeline_run_for_image_build|def start_pipeline_run_for_image_build' \
doozer/doozerlib/backend/konflux_client.py artcommonlib 2>/dev/null | head -n 360
printf '%s\n' '--- release payload tests and group/assembly inputs ---'
sed -n '1,180p' doozer/tests/cli/test_release_payload.py
rg -n --glob '*.py' --glob '*.yml' --glob '*.yaml' \
'group\s*=|group:|assembly\s*=|assembly:' doozer/doozerlib/cli/release_payload.py doozer/tests/cli/test_release_payload.py config ocp-build-data 2>/dev/null \
| head -n 300Repository: openshift-eng/art-tools
Length of output: 16508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- construct_dest_branch body ---'
sed -n '108,145p' doozer/doozerlib/backend/rebaser.py
printf '%s\n' '--- component resource and replacement behavior ---'
sed -n '720,875p' doozer/doozerlib/backend/konflux_client.py
sed -n '1460,1565p' doozer/doozerlib/backend/konflux_client.py
printf '%s\n' '--- group validation and release-payload invocation ---'
rg -n -C 8 \
'group_name|runtime\.group|--group|group.*pattern|group.*replace|assembly.*group' \
doozer/doozerlib/cli/release_payload.py doozer/doozerlib/runtime.py doozer/doozerlib/cli doozer/tests/cli/test_release_payload.py \
| head -n 420
printf '%s\n' '--- normalization probe ---'
python3 - <<'PY'
def component(group):
return f"release-payload-{group}".replace(".", "-").replace("_", "-")
for left, right in [("foo.bar", "foo_bar"), ("openshift-4.21", "openshift_4-21")]:
print(left, "=>", component(left))
print(right, "=>", component(right))
print("collision:", component(left) == component(right))
PYRepository: openshift-eng/art-tools
Length of output: 49899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact resource mutation helpers ---'
rg -n -C 16 'async def _create_or_replace|async def _replace|def _new_pipelinerun_for_image_build' \
doozer/doozerlib/backend/konflux_client.py
printf '%s\n' '--- PipelineRun source fields ---'
sed -n '1280,1475p' doozer/doozerlib/backend/konflux_client.py
printf '%s\n' '--- group option and runtime assignment ---'
rg -n -C 12 \
'click\.option\(.*group|group: Optional|self\.group\s*=|group_config.*name|group_config.*load|--group' \
doozer/doozerlib/cli/__init__.py doozer/doozerlib/runtime.py doozer/doozerlib \
| head -n 360Repository: openshift-eng/art-tools
Length of output: 41549
Use a collision-free Component name when group names allow . and _.
foo.bar and foo_bar map to the same Component. start_pipeline_run_for_image_build does pass each build's explicit git_url and commit_sha, so assembly-specific source commits remain isolated. Add group-name validation or encode separators without collisions.
🤖 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/release_payload.py` around lines 100 - 107, Update
get_component_name so distinct group names containing "." and "_" cannot produce
the same Component name; either validate and reject ambiguous group names before
formatting or encode these separators bijectively while preserving the existing
release-payload prefix and valid-name requirements.
The Component (and its git branch) is now shared by every assembly of a group, so include the assembly in the generateName prefix; otherwise builds for different assemblies are indistinguishable by name in the Konflux UI PipelineRun list. Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
Drop the group/component prefix from the PipelineRun generateName and use release-payload-<assembly> instead; the group is redundant since assembly names already encode it (e.g. group openshift-4.21, assembly 4.21.1). Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
2 participants
Summary
Implements the doozer rebase+build flow described in ART-21775 (part of the ART-14237 epic to build named release payloads in Konflux instead of running
oc adm release new --to-imagedirectly on buildvm).Adds a new
doozer beta:release-payload:rebase-and-buildcommand that:oc adm release new --to-dirto snapshot the release manifests already populated in the group's build-sync imagestream (imagestream name/namespace derived automatically from--group/--assembly, or optionally sourced from an existing release pullspec via--from-release)cluster-version-operatorpullspec from the generatedimage-referencesmanifestFROM <cvo-pullspec>+COPY release-manifests/ /release-manifests/)openshift-priv/ocp-release-payloadson a per-group/assembly branch, reusingBuildRepoPipelineRunviaKonfluxClient, reusing existing Konflux build infrastructure--push(git push + Konflux build vs. local-only rebase),--dry-run(skip actual git pushes / Konflux API calls), and--output jsonfor machine-parseable resultsKonflux builds a single multi-arch manifest list per PipelineRun, so this command determines all supported architectures from the group config (
runtime.get_global_konflux_arches()) and passes them asbuilding_arches. The--archflag only selects which brew-arch imagestream to source manifests from (e.g.ocpvsocp-s390x).Test plan
doozer/tests/cli/test_release_payload.pycovering naming helpers, imagestream resolution, manifest generation (success + error cases), rebase (Dockerfile/commit), build (Konflux API + outcome handling), and the top-levelrun()flow (--push/--dry-runinteractions, error propagation)uv run pytest doozer/tests/cli/test_release_payload.py-- 21 passeduv run pytest doozer/tests/-- full doozer suite passes (1451 passed, 15 skipped)uv run ruff check/uv run ruff format --checkpass on all new/modified filesSummary by CodeRabbit
New Features
Tests