-
Notifications
You must be signed in to change notification settings - Fork 167
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
feat(mr): mr settings scaffolding #2810
feat(mr): mr settings scaffolding #2810
Conversation
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 wanna have a conversation about whether we should have an extra feature flag or not, I see the value on that but I'm not sure 100% if it's needed.
And we should add the extra checks in the supported area.
e1f1be1
to
7a501f4
Compare
/retest-required |
7a501f4
to
ca003db
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2810 +/- ##
==========================================
- Coverage 78.13% 78.08% -0.06%
==========================================
Files 1076 1077 +1
Lines 22629 22637 +8
Branches 5722 5722
==========================================
- Hits 17682 17676 -6
- Misses 4947 4961 +14
... and 4 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.
As I noted on Slack... not sure you should be creating a new area... a new flag is a bad idea, but the new area seems confusing. Why not use the existing area & feature flag?
ca003db
to
ac8ebd8
Compare
b008beb
to
74ce436
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
Signed-off-by: gitdallas <[email protected]>
Signed-off-by: gitdallas <[email protected]>
Signed-off-by: gitdallas <[email protected]>
74ce436
to
39b7693
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 other than a question for @lucferbux above
Requests were addressed
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lucferbux, 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 |
Closes: https://issues.redhat.com/browse/RHOAIENG-7057
Description
Scaffolding for model registry admin settings, component with just empty state for now
How Has This Been Tested?
manually tested that the nav item shows up based on useArea, and that the page renders the empty state properly
Test Impact
none, just scaffolding for now
Request review criteria:
make sure the nav item appears properly and that the page renders empty state with an action button that navs to model registries.
i had to run
/backend
vs:ext
from/frontend
as the:ext
doesn't have the setting for it yet.Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main