-
Notifications
You must be signed in to change notification settings - Fork 163
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
Model registry deploy modal has faulty submit button disable detection #3209
Model registry deploy modal has faulty submit button disable detection #3209
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3209 +/- ##
==========================================
+ Coverage 85.28% 85.32% +0.04%
==========================================
Files 1258 1270 +12
Lines 27689 27902 +213
Branches 7372 7422 +50
==========================================
+ Hits 23614 23808 +194
- Misses 4075 4094 +19
... and 54 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
A couple small things
return createDataInferenceService.storage.dataConnection !== ''; | ||
return ( | ||
createDataInferenceService.storage.dataConnection !== '' || | ||
(dataConnections.length > 0 && createDataInferenceService.storage.path !== '') |
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.
Is this line necessary now? I'm not sure I fully understand the case here. If we're using EXISTING_STORAGE we should always have a dataConnection name right?
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.
yeah It was part of me trying to secure all parts of button enabling, but you're right it's not necessary. removed it.
AwsKeys.DEFAULT_REGION, | ||
]; | ||
const filteredEmptyAwsSecretData = EMPTY_AWS_SECRET_DATA.filter( | ||
(item): item is (typeof EMPTY_AWS_SECRET_DATA)[number] => |
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.
I think the is
keyword here is slightly misused, what was the reason for it here? is
creates a type guard: the function here (item): item is T => { ... }
tells TypeScript "if this returns true, item
has the type T
" and it can be used for narrowing the type of a variable or parameter when it's too broad. I don't think it does anything for us when used as a callback like this, removing it doesn't seem to cause any problems.
If you were trying to make sure all your prefilledKeys
are keys present in EMPTY_AWS_SECRET_DATA
, you could instead change the const prefilledKeys
line to have a type assertion like:
const prefilledKeys: (typeof EMPTY_AWS_SECRET_DATA)[number]['key'][] = [
AwsKeys.NAME,
AwsKeys.AWS_S3_BUCKET,
AwsKeys.S3_ENDPOINT,
AwsKeys.DEFAULT_REGION,
];
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.
LGTM, thanks @YuliaKrimerman !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mturley 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 |
/retest |
(https://issues.redhat.com/browse/RHOAIENG-12116)
Description
Updated the UI to enable the Deploy button once all the required fields are filled, without the need to update the pre-filled fields/
How Has This Been Tested?
On the UI in - try to deploy a version of a model with pre-existing connection fields
Test Impact
Added a unit test that makes sure the awsData array never has more than one element per key, as this was the issues that was causing the buggy behavior. ## Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main