-
Notifications
You must be signed in to change notification settings - Fork 179
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
fix/12783-error-for-dup-model-name #3708
fix/12783-error-for-dup-model-name #3708
Conversation
22bc434
to
63af785
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3708 +/- ##
=======================================
Coverage 84.25% 84.25%
=======================================
Files 1463 1463
Lines 33746 33753 +7
Branches 9358 9360 +2
=======================================
+ Hits 28433 28440 +7
Misses 5313 5313
... and 5 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.
Small nit about the abstraction
frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx
Outdated
Show resolved
Hide resolved
63af785
to
dd95778
Compare
frontend/src/pages/modelRegistry/screens/RegisterModel/RegisterModel.tsx
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registerModel.cy.ts
Outdated
Show resolved
Hide resolved
dd95778
to
da9dbf8
Compare
Signed-off-by: gitdallas <[email protected]>
Signed-off-by: gitdallas <[email protected]>
Signed-off-by: gitdallas <[email protected]>
Signed-off-by: gitdallas <[email protected]>
da9dbf8
to
f958232
Compare
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 @gitdallas , thank youuuuu for your patience with nitpicks
[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 |
* fix/12783-error-for-dup-model-name Signed-off-by: gitdallas <[email protected]> * refactor model loading and model name validation Signed-off-by: gitdallas <[email protected]> * fix broken error message, add test Signed-off-by: gitdallas <[email protected]> * add util unit tests Signed-off-by: gitdallas <[email protected]> --------- Signed-off-by: gitdallas <[email protected]>
fises: https://issues.redhat.com/browse/RHOAIENG-12783
Description
model name field will check against existing names and give an error help text if it matches one, it will also prevent form submission
![image](https://private-user-images.githubusercontent.com/5322142/408752935-ef4f66a8-c247-45ec-a898-768dfcd3f897.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMTIxNTYsIm5iZiI6MTczOTExMTg1NiwicGF0aCI6Ii81MzIyMTQyLzQwODc1MjkzNS1lZjRmNjZhOC1jMjQ3LTQ1ZWMtYTg5OC03NjhkZmNkM2Y4OTcucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDlUMTQzNzM2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZmYzOGRmZTI0NTBiNzcxN2RhMmU3NmI5ZTNhY2U3ZWQ4Mzg3YWRlYWYxMDkxNDNmZDY2MTU5ZWQ1NjBhNTc2OSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.S1qpQin1Bz2AkjJP_0_x82Np2tVZpZf2GY9deExlBuY)
How Has This Been Tested?
tested locally by using duplicate model name while trying to register a new model
Test Impact
added a test to check that duplicate model name prevents form submission
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main