OCPCRT-459: Add support for reference-based release payloads#759
OCPCRT-459: Add support for reference-based release payloads#759AlexNPavel wants to merge 6 commits intoopenshift:mainfrom
Conversation
Introduce a new "Reference" payload type where release images are created directly in an external repository instead of a local imagestream. This enables release workflows that do not require a cluster-local copy of the payload. Key changes: - Add PayloadType (Local/Reference) to the ReleasePayload CRD - Add ReferenceRelease config (push/pull repos, secret) to ReleaseConfig - Create reference release jobs using oc adm release new with --from-image-stream-file to push payloads to external repos - Add reference removal jobs that push remove__ prefixed tags to signal external cleanup tooling - Centralize pull spec resolution via ReleasePullSpec() replacing hardcoded Target.Status.PublicDockerImageRepository references - Compute content hashes from spec tag references for imagestreams with reference: true tags (status tags are never populated) - Handle reference releases in mirror imagestream creation, prow job env injection, upgrade resolution, and HTTP API handlers - Extract duplicated publishSpec/publishDescription logic into shared helpers in http_helper.go - Add comprehensive tests for new and refactored functions
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexNPavel The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
📝 WalkthroughWalkthroughIntroduce reference-mode payload support and centralize pull-spec resolution: add PayloadType (Local|Reference), ReferenceRelease config and helpers, propagate reference-mode branching across controller, mirror, job orchestration, tag handling, audit/upgrade/Jira/Prow, and API/template layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Release Controller
participant SourceIS as Source ImageStream
participant Mirror as Mirror Logic
participant JobBuilder as Job Builder
participant K8s as Kubernetes API
participant RefRepo as Reference Repo
Controller->>SourceIS: Check HasReferenceSpecTags(release.Source)
alt reference mode
Controller->>Mirror: calculateReferenceMirrorImageStream(release)
Mirror->>K8s: create/update ImageStream (copy spec tags, ImportPolicy.Scheduled=false)
Controller->>JobBuilder: buildReferenceReleaseJob(release, tag)
JobBuilder->>K8s: create Job (oc adm release new --from-image-stream-file ... --reference-mode)
K8s->>RefRepo: Job runs → push rc_payload__TAG to external repo
RefRepo-->>K8s: image/tag/digest becomes available
else local mode
Controller->>Mirror: calculateMirrorImageStream(release)
Mirror->>K8s: create/update ImageStream (status-driven imports)
Controller->>JobBuilder: ensureReleaseJob(release)
K8s->>K8s: Job runs → create local imagestream-backed payload
end
Controller->>Controller: ReleasePullSpec(release, tag) returns mode-appropriate pull spec
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/hold Once this is reviewed and looks good to merge, I'll create a separate PR for just the CRD, as that will need to be applied before this merges. |
openshift-ci
bot
added
the
do-not-merge/hold
|
There are 2 items that need some discussion for this PR. First, how exactly should we handle tag deletion failures. With the new code, if either the job creating the Second, this PR now adds |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
pkg/release-controller/release.go (1)
235-247: Hash stability depends on tag ordering.Both
hashReferenceSpecTagsandhashStatusTagsiterate over tags in their natural order without sorting. If the tag order changes in the ImageStream (even with identical content), the resulting hash will differ, potentially triggering unnecessary release processing.Consider sorting tags by name before hashing to ensure deterministic output regardless of API return order.
♻️ Suggested fix to sort tags before hashing
func hashReferenceSpecTags(is *imagev1.ImageStream) string { h := sha256.New() + tags := make([]imagev1.TagReference, len(is.Spec.Tags)) + copy(tags, is.Spec.Tags) + sort.Slice(tags, func(i, j int) bool { return tags[i].Name < tags[j].Name }) - for _, tag := range is.Spec.Tags { + for _, tag := range tags { if tag.From == nil { continue } h.Write([]byte(tag.From.Name)) } return fmt.Sprintf("sha256:%x", h.Sum(nil)) }Similar change would apply to
hashStatusTags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/release.go` around lines 235 - 247, The hashing functions (hashReferenceSpecTags and hashStatusTags) are non-deterministic because they iterate is.Spec.Tags / is.Status.Tags in API order; sort the tag slice by tag name before computing the sha256 to ensure stable hashes. Update hashReferenceSpecTags to copy/sort is.Spec.Tags (compare tag.Name) and then write tag.From.Name into the hasher in sorted order, and make the analogous change in hashStatusTags so both functions produce deterministic hashes regardless of API ordering.cmd/release-controller/sync_release_payload_test.go (1)
34-35: Make the default/local payload type explicit in the existing cases.Most table entries still leave
payloadTypeat the zero value, so these tests won’t fail ifPayloadTypeLocalstops being populated by callers. Now thatPayloadTypeis part of the contract, it’s worth settingpayloadType: v1alpha1.PayloadTypeLocaland assertingSpec.PayloadTypefor the non-reference cases too.Also applies to: 992-995
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_release_payload_test.go` around lines 34 - 35, Update the table-driven tests in sync_release_payload_test.go to explicitly set payloadType: v1alpha1.PayloadTypeLocal for cases that currently rely on the zero value, and update the expected *v1alpha1.ReleasePayload objects to assert Spec.PayloadType equals v1alpha1.PayloadTypeLocal for the non-reference cases; locate the table rows that populate the payloadType field (and the expected variable) and add the explicit payloadType and corresponding Spec.PayloadType assertion so tests will fail if callers stop populating the default.cmd/release-controller/sync_release.go (1)
67-109: Consider the robustness of the awk-based YAML status stripping.The awk command (lines 96-98) strips the
status:section from the imagestream YAML:awk '/^status:/{s=1;next} s&&/^[^ ]/{s=0} !s' /tmp/mirror-full.yaml > /tmp/mirror.yamlThis approach assumes:
status:appears at column 0 (no leading whitespace)- The next top-level key also starts at column 0
While this should work for standard Kubernetes YAML output, consider adding error handling or validation in the job to detect malformed output.
Alternative: use yq if available
- awk '/^status:/{s=1;next} s&&/^[^ ]/{s=0} !s' /tmp/mirror-full.yaml > /tmp/mirror.yaml + yq eval 'del(.status)' /tmp/mirror-full.yaml > /tmp/mirror.yaml || \ + awk '/^status:/{s=1;next} s&&/^[^ ]/{s=0} !s' /tmp/mirror-full.yaml > /tmp/mirror.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_release.go` around lines 67 - 109, In buildReferenceReleaseJob, make the imagestream YAML stripping step more robust by validating the result before calling oc adm release: after the awk line that writes /tmp/mirror.yaml, add checks to ensure the file exists, is non-empty, and contains an ImageStream/kind header (e.g. grep -q '^kind:.*ImageStream' /tmp/mirror.yaml), and if validation fails emit a clear error and exit non-zero; update the job command construction (job.Spec.Template.Spec.Containers[0].Command) to include these checks and fail fast instead of running oc adm release on malformed output, so the Reference release creation is deterministic and debuggable.cmd/release-controller/sync_tags.go (1)
160-202: Silently proceeding when CLI image is unavailable may leave orphaned external payloads.When
cliImagecannot be determined (lines 170-173), the function logs a warning but returnstrue, allowing local tag deletion to proceed. This could leave orphaned payloads in the external repository that external cleanup tooling won't know to remove.Consider returning an error or at least changing the log level to make this more visible:
Suggested improvement
if len(cliImage) == 0 { - klog.Warningf("Unable to determine CLI image for reference removal tags, skipping removal tagging") - return true, nil + klog.Errorf("Unable to determine CLI image for reference removal tags for release %s, external payloads may be orphaned", release.Config.Name) + return true, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_tags.go` around lines 160 - 202, The function ensureReferenceRemovalTags currently logs a warning and returns (true, nil) when cliImage cannot be determined, which allows local tag deletion to proceed and may orphan external payloads; change this behavior so that when cliImage is empty (after checking release.Config.OverrideCLIImage and releasecontroller.FindSpecTag), you log an error (klog.Errorf) and return (false, fmt.Errorf(...)) describing the missing CLI image (include release name or tag context), so callers know removal tagging couldn't run; update imports to include fmt if needed and keep the existing logic that proceeds when cliImage is present.cmd/release-controller/sync_release_reference_test.go (2)
294-317: Add a test forDisableManifestListModeprecedence.Current tests verify
manifestListMode=true/false, but not themanifestListMode=true+DisableManifestListMode=truebranch.Additional test case
+ t.Run("manifest list mode forced off when DisableManifestListMode is true", func(t *testing.T) { + releaseCopy := *release + configCopy := *release.Config + configCopy.DisableManifestListMode = true + releaseCopy.Config = &configCopy + + job, err := buildReferenceReleaseJob(&releaseCopy, releaseName, mirror, true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + args := job.Spec.Template.Spec.Containers[0].Command + lastArg := args[len(args)-1] + if lastArg != "false" { + t.Errorf("expected manifest list mode %q, got %q", "false", lastArg) + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_release_reference_test.go` around lines 294 - 317, Add a test case that ensures DisableManifestListMode overrides manifestListMode when both are set: call buildReferenceReleaseJob with the Release spec where ManifestListMode is true and DisableManifestListMode is true, then inspect job.Spec.Template.Spec.Containers[0].Command and assert the last arg equals "false" (use the same pattern as the existing manifest list mode tests and reference buildReferenceReleaseJob and the Release fields ManifestListMode/DisableManifestListMode to locate where to modify the input).
97-101: Decouple expected release-tag annotation fromreleaseNameto avoid false positives.On Line 188, expected value is
releaseName, but fixture on Line 100 is the same string. This can mask regressions where code uses release name instead of copying mirror annotation.Refactor suggestion
+ mirrorReleaseTag := "mirror-annotation-tag" mirror := &imagev1.ImageStream{ ObjectMeta: metav1.ObjectMeta{ Name: "4.17-art-latest-2024-08-30-110931", Namespace: "ocp", Annotations: map[string]string{ releasecontroller.ReleaseAnnotationSource: "ocp/4.17-art-latest", releasecontroller.ReleaseAnnotationTarget: "ocp/release", - releasecontroller.ReleaseAnnotationReleaseTag: "4.17.0-0.nightly-2024-08-30-110931", + releasecontroller.ReleaseAnnotationReleaseTag: mirrorReleaseTag, }, }, ... - if job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag] != releaseName { - t.Errorf("expected releaseTag annotation %q, got %q", releaseName, job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag]) + if job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag] != mirrorReleaseTag { + t.Errorf("expected releaseTag annotation %q, got %q", mirrorReleaseTag, job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag]) }Also applies to: 188-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_release_reference_test.go` around lines 97 - 101, The test currently compares the ReleaseAnnotationReleaseTag using the variable releaseName which matches the fixture value and can hide bugs; change the assertion to compare against the explicit fixture release-tag string (or a new test-constant, e.g. expectedTag := "4.17.0-0.nightly-2024-08-30-110931") instead of releaseName, and update the other related assertions that reference releaseName (the ones around releasecontroller.ReleaseAnnotationReleaseTag) so they assert the literal/constant expectedTag; use the annotation key releasecontroller.ReleaseAnnotationReleaseTag to locate where to change the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/release-controller/audit.go`:
- Around line 346-348: The code currently uses
releasecontroller.FindImageIDForTag(from, tag.Name) to get the digest (ID) even
when reference mode may not have a cluster-local copy; change the logic in
syncAuditTag so that if record.ID is empty or the release is in reference mode
you resolve the digest from the reference-aware location
(releasecontroller.ReleasePullSpec(release, tag.Name)) instead of the local
imagestream—i.e., detect empty record.ID or reference mode, call the registry
lookup for the pull spec returned by ReleasePullSpec to obtain the digest, and
use that digest for verification/signing so syncAuditTag no longer bails out
when the payload is reference-only.
In `@cmd/release-controller/jira.go`:
- Around line 245-250: The current guard only rejects empty pull specs but
allows malformed values like ":tag"; update the check around ReleasePullSpec
calls (prevPullSpec and curPullSpec) to detect and reject invalid pull specs
(e.g., strings that start with ":" or otherwise lack a registry component)
before logging/returning; increment jiraNoRegistry via
c.jiraErrorMetrics.WithLabelValues(jiraNoRegistry).Inc() and return an error
when either prevPullSpec or curPullSpec is invalid so malformed specs are not
propagated into Bugs/ChangeLog.
In `@cmd/release-controller/sync_release_payload.go`:
- Around line 21-25: The payloadType determination is inconsistent: change the
logic that sets payloadType (currently using
releasecontroller.HasReferenceSpecTags in the payloadType assignment) to use
releasecontroller.IsReferenceRelease(release) so it matches the check used later
for ReleaseCoordinates; update the code that builds the ReleasePayload (the
payloadType variable used in the newReleasePayload(...) Create call) to rely on
IsReferenceRelease(release) instead of HasReferenceSpecTags(release.Source) to
ensure both branches use the same reference-release criteria.
---
Nitpick comments:
In `@cmd/release-controller/sync_release_payload_test.go`:
- Around line 34-35: Update the table-driven tests in
sync_release_payload_test.go to explicitly set payloadType:
v1alpha1.PayloadTypeLocal for cases that currently rely on the zero value, and
update the expected *v1alpha1.ReleasePayload objects to assert Spec.PayloadType
equals v1alpha1.PayloadTypeLocal for the non-reference cases; locate the table
rows that populate the payloadType field (and the expected variable) and add the
explicit payloadType and corresponding Spec.PayloadType assertion so tests will
fail if callers stop populating the default.
In `@cmd/release-controller/sync_release_reference_test.go`:
- Around line 294-317: Add a test case that ensures DisableManifestListMode
overrides manifestListMode when both are set: call buildReferenceReleaseJob with
the Release spec where ManifestListMode is true and DisableManifestListMode is
true, then inspect job.Spec.Template.Spec.Containers[0].Command and assert the
last arg equals "false" (use the same pattern as the existing manifest list mode
tests and reference buildReferenceReleaseJob and the Release fields
ManifestListMode/DisableManifestListMode to locate where to modify the input).
- Around line 97-101: The test currently compares the
ReleaseAnnotationReleaseTag using the variable releaseName which matches the
fixture value and can hide bugs; change the assertion to compare against the
explicit fixture release-tag string (or a new test-constant, e.g. expectedTag :=
"4.17.0-0.nightly-2024-08-30-110931") instead of releaseName, and update the
other related assertions that reference releaseName (the ones around
releasecontroller.ReleaseAnnotationReleaseTag) so they assert the
literal/constant expectedTag; use the annotation key
releasecontroller.ReleaseAnnotationReleaseTag to locate where to change the
assertion.
In `@cmd/release-controller/sync_release.go`:
- Around line 67-109: In buildReferenceReleaseJob, make the imagestream YAML
stripping step more robust by validating the result before calling oc adm
release: after the awk line that writes /tmp/mirror.yaml, add checks to ensure
the file exists, is non-empty, and contains an ImageStream/kind header (e.g.
grep -q '^kind:.*ImageStream' /tmp/mirror.yaml), and if validation fails emit a
clear error and exit non-zero; update the job command construction
(job.Spec.Template.Spec.Containers[0].Command) to include these checks and fail
fast instead of running oc adm release on malformed output, so the Reference
release creation is deterministic and debuggable.
In `@cmd/release-controller/sync_tags.go`:
- Around line 160-202: The function ensureReferenceRemovalTags currently logs a
warning and returns (true, nil) when cliImage cannot be determined, which allows
local tag deletion to proceed and may orphan external payloads; change this
behavior so that when cliImage is empty (after checking
release.Config.OverrideCLIImage and releasecontroller.FindSpecTag), you log an
error (klog.Errorf) and return (false, fmt.Errorf(...)) describing the missing
CLI image (include release name or tag context), so callers know removal tagging
couldn't run; update imports to include fmt if needed and keep the existing
logic that proceeds when cliImage is present.
In `@pkg/release-controller/release.go`:
- Around line 235-247: The hashing functions (hashReferenceSpecTags and
hashStatusTags) are non-deterministic because they iterate is.Spec.Tags /
is.Status.Tags in API order; sort the tag slice by tag name before computing the
sha256 to ensure stable hashes. Update hashReferenceSpecTags to copy/sort
is.Spec.Tags (compare tag.Name) and then write tag.From.Name into the hasher in
sorted order, and make the analogous change in hashStatusTags so both functions
produce deterministic hashes regardless of API ordering.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e64b86d-31a4-44a7-a661-5d7f5a69fe9b
📒 Files selected for processing (25)
artifacts/release.openshift.io_releasepayloads.yamlcmd/release-controller-api/http.gocmd/release-controller-api/http_compare.gocmd/release-controller-api/http_helper.gocmd/release-controller-api/http_helper_test.gocmd/release-controller-api/http_upgrade_graph.gocmd/release-controller/audit.gocmd/release-controller/jira.gocmd/release-controller/legacy_results.gocmd/release-controller/sync.gocmd/release-controller/sync_mirror.gocmd/release-controller/sync_release.gocmd/release-controller/sync_release_payload.gocmd/release-controller/sync_release_payload_test.gocmd/release-controller/sync_release_reference_test.gocmd/release-controller/sync_tags.gocmd/release-controller/sync_upgrade.gocmd/release-controller/sync_verify.gocmd/release-controller/sync_verify_prow.gocmd/release-controller/sync_verify_prow_test.gopkg/apis/release/v1alpha1/types.gopkg/release-controller/release.gopkg/release-controller/release_test.gopkg/release-controller/types.gopkg/release-controller/types_test.go
AlexNPavel
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@AlexNPavel: This pull request references OCPCRT-459 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. |
|
@AlexNPavel: This pull request references OCPCRT-459 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/release-controller/sync_tags.go (1)
177-180: Consider adding a TODO for the discussed cleanup strategy.Per the PR objectives, when CLI image determination fails, the function proceeds with tag deletion without creating removal tags. This may prevent QCI pruner cleanup for those releases. The current behavior is intentional but flagged for discussion.
📝 Suggested improvement
if len(cliImage) == 0 { - klog.Warningf("Unable to determine CLI image for reference removal tags, skipping removal tagging") + // TODO(OCPCRT-459): Discuss cleanup strategy for releases where removal tags cannot be created. + // Currently proceeds with deletion, which may leave orphaned payloads in ReferenceRepository. + klog.Warningf("Unable to determine CLI image for reference removal tags, skipping removal tagging for release %s", release.Config.Name) return true, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_tags.go` around lines 177 - 180, The early return when cliImage is empty in the function containing the cliImage check (the block that logs "Unable to determine CLI image for reference removal tags, skipping removal tagging") can leave releases without removal tags and thus not collectible by the QCI pruner; add a TODO comment next to this if len(cliImage) == 0 branch that explains the intended cleanup strategy and the trade-off (e.g., whether to proceed with deletion without tagging or to fail/queue for retry), referencing the cliImage variable and this conditional branch so future maintainers can follow up and implement a cleanup/ retry mechanism or alternative tagging approach.cmd/release-controller/sync_release_reference_test.go (1)
15-19: Remove unusedexpectSkipfield from test struct.The
expectSkipfield is declared but never assigned or used in any test case.♻️ Suggested cleanup
testCases := []struct { name string release *releasecontroller.Release expectTags int - expectSkip int }{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_release_reference_test.go` around lines 15 - 19, The test struct in sync_release_reference_test.go declares an unused field expectSkip; remove the expectSkip field from the testCases struct definition and delete any unused references to it in tests (e.g., in the testCases variable and any assertions), leaving only the used fields like name, release, and expectTags so the test struct matches actual usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/release-controller/sync_release_reference_test.go`:
- Around line 15-19: The test struct in sync_release_reference_test.go declares
an unused field expectSkip; remove the expectSkip field from the testCases
struct definition and delete any unused references to it in tests (e.g., in the
testCases variable and any assertions), leaving only the used fields like name,
release, and expectTags so the test struct matches actual usage.
In `@cmd/release-controller/sync_tags.go`:
- Around line 177-180: The early return when cliImage is empty in the function
containing the cliImage check (the block that logs "Unable to determine CLI
image for reference removal tags, skipping removal tagging") can leave releases
without removal tags and thus not collectible by the QCI pruner; add a TODO
comment next to this if len(cliImage) == 0 branch that explains the intended
cleanup strategy and the trade-off (e.g., whether to proceed with deletion
without tagging or to fail/queue for retry), referencing the cliImage variable
and this conditional branch so future maintainers can follow up and implement a
cleanup/ retry mechanism or alternative tagging approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cb10ffd7-a6ad-4c06-ba17-49fce226d744
📒 Files selected for processing (2)
cmd/release-controller/sync_release_reference_test.gocmd/release-controller/sync_tags.go
- audit: resolve digest via GetImageInfo when local imagestream status is empty (reference tags are not imported, so FindImageIDForTag returns empty) - jira: reject malformed pull specs starting with ":" that slip past the empty-length check when PublicDockerImageRepository is unset - sync_release_payload: use IsReferenceRelease instead of HasReferenceSpecTags for payloadType so it matches the ReleaseCoordinates check - legacy_results: align payloadType detection with sync_release_payload by using IsReferenceRelease, preventing a nil dereference when release.Source is nil - release: sort tags by name before hashing in hashStatusTags and hashReferenceSpecTags for deterministic output across API calls - sync_release: validate mirror.yaml is non-empty before passing it to oc adm release new in the reference release job - tests: add explicit PayloadTypeLocal assertions, add DisableManifestListMode override test
|
Changes based on the coderabbit comments have been resolved |
|
@AlexNPavel: This pull request references OCPCRT-459 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. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/release-controller/release.go (1)
94-105:⚠️ Potential issue | 🟠 MajorLet reference-backed streams bypass the source registry gate.
Lines 94-97 still require
is.Status.PublicDockerImageRepositorybefore the new reference-tag path runs. For reference releases, the controller now builds fromspec.tagsplusReferenceRelease, so a stream with no public imagestream repo will sit in "waiting" forever even though the payload is otherwise resolvable.Possible fix
- if is.Status.PublicDockerImageRepository == "" { + if is.Status.PublicDockerImageRepository == "" && !(cfg.ReferenceRelease != nil && HasReferenceSpecTags(is)) { klog.V(4).Infof("The release input has no public docker image repository, waiting") return nil, false, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/release.go` around lines 94 - 105, The code currently returns early when is.Status.PublicDockerImageRepository == "" which blocks reference-backed streams; change the logic so the public registry gate is skipped for reference-based releases by checking HasReferenceSpecTags(is) (and that is.Spec.Tags is non-empty) before returning. In practice, update the early check around is.Status.PublicDockerImageRepository to: if PublicDockerImageRepository is empty AND NOT (HasReferenceSpecTags(is) && len(is.Spec.Tags) > 0) then log and return; otherwise allow processing of spec.tags/ReferenceRelease paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/release-controller/jira.go`:
- Around line 245-246: prevPullSpec is being built from the current release
instead of the prior one; change the call to ReleasePullSpec to use the previous
release object loaded for the prior tag (the variable that corresponds to the
previous release you obtained when reading verifyIssues.PreviousReleaseTag),
e.g. replace ReleasePullSpec(release, prevTag.Name) with
ReleasePullSpec(previousRelease, prevTag.Name) (or the actual identifier used
when you fetched the previous release) so prevPullSpec is resolved from the
previous release's PullRepository rather than the current release.
In `@cmd/release-controller/sync_release.go`:
- Around line 68-85: The code builds toImage from
release.Config.ReferenceRelease.PushRepository without validating it's
non-empty, which yields an invalid image; add a check after confirming
ReferenceRelease is non-nil (before constructing toImage) to return an error if
release.Config.ReferenceRelease.PushRepository is empty, e.g. validate
release.Config.ReferenceRelease.PushRepository and return fmt.Errorf("release %q
ReferenceRelease.PushRepository is empty", name) so the function fails fast and
avoids creating a broken job.
In `@pkg/release-controller/release.go`:
- Around line 212-216: The current HashSpecTagImageDigests uses
HasReferenceSpecTags (an any-match) to choose between hashReferenceSpecTags and
hashStatusTags, which causes mixed streams with one Reference=true tag to ignore
Status.Tags and miss changes; update HashSpecTagImageDigests to compute a
combined hash that accounts per-tag on whether a tag is reference-spec (use
hashReferenceSpecTags logic for those tags) and falls back to status-based
hashing for non-reference tags (use hashStatusTags logic for those tags) so that
both Spec and Status contributions are considered; locate and modify
HashSpecTagImageDigests (and the similar code paths around the other occurrences
noted) to iterate tags and apply the correct per-tag hashing instead of
switching the whole stream based on HasReferenceSpecTags.
- Around line 630-635: ReleasePullSpec can return a malformed string like
":{tag}" when the repository field is empty; update ReleasePullSpec so it
validates the chosen repository before formatting: in the IsReferenceRelease
branch check release.Config.ReferenceRelease.PullRepository and in the normal
branch check release.Target.Status.PublicDockerImageRepository, and if the
selected repo is empty return an empty string (or propagate an error) instead of
constructing ":<tag>"; keep using ReferencePayloadTag(tagName) and the existing
formatting when repo is present.
---
Outside diff comments:
In `@pkg/release-controller/release.go`:
- Around line 94-105: The code currently returns early when
is.Status.PublicDockerImageRepository == "" which blocks reference-backed
streams; change the logic so the public registry gate is skipped for
reference-based releases by checking HasReferenceSpecTags(is) (and that
is.Spec.Tags is non-empty) before returning. In practice, update the early check
around is.Status.PublicDockerImageRepository to: if PublicDockerImageRepository
is empty AND NOT (HasReferenceSpecTags(is) && len(is.Spec.Tags) > 0) then log
and return; otherwise allow processing of spec.tags/ReferenceRelease paths.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b37c6582-90c7-4e8c-a575-a75d1a9d95a3
📒 Files selected for processing (8)
cmd/release-controller/audit.gocmd/release-controller/jira.gocmd/release-controller/legacy_results.gocmd/release-controller/sync_release.gocmd/release-controller/sync_release_payload.gocmd/release-controller/sync_release_payload_test.gocmd/release-controller/sync_release_reference_test.gopkg/release-controller/release.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/release-controller/audit.go
| func HashSpecTagImageDigests(is *imagev1.ImageStream) string { | ||
| if HasReferenceSpecTags(is) { | ||
| return hashReferenceSpecTags(is) | ||
| } | ||
| return hashStatusTags(is) |
There was a problem hiding this comment.
Don't switch the whole hash to spec-only on a mixed stream.
HasReferenceSpecTags is an any-match check, so one Reference=true tag moves the entire digest calculation to hashReferenceSpecTags. That path ignores Status.Tags, which means imported/non-reference tags can change without changing the hash and release regeneration can be skipped incorrectly.
Also applies to: 241-251, 279-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/release-controller/release.go` around lines 212 - 216, The current
HashSpecTagImageDigests uses HasReferenceSpecTags (an any-match) to choose
between hashReferenceSpecTags and hashStatusTags, which causes mixed streams
with one Reference=true tag to ignore Status.Tags and miss changes; update
HashSpecTagImageDigests to compute a combined hash that accounts per-tag on
whether a tag is reference-spec (use hashReferenceSpecTags logic for those tags)
and falls back to status-based hashing for non-reference tags (use
hashStatusTags logic for those tags) so that both Spec and Status contributions
are considered; locate and modify HashSpecTagImageDigests (and the similar code
paths around the other occurrences noted) to iterate tags and apply the correct
per-tag hashing instead of switching the whole stream based on
HasReferenceSpecTags.
There was a problem hiding this comment.
@deepsm007 Is this something we need to worry about? The code currently assumes that if any tag in a stream has reference: true, then we should treat is like a reference-based release, meaning that the mirror snapshot we create is based on just the spec tags and the release itself is created from an imagestream file that contains just the spec tags. This would mean that images pushed directly to the stream would not be included.
If we need to handle imagestreams where some images are references and some are pushed, this become a lot more complicated.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
- Use previous release's ReleaseDefinition for prevPullSpec in jira so reference-based releases resolve pull specs from the correct imagestream instead of the current release's - Skip PublicDockerImageRepository gate for reference releases that have ReferenceRelease config and reference spec tags - Validate PullRepository/PushRepository are non-empty before using them to construct pull specs and job image references
|
@AlexNPavel: This pull request references OCPCRT-459 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pkg/release-controller/release_test.go (1)
98-106: Consider parallel subtests for faster execution.The outer
t.Parallel()is called, but individual subtests don't capture the loop variable or callt.Parallel(). In Go 1.22+, the loop variable capture issue is resolved, but for older versions or explicit clarity, capturing the variable is safer.♻️ Suggested pattern for parallel subtests
t.Parallel() for _, tc := range testCases { + tc := tc // capture for Go < 1.22 t.Run(tc.name, func(t *testing.T) { + t.Parallel() actual := IsReferenceRelease(tc.release)Same pattern applies to
TestReleasePullSpecat lines 258-266.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/release_test.go` around lines 98 - 106, The subtests should capture the loop variable and run in parallel: inside each t.Run callback, add "tc := tc" to capture the current loop value and call t.Parallel() at the start of the subtest; apply this change to the table-driven tests that call IsReferenceRelease (the loop using variable tc) and likewise to TestReleasePullSpec's subtests so each subtest runs independently and in parallel.cmd/release-controller/sync_release_reference_test.go (1)
306-308: Annotation assertion may have incorrect expected value.The test checks
ReleaseAnnotationReleaseTagagainstreleaseName, but the mirror's annotation (line 218) is set to"4.17.0-0.nightly-2024-08-30-110931"which equalsreleaseName. This happens to pass, but the assertion reads as if it expects the job's annotation to matchreleaseNamedirectly rather than verifying the annotation was copied from the mirror. Consider clarifying the intent.♻️ Clearer assertion
- if job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag] != releaseName { - t.Errorf("expected releaseTag annotation %q, got %q", releaseName, job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag]) + expectedReleaseTag := mirror.Annotations[releasecontroller.ReleaseAnnotationReleaseTag] + if job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag] != expectedReleaseTag { + t.Errorf("expected releaseTag annotation %q, got %q", expectedReleaseTag, job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_release_reference_test.go` around lines 306 - 308, The assertion currently compares job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag] to releaseName which obscures whether the value was copied from the mirror; change the check to compare against the mirror's annotation (mirror.Annotations[releasecontroller.ReleaseAnnotationReleaseTag]) or otherwise rename releaseName to reflect its origin so the intent is explicit; update the t.Errorf message accordingly to reference the mirror-derived expected value and use the symbols job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag], mirror.Annotations[releasecontroller.ReleaseAnnotationReleaseTag], and releaseName to locate the change.pkg/release-controller/release.go (1)
94-94: Apply De Morgan's law for clarity.The nested negation makes the condition harder to read. Static analysis (golangci-lint QF1001) suggests simplifying.
♻️ Suggested simplification
- if is.Status.PublicDockerImageRepository == "" && !(cfg.ReferenceRelease != nil && HasReferenceSpecTags(is) && len(is.Spec.Tags) > 0) { + if is.Status.PublicDockerImageRepository == "" && (cfg.ReferenceRelease == nil || !HasReferenceSpecTags(is) || len(is.Spec.Tags) == 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/release.go` at line 94, The condition uses a negated conjunction which is hard to read; replace the negation by applying De Morgan so the second clause becomes an OR of negations. Specifically, change the check that currently negates (cfg.ReferenceRelease != nil && HasReferenceSpecTags(is) && len(is.Spec.Tags) > 0) into (cfg.ReferenceRelease == nil || !HasReferenceSpecTags(is) || len(is.Spec.Tags) == 0) while keeping the leading check on is.Status.PublicDockerImageRepository == ""; references to update: is.Status.PublicDockerImageRepository, cfg.ReferenceRelease, HasReferenceSpecTags(is), and is.Spec.Tags.cmd/release-controller/sync_release.go (1)
96-105: Shell script for status stripping may be fragile.The
awkcommand assumesstatus:appears at the start of a line and that subsequent indented lines belong to it. This works for standardoc get -o yamloutput but could break with non-standard formatting or if the YAML contains a field namedstatusat a nested level.Consider using
yqor a similar tool for more robust YAML manipulation if available in the image, or document this assumption.# Alternative using yq (if available): # yq 'del(.status)' /tmp/mirror-full.yaml > /tmp/mirror.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_release.go` around lines 96 - 105, Replace the fragile awk-based status stripping in job.Spec.Template.Spec.Containers[0].Command with a robust YAML operation: use yq to delete the top-level status field from /tmp/mirror-full.yaml and write the result to /tmp/mirror.yaml (i.e. remove only .status), and if yq is not present add a clear inline comment documenting the dependency and fallback behavior; if keeping a fallback awk, make it explicitly match only a top-level "status:" line and its immediately indented block so nested "status" keys are not removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/release-controller/sync_release_reference_test.go`:
- Around line 306-308: The assertion currently compares
job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag] to releaseName
which obscures whether the value was copied from the mirror; change the check to
compare against the mirror's annotation
(mirror.Annotations[releasecontroller.ReleaseAnnotationReleaseTag]) or otherwise
rename releaseName to reflect its origin so the intent is explicit; update the
t.Errorf message accordingly to reference the mirror-derived expected value and
use the symbols job.Annotations[releasecontroller.ReleaseAnnotationReleaseTag],
mirror.Annotations[releasecontroller.ReleaseAnnotationReleaseTag], and
releaseName to locate the change.
In `@cmd/release-controller/sync_release.go`:
- Around line 96-105: Replace the fragile awk-based status stripping in
job.Spec.Template.Spec.Containers[0].Command with a robust YAML operation: use
yq to delete the top-level status field from /tmp/mirror-full.yaml and write the
result to /tmp/mirror.yaml (i.e. remove only .status), and if yq is not present
add a clear inline comment documenting the dependency and fallback behavior; if
keeping a fallback awk, make it explicitly match only a top-level "status:" line
and its immediately indented block so nested "status" keys are not removed.
In `@pkg/release-controller/release_test.go`:
- Around line 98-106: The subtests should capture the loop variable and run in
parallel: inside each t.Run callback, add "tc := tc" to capture the current loop
value and call t.Parallel() at the start of the subtest; apply this change to
the table-driven tests that call IsReferenceRelease (the loop using variable tc)
and likewise to TestReleasePullSpec's subtests so each subtest runs
independently and in parallel.
In `@pkg/release-controller/release.go`:
- Line 94: The condition uses a negated conjunction which is hard to read;
replace the negation by applying De Morgan so the second clause becomes an OR of
negations. Specifically, change the check that currently negates
(cfg.ReferenceRelease != nil && HasReferenceSpecTags(is) && len(is.Spec.Tags) >
0) into (cfg.ReferenceRelease == nil || !HasReferenceSpecTags(is) ||
len(is.Spec.Tags) == 0) while keeping the leading check on
is.Status.PublicDockerImageRepository == ""; references to update:
is.Status.PublicDockerImageRepository, cfg.ReferenceRelease,
HasReferenceSpecTags(is), and is.Spec.Tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 510062de-7946-4892-be06-d62c2e89ba5d
📒 Files selected for processing (5)
cmd/release-controller/jira.gocmd/release-controller/sync_release.gocmd/release-controller/sync_release_reference_test.gopkg/release-controller/release.gopkg/release-controller/release_test.go
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
|
@AlexNPavel: This pull request references OCPCRT-459 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/release-controller/release.go (1)
212-216:⚠️ Potential issue | 🟠 MajorMixed streams still hash incorrectly.
HasReferenceSpecTagsis still an any-match check, so a singleReference=truetag switches the entire digest tohashReferenceSpecTagsand ignores imported tags inStatus.Tags. Changes in those non-reference tags can still be missed, which skips regeneration on mixed streams.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/release.go` around lines 212 - 216, HashSpecTagImageDigests currently treats HasReferenceSpecTags as exclusive and returns hashReferenceSpecTags for any stream with at least one Reference=true tag, which ignores imported tags in Status.Tags and misses changes for mixed streams; update HashSpecTagImageDigests so that when HasReferenceSpecTags(is) is true it computes a deterministic combined hash that includes both hashReferenceSpecTags(is) and hashStatusTags(is) (e.g. concatenate the two digests and re-hash or mix them deterministically) instead of returning only hashReferenceSpecTags, ensuring changes in imported Status.Tags are detected for mixed streams.
🧹 Nitpick comments (1)
cmd/release-controller/sync_release_payload_test.go (1)
821-874: Avoid testing an impossiblePayloadType/release combination.This case passes
PayloadTypeReferencefor a release with no reference tags orReferenceReleaseconfig, whilenewReleasePayloadstill derivesReleaseCoordinatesfromIsReferenceRelease(release). That bakes an internally inconsistent payload shape into the tests instead of checking the real invariant. Prefer derivingpayloadTypefromrelease, or make this fixture a real reference release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_release_payload_test.go` around lines 821 - 874, The test constructs an impossible combination: it sets payloadType to v1alpha1.PayloadTypeReference while the test release has no reference tags or ReferenceRelease config, but newReleasePayload calls IsReferenceRelease(release) to build ReleaseCoordinates; fix by either deriving payloadType from the release (call IsReferenceRelease(release) and set payloadType accordingly) or make the test fixture a real reference release (add a ReferenceRelease config or the expected reference tag to the release object) so PayloadTypeReference matches the release; update the test case that uses the release variable, payloadType, and expected ReleasePayload to remain internally consistent with newReleasePayload/IsReferenceRelease behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/release-controller/release.go`:
- Around line 99-105: The current branch treats any imagestream with spec tags
marked reference (HasReferenceSpecTags(is)) as ready when Status.Tags is empty;
change this to only skip the status-tag wait for fully configured reference
releases by also verifying the controller's reference release configuration is
present (e.g. check r.cfg.ReferenceRelease != nil or cfg.ReferenceRelease != nil
depending on receiver naming). Update the condition that reads
"len(is.Status.Tags) == 0 && len(is.Spec.Tags) > 0 && HasReferenceSpecTags(is)"
to require the non-nil reference-release config (and keep the existing log
message for the reference path), so misconfigured sources without a configured
ReferenceRelease will still return nil, false, nil and wait as before.
---
Duplicate comments:
In `@pkg/release-controller/release.go`:
- Around line 212-216: HashSpecTagImageDigests currently treats
HasReferenceSpecTags as exclusive and returns hashReferenceSpecTags for any
stream with at least one Reference=true tag, which ignores imported tags in
Status.Tags and misses changes for mixed streams; update HashSpecTagImageDigests
so that when HasReferenceSpecTags(is) is true it computes a deterministic
combined hash that includes both hashReferenceSpecTags(is) and
hashStatusTags(is) (e.g. concatenate the two digests and re-hash or mix them
deterministically) instead of returning only hashReferenceSpecTags, ensuring
changes in imported Status.Tags are detected for mixed streams.
---
Nitpick comments:
In `@cmd/release-controller/sync_release_payload_test.go`:
- Around line 821-874: The test constructs an impossible combination: it sets
payloadType to v1alpha1.PayloadTypeReference while the test release has no
reference tags or ReferenceRelease config, but newReleasePayload calls
IsReferenceRelease(release) to build ReleaseCoordinates; fix by either deriving
payloadType from the release (call IsReferenceRelease(release) and set
payloadType accordingly) or make the test fixture a real reference release (add
a ReferenceRelease config or the expected reference tag to the release object)
so PayloadTypeReference matches the release; update the test case that uses the
release variable, payloadType, and expected ReleasePayload to remain internally
consistent with newReleasePayload/IsReferenceRelease behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 181f7870-917f-40f7-be51-1f9d385b26df
📒 Files selected for processing (2)
cmd/release-controller/sync_release_payload_test.gopkg/release-controller/release.go
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@AlexNPavel: This pull request references OCPCRT-459 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/release-controller/release.go (1)
616-641: LGTM! Malformed pull spec issue is addressed.The function now correctly returns
""when the required repository is missing, preventing callers from receiving malformed specs like":<tag>".Optional: Consider adding a nil-guard for
release.Targetin the non-reference branch. IfIsReferenceReleasereturns false (e.g., whenrelease.Sourceis nil), andrelease.Targetis also nil, line 637 would panic.🛡️ Optional defensive nil-check
func ReleasePullSpec(release *Release, tagName string) string { if IsReferenceRelease(release) { if release.Config.ReferenceRelease.PullRepository == "" { return "" } return fmt.Sprintf("%s:%s", release.Config.ReferenceRelease.PullRepository, ReferencePayloadTag(tagName)) } + if release.Target == nil { + return "" + } if release.Target.Status.PublicDockerImageRepository == "" { return "" } return release.Target.Status.PublicDockerImageRepository + ":" + tagName }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/release-controller/release.go` around lines 616 - 641, The non-reference branch of ReleasePullSpec can panic if release.Target is nil; add a defensive nil-check for release.Target before accessing release.Target.Status.PublicDockerImageRepository and return "" when release.Target is nil or PublicDockerImageRepository is empty; update the ReleasePullSpec function to first verify release.Target != nil then the repository string to avoid panics when callers pass releases with nil Target.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/release-controller/release.go`:
- Around line 616-641: The non-reference branch of ReleasePullSpec can panic if
release.Target is nil; add a defensive nil-check for release.Target before
accessing release.Target.Status.PublicDockerImageRepository and return "" when
release.Target is nil or PublicDockerImageRepository is empty; update the
ReleasePullSpec function to first verify release.Target != nil then the
repository string to avoid panics when callers pass releases with nil Target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b97d539-9072-4235-b963-381ba8b4acac
📒 Files selected for processing (2)
cmd/release-controller/sync_release_payload_test.gopkg/release-controller/release.go
|
@AlexNPavel: all tests passed! 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. |
2 participants
Introduce a new "Reference" payload type where release images are created directly in an external repository instead of a local imagestream. This enables release workflows that do not require a cluster-local copy of the payload.
Key changes:
Summary by CodeRabbit
New Features
Refactor
Tests