-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update model serving compatibility for connection types #3417
update model serving compatibility for connection types #3417
Conversation
0d8a09e
to
c2d31af
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3417 +/- ##
==========================================
+ Coverage 85.85% 85.90% +0.04%
==========================================
Files 1338 1338
Lines 30267 30282 +15
Branches 8346 8345 -1
==========================================
+ Hits 25987 26014 +27
+ Misses 4280 4268 -12
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
c2d31af
to
da29525
Compare
envVars: S3ConnectionTypeKeys, | ||
isConnectionCompatible: (connection) => | ||
connection.metadata.annotations['opendatahub.io/connection-type'] === 's3' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct because the type
makes it into the storage-config
and is necessary for model serving to work.
That being said, if all env vars for s3 are populated then we would add the annotation on edit / save. Unless I missed a case.
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well. LGTM
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: jeff-phillips-18 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
Connections:
Connection types:
How Has This Been Tested?
disableConnectionTypes
Settings -> Connection types
Test Impact
Updated tests.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main
cc @simrandhaliw