fix: spec-first official ocp input import for 4.11–4.22#5217
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds official OCP image resolution helpers, uses them to compute ImageStreamTag From/reference policy/source pull spec in InputImageTagStep, reuses source resolution in snapshotStream, and adds unit tests plus an e2e release-version update. ChangesOfficial image resolution and reference policy handling
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels:
Suggested reviewers:
🚥 Pre-merge checks | ✅ 14 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/steps/input_image_tag_test.go (1)
255-309: 💤 Low valueConsider adding
examineStepto verifyInputs()return value for stable-first scenario.Unlike
TestInputImageTagStepOfficialSpecandTestInputImageTagStepLegacyStream, this test doesn't callexamineStepto validate theInputs()behavior. For stable-first resolution,Inputs()returns"target-namespace/stable:cli"as the input definition—verifying this matches expectations would strengthen coverage.🤖 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 `@pkg/steps/input_image_tag_test.go` around lines 255 - 309, TestInputImageTagStepStableFirst is missing a call to examineStep to assert the Inputs() return for the stable-first case; add an examineStep invocation against the created step (iits) before executeStep that checks iits.Inputs() (or Inputs()) returns the expected input definition "target-namespace/stable:cli" (matching jobspec.Namespace() + "/stable:cli") so this test validates the Inputs() behavior like the other tests (e.g., TestInputImageTagStepOfficialSpec).pkg/steps/input_image_tag.go (1)
86-111: 💤 Low valueConsider caching all
resolveOfficialImportresults to avoid duplicate API calls.
resolveOfficialImportis called in bothInputs()(line 64) andrun()(line 107). Each call invokesResolveOfficialInputFrom, which performsGetoperations against the cluster for stable/source ImageStreams. SinceInputs()only storespullSpecins.imageName,run()must call again to getfromandrefPolicy.Could cache all four return values in struct fields during the first call.
Also applies to: 157-171
🤖 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 `@pkg/steps/input_image_tag.go` around lines 86 - 111, Cache the results of resolveOfficialImport to avoid duplicate cluster API calls: add struct fields on the step (e.g., cachedFrom *coreapi.ObjectReference, cachedRefPolicy imagev1.TagReferencePolicyType, cachedSourcePullSpec string, cachedResolveErr error) and change Inputs() and run() to call a small helper (or resolveOfficialImport wrapper) that returns the cached values if present, otherwise calls resolveOfficialImport once, stores from/refPolicy/sourcePullSpec/error into those fields (and sets s.imageName = sourcePullSpec if needed) and returns them; update both calls (resolveOfficialImport usage in Inputs() and in run(), and the other occurrence around the 157-171 region) to use the cached fields so subsequent invocations reuse the stored from/refPolicy/sourcePullSpec instead of performing additional Get operations.
🤖 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 `@pkg/steps/release/snapshot.go`:
- Around line 85-91: Add unit tests covering the refactored source-resolution
branches around the source imagestream handling: create table-driven tests for
the code path that checks api.IsCreatedForClusterBotJob(sourceNamespace) (test
both true — where no client.Get call should occur and source remains nil — and
false — where client.Get is invoked and you assert success and failure
behaviors), plus tests exercising the prefetch vs lazy-fetch and the
consolidated official-resolution behavior that follow this block; target the
function containing the source variable and client.Get call (the snapshot/stream
resolution code in pkg/steps/release/snapshot.go) and use a fake
ctrlruntimeclient to simulate Get returning the ImageStream or an error so
regression cases fail without the fix.
---
Nitpick comments:
In `@pkg/steps/input_image_tag_test.go`:
- Around line 255-309: TestInputImageTagStepStableFirst is missing a call to
examineStep to assert the Inputs() return for the stable-first case; add an
examineStep invocation against the created step (iits) before executeStep that
checks iits.Inputs() (or Inputs()) returns the expected input definition
"target-namespace/stable:cli" (matching jobspec.Namespace() + "/stable:cli") so
this test validates the Inputs() behavior like the other tests (e.g.,
TestInputImageTagStepOfficialSpec).
In `@pkg/steps/input_image_tag.go`:
- Around line 86-111: Cache the results of resolveOfficialImport to avoid
duplicate cluster API calls: add struct fields on the step (e.g., cachedFrom
*coreapi.ObjectReference, cachedRefPolicy imagev1.TagReferencePolicyType,
cachedSourcePullSpec string, cachedResolveErr error) and change Inputs() and
run() to call a small helper (or resolveOfficialImport wrapper) that returns the
cached values if present, otherwise calls resolveOfficialImport once, stores
from/refPolicy/sourcePullSpec/error into those fields (and sets s.imageName =
sourcePullSpec if needed) and returns them; update both calls
(resolveOfficialImport usage in Inputs() and in run(), and the other occurrence
around the 157-171 region) to use the cached fields so subsequent invocations
reuse the stored from/refPolicy/sourcePullSpec instead of performing additional
Get operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 97408c05-2078-4eed-9b7d-c2c29be2a48b
📒 Files selected for processing (5)
pkg/steps/input_image_tag.gopkg/steps/input_image_tag_test.gopkg/steps/release/snapshot.gopkg/steps/utils/image.gopkg/steps/utils/image_test.go
|
/hold for dependent PR to merge |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/lgtm |
|
Scheduling tests matching the |
|
/test e2e |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
deepsm007
force-pushed
the
fix/spec-first-official-ocp-import
branch
from
dc4401b to
9884a79
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/steps/input_image_tag.go (1)
154-168: 💤 Low valueConsider wrapping the error for better context.
When
ResolveOfficialInputFromfails, wrapping the error here provides immediate context about which base image was being resolved.Proposed fix
func (s *inputImageTagStep) resolveOfficialImport(ctx context.Context) (*coreapi.ObjectReference, imagev1.TagReferencePolicyType, string, error) { from, ok, err := utils.ResolveOfficialInputFrom(ctx, s.client, s.jobSpec.Namespace(), s.config.BaseImage) if err != nil { - return nil, imagev1.SourceTagReferencePolicy, "", err + return nil, imagev1.SourceTagReferencePolicy, "", fmt.Errorf("resolving %s: %w", s.config.BaseImage.ISTagName(), err) }🤖 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 `@pkg/steps/input_image_tag.go` around lines 154 - 168, In resolveOfficialImport, wrap the error returned from utils.ResolveOfficialInputFrom to add context about which base image failed to resolve (include s.config.BaseImage and/or namespace); replace the bare return of err with a wrapped error (e.g. using fmt.Errorf("resolve official input for base image %s: %w", s.config.BaseImage, err) or errors.Wrap) so callers of resolveOfficialImport can see which image caused the failure.
🤖 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 `@pkg/steps/input_image_tag.go`:
- Around line 104-107: The call to s.resolveOfficialImport(...) returns an error
that is currently returned raw; update the error handling to wrap the error with
contextual information (e.g., include the import attempt and relevant
identifiers) before returning, matching the wrapping style used elsewhere in
this file (see other returns around lines 80/125/135); locate the call to
s.resolveOfficialImport in the InputImageTag step and replace the plain "return
err" with a wrapped error (using the same error-wrapping utility/pattern used in
this package) that mentions resolveOfficialImport and the image/import being
resolved.
---
Nitpick comments:
In `@pkg/steps/input_image_tag.go`:
- Around line 154-168: In resolveOfficialImport, wrap the error returned from
utils.ResolveOfficialInputFrom to add context about which base image failed to
resolve (include s.config.BaseImage and/or namespace); replace the bare return
of err with a wrapped error (e.g. using fmt.Errorf("resolve official input for
base image %s: %w", s.config.BaseImage, err) or errors.Wrap) so callers of
resolveOfficialImport can see which image caused the failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 91703885-88d2-446a-b555-8ae10270c9d6
📒 Files selected for processing (5)
pkg/steps/input_image_tag.gopkg/steps/input_image_tag_test.gopkg/steps/release/snapshot.gopkg/steps/utils/image.gopkg/steps/utils/image_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/steps/utils/image.go
- pkg/steps/utils/image_test.go
- pkg/steps/release/snapshot.go
- pkg/steps/input_image_tag_test.go
deepsm007
force-pushed
the
fix/spec-first-official-ocp-import
branch
from
9884a79 to
84bb536
Compare
|
/test e2e |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
deepsm007
force-pushed
the
fix/spec-first-official-ocp-import
branch
from
84bb536 to
3a4eff9
Compare
deepsm007
force-pushed
the
fix/spec-first-official-ocp-import
branch
from
3a4eff9 to
e6dd6dc
Compare
|
@deepsm007: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bradmwilliams, danilo-gemoli, deepsm007, hector-vido 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 |
|
/test e2e |
|
/override ci/prow/images |
|
@deepsm007: Overrode contexts on behalf of deepsm007: ci/prow/images 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 kubernetes-sigs/prow repository. |
4 participants
Official
ocp/4.11–4.22inputs resolve fromstable:in the job ns, then spec/status@sha256on the source IS, withLocalTagReferencePolicyfor pipeline tags. Legacy streams (4.23, 5.0) keep quay-proxy + Source policy.https://prow.ci.openshift.org/view/gs/test-platform-results/logs/openshift-api-2854-openshift-installer-10566-openshift-origin-31211-openshift-machine-config-operator-6076-ci-5.0-e2e-aws-ovn-techpreview-serial-3of3/2060029597415641088#1:build-log.txt%3A150-157
dependent on #5216
/hold
Overview
This PR changes how ci-operator resolves and imports official OpenShift Container Platform (OCP) base images for versions 4.11–4.22 to a spec-first flow: prefer a job-namespace stable: ImageStream, otherwise prefer the source ImageStream spec/status digest (
@sha256), and create pipeline ImageStreamTags that reference the digest (LocalTagReferencePolicy). Legacy streams (4.23 and 5.0) remain on the existing quay-proxy SourceTagReferencePolicy. The change depends on PR#5216.Affected component and runtime behavior
@sha256) when available.Key implementation notes
Practical impact for CI users and operators
#5216alongside this change to ensure the new resolution path functions as intended.Meta / workflow notes
#5216) and later unheld.