add model serving compatible select to connection type form #3396
Conversation
|
LGTM! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3396 +/- ##
==========================================
+ Coverage 85.14% 85.93% +0.78%
==========================================
Files 1338 1338
Lines 30122 30178 +56
Branches 8270 8290 +20
==========================================
+ Hits 25648 25934 +286
+ Misses 4474 4244 -230
... and 29 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
christianvogt
force-pushed
the
ct-model-serving-select
branch
2 times, most recently
from
80266ff
to
2e00de8
Compare
| export const isCompatibleWithConnectionType = ( | ||
| fields: ConnectionTypeField[], | ||
| connectionType: ConnectionTypeConfigMapObj, | ||
| ): boolean => { | ||
| const compatibleDataFields = connectionType.data?.fields?.filter(isConnectionTypeDataField); | ||
| const dataFields = fields.filter(isConnectionTypeDataField); | ||
| if (compatibleDataFields) { | ||
| return compatibleDataFields | ||
| .filter(isConnectionTypeDataField) | ||
| .every((cf) => dataFields.find((f) => cf.envVar === f.envVar)); | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Do we need to only check the data fields that are required when checking compatibility? Because if region is missing from the new connection type, it says it's not compatible with s3, but I believe it still is.
Although the bucket isn't required, but it still needed for model serving
There was a problem hiding this comment.
@emilys314 ya there's this discrepancy between compatible with model serving and compatible with the connection type ¯_(ツ)_/¯
There was a problem hiding this comment.
After discussion in slack, we're going to make the dropdown represent the spirit of what each OOTB connection type represents but not specifically the OOTB connection type.
What this means is that the dropdown labels need to change and we will also ignore optional fields when comparing compatibility.
christianvogt
force-pushed
the
ct-model-serving-select
branch
from
2e00de8
to
cdb60eb
Compare
christianvogt
force-pushed
the
ct-model-serving-select
branch
from
cdb60eb
to
416e7ad
Compare
|
updated with latest changes to labels and compatibility checks |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 |
https://issues.redhat.com/browse/RHOAIENG-14813
Description
Adds a dropdown to the connection type creation page to add model serving compatible fields.
All OOTB types are present in the dropdown.
Selecting one of the types inserts a new section field along with all other fields found in the connection type.
When the connection type is found to be compatible (ie it has the same env vars), then we show an alert. Currently this is based strictly on the env var name and not of the field type or other properties such as required, name or description.
cc @simrandhaliw
How Has This Been Tested?
Ensure your cluster has the OOTB connection types applied. #3335
You can manually add them to your cluster with
oc apply -f .... They are found in themanifests/common/connection-typesdirectory of the git repo. Note that if they are not present, then the dropdown menu will not show up.disableConnectionTypesSettings -> Connection typesas an adminTest Impact
Added cypress test.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main