OCPBUGS-93368: Simplify IRI api dependency - #10528
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@andfasano: This pull request references Jira Issue OCPBUGS-93368, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (15)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe PR removes feature-gate-based branching from ignition manifest generation and updates internal release image handling to track manifest presence with ChangesIgnition manifest patching
Sequence Diagram(s)sequenceDiagram
participant installerGenerator
participant patchInternalReleaseManifests
participant iriPatcher
participant updateBootstrap
installerGenerator->>patchInternalReleaseManifests: call
patchInternalReleaseManifests->>iriPatcher: PatchManifests(...)
installerGenerator->>updateBootstrap: call
updateBootstrap->>iriPatcher: UpdateBootstrap(config)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/L
|
@andfasano: This pull request references Jira Issue OCPBUGS-93368, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ignition/internalreleaseimage_patcher.go (1)
147-166: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
iriFoundbefore scanning the current manifests.Line 165 only ever sets the flag to
true; it never clears a previous match. A reused patcher or retry with a different manifest set can still pass the Line 186 and Line 332 gates after the IRI manifest is no longer present.Proposed fix
func (i *internalReleaseImagePatcher) getInternalReleaseImageManifest(ctx context.Context, manifestFiles []s3wrapper.ObjectInfo) error { + i.iriFound = false for _, f := range manifestFiles { content, err := i.getManifestContent(ctx, f.Path)Also applies to: 186-188, 330-333
🤖 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 `@internal/ignition/internalreleaseimage_patcher.go` around lines 147 - 166, Reset the internalReleaseImagePatcher state before each manifest scan so stale matches do not leak across retries or reused instances. In getInternalReleaseImageManifest, clear i.iriFound at the start of the scan before looping over manifestFiles, then continue setting it to true only when the iriKind/iriInstanceName match is found. This ensures the later checks in the same patcher flow that depend on i.iriFound reflect the current manifest set only.
🤖 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.
Outside diff comments:
In `@internal/ignition/internalreleaseimage_patcher.go`:
- Around line 147-166: Reset the internalReleaseImagePatcher state before each
manifest scan so stale matches do not leak across retries or reused instances.
In getInternalReleaseImageManifest, clear i.iriFound at the start of the scan
before looping over manifestFiles, then continue setting it to true only when
the iriKind/iriInstanceName match is found. This ensures the later checks in the
same patcher flow that depend on i.iriFound reflect the current manifest set
only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 65fb22e0-1247-4579-9bf8-c6a7214d563b
📒 Files selected for processing (4)
internal/ignition/installmanifests.gointernal/ignition/installmanifests_test.gointernal/ignition/internalreleaseimage_patcher.gointernal/ignition/internalreleaseimage_patcher_test.go
💤 Files with no reviewable changes (1)
- internal/ignition/installmanifests_test.go
|
/lgtm |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10528 +/- ##
==========================================
- Coverage 44.36% 44.35% -0.01%
==========================================
Files 423 423
Lines 73574 73541 -33
==========================================
- Hits 32640 32620 -20
+ Misses 38000 37989 -11
+ Partials 2934 2932 -2
🚀 New features to boost your workflow:
|
c7ff491 to
5dfab63
Compare
The patcher is created fresh per cluster in NewGenerator() and getInternalReleaseImageManifest is only called once per instance from PatchManifests. There's no reuse or retry path — the field starts as false (zero value) and can only go to true. |
|
/assign @danielerez |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano, danielerez 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
|
/test edge-subsystem-kubeapi-aws |
|
@andfasano: 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. |
|
@andfasano: Jira Issue OCPBUGS-93368: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-93368 has been moved to the MODIFIED state. 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. |
|
/cherry-pick release-4.22 |
|
@andfasano: new pull request created: #10539 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. |
|
Fix included in release 5.0.0-0.nightly-2026-06-29-195240 |
7 participants
This patch simplifies the InternalReleaseImage api dependency, since only the IRI manifest presence check is required.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Assisted-by: Claude
Summary by CodeRabbit