Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(mr settings): permissions table #2969

Merged
merged 4 commits into from
Jul 9, 2024
Merged

feat(mr settings): permissions table #2969

merged 4 commits into from
Jul 9, 2024

Conversation

gitdallas
Copy link
Contributor

Closes: https://issues.redhat.com/browse/RHOAIENG-6636

Description

Add model registry settings > permissions page. Factored out components from ProjectSharing (project permissions).
image

How Has This Been Tested?

Ran through creating/editing/deleting rolebindings for users/groups in model registries, also tested project permissions since that was factored out.

Test Impact

added cypress testing for the new page

Request review criteria:

make sure that you can create/edit/delete user/group rolebindings for model registries and that project permissions isn't broken

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@gitdallas gitdallas requested a review from mturley June 30, 2024 03:11
@openshift-ci openshift-ci bot requested review from alexcreasy and dpanshug June 30, 2024 03:12
@gitdallas gitdallas force-pushed the feat/6636-mr-rbac-users branch from 0066695 to f37ed0c Compare June 30, 2024 03:16
@gitdallas
Copy link
Contributor Author

i just noticed the ? icon for the default group permission in the new mock and if i aggregate the previous mocks i need it to have a tooltip that says "This group is created by default. You can add users to this group via the API."

so i'll add that

mturley
mturley requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good stuff @gitdallas . I have a couple of things, hopefully it won't be too bad to address.

frontend/src/__mocks__/mockRoleBindingK8sResource.ts Outdated Show resolved Hide resolved
frontend/src/api/k8s/roleBindings.ts Show resolved Hide resolved
frontend/src/concepts/roleBinding/RoleBindingPermissions.tsx Outdated Show resolved Hide resolved
frontend/src/concepts/roleBinding/RoleBindingPermissionsTableRow.tsx Outdated Show resolved Hide resolved
frontend/src/concepts/roleBinding/RoleBindingPermissionsTableRow.tsx Outdated Show resolved Hide resolved
frontend/src/concepts/roleBinding/utils.ts Outdated Show resolved Hide resolved
frontend/src/k8sTypes.ts Outdated Show resolved Hide resolved
frontend/src/pages/modelRegistrySettings/ModelRegistriesPermissions.tsx Show resolved Hide resolved
frontend/src/pages/modelRegistrySettings/useRoleBindings.ts Outdated Show resolved Hide resolved
frontend/src/pages/modelRegistrySettings/useRoleBindings.ts Outdated Show resolved Hide resolved
@gitdallas gitdallas force-pushed the feat/6636-mr-rbac-users branch from f37ed0c to 9836d03 Compare July 2, 2024 19:36
@mturley
Copy link
Contributor

mturley commented Jul 3, 2024 •
edited
Loading

@gitdallas while you're in here, can you actually also please look at the DeleteModelRegistryModal and replace the placeholder text "TODO: group here" with ${mr.metadata.name}-users? You can just inline it and get rid of the defaultPermissionsGroup variable. I thought we'd have to fetch something to look that up but since we're making the naming convention assumption here, we can use it there too.

@mturley
Copy link
Contributor

mturley commented Jul 3, 2024

Looks like the test failure is just Prettier

@gitdallas gitdallas force-pushed the feat/6636-mr-rbac-users branch from 9836d03 to bbc9a6f Compare July 3, 2024 18:59
mturley
mturley reviewed Jul 3, 2024
View reviewed changes
Copy link
Contributor

@mturley mturley left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it's working well for me on the MR side, and the failures on the project side are caused by the lines I comment on below. Also a couple more minor nits. Otherwise all the changes LGTM.

frontend/src/api/k8s/roleBindings.ts Outdated Show resolved Hide resolved
frontend/src/concepts/roleBinding/RoleBindingPermissionsTable.tsx Outdated Show resolved Hide resolved
frontend/src/concepts/roleBinding/RoleBindingPermissionsTable.tsx Outdated Show resolved Hide resolved
frontend/src/concepts/roleBinding/RoleBindingPermissionsTableRow.tsx Outdated Show resolved Hide resolved
frontend/src/concepts/roleBinding/RoleBindingPermissionsTableRowAdd.tsx Show resolved Hide resolved
lucferbux
lucferbux requested changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I agree on @mturley changes and I added one more thing to do. Rest lgtm

frontend/src/api/k8s/roleBindings.ts Outdated Show resolved Hide resolved
frontend/src/api/k8s/roleBindings.ts Outdated Show resolved Hide resolved
@gitdallas gitdallas force-pushed the feat/6636-mr-rbac-users branch 3 times, most recently from 36049a1 to c7a782b Compare July 7, 2024 13:54
@gitdallas
Copy link
Contributor Author

failing test seems unrelated (compareRuns graph)

@gitdallas gitdallas requested review from mturley and lucferbux July 8, 2024 11:51
lucferbux
lucferbux requested changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just a nit, rest of code looks great, once you have that change we can merge it!

frontend/src/pages/modelRegistrySettings/ModelRegistriesPermissions.tsx Outdated Show resolved Hide resolved
@gitdallas gitdallas force-pushed the feat/6636-mr-rbac-users branch from c7a782b to ff4a46b Compare July 9, 2024 11:56
lucferbux
lucferbux approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 9, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lucferbux

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Jul 9, 2024
@lucferbux
Copy link
Contributor

mmmm weird, some pipeline test has failed. let me try and retest it
/retest

@gitdallas
Copy link
Contributor Author

@lucferbux - yea, mentioned that #2969 (comment)

seems consistent

gitdallas added 2 commits July 9, 2024 09:36
@gitdallas
feat(mr settings): permissions table
76a6f4a 
Signed-off-by: gitdallas <[email protected]>

auto lint fixes

Signed-off-by: gitdallas <[email protected]>

fix lints

Signed-off-by: gitdallas <[email protected]>

some cleanup

Signed-off-by: gitdallas <[email protected]>

tests for mr permissions

Signed-off-by: gitdallas <[email protected]>
@gitdallas
refactoring around some type consolidation
5ebf3ea 
Signed-off-by: gitdallas <[email protected]>
@gitdallas gitdallas force-pushed the feat/6636-mr-rbac-users branch from ff4a46b to d29d5e1 Compare July 9, 2024 14:36
@openshift-ci openshift-ci bot removed the lgtm label Jul 9, 2024
@lucferbux
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 9, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit c29e3f0 into opendatahub-io:main Jul 9, 2024
6 checks passed
@gitdallas gitdallas deleted the feat/6636-mr-rbac-users branch July 9, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucferbux lucferbux lucferbux approved these changes

@alexcreasy alexcreasy Awaiting requested review from alexcreasy

@dpanshug dpanshug Awaiting requested review from dpanshug

@mturley mturley Awaiting requested review from mturley

Assignees

@mturley mturley

@lucferbux lucferbux

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gitdallas @mturley @lucferbux