multi_stage: make release:latest dependency conditional on config#5015
multi_stage: make release:latest dependency conditional on config#5015raelga wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: raelga The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughConditionally mark release payloads as needed for cluster profiles only when either Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
There was a problem hiding this comment.
Pull request overview
Adjusts ci-operator’s multi-stage dependency graph so cluster_profile tests only depend on release:latest when a latest release is actually configured, avoiding graph resolution failures for credential-only jobs.
Changes:
- Make
multiStageTestStep.Requires()addRELEASE_IMAGE_latest/IMAGE_FORMATdependencies only whenconfig.Releases["latest"]exists. - Update
TestRequiresto covercluster_profilewith and withoutreleases.latest.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/steps/multi_stage/multi_stage.go | Makes release:latest/IMAGE_FORMAT dependencies conditional based on presence of releases.latest. |
| pkg/steps/multi_stage/multi_stage_test.go | Updates/adds Requires() unit tests for the new conditional dependency behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/steps/multi_stage/multi_stage_test.go (1)
52-59: Add a credential-only cluster-profile test case to lock in the reported regression.This new case still includes
From: "from-release", so it validates fallback toReleaseImagesLink, not the “cluster_profile only, no release dependency” scenario from the PR motivation.Suggested test addition
@@ }, { name: "step has a cluster profile without releases configured, should not require release payload", steps: api.MultiStageTestConfigurationLiteral{ ClusterProfile: api.ClusterProfileAWS, Test: []api.LiteralTestStep{{From: "from-release"}}, }, req: []api.StepLink{ api.ReleaseImagesLink(api.LatestReleaseName), }, + }, { + name: "cluster profile only and no releases configured should not require release links", + steps: api.MultiStageTestConfigurationLiteral{ + ClusterProfile: api.ClusterProfileAWS, + }, + req: nil, }, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/multi_stage/multi_stage_test.go` around lines 52 - 59, The new test case currently named "step has a cluster profile without releases configured, should not require release payload" still sets a test step From: "from-release" and therefore asserts fallback to ReleaseImagesLink; instead add (or replace with) a credential-only cluster-profile test where api.MultiStageTestConfigurationLiteral has ClusterProfile: api.ClusterProfileAWS and a Test step that does NOT include From (remove From: "from-release"), and set req to an empty slice (no api.ReleaseImagesLink). Ensure the test uses the same file's test harness (multi_stage_test.go) and types (api.LiteralTestStep, api.StepLink) so it verifies the “cluster_profile only, no release dependency” behavior.
🤖 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/steps/multi_stage/multi_stage_test.go`:
- Around line 52-59: The new test case currently named "step has a cluster
profile without releases configured, should not require release payload" still
sets a test step From: "from-release" and therefore asserts fallback to
ReleaseImagesLink; instead add (or replace with) a credential-only
cluster-profile test where api.MultiStageTestConfigurationLiteral has
ClusterProfile: api.ClusterProfileAWS and a Test step that does NOT include From
(remove From: "from-release"), and set req to an empty slice (no
api.ReleaseImagesLink). Ensure the test uses the same file's test harness
(multi_stage_test.go) and types (api.LiteralTestStep, api.StepLink) so it
verifies the “cluster_profile only, no release dependency” behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f8710e2-6c20-480c-b084-6ef5b82b5150
📒 Files selected for processing (2)
pkg/steps/multi_stage/multi_stage.gopkg/steps/multi_stage/multi_stage_test.go
When a test configures cluster_profile but does not define releases.latest, ci-operator's dependency graph unconditionally requires release:latest, causing the job to fail. This is unnecessary for jobs that only use the profile for cloud credentials without installing an OpenShift cluster. Make the release:latest dependency conditional: only require it when the config explicitly defines a latest release in releases.
When tag_specification (ReleaseTagConfiguration) is set, ci-operator also produces a release:latest image stream. The conditional check must account for this in addition to explicit releases.latest entries.
Add a test case for the scenario where cluster_profile is set without any releases or tag_specification configured, verifying that no release dependencies are required. This covers the credential-only use case (e.g., Azure cleanup jobs using profile just for cloud credentials).
raelga
force-pushed
the
fix/optional-release-for-cluster-profile
branch
from
c8b43a4 to
7e776b9
Compare
There was a problem hiding this comment.
Pull request overview
Updates ci-operator’s multi-stage test dependency calculation so cluster_profile only pulls in release:latest/IMAGE_FORMAT dependencies when the job configuration can actually produce a latest release payload (via releases.latest or tag_specification).
Changes:
- Make
multiStageTestStep.Requires()addRELEASE_IMAGE_latest/IMAGE_FORMATlinks only whenreleases.latestortag_specificationis present. - Update/extend
TestRequiresto covercluster_profilewith: (a)releases.latest, (b) no releases, and (c)tag_specification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/steps/multi_stage/multi_stage.go | Gates needsReleasePayload and profile env-var links on presence of releases.latest or ReleaseTagConfiguration. |
| pkg/steps/multi_stage/multi_stage_test.go | Adds coverage for cluster_profile without releases and for tag_specification behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
Scheduling tests matching the |
|
/test breaking-changes |
|
@raelga: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary
Make the
release:latestdependency in ci-operator's dependency graph conditional on whether the config actually defines alatestrelease, rather than unconditionally requiring it whenevercluster_profileis set.Problem
When a CI test configures
cluster_profilefor cloud credentials but does not definereleases.latest, ci-operator fails with:This is because
multiStageTestStep.Requires()unconditionally addsrelease:latestandIMAGE_FORMATas dependencies whenevercluster_profileis present (multi_stage.go:365-371), assuming every profiled test needs a release payload for cluster installation.Why this matters
Many teams use
cluster_profilesolely for cloud credentials — not for installing OpenShift clusters. For example, ARO-HCP periodic jobs run cleanup tasks (deleting expired Azure resources, Kusto role assignments) that need Azure credentials from the profile but never touch a release payload. Today, these configs are forced to declare areleases.latestthey never use:This adds unnecessary complexity, pulls an unused release payload (wasting resources), and is confusing for teams that don't understand why their credential-only jobs need a release image.
Changes
Test plan