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.
|
| return createDataInferenceService.storage.dataConnection !== ''; | ||
| return ( | ||
| createDataInferenceService.storage.dataConnection !== '' || | ||
| (dataConnections.length > 0 && createDataInferenceService.storage.path !== '') |
There was a problem hiding this comment.
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.
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.
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,
];
|
[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