NO-ISSUE: Fix incorrect OSImageStreams log - #6338
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@pablintino: This pull request explicitly references no jira issue. 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 selected for processing (1)
WalkthroughThe OSImageStream controller now propagates context through its worker flow. Build and update operations return the resulting object, allowing ChangesOSImageStream synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pablintino 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/controller/osimagestream/osimagestream_controller.go`:
- Around line 296-300: Update handleOSImageStreamUpdate and its caller so the
helper returns the updated object, then assign that returned object to current
in the existing != nil path. Propagate any error while preserving the current
behavior for unchanged streams, ensuring initial-sync logging uses the updated
Status.DefaultStream.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 20884649-2b66-4fa4-a4fb-de1fa3a3f68d
📒 Files selected for processing (1)
pkg/controller/osimagestream/osimagestream_controller.go
1c7790f to
d0c6690
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/controller/osimagestream/osimagestream_controller.go`:
- Around line 289-301: Update syncOSImageStream and its callers to accept and
propagate the worker/lifecycle context instead of creating context.TODO() for
buildOSImageStream and handleOSImageStreamUpdate. Ensure the same cancellable
context reaches the underlying Create, Update, and UpdateStatus calls,
preserving cancellation during shutdown; alternatively wrap the sync operation
in an appropriate bounded timeout.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dcce8326-4df1-4af0-9e7d-20fc521bb707
📒 Files selected for processing (1)
pkg/controller/osimagestream/osimagestream_controller.go
The log that prints the available streams once discovered was using the lister instead of the recently built OSImageStream instance. The lister may not have updated the status (refreshed a few lines before) so what the log prints may be incorrect. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
d0c6690 to
6b9f78b
Compare
|
@pablintino: 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. |
|
/hold Holding to allow the Kube rebase to land in #6321. Please ensure this will not cause merge conflicts for the Kube rebase before unholding this PR. |
- What I did
The log that prints the available streams once discovered was using the lister instead of the recently built OSImageStream instance. The lister may not have updated the status (refreshed a few lines before) so what the log prints may be incorrect.
- How to verify it
e2e's are enough.
- Description for the changelog
Fix internal OSImageStream controller log that was printing an incorrect value because of a race condition.
Summary by CodeRabbit