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 |
📝 WalkthroughWalkthroughThis PR introduces official OCP image resolution utilities and integrates them into the InputImageTagStep, then applies similar optimizations to SnapshotStream. Reference policy (source vs. local tag) is now computed per-case and applied consistently. All three affected steps now support consolidated Quay promotion semantics with stable image precedence. ChangesOfficial image resolution and reference policy handling
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (13 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
| var source *imagev1.ImageStream | ||
| if !api.IsCreatedForClusterBotJob(sourceNamespace) { | ||
| source = &imagev1.ImageStream{} | ||
| if err := client.Get(ctx, ctrlruntimeclient.ObjectKey{Namespace: sourceNamespace, Name: sourceName}, source); err != nil { | ||
| return nil, fmt.Errorf("could not resolve source imagestream %s/%s for release %s: %w", sourceNamespace, sourceName, targetRelease, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add regression coverage for the refactored source-resolution branches.
The logic at Line 85 through Line 117 changed control flow (prefetch vs lazy fetch + consolidated official resolution), but no corresponding snapshot-stream test changes are present here. Please add/point to tests that cover these branches to prevent silent behavior drift.
As per coding guidelines, "New or modified functionality should include test coverage. New Go functions and methods should have corresponding unit tests. Bug fixes should include a regression test that fails without the fix. Pure functions should always be tested. Prefer table-driven tests."
Also applies to: 98-103, 109-117
🤖 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/release/snapshot.go` around lines 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.
|
/hold for dependent PR to merge |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, deepsm007 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 |
|
Scheduling tests matching the |
|
@deepsm007: 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. |
|
/test e2e |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
2 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