feat(KFLUXUI-175): give up SpaceBindingRequest and enjoy RoleBinding #83
Conversation
testcara
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #83 +/- ##
==========================================
+ Coverage 80.09% 80.24% +0.14%
==========================================
Files 570 574 +4
Lines 21476 21657 +181
Branches 5326 5352 +26
==========================================
+ Hits 17202 17378 +176
- Misses 4249 4253 +4
- Partials 25 26 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
testcara
force-pushed
the
KFLUXUI-251
branch
2 times, most recently
from
a75a873 to
3371d76
Compare
testcara
changed the title
testcara
requested review from
sahil143,
CryptoRodeo,
caugello,
JoaoPedroPP,
StanislavJochman and
marcin-michal
|
looks good first pass, I'll run this locally and label |
testcara
force-pushed
the
KFLUXUI-251
branch
2 times, most recently
from
cb18cea to
3453b1f
Compare
|
@sahil143 , hi, sahil, all comments have been addressed. Thank you for the help for continuous review. |
| apiVersion: 'rbac.authorization.k8s.io/v1', | ||
| kind: 'RoleBinding', | ||
| metadata: { name: 'konflux-maintainer-user1-actions-user', namespace: 'test-ns' }, | ||
| roleRef: { | ||
| apiGroup: 'rbac.authorization.k8s.io', | ||
| kind: 'ClusterRole', | ||
| name: 'konflux-maintainer-user-actions', | ||
| }, | ||
| subjects: [ | ||
| { | ||
| apiGroup: 'rbac.authorization.k8s.io', | ||
| kind: 'User', | ||
| name: 'user1', | ||
| namespace: 'test-ns', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
use only data that's needed for this test, expect.objectContaining( supports checking partial data in an object
There was a problem hiding this comment.
I have improved the test to ensure it's shorter and clearer.
| const existingRBs = await getRBs(roleBinding.metadata.namespace); | ||
|
|
||
| const roleBindingExists = existingRBs.some( | ||
| (binding) => binding.metadata.name === roleBinding.metadata.name, | ||
| ); | ||
|
|
||
| if (roleBindingExists && newRoleBinding) { | ||
| await deleteRB(roleBinding, dryRun); | ||
| } |
There was a problem hiding this comment.
Why do we need to delete the role binding first?
There was a problem hiding this comment.
We delete the obsoleted rolebinding after the new rolebinding is created successfully.
The comment is inconsistent with real application. I would fix it.
src/types/workspace.ts
Outdated
| role: NamespaceRole; | ||
| availableRoles?: NamespaceRole[]; | ||
| bindings?: RoleBinding[]; |
There was a problem hiding this comment.
why change the type here?
There was a problem hiding this comment.
I have removed the useless change.
testcara
force-pushed
the
KFLUXUI-251
branch
2 times, most recently
from
fa6d02d to
d787802
Compare
|
@sahil143, thank you a million for the continuous review. I have updated the patch with your suggestions. Thank you again. |
Fixes
KFLUXUI-175
Description
Konflux is migrating off workspace api and also the related SpaceBindingRequest.
For Konflux UI, we need to enjoy the shared Konflux cluster role and rolebinding for the namespace access and permission checks.
For the details:
We use k8swatch the rolebinding of the namespace for the list page.
And for the form actions, we use the create rolebinding, delete rolebinding, get rolebindings.
More, the status of the user access list are removed.
Type of change
Screen shots / Gifs for design review
Screen.Recording.2025-01-22.at.15.30.33.mov
How to test or reproduce?
Just enjoy the use access page as previous.
You can visit access list, grant access, edit access, revoke access like before.
Browser conformance: