ART-20930: add golang-builder-shipment pipeline - #3231
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@lgarciaaco: This pull request references ART-20930 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 task 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. |
|
[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 |
WalkthroughAdds a ChangesGolang builder shipment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant KonfluxBuildRecords
participant GolangBuilderShipmentHandler
participant Elliott
participant ShipmentDataRepository
participant GitLab
CLI->>KonfluxBuildRecords: resolve RPM NVRs to image NVRs
CLI->>GolangBuilderShipmentHandler: create shipment from NVRs
GolangBuilderShipmentHandler->>Elliott: create snapshot
Elliott-->>GolangBuilderShipmentHandler: return snapshot YAML
GolangBuilderShipmentHandler->>ShipmentDataRepository: write and push shipment YAML
GolangBuilderShipmentHandler->>GitLab: create draft merge request
GitLab-->>CLI: return merge request URL
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, 1 inconclusive)
✅ Passed checks (7 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.
Actionable comments posted: 8
🧹 Nitpick comments (4)
pyartcd/tests/pipelines/test_golang_builder_shipment.py (3)
613-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestShipmentFilePathdoes not test production code.The test builds
expected_prefixandactualfrom the same local literals and compares them. It always passes and it never callscreate_shipment_mror any function ingolang_builder_shipment.py. It gives no protection against a change in the shipment path convention.Assert the real path instead. Call
create_shipment_mrwith a mocked repository and check thefilepathpassed towrite_file, including thestream.image.<timestamp>.yamlfilename.🤖 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_golang_builder_shipment.py` around lines 613 - 623, Replace the self-comparison in TestShipmentFilePath.test_path_format with an invocation of create_shipment_mr using a mocked repository and the relevant shipment inputs. Capture the filepath supplied to write_file and assert it matches the expected shipment directory plus stream.image.<timestamp>.yaml filename, using mocks for nondeterministic values such as the timestamp.
381-394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
resolve_konflux_image_nvrs.The suite covers
derive_golang_group,resolve_lifecycle_env, and_create_snapshot, but it never exercisesresolve_konflux_image_nvrs. That function has two uncovered failure paths:isolate_el_version_in_releasereturnsNone, andKonfluxDb.search_builds_by_fieldsyields no record. Both raise and both are reachable from the CLI--golang-nvrsflow. PatchKonfluxDband add cases for the success path and for both errors.🤖 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_golang_builder_shipment.py` around lines 381 - 394, Extend TestCreateSnapshotErrors with tests for resolve_konflux_image_nvrs, patching KonfluxDb and covering successful NVR resolution plus failures when isolate_el_version_in_release returns None and when search_builds_by_fields yields no record. Verify both error paths raise as expected and preserve the CLI --golang-nvrs behavior.
64-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the constructed raw URL.
These tests confirm the parsed phase but never assert the URL passed to
session.get. The URL construction at Lines 140-147 ofgolang_builder_shipment.pycontains the branch forgithub.comand the non-GitHub fallback, and neither branch is covered. Record the call arguments in_FakeSession.getand assert the resulting URL for agithub.combase and for a GitLab-style base.🤖 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_golang_builder_shipment.py` around lines 64 - 93, Extend TestResolveLifecycleEnv to capture session.get arguments through _FakeSession.get, then add assertions covering both URL-construction branches in resolve_lifecycle_env: verify the expected raw URL for a github.com base and for a GitLab-style base. Keep the existing lifecycle phase and error behavior tests unchanged.pyartcd/pyartcd/pipelines/golang_builder_shipment.py (1)
79-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAnchor the NVR pattern and validate the image name.
The pattern at Line 81 is unanchored and starts with a free
v(\d+). Any NVR that contains a substring such asv1.2.3.el9matches, including images that are not golang builders. The function then returns a golang group derived from unrelated input. Anchor the pattern and require the golang builder name prefix.The path instructions ask for anchored regexes.
♻️ Proposed change
- m = re.search(r"v(\d+)\.(\d+)\.\d+.*\.el(\d+)", nvr) + m = re.match(rf"^{re.escape(GOLANG_BUILDER_CVE_COMPONENT)}-v(\d+)\.(\d+)\.\d+.*\.el(\d+)$", nvr)🤖 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/golang_builder_shipment.py` around lines 79 - 93, Update the Konflux NVR regex in the loop within the golang group derivation function to anchor the full string and require the golang builder image-name prefix before matching its version and EL suffix. Preserve the existing group construction, while allowing the documented trailing NVR content only after the validated prefix/version structure.Source: Path instructions
🤖 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 `@elliott/tests/test_process_release_from_fbc_bugs_cli.py`:
- Around line 189-192: Update the test for the missing-pscomponent path to
assert that “OADP-8888” remains in result.issues.fixed, while preserving the
existing type, CVE, and mock-call assertions.
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py`:
- Around line 428-436: The _get_project flow currently uses self.gitlab_url
regardless of the repository URL host. Ensure the GitLab client is created with
the host derived from the shipment repository URL, or validate that both
shipment URLs match self.gitlab_url before client creation, and preserve
project-path lookup for the validated host.
- Around line 140-147: Update the base_url normalization near the raw_url
construction to remove both trailing slashes and a trailing “.git” suffix from
data_path or constants.OCP_BUILD_DATA_URL before building the GitHub or GHE URL.
Preserve the existing provider-specific raw_url logic and branch/group.yml path.
- Around line 499-507: Update the input validation around resolved_nvrs and
golang_nvrs to raise click.UsageError when both positional Konflux image NVRs
and --golang-nvrs are provided. Perform this conflict check before the existing
resolution branch, while preserving normal resolution when only golang_nvrs is
supplied and the existing missing-input error otherwise.
- Around line 108-122: Update the extra_patterns construction in the Go builder
shipment search to escape go_version before embedding it in the NVR pattern,
then anchor the resulting regular expression to the complete version boundary so
v1.25.9 cannot match v1.25.91. Preserve the existing search fields, ordering,
and limit behavior.
- Around line 234-238: Update basic_auth_url to reject any URL whose parsed
scheme is not HTTPS before embedding the token, while preserving the configured
host and explicit port when reconstructing the authenticated URL. Keep the
existing path handling and token placement unchanged.
- Around line 67-70: Update resolve_release_plan to select a ReleasePlan
matching the requested RHEL major version, adding EL-specific entries to
GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP as needed; alternatively, validate the
input and reject RHEL 8 before shipment creation. Ensure derive_golang_group and
resolve_konflux_image_nvrs cannot lead to an RHEL 8 shipment using an RHEL 9
plan.
- Around line 354-364: Update the `cmd_gather_async` call in the shipment flow
to capture stderr by removing `stderr=None` or passing
`asyncio.subprocess.PIPE`, ensuring the existing `RuntimeError` includes elliott
diagnostics. In the `finally` cleanup for `builds_file`, make `os.unlink`
tolerate an already-missing file by using `missing_ok=True`, preserving any
original exception.
---
Nitpick comments:
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py`:
- Around line 79-93: Update the Konflux NVR regex in the loop within the golang
group derivation function to anchor the full string and require the golang
builder image-name prefix before matching its version and EL suffix. Preserve
the existing group construction, while allowing the documented trailing NVR
content only after the validated prefix/version structure.
In `@pyartcd/tests/pipelines/test_golang_builder_shipment.py`:
- Around line 613-623: Replace the self-comparison in
TestShipmentFilePath.test_path_format with an invocation of create_shipment_mr
using a mocked repository and the relevant shipment inputs. Capture the filepath
supplied to write_file and assert it matches the expected shipment directory
plus stream.image.<timestamp>.yaml filename, using mocks for nondeterministic
values such as the timestamp.
- Around line 381-394: Extend TestCreateSnapshotErrors with tests for
resolve_konflux_image_nvrs, patching KonfluxDb and covering successful NVR
resolution plus failures when isolate_el_version_in_release returns None and
when search_builds_by_fields yields no record. Verify both error paths raise as
expected and preserve the CLI --golang-nvrs behavior.
- Around line 64-93: Extend TestResolveLifecycleEnv to capture session.get
arguments through _FakeSession.get, then add assertions covering both
URL-construction branches in resolve_lifecycle_env: verify the expected raw URL
for a github.com base and for a GitLab-style base. Keep the existing lifecycle
phase and error behavior tests unchanged.
🪄 Autofix
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: d179fa10-5d2b-47e9-a4f0-93c73d268c46
📒 Files selected for processing (6)
elliott/elliottlib/cli/process_release_from_fbc_bugs_cli.pyelliott/tests/test_process_release_from_fbc_bugs_cli.pypyartcd/pyartcd/__main__.pypyartcd/pyartcd/pipelines/__init__.pypyartcd/pyartcd/pipelines/golang_builder_shipment.pypyartcd/tests/pipelines/test_golang_builder_shipment.py
| self.assertEqual(result.type, "RHBA") | ||
| self.assertIsNone(result.cves) | ||
| mock_get_delivery.assert_not_called() | ||
| mock_get_konflux.assert_not_called() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert JIRA retention for the missing-pscomponent path.
This test does not verify that OADP-8888 remains in result.issues.fixed. A regression that drops the JIRA before this skip path would still pass.
Proposed test update
self.assertEqual(result.type, "RHBA")
self.assertIsNone(result.cves)
+ fixed_ids = [issue.id for issue in result.issues.fixed]
+ self.assertIn("OADP-8888", fixed_ids)
mock_get_delivery.assert_not_called()
mock_get_konflux.assert_not_called()📝 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.
| self.assertEqual(result.type, "RHBA") | |
| self.assertIsNone(result.cves) | |
| mock_get_delivery.assert_not_called() | |
| mock_get_konflux.assert_not_called() | |
| self.assertEqual(result.type, "RHBA") | |
| self.assertIsNone(result.cves) | |
| fixed_ids = [issue.id for issue in result.issues.fixed] | |
| self.assertIn("OADP-8888", fixed_ids) | |
| mock_get_delivery.assert_not_called() | |
| mock_get_konflux.assert_not_called() |
🤖 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 `@elliott/tests/test_process_release_from_fbc_bugs_cli.py` around lines 189 -
192, Update the test for the missing-pscomponent path to assert that “OADP-8888”
remains in result.issues.fixed, while preserving the existing type, CVE, and
mock-call assertions.
| extra_patterns = {"nvr": f"{GOLANG_BUILDER_CVE_COMPONENT}-v{go_version}"} | ||
| record = await anext( | ||
| db.search_builds_by_fields( | ||
| where={ | ||
| "name": GOLANG_BUILDER_IMAGE_NAME, | ||
| "el_target": f"el{el_v}", | ||
| "artifact_type": str(ArtifactType.IMAGE), | ||
| "outcome": str(KonfluxBuildOutcome.SUCCESS), | ||
| "engine": str(Engine.KONFLUX), | ||
| }, | ||
| extra_patterns=extra_patterns, | ||
| limit=1, | ||
| ), | ||
| None, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect search_builds_by_fields signature, default ordering, and extra_patterns semantics.
fd -t f 'konflux_db.py' | xargs -r -I{} ast-grep outline {} --items all
fd -t f 'konflux_db.py' | xargs -r rg -n -C 12 'def search_builds_by_fields|extra_patterns|order_by|ORDER BY|limit'Repository: openshift-eng/art-tools
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
file=$(fd -t f 'konflux_db.py' | head -n 1)
sed -n '751,845p' "$file"
printf '\n--- caller and version sources ---\n'
rg -n -C 10 'resolve_konflux_image_nvrs|GOLANG_BUILDER_CVE_COMPONENT|go_version|extra_patterns=\{"nvr"' pyartcd artcommon
printf '\n--- regex behavior ---\n'
python3 - <<'PY'
import re
pattern = "openshift-golang-builder-container-v1.25.9"
values = [
"openshift-golang-builder-container-v1.25.9-123",
"openshift-golang-builder-container-v1.25.91-123",
"openshift-golang-builder-container-v1.25.90-123",
"openshift-golang-builder-container-v1.25.9",
]
for value in values:
print(value, bool(re.search(pattern, value)))
PYRepository: openshift-eng/art-tools
Length of output: 50380
Anchor and escape the Go version pattern.
search_builds_by_fields already orders by start_time DESC. However, REGEXP_CONTAINS treats the current pattern as an unanchored regular expression, so v1.25.9 can match v1.25.91. Escape go_version and anchor the pattern to the NVR version boundary.
🤖 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/golang_builder_shipment.py` around lines 108 - 122,
Update the extra_patterns construction in the Go builder shipment search to
escape go_version before embedding it in the NVR pattern, then anchor the
resulting regular expression to the complete version boundary so v1.25.9 cannot
match v1.25.91. Preserve the existing search fields, ordering, and limit
behavior.
| quay_auth_file = os.getenv("QUAY_AUTH_FILE") | ||
| if quay_auth_file: | ||
| cmd.append(f"--pull-secret={quay_auth_file}") | ||
|
|
||
| rc, stdout, stderr = await exectools.cmd_gather_async(cmd, stderr=None, check=False) | ||
| if rc != 0: | ||
| raise RuntimeError(f"elliott snapshot new failed (rc={rc}): {stderr or stdout}") | ||
| if stdout: | ||
| _LOGGER.info("elliott snapshot new output:\n%s", stdout) | ||
| finally: | ||
| os.unlink(builds_file) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect cmd_gather_async to confirm what stderr=None means for the returned tuple.
fd -t f 'exectools.py' | xargs -r rg -n -C 25 'async def cmd_gather_async'Repository: openshift-eng/art-tools
Length of output: 4186
🏁 Script executed:
#!/bin/bash
sed -n '720,805p' artcommon/artcommonlib/exectools.py
printf '\n--- target imports and runtime metadata ---\n'
sed -n '1,45p' pyartcd/pyartcd/pipelines/golang_builder_shipment.py
rg -n 'python_requires|requires-python|Python|python_version|missing_ok' pyproject.toml setup.cfg setup.py tox.ini .github pyartcd artcommon 2>/dev/null | head -80Repository: openshift-eng/art-tools
Length of output: 7804
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- target method ---'
sed -n '320,385p' pyartcd/pyartcd/pipelines/golang_builder_shipment.py
printf '%s\n' '--- related stderr usage ---'
rg -n -C 4 'cmd_gather_async\(.*|stderr=None|elliott snapshot new|os\.unlink\(builds_file\)' pyartcd artcommon | head -240
printf '%s\n' '--- tests for the target pipeline ---'
fd -i -t f 'golang_builder_shipment' pyartcd | xargs -r -n1 sh -c 'echo "--- $0"; rg -n -C 5 "_create_snapshot|cmd_gather_async|RuntimeError|NamedTemporaryFile" "$0"'Repository: openshift-eng/art-tools
Length of output: 28515
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import asyncio
import os
import sys
import tempfile
from pathlib import Path
async def run(**kwargs):
proc = await asyncio.create_subprocess_exec(
sys.executable, "-c",
"import sys; print('out'); print('err', file=sys.stderr); sys.exit(7)",
stdout=asyncio.subprocess.PIPE,
**kwargs,
)
out, err = await proc.communicate()
return proc.returncode, out.decode() if out else "", err.decode() if err else ""
async def main():
inherited_rc, inherited_out, inherited_err = await run(stderr=None)
captured_rc, captured_out, captured_err = await run(stderr=asyncio.subprocess.PIPE)
print("stderr=None:", inherited_rc, repr(inherited_out), repr(inherited_err))
print("stderr=PIPE:", captured_rc, repr(captured_out), repr(captured_err))
path = tempfile.mktemp()
try:
open(path, "w").close()
try:
try:
raise RuntimeError("command failure")
finally:
os.unlink(path)
raise OSError("cleanup failure")
except BaseException as exc:
print("finally exception:", type(exc).__name__, str(exc))
finally:
try:
Path(path).unlink(missing_ok=True)
except OSError:
pass
asyncio.run(main())
PYRepository: openshift-eng/art-tools
Length of output: 265
Capture elliott diagnostics and make cleanup tolerant of missing files.
- Remove
stderr=Noneor passasyncio.subprocess.PIPE; otherwisecmd_gather_asyncreturns an emptystderr, so the failure message omitselliottdiagnostics. - Use
missing_ok=Trueso a missing temporary file does not replace the original exception.
Proposed cleanup guard
finally:
- os.unlink(builds_file)
+ Path(builds_file).unlink(missing_ok=True)🤖 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/golang_builder_shipment.py` around lines 354 - 364,
Update the `cmd_gather_async` call in the shipment flow to capture stderr by
removing `stderr=None` or passing `asyncio.subprocess.PIPE`, ensuring the
existing `RuntimeError` includes elliott diagnostics. In the `finally` cleanup
for `builds_file`, make `os.unlink` tolerate an already-missing file by using
`missing_ok=True`, preserving any original exception.
| if not resolved_nvrs and golang_nvrs: | ||
| rpm_nvrs = [n.strip() for n in golang_nvrs.replace(",", " ").split() if n.strip()] | ||
| _LOGGER.info("Resolving golang RPM NVRs to Konflux image NVRs: %s", rpm_nvrs) | ||
| resolved_nvrs = await resolve_konflux_image_nvrs(rpm_nvrs) | ||
| if not golang_group: | ||
| golang_group = derive_golang_group(rpm_nvrs) | ||
|
|
||
| if not resolved_nvrs: | ||
| raise click.UsageError("Provide Konflux image NVRs as arguments or --golang-nvrs with golang RPM NVRs") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--golang-nvrs is ignored without notice.
The condition at Line 499 requires not resolved_nvrs. If the user supplies positional NVRs and --golang-nvrs together, the pipeline silently drops --golang-nvrs. Raise a click.UsageError when both are set, so the user learns the inputs conflict.
🐛 Proposed fix
resolved_nvrs: List[str] = list(nvrs)
+ if resolved_nvrs and golang_nvrs:
+ raise click.UsageError("Provide either positional Konflux image NVRs or --golang-nvrs, not both")
+
if not resolved_nvrs and golang_nvrs:🤖 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/golang_builder_shipment.py` around lines 499 - 507,
Update the input validation around resolved_nvrs and golang_nvrs to raise
click.UsageError when both positional Konflux image NVRs and --golang-nvrs are
provided. Perform this conflict check before the existing resolution branch,
while preserving normal resolution when only golang_nvrs is supplied and the
existing missing-input error otherwise.
Adds artcd golang-builder-shipment command that creates shipment MRs in
ocp-shipment-data for golang builder images. Moves golang builders from
silent auto-release to a shipment-gated ERT-approved delivery path targeting
the new ocp-art-golang-builder-{prod,ec}-rhel9 ReleasePlans.
- Accepts golang RPM NVRs or Konflux image NVRs
- Auto-detects prod/ec ReleasePlan from ocp-build-data software_lifecycle.phase
- Resolves RPM NVRs to Konflux image NVRs via KonfluxDb
- Opens a draft MR in ocp-shipment-data with YAML shipment config
- Removes auto-release path for golang builders from process_release_from_fbc_bugs
Jira: ART-20930
d6ab558 to
01a8dad
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pyartcd/tests/pipelines/test_golang_builder_shipment.py (1)
50-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the requested URL in
_FakeSession.get.
_FakeSession.getdiscards its arguments. No test checks theraw_urlthatresolve_lifecycle_envbuilds. The branch selection at Lines 144-147 ofgolang_builder_shipment.pyand the.gitnormalization gap are therefore untested.Record the URL and assert it in at least one test.
💚 Proposed test change
class _FakeSession: def __init__(self, response): self._response = response + self.requested_urls = [] def get(self, *args, **kwargs): + if args: + self.requested_urls.append(args[0]) return self._response🤖 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_golang_builder_shipment.py` around lines 50 - 93, Update the test helper _FakeSession.get to record the requested URL, then assert the expected normalized raw_url in at least one TestResolveLifecycleEnv test. Ensure coverage verifies the resolve_lifecycle_env branch that builds the URL and removes the “.git” suffix.pyartcd/pyartcd/pipelines/golang_builder_shipment.py (1)
388-391: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid reaching into
GitRepository._directory.The pipeline uses the private attribute
_directoryto create the target directory, then uses the publicwrite_filefor the file itself. This couples the pipeline toGitRepositoryinternals.Expose a public accessor for the repository directory on
GitRepository, or letwrite_filecreate parent directories.🤖 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/golang_builder_shipment.py` around lines 388 - 391, Remove the direct use of the private GitRepository._directory in the target-directory setup around the shipment path construction. Expose and use a public repository-directory accessor on GitRepository, or update write_file to create parent directories and rely on it, while preserving the existing relative target path and directory creation behavior.
🤖 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/golang_builder_shipment.py`:
- Around line 210-217: Update the initialization of shipment_data_repo_push_url
alongside shipment_data_repo_pull_url so the shipment_data_repo_url CLI override
applies to both repository URLs, preserving config and template fallbacks when
the override is absent. Keep the existing MR target and branch push behavior
consistent with the same repository.
- Around line 79-93: Update the NVR group-derivation loop to evaluate every NVR
instead of returning on the first match, collecting each successfully derived
group from both the Konflux regex and RPM parsing paths. After processing all
NVRs, return the common group only when all derived values match; raise
ValueError when any NVR derives a different group, while preserving the existing
failure for NVRs that cannot be parsed.
In `@pyartcd/tests/pipelines/test_golang_builder_shipment.py`:
- Around line 613-623: Update TestShipmentFilePath.test_path_format to exercise
create_shipment_mr (or the relevant path-producing helper) and assert against
its returned/generated shipment path, using the expected convention
independently of the implementation. Remove the test if that function cannot be
invoked meaningfully; do not compare two paths constructed from the same
literals.
---
Nitpick comments:
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py`:
- Around line 388-391: Remove the direct use of the private
GitRepository._directory in the target-directory setup around the shipment path
construction. Expose and use a public repository-directory accessor on
GitRepository, or update write_file to create parent directories and rely on it,
while preserving the existing relative target path and directory creation
behavior.
In `@pyartcd/tests/pipelines/test_golang_builder_shipment.py`:
- Around line 50-93: Update the test helper _FakeSession.get to record the
requested URL, then assert the expected normalized raw_url in at least one
TestResolveLifecycleEnv test. Ensure coverage verifies the resolve_lifecycle_env
branch that builds the URL and removes the “.git” suffix.
🪄 Autofix
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: ba5d79d6-afba-43d0-bbf7-31581af21280
📒 Files selected for processing (4)
pyartcd/pyartcd/__main__.pypyartcd/pyartcd/pipelines/__init__.pypyartcd/pyartcd/pipelines/golang_builder_shipment.pypyartcd/tests/pipelines/test_golang_builder_shipment.py
🚧 Files skipped from review as they are similar to previous changes (2)
- pyartcd/pyartcd/pipelines/init.py
- pyartcd/pyartcd/main.py
| self.shipment_data_repo_pull_url = ( | ||
| shipment_data_repo_url | ||
| or runtime.config.get("shipment_config", {}).get("shipment_data_url") | ||
| or SHIPMENT_DATA_URL_TEMPLATE | ||
| ) | ||
| self.shipment_data_repo_push_url = ( | ||
| runtime.config.get("shipment_config", {}).get("shipment_data_push_url") or SHIPMENT_DATA_URL_TEMPLATE | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--shipment-data-repo-url changes only the pull URL.
shipment_data_repo_url feeds shipment_data_repo_pull_url. shipment_data_repo_push_url ignores it and falls back to config or SHIPMENT_DATA_URL_TEMPLATE. The CLI help states the option overrides the "ocp-shipment-data repo URL".
An operator who sets the option gets the override as the MR target project at Line 436, while the branch is still pushed to the default repository at Line 435. The MR then points at a source branch that does not exist in the source project.
Apply the override to both URLs, or rename the option and document that it sets the target repository only.
🤖 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/golang_builder_shipment.py` around lines 210 - 217,
Update the initialization of shipment_data_repo_push_url alongside
shipment_data_repo_pull_url so the shipment_data_repo_url CLI override applies
to both repository URLs, preserving config and template fallbacks when the
override is absent. Keep the existing MR target and branch push behavior
consistent with the same repository.
|
@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. |
Move golang builder shipment logic to doozer backend handler (like BaseImageHandler). Trigger inline from konflux_image_builder for golang builders (non-fatal). pyartcd CLI becomes thin wrapper.
d96f246 to
71f8baa
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
doozer/tests/backend/test_golang_builder_shipment.py (1)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused elliott mocks from these two tests.
test_build_shipment_config_prodandtest_build_shipment_config_eccall_build_inline_snapshotand_build_shipment_configonly. Neither test reachescmd_gather_asyncoros.unlink, so the patches and the YAML fixtures are dead setup that suggests the wrong code path is under test.Also applies to: 125-128
🤖 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/backend/test_golang_builder_shipment.py` around lines 76 - 79, Remove the unused patches from test_build_shipment_config_prod and test_build_shipment_config_ec by updating the test decorators around _build_inline_snapshot and _build_shipment_config. The cmd_gather_async and os.unlink mocks are never exercised in these tests, so drop those patch lines and keep only the setup that matches the code path actually under test, including the existing YAML fixture usage if it is still needed for the config assertions.pyartcd/pyartcd/pipelines/golang_builder_shipment.py (1)
186-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
--ocp-versionguard.Lines 186-187 either set
golang_groupor raise fromderive_golang_group.golang_groupis therefore always truthy at Line 190, so theclick.UsageErrorat Line 191 cannot run.♻️ Proposed cleanup
if not ocp_version: - if not golang_group: - raise click.UsageError("--ocp-version is required when --golang-group cannot be derived") _LOGGER.warning(🤖 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/golang_builder_shipment.py` around lines 186 - 196, Remove the redundant `if not golang_group` check and its `click.UsageError` from the `if not ocp_version` branch in the Golang shipment pipeline; `derive_golang_group` already establishes or raises for the missing group, so retain only the warning and prod-environment defaulting behavior.pyartcd/tests/pipelines/test_golang_builder_shipment.py (1)
13-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
TestResolveReleasePlanduplicates the doozer test suite.
resolve_release_planandGOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAPnow live indoozerlib.backend.golang_builder_shipment, anddoozer/tests/backend/test_golang_builder_shipment.pyLines 16-31 already assert the same behavior with the same class name. Keep the coverage in the doozer suite and remove it here, so the plan names only need one update.🤖 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_golang_builder_shipment.py` around lines 13 - 28, Remove the duplicate TestResolveReleasePlan test class and its resolve_release_plan/map assertions from this test module. Retain the coverage in doozer/tests/backend/test_golang_builder_shipment.py, where GolangBuilderShipmentHandler and GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP are defined and already tested.
🤖 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/backend/golang_builder_shipment.py`:
- Around line 53-67: Update derive_golang_group to derive and collect a group
for every NVR instead of returning on the first match, then raise ValueError
when the collected groups are not identical; return the shared group only when
all NVRs agree, preserving the existing error for NVRs that cannot derive a
group.
- Around line 358-364: Update the elliott invocation in
golang_builder_shipment.py around the cmd_gather_async call so stderr is
captured and included in the RuntimeError when rc is nonzero, falling back to
stdout only if stderr is empty. Also make the finally block that unlinks
builds_file tolerate FileNotFoundError so a missing temporary file does not mask
the original failure.
- Around line 47-50: Update GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP and
resolve_release_plan so the selected ReleasePlan reflects the EL version
returned by derive_golang_group, preventing el8 NVRs from using the existing
rhel9 plans; alternatively, validate and reject non-RHEL-9 input before creating
the shipment.
- Around line 436-445: Update the GitLab client setup around _get_project to
ensure repository URL hosts cannot be silently discarded. Either derive the API
base URL from the push repository URL before constructing gl, or validate that
both push and pull URL hosts match self.gitlab_url and reject mismatches before
project lookup.
- Around line 303-306: Update the Environments construction to assign a
stage-specific ReleasePlan to environments.stage instead of reusing
release_plan, while retaining the production ReleasePlan for environments.prod.
Ensure release new environment selection passes the correct stage or production
plan through shipment.environments, and add coverage for both environment
selections.
In `@doozer/tests/backend/test_golang_builder_shipment.py`:
- Around line 466-474: Update TestShipmentFilePath.test_path_format so it
verifies the path produced by _create_shipment_mr via
shipment_data_repo.write_file instead of comparing two Path values built from
the same literals. Use the visible _create_shipment_mr and write_file symbols as
the assertion target, or remove this test if it cannot observe the generated
shipment path layout.
- Around line 6-13: Reorder the imports in the test module so the
doozerlib.backend.golang_builder_shipment import appears before
doozerlib.constants, matching Ruff/isort ordering while preserving all imported
symbols.
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py`:
- Around line 200-212: Ensure GolangBuilderShipmentHandler receives a runtime
group that resolves to the OCP version when ocp_version is omitted, rather than
golang_group. Update _CliRuntimeAdapter construction or its group assignment to
use the OCP group name, or pass the established explicit placeholder through the
handler while preserving explicit --ocp-version behavior.
---
Nitpick comments:
In `@doozer/tests/backend/test_golang_builder_shipment.py`:
- Around line 76-79: Remove the unused patches from
test_build_shipment_config_prod and test_build_shipment_config_ec by updating
the test decorators around _build_inline_snapshot and _build_shipment_config.
The cmd_gather_async and os.unlink mocks are never exercised in these tests, so
drop those patch lines and keep only the setup that matches the code path
actually under test, including the existing YAML fixture usage if it is still
needed for the config assertions.
In `@pyartcd/pyartcd/pipelines/golang_builder_shipment.py`:
- Around line 186-196: Remove the redundant `if not golang_group` check and its
`click.UsageError` from the `if not ocp_version` branch in the Golang shipment
pipeline; `derive_golang_group` already establishes or raises for the missing
group, so retain only the warning and prod-environment defaulting behavior.
In `@pyartcd/tests/pipelines/test_golang_builder_shipment.py`:
- Around line 13-28: Remove the duplicate TestResolveReleasePlan test class and
its resolve_release_plan/map assertions from this test module. Retain the
coverage in doozer/tests/backend/test_golang_builder_shipment.py, where
GolangBuilderShipmentHandler and GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP are
defined and already tested.
🪄 Autofix
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: d9a4a064-7eef-47d3-9e7b-96db6eb84d3f
📒 Files selected for processing (5)
doozer/doozerlib/backend/golang_builder_shipment.pydoozer/doozerlib/backend/konflux_image_builder.pydoozer/tests/backend/test_golang_builder_shipment.pypyartcd/pyartcd/pipelines/golang_builder_shipment.pypyartcd/tests/pipelines/test_golang_builder_shipment.py
| GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP = { | ||
| "prod": "ocp-art-golang-builder-prod-rhel9", | ||
| "ec": "ocp-art-golang-builder-ec-rhel9", | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
RHEL 8 shipments get an RHEL 9 ReleasePlan.
derive_golang_group at Lines 53-67 accepts el8 NVRs and produces groups such as rhel-8-golang-1.25. Both entries in GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP end with -rhel9, and resolve_release_plan only keys on the environment. An RHEL 8 shipment therefore targets the RHEL 9 ReleasePlan.
Key the plan on the EL version as well, or reject non-RHEL-9 input before the shipment is created.
🤖 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/backend/golang_builder_shipment.py` around lines 47 - 50,
Update GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP and resolve_release_plan so the
selected ReleasePlan reflects the EL version returned by derive_golang_group,
preventing el8 NVRs from using the existing rhel9 plans; alternatively, validate
and reject non-RHEL-9 input before creating the shipment.
| def derive_golang_group(nvrs: List[str]) -> str: | ||
| """Derive the ocp-build-data golang group from NVR patterns.""" | ||
| for nvr in nvrs: | ||
| m = re.search(r"v(\d+)\.(\d+)\.\d+.*\.el(\d+)", nvr) | ||
| if m: | ||
| return f"rhel-{m.group(3)}-golang-{m.group(1)}.{m.group(2)}" | ||
|
|
||
| parsed = parse_nvr(nvr) | ||
| if parsed["name"] == "golang": | ||
| major_minor = ".".join(parsed["version"].split(".")[:2]) | ||
| el_v = isolate_el_version_in_release(parsed["release"]) | ||
| if el_v is not None: | ||
| return f"rhel-{el_v}-golang-{major_minor}" | ||
|
|
||
| raise ValueError(f"Cannot derive golang group from NVRs: {nvrs}") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate that all NVRs derive the same golang group.
The loop returns on the first NVR that matches. create_shipment_from_nvrs sorts nvrs first, so mixed EL versions or mixed Go minor versions silently produce a shipment labeled with the group of one NVR, while every NVR still enters that shipment and target directory.
Collect the derived group for every NVR and raise when the values disagree.
🤖 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/backend/golang_builder_shipment.py` around lines 53 - 67,
Update derive_golang_group to derive and collect a group for every NVR instead
of returning on the first match, then raise ValueError when the collected groups
are not identical; return the shared group only when all NVRs agree, preserving
the existing error for NVRs that cannot derive a group.
| environments = Environments( | ||
| stage=ShipmentEnv(releasePlan=release_plan), | ||
| prod=ShipmentEnv(releasePlan=release_plan), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- how other shipment builders populate Environments ---'
rg -n -C 6 'Environments\(' --type=py .
echo '--- stage release plan naming ---'
rg -n 'releasePlan|release_plan.*stage|stage.*release_plan' --type=py elliott doozer pyartcd | head -60Repository: openshift-eng/art-tools
Length of output: 24882
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- target builder and tests ---'
sed -n '250,330p' doozer/doozerlib/backend/golang_builder_shipment.py
sed -n '80,185p' doozer/tests/backend/test_golang_builder_shipment.py
echo '--- environment selection and release-plan use ---'
sed -n '270,375p' elliott/elliottlib/cli/konflux_release_cli.py
sed -n '35,110p' elliott/elliottlib/cli/shipment_cli.py
echo '--- golang-builder configuration references ---'
rg -n -C 4 'golang-builder|ocp-art-golang-builder|releasePlan' --glob 'config.y*ml' --glob '*.yaml' --glob '*.yml' .Repository: openshift-eng/art-tools
Length of output: 19002
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- all builder call sites and release-plan construction ---'
rg -n -C 10 '_build_shipment_config|release_plan|release-plan|releasePlan' doozer/doozerlib/backend/golang_builder_shipment.py
echo '--- handler command flow and environment selection ---'
sed -n '1,260p' doozer/doozerlib/backend/golang_builder_shipment.py
echo '--- tests covering stage/prod or env behavior ---'
rg -n -C 8 'env=|stage|prod|release_plan|releasePlan' doozer/tests/backend/test_golang_builder_shipment.py
echo '--- shipment release command callers ---'
rg -n -C 8 'get_release_config|new_release|release_env|shipment.environments' elliott/elliottlib pyartcd/pyartcd doozer/doozerlib --glob '*.py'Repository: openshift-eng/art-tools
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- repository-wide exact release-plan references ---'
rg -n -F 'ocp-art-golang-builder-prod-rhel9' . || true
rg -n -F 'ocp-art-golang-builder-ec-rhel9' . || true
echo '--- release command environment options and shipment path handling ---'
sed -n '400,485p' elliott/elliottlib/cli/konflux_release_cli.py
rg -n -C 8 'shipment/|environments.*stage|environments.*prod|releasePlan' elliott/elliottlib pyartcd/pyartcd doozer/doozerlib --glob '*.py' | head -240
echo '--- model documentation and environment semantics ---'
sed -n '90,155p' elliott/elliottlib/shipment_model.py
rg -n -C 5 'stage.*prod|prod.*stage|release plan|ReleasePlan' README.md elliott pyartcd doozer --glob '*.md' --glob '*.py' | head -240Repository: openshift-eng/art-tools
Length of output: 48241
🌐 Web query:
"ocp-art-golang-builder-prod-rhel9" "ocp-art-golang-builder-ec-rhel9"
💡 Result:
The identifiers ocp-art-golang-builder-prod-rhel9 and ocp-art-golang-builder-ec-rhel9 refer to specific release plans or target configurations used by the Red Hat OpenShift Automated Release Team (ART) in their internal build pipelines [1]. These identifiers are associated with the golang-builder-shipment pipeline, which is part of the ART tooling infrastructure [1]. Specifically: 1. Relationship to ART pipelines: These identifiers map lifecycle phases to their respective Release Plans within the ART golang-builder-shipment pipeline [1]. The prod lifecycle phase is mapped to ocp-art-golang-builder-prod-rhel9, and the ec (likely early access or equivalent) lifecycle phase is mapped to ocp-art-golang-builder-ec-rhel9 [1]. 2. Function: This pipeline helps automate the generation of shipment configuration files (ShipmentConfig YAML) used when creating snapshot builds or managing the distribution of OpenShift components [1]. 3. Context: ART manages various golang builder images (e.g., registry.ci.openshift.org/ocp/builder:rhel-9-golang-...) which are used to ensure that CI/CD and productized builds are consistent [2][3]. These internal identifiers, however, function as administrative labels within ART's orchestration code (e.g., in pyartcd) to handle the automated lifecycle management of these builder assets [1]. In summary, these are not end-user or publicly consumable container image names, but rather internal configuration keys used by the OpenShift ART team to automate the release and shipment workflows for Go-based builder images [1].
Citations:
- 1: ART-20930: add golang-builder-shipment pipeline #3083
- 2: https://docs.ci.openshift.org/architecture/images/
- 3: openshift/machine-config-operator@1386e52
🌐 Web query:
site:github.com/openshift-eng/art-tools/pull/3083 golang builder shipment stage prod releasePlan
💡 Result:
The terms builder, shipment, stage, prod, and releasePlan refer to components within the OpenShift Automated Release Team (ART) tooling, specifically regarding the golang-builder-shipment pipeline [1]. In this context: 1. golang-builder-shipment: This is a specific automation pipeline implemented in the pyartcd library, designed to manage the shipment of Go-based components [1]. 2. releasePlan: This refers to a configuration mapping used to define the target environment for a build [1]. The pipeline maps software lifecycle phases to specific release plans [1]. For example, the prod lifecycle phase is mapped to the ocp-art-golang-builder-prod-rhel9 release plan, and the ec (early candidate) phase is mapped to ocp-art-golang-builder-ec-rhel9 [1]. 3. stage/prod: These represent the environment targets within the shipment configuration [1]. When assembling the shipment, the pipeline assigns the resolved release plan to both the stage and prod environment fields in the configuration [1]. The pipeline handles environment lifecycle detection by fetching the software lifecycle phase (such as release or pre-release) from build metadata (specifically group.yml in ocp-build-data) and resolving it to the appropriate environment and release plan [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -eu
echo '--- introducing commits and concise history for the target file ---'
git log --oneline --all -- doozer/doozerlib/backend/golang_builder_shipment.py | head -20
echo '--- test assertions for both shipment environments ---'
rg -n -C 12 'environments\.(stage|prod)|ShipmentEnv|stage.*releasePlan|prod.*releasePlan' doozer/tests/backend/test_golang_builder_shipment.py pyartcd/tests/pipelines/test_golang_builder_shipment.pyRepository: openshift-eng/art-tools
Length of output: 5041
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import json
import urllib.request
for url in [
"https://api.github.com/repos/openshift-eng/art-tools/commits/71f8baab",
"https://api.github.com/repos/openshift-eng/art-tools/pulls/3083",
]:
print(f"--- {url} ---")
try:
with urllib.request.urlopen(url, timeout=10) as response:
data = json.load(response)
for key in ("sha", "message", "title", "body", "html_url"):
if key in data:
value = data[key]
if isinstance(value, str):
print(f"{key}: {value[:5000]}")
else:
print(f"{key}: {value}")
except Exception as exc:
print(f"request failed: {exc}")
PYRepository: openshift-eng/art-tools
Length of output: 613
Use a stage-specific ReleasePlan. release new --env stage passes shipment.environments.stage.releasePlan directly to spec.releasePlan. Since both entries use release_plan, a stage release can use ocp-art-golang-builder-prod-rhel9. Add the stage ReleasePlan and test both environment selections.
🤖 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/backend/golang_builder_shipment.py` around lines 303 - 306,
Update the Environments construction to assign a stage-specific ReleasePlan to
environments.stage instead of reusing release_plan, while retaining the
production ReleasePlan for environments.prod. Ensure release new environment
selection passes the correct stage or production plan through
shipment.environments, and add coverage for both environment selections.
| rc, stdout, stderr = await exectools.cmd_gather_async(cmd, stderr=None, check=False) | ||
| if rc != 0: | ||
| raise RuntimeError(f"elliott snapshot new failed (rc={rc}): {stderr or stdout}") | ||
| if stdout: | ||
| self.logger.info("elliott snapshot new output:\n%s", stdout) | ||
| finally: | ||
| os.unlink(builds_file) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Capture elliott stderr and tolerate a missing temporary file.
stderr=None makes cmd_gather_async return an empty stderr, so the RuntimeError at Line 360 loses the elliott diagnostics whenever stdout is also empty. In the finally block, os.unlink raises FileNotFoundError if the file is already gone, and that replaces the original exception.
🐛 Proposed fix
- rc, stdout, stderr = await exectools.cmd_gather_async(cmd, stderr=None, check=False)
+ rc, stdout, stderr = await exectools.cmd_gather_async(cmd, check=False)
if rc != 0:
raise RuntimeError(f"elliott snapshot new failed (rc={rc}): {stderr or stdout}")
if stdout:
self.logger.info("elliott snapshot new output:\n%s", stdout)
finally:
- os.unlink(builds_file)
+ Path(builds_file).unlink(missing_ok=True)🤖 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/backend/golang_builder_shipment.py` around lines 358 - 364,
Update the elliott invocation in golang_builder_shipment.py around the
cmd_gather_async call so stderr is captured and included in the RuntimeError
when rc is nonzero, falling back to stdout only if stderr is empty. Also make
the finally block that unlinks builds_file tolerate FileNotFoundError so a
missing temporary file does not mask the original failure.
| gitlab_token = os.getenv("GITLAB_TOKEN") | ||
| gl = python_gitlab.Gitlab(self.gitlab_url, private_token=gitlab_token) | ||
|
|
||
| def _get_project(url): | ||
| parsed = urlparse(url) | ||
| project_path = parsed.path.strip("/").removesuffix(".git") | ||
| return gl.projects.get(project_path) | ||
|
|
||
| source_project = _get_project(self.shipment_data_repo_push_url) | ||
| target_project = _get_project(self.shipment_data_repo_pull_url) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The API host and the repository URL host can disagree.
gl targets self.gitlab_url. _get_project keeps only the path of the push and pull URLs and discards their host. If a caller passes shipment_data_repo_pull_url or shipment_data_repo_push_url for a different GitLab host, the project lookup runs against self.gitlab_url. The lookup then either fails or resolves an unrelated project with the same path, and the MR is created in the wrong place.
Derive the API base URL from the push URL, or validate that both hosts equal self.gitlab_url before you create the client.
🤖 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/backend/golang_builder_shipment.py` around lines 436 - 445,
Update the GitLab client setup around _get_project to ensure repository URL
hosts cannot be silently discarded. Either derive the API base URL from the push
repository URL before constructing gl, or validate that both push and pull URL
hosts match self.gitlab_url and reject mismatches before project lookup.
| from artcommonlib.model import Model | ||
| from doozerlib.constants import ART_IMAGES_BASE_APPLICATION | ||
| from doozerlib.backend.golang_builder_shipment import ( | ||
| GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP, | ||
| GolangBuilderShipmentHandler, | ||
| derive_golang_group, | ||
| resolve_env_from_runtime, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the import order to clear the Ruff I001 failure.
The unit-tests job fails with Ruff I001. doozerlib.constants precedes doozerlib.backend.golang_builder_shipment, so isort reorders the block.
🔧 Proposed fix
from artcommonlib.model import Model
-from doozerlib.constants import ART_IMAGES_BASE_APPLICATION
from doozerlib.backend.golang_builder_shipment import (
GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP,
GolangBuilderShipmentHandler,
derive_golang_group,
resolve_env_from_runtime,
)
+from doozerlib.constants import ART_IMAGES_BASE_APPLICATION📝 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.
| from artcommonlib.model import Model | |
| from doozerlib.constants import ART_IMAGES_BASE_APPLICATION | |
| from doozerlib.backend.golang_builder_shipment import ( | |
| GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP, | |
| GolangBuilderShipmentHandler, | |
| derive_golang_group, | |
| resolve_env_from_runtime, | |
| ) | |
| from artcommonlib.model import Model | |
| from doozerlib.backend.golang_builder_shipment import ( | |
| GOLANG_BUILDER_SHIPMENT_RELEASE_PLAN_MAP, | |
| GolangBuilderShipmentHandler, | |
| derive_golang_group, | |
| resolve_env_from_runtime, | |
| ) | |
| from doozerlib.constants import ART_IMAGES_BASE_APPLICATION |
🤖 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/backend/test_golang_builder_shipment.py` around lines 6 - 13,
Reorder the imports in the test module so the
doozerlib.backend.golang_builder_shipment import appears before
doozerlib.constants, matching Ruff/isort ordering while preserving all imported
symbols.
Source: Pipeline failures
| class TestShipmentFilePath(unittest.TestCase): | ||
| def test_path_format(self): | ||
| application = "art-images-base" | ||
| product = "ocp" | ||
| golang_group = "rhel-9-golang-1.25" | ||
| env = "prod" | ||
| expected_prefix = Path("shipment") / "ocp" / "rhel-9-golang-1.25" / "art-images-base" / "prod" | ||
| actual = Path("shipment") / product / golang_group / application / env | ||
| self.assertEqual(actual, expected_prefix) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
test_path_format cannot fail.
expected_prefix and actual are both built in the test from the same literals. The test never calls _create_shipment_mr. If the path layout at Lines 396-402 changes, this test still passes.
Assert on the path that _create_shipment_mr writes through shipment_data_repo.write_file, or remove the test.
🤖 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/backend/test_golang_builder_shipment.py` around lines 466 - 474,
Update TestShipmentFilePath.test_path_format so it verifies the path produced by
_create_shipment_mr via shipment_data_repo.write_file instead of comparing two
Path values built from the same literals. Use the visible _create_shipment_mr
and write_file symbols as the assertion target, or remove this test if it cannot
observe the generated shipment path layout.
| adapter = _CliRuntimeAdapter(runtime, golang_group) | ||
| handler = GolangBuilderShipmentHandler( | ||
| runtime=adapter, | ||
| dry_run=runtime.dry_run, | ||
| art_jira=art_jira, | ||
| ocp_version=ocp_version, | ||
| shipment_data_repo_pull_url=shipment_data_repo_url, | ||
| ) | ||
| mr_url = await handler.create_shipment_from_nvrs( | ||
| resolved_nvrs, | ||
| golang_group=golang_group, | ||
| env=env, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Release notes get the golang group as the OCP version when --ocp-version is omitted.
--ocp-version is now optional, so ocp_version=None reaches GolangBuilderShipmentHandler. _resolve_ocp_version then falls back to self.runtime.group (doozer/doozerlib/backend/golang_builder_shipment.py Lines 145-152). _CliRuntimeAdapter sets self.group = golang_group at Line 132, and the regex openshift-(\d+\.\d+) does not match a value such as rhel-9-golang-1.25. The handler therefore returns the golang group, and it is embedded in the shipment synopsis, topic, and description, and in the MR body.
Pass an explicit placeholder, or set the adapter group to the OCP group name so the version resolution stays correct.
🤖 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/golang_builder_shipment.py` around lines 200 - 212,
Ensure GolangBuilderShipmentHandler receives a runtime group that resolves to
the OCP version when ocp_version is omitted, rather than golang_group. Update
_CliRuntimeAdapter construction or its group assignment to use the OCP group
name, or pass the established explicit placeholder through the handler while
preserving explicit --ocp-version behavior.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
- Add REDHAT_GITLAB_URL constant to artcommonlib
- Move GolangBuilderShipmentHandler import to top-level in konflux_image_builder.py
- Remove config.get("shipment_config") dead lookup (no such config in doozer)
- Replace runtime.working_dir with tempfile.mkdtemp
- Hardcode product="ocp" as module constant
- Remove dead _resolve_ocp_version method
- Remove resolve_env_from_lifecycle_phase (CLI has its own)
- Fetch GITLAB_TOKEN once in _setup_repos, reuse as self._gitlab_token
- Make basic_auth_url a module-level function
- Use REDHAT_GITLAB_URL as default gitlab_url
- Remove working_dir/config/product from _CliRuntimeAdapter
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
doozer/doozerlib/backend/golang_builder_shipment.py (2)
331-331: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInsecure Deserialization (CWE-502): Deserialization of Untrusted Data
Reachability path
● Entry doozer/doozerlib/backend/konflux_image_builder.py:439 GolangBuilderShipmentHandler │ ▼ ● Sink doozer/doozerlib/backend/golang_builder_shipment.pyUse a safe YAML loader for Elliott output.
yaml.load(stdout)parses Elliott output before the dict validation at Lines 332-337. Unsafe YAML tags can construct attacker-controlled Python objects. Useyaml.safe_load(stdout)and reject unsupported schema values.As per coding guidelines, flag
yaml.loadwithoutSafeLoader. As per path instructions, do not useyaml.load()on untrusted data.#!/bin/bash set -euo pipefail # Inspect the declared YAML dependency and all unsafe loader call sites. rg -n -i --glob 'pyproject.toml' --glob 'requirements*.txt' --glob 'setup.py' \ --glob 'tox.ini' --glob 'Pipfile*' 'pyyaml|yaml' . || true rg -n -C 4 --glob '*.py' '\byaml\.load\s*\(' doozer elliott || true🤖 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/backend/golang_builder_shipment.py` at line 331, Update the YAML parsing in the shipment builder around snapshot_obj to use yaml.safe_load(stdout) instead of yaml.load, then preserve or strengthen the existing validation so unsupported or non-dictionary schema values are rejected before further processing.
92-107: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal
Reachability path
● Entry doozer/doozerlib/backend/konflux_image_builder.py:439 GolangBuilderShipmentHandler │ ▼ ● Sink doozer/doozerlib/backend/golang_builder_shipment.pyRequire HTTPS for
gitlab_urlandshipment_data_repo_push_url.Reject HTTP overrides before
basic_auth_urlinjectsGITLAB_TOKENorpython_gitlab.Gitlabreceives it. The pull URL does not receive the token.🤖 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/backend/golang_builder_shipment.py` around lines 92 - 107, Validate gitlab_url and shipment_data_repo_push_url in the GolangBuilderShipment initializer before either URL is passed to basic_auth_url or python_gitlab.Gitlab, rejecting any non-HTTPS override while preserving the default URL behavior. Leave shipment_data_repo_pull_url unrestricted because it does not receive the GitLab token.
🤖 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/backend/golang_builder_shipment.py`:
- Line 104: Ensure the temporary repository created for _shipment_data_repo_dir
is removed after every shipment flow, including failures. Update both public
shipment-flow methods to perform cleanup in finally blocks, or replace the raw
mkdtemp ownership with TemporaryDirectory and invoke cleanup, while preserving
existing success and error behavior.
- Line 133: Update both shipment paths in the handler, including the logic
around ocp_version and the corresponding line near the second shipment path, to
obtain and pass the actual OpenShift version from the build context instead of
falling back to golang_group. Ensure _build_shipment_config receives the real
OCP version when the production caller omits the handler’s ocp_version.
In `@doozer/tests/backend/test_golang_builder_shipment.py`:
- Line 211: Remove the `_gitlab_token = "test-token"` assignment from the test
setup in the mocked GitLab client test, leaving the remaining fixture
initialization unchanged.
---
Outside diff comments:
In `@doozer/doozerlib/backend/golang_builder_shipment.py`:
- Line 331: Update the YAML parsing in the shipment builder around snapshot_obj
to use yaml.safe_load(stdout) instead of yaml.load, then preserve or strengthen
the existing validation so unsupported or non-dictionary schema values are
rejected before further processing.
- Around line 92-107: Validate gitlab_url and shipment_data_repo_push_url in the
GolangBuilderShipment initializer before either URL is passed to basic_auth_url
or python_gitlab.Gitlab, rejecting any non-HTTPS override while preserving the
default URL behavior. Leave shipment_data_repo_pull_url unrestricted because it
does not receive the GitLab token.
🪄 Autofix
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: a548b6b9-3f3f-42d4-ada6-468dd5e941db
📒 Files selected for processing (5)
artcommon/artcommonlib/constants.pydoozer/doozerlib/backend/golang_builder_shipment.pydoozer/doozerlib/backend/konflux_image_builder.pydoozer/tests/backend/test_golang_builder_shipment.pypyartcd/pyartcd/pipelines/golang_builder_shipment.py
💤 Files with no reviewable changes (1)
- pyartcd/pyartcd/pipelines/golang_builder_shipment.py
🚧 Files skipped from review as they are similar to previous changes (1)
- doozer/doozerlib/backend/konflux_image_builder.py
| self.ocp_version = ocp_version | ||
| self.logger = getattr(runtime, "logger", logging.getLogger(__name__)) | ||
| self.gitlab_url = gitlab_url | ||
| self._shipment_data_repo_dir = Path(tempfile.mkdtemp(prefix="golang-shipment-")) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the temporary repository after each shipment flow.
Line 104 creates a persistent temporary directory. Both public shipment flows populate it, but neither removes it after success or failure. The production builder creates handlers per shipment, so repository checkouts accumulate under the temporary directory.
Run cleanup from a finally block in both public flows, or retain a TemporaryDirectory owner and call its cleanup method.
🤖 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/backend/golang_builder_shipment.py` at line 104, Ensure the
temporary repository created for _shipment_data_repo_dir is removed after every
shipment flow, including failures. Update both public shipment-flow methods to
perform cleanup in finally blocks, or replace the raw mkdtemp ownership with
TemporaryDirectory and invoke cleanup, while preserving existing success and
error behavior.
| golang_group = derive_golang_group([nvr]) | ||
| env = resolve_env_from_runtime(self.runtime) | ||
| release_plan = self.resolve_release_plan(env) | ||
| ocp_version = self.ocp_version or golang_group |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not use golang_group as the OpenShift version.
These fallbacks set ocp_version to values such as rhel-9-golang-1.25. The production caller in doozer/doozerlib/backend/konflux_image_builder.py constructs this handler without ocp_version. _build_shipment_config then writes that Golang group into RHBA release notes and the MR description as an OpenShift version.
Pass the actual OpenShift version from the build context. Update both shipment paths.
Also applies to: 184-184
🤖 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/backend/golang_builder_shipment.py` at line 133, Update both
shipment paths in the handler, including the logic around ocp_version and the
corresponding line near the second shipment path, to obtain and pass the actual
OpenShift version from the build context instead of falling back to
golang_group. Ensure _build_shipment_config receives the real OCP version when
the production caller omits the handler’s ocp_version.
| @patch("doozerlib.backend.golang_builder_shipment.python_gitlab") | ||
| async def test_creates_mr_with_correct_title(self, mock_gitlab): | ||
| handler = self._make_handler(dry_run=False) | ||
| handler._gitlab_token = "test-token" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the literal token fixture.
Line 211 assigns a string literal to _gitlab_token. Ruff S105 reports this as a hardcoded password. Remove this assignment because the mocked GitLab client does not need a credential in this test. As per coding guidelines, “Flag hardcoded secrets … and variables named api_key, secret, token, or password assigned to string literals.”
Proposed fix
- handler._gitlab_token = "test-token"📝 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.
| handler._gitlab_token = "test-token" |
🧰 Tools
🪛 Ruff (0.16.1)
[error] 211-211: Possible hardcoded password assigned to: "_gitlab_token"
(S105)
🤖 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/backend/test_golang_builder_shipment.py` at line 211, Remove the
`_gitlab_token = "test-token"` assignment from the test setup in the mocked
GitLab client test, leaving the remaining fixture initialization unchanged.
Sources: Coding guidelines, Linters/SAST tools
|
Work in progress — continuing on branch art-20930/golang-builder-shipment-v3. Closing for now, branch preserved. |
2 participants
What
Adds
artcd golang-builder-shipment— a new pyartcd pipeline that createsshipment MRs in ocp-shipment-data for golang builder images, moving them from
silent auto-release to a shipment-gated, ERT-approved delivery path.
Why
Golang builders currently release via
base-image-releasewith no ERT gate.ART-20920 requires them to go through the new
ocp-art-golang-builder-{prod,ec}-rhel9ReleasePlanAdmissions (already merged in konflux-release-data).
What the pipeline does
--golang-nvrs) or direct Konflux image NVRssoftware_lifecycle.phasefrom ocp-build-data to selectprodorecReleasePlanShipmentConfigYAML usingelliott snapshot newAlso removes the now-superseded golang builder auto-release path from
process_release_from_fbc_bugs_cli.Test
32 unit tests for
golang_builder_shipment, 15 forprocess_release_from_fbc_bugs_cli— all green.Jira
https://redhat.atlassian.net/browse/ART-20930
Notes
Supersedes #3083 — that PR accumulated unrelated lockfile/OKD/quay/microshift
changes on a long-lived branch. This PR contains only the ART-20930 scope (6 files).
Summary by CodeRabbit
New Features
Tests