Workbench images with incomplete imagestreams are not listed in the Spawn basic workbench odh-dashboard page #4629
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA logical error in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nananosirova
changed the title
|
@nananosirova: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
nananosirova
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
nananosirova
force-pushed
the
RHOAIENG-31381
branch
from
0f0e774 to
ca4c3b7
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
🔭 Outside diff range comments (1)
backend/src/routes/api/images/imageUtils.ts (1)
169-173:getTagInfomay returnundefined– propagate crashEarly-returning without a value breaks the declared return type
ImageTagInfo[]and will blow up any caller that does
(getTagInfo(...) || []).map(...).- if (!tags?.length) { - console.error(`${imageStream.metadata.name} does not have any tags.`); - return; - } + if (!tags?.length) { + console.error(`${imageStream.metadata.name} does not have any tags.`); + return []; + }
🧹 Nitpick comments (1)
backend/src/routes/api/images/imageUtils.ts (1)
208-220: SimplifypackagesToStringimplementationThe manual string building is verbose, error-prone and slower than
JSON.stringify.- if (packages.length > 0) { - let packageAsString = '['; - packages.forEach((value, index) => { - packageAsString = packageAsString + JSON.stringify(value); - if (index !== packages.length - 1) { - packageAsString = packageAsString + `,`; - } else { - packageAsString = packageAsString + ']'; - } - }); - return packageAsString; - } - return '[]'; + return JSON.stringify(packages);
| error: isBYONImage(imageStream) && getBYONImageErrorMessage(imageStream), | ||
| }; |
There was a problem hiding this comment.
Guard against false leaking into error field
&& will assign the boolean false to error whenever the image is not BYON.
Down-stream consumers almost certainly expect either undefined/null or a
string message – not a boolean. Use a ternary to keep the type stable.
- error: isBYONImage(imageStream) && getBYONImageErrorMessage(imageStream),
+ error: isBYONImage(imageStream)
+ ? getBYONImageErrorMessage(imageStream)
+ : undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error: isBYONImage(imageStream) && getBYONImageErrorMessage(imageStream), | |
| }; | |
| error: isBYONImage(imageStream) | |
| ? getBYONImageErrorMessage(imageStream) | |
| : undefined, | |
| }; |
🤖 Prompt for AI Agents
In backend/src/routes/api/images/imageUtils.ts around lines 145 to 146, the
error field is assigned using a logical AND which results in a boolean false
when the image is not BYON, but downstream expects either undefined/null or a
string. Replace the logical AND with a ternary expression that assigns the error
message string if the image is BYON, otherwise assigns undefined or null to keep
the error field type consistent.
ca4c3b7 to
d9a98f6
Compare
|
/lgtm |
|
/approve |
1 similar comment
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Gkrumbach07, jrenee42 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 530e266
into
opendatahub-io:v2.35.0-fixes
https://issues.redhat.com/browse/RHOAIENG-31381
Description
We currently handle two types of image streams:
When a tag refers to an image that cannot be retrieved , the tag is marked with an error condition. For example:
To avoid showing invalid images in the UI, we check tag import statuses after fetching image streams.
For BYON image streams, this validation works by inspecting the status of the single available tag.
However, the current logic incorrectly treats non-BYON image streams the same way by checking only the first tag in the list.
Since the tag list is sorted in ascending order (e.g: '2023.2', '2024.1', '2024.2'), this means that if the oldest tag failed to import (which could happen in disconnected environments where unsupported images are excluded), the entire image stream would be considered invalid and filtered out even if newer valid tags were present.
In this PR, we fix the issue by correcting the differentiation between BYON and non-BYON image streams.
How Has This Been Tested?
Tested on a cluster by using the image produced from branch.
Test Impact
In a cluster with ODH 2.30 installed (equivalent to RHOAI 2.22):
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit