-
Notifications
You must be signed in to change notification settings - Fork 162
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(MR): 12864 disable archive for mr/mv if has deploys #3292
base: main
Are you sure you want to change the base?
fix(MR): 12864 disable archive for mr/mv if has deploys #3292
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3292 +/- ##
=======================================
Coverage 84.84% 84.84%
=======================================
Files 1321 1321
Lines 29446 29468 +22
Branches 8037 8051 +14
=======================================
+ Hits 24983 25003 +20
- Misses 4463 4465 +2
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
980ca3b
to
1cb71d6
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.
Works fine.
About the tests, @gitdallas did you try adding an intercept with mock data ?
frontend/src/pages/modelRegistry/screens/ModelVersions/ModelVersionsTable.tsx
Outdated
Show resolved
Hide resolved
@manaswinidas yea i just threw this PR up prior to going on PTO incase it needed to get picked up urgently or something happened 😆 i was mocking it, but after debugging with mike it turns out the intercept wasn't working because it wasn't also intercepting the call to projects, which affected the URL of the inferenceService intercept 😵 updating PR soon with tests and cleanup |
Signed-off-by: gitdallas <[email protected]>
1cb71d6
to
284829b
Compare
closes: https://issues.redhat.com/browse/RHOAIENG-12864
Description
disable archiving models/versions with deploys
mr list page:
mr detail and versions list page:
mr version detail page:
How Has This Been Tested?
tested locally with mr cluster
Test Impact
tried adding tests, but cypress doesn't seem to call the k8s inferencemodel endpoint? will try again when i come back from vacation if still necesssary
cypress activity:
network activity for k8s inferencemodel in the actual app:
Request review criteria:
make sure that in all places where you can archive a model or version, you cannot do it if it has a deploy... but otherwise you can.
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main