-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(ui): show access management tab for containers #14122
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
base: master
Are you sure you want to change the base?
Conversation
Bundle ReportChanges will decrease total bundle size by 70.72kB (-0.32%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
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.
Thanks for the contribution! High-level this makes sense, I just have a couple questions / requests
!!container?.container?.access | ||
); | ||
}, | ||
enabled: (_, _2) => true, |
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.
In the v1 ContainerEntity, we have:
enabled: (_, container: GetContainerQuery) => {
const accessAspect = container?.container?.access;
const rolesList = accessAspect?.roles;
return !!accessAspect && !!rolesList && rolesList.length > 0;
},
Did you omit this for a specific reason?
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.
Thanks for the quick response!
I omitted this from ContainerEntity, to be consistent with DatasetEntity which too dropped the rules when going from entity to entityV2. This might make it clearer and more consistent for the user instead of the tab not being enabled for one entity type, but enabled for other.
There might have been additional reasoning - I will consult with my colleague.
@@ -117,6 +121,20 @@ export class ContainerEntity implements Entity<Container> { | |||
component: PropertiesTab, | |||
icon: ListBullets, | |||
}, | |||
{ | |||
name: 'Access Management', |
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.
name: 'Access Management', | |
name: 'Access', |
For consistency with the rest of the platform
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.
Hey 👋 Yup, changed this.
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.
Makes sense to me, thanks for your contribution!
We use the Access Management feature of Datahub in our organisation and saw that this aspect is not currently supported for Containers. This PR aims to change that.
Checkist