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.

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

docs: custom root roles #4451

Merged
merged 9 commits into from
Aug 10, 2023
Merged

docs: custom root roles #4451

merged 9 commits into from
Aug 10, 2023

Conversation

nunogois
Copy link
Member

@nunogois nunogois commented Aug 8, 2023

https://linear.app/unleash/issue/2-1136/custom-root-roles-documentation

Questions:

  • Is it worth expanding the "Assigning custom root roles" section in the "How to create and assign custom root roles" guide to include the steps for assigning a root role for each entity (user, service account, group)?
  • Should this PR include an update to the existing "How to create and assign custom project roles" guide? We've since updated the UI;

@vercel
Copy link

vercel bot commented Aug 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 9, 2023 5:53pm
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 9, 2023 5:53pm

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

LGTM! I've answered my own comment, just leaving it open for sharing purposes

@@ -9,7 +9,7 @@ This document forms the specifications for [Role-Based Access Control](https://e

Unleash has two levels in its hierarchy of resources:

1. **Global resources** - Everything that lives across the entire Unleash instance. Examples of this include:
1. **Root resources** - Everything that lives across the entire Unleash instance. Examples of this include:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to switch from Global resources to root resources? I think switching from Global Roles to Root Roles makes sense, root resources feel a bit weird IMO. But, maybe it is just me

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I had the exact same thought, but I think I did the same thing you did

@@ -8,17 +8,17 @@ This guide's purpose is to give you a conceptual overview of how Unleash works.
The end of this guide presents a [short use case, explaining how you might configure Unleash](#use-case) to start working with feature toggles.


## The global level
## The root level
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, back to my other comment, maybe because of this concept of "root level" makes sense to also change to "root resources"

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Very nice work on this 🚒 I have a few minor change suggestions and comments, but not much. There's a lot of work in here, so thanks for going through and doing it all 🙏🏼

frontend/src/component/admin/users/EditUser/EditUser.tsx Outdated Show resolved Hide resolved
website/docs/reference/rbac.md Outdated Show resolved Hide resolved
Comment on lines 58 to 175
Lets the user read frontend API tokens.

- **Create frontend API tokens**

Lets the user create frontend API tokens.

- **Update frontend API tokens**

Lets the user update frontend API tokens.

- **Delete frontend API tokens**

Lets the user delete frontend API tokens.

- **Read client API tokens**

Lets the user read client API tokens.

- **Create client API tokens**

Lets the user create client API tokens.

- **Update client API tokens**

Lets the user update client API tokens.

- **Delete client API tokens**

Lets the user delete client API tokens.

#### Application permissions

- **Update applications**

Lets the user update applications.

#### Context field permissions

- **Create context fields**

Lets the user create context fields.

- **Update context fields**

Lets the user update context fields.

- **Delete context fields**

Lets the user delete context fields.

#### Project permissions

- **Create projects**

Lets the user create projects.

#### Role permissions

- **Read roles**

Lets the user read roles.

#### Segment permissions

- **Create segments**

Lets the user create segments.

- **Edit segments**

Lets the user edit segments.

- **Delete segments**

Lets the user delete segments.

#### Strategy permissions

- **Create strategies**

Lets the user create strategies.

- **Update strategies**

Lets the user update strategies.

- **Delete strategies**

Lets the user delete strategies.

#### Tag type permissions

- **Update tag types**

Lets the user update tag types.

- **Delete tag types**

Lets the user delete tag types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, but we might want to consider using description lists or tables here. That would create stronger semantic links between the permission name and its definition.

Unfortunately, markdown doesn't have a syntax for description lists, so we'd have to drop down to raw html for it. That makes editing it a little trickier, but I think it's a fair tradeoff.

What do you think?

Copy link
Member Author

@nunogois nunogois Aug 9, 2023

Choose a reason for hiding this comment

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

Good point, but I have the feeling tables help us represent this in a more structured and succinct way. Tried addressing this in 0517cd0 by adapting not only this new list of permissions, but also the existing ones for custom project roles.

https://unleash-docs-git-docs-custom-root-roles-unleash-team.vercel.app/reference/rbac#root-permissions

Let me know what you think 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's very nice! 😄 At least it looks very nice. I'm a little worried about the semantics for people who can't see the table. How do screen readers treat empty columns? Probably not as 'repeat the last non-empty entry in this column' 🤔 So how about splitting it up into multiple tables, each under its own heading? (keeping the heading structure from before)

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in f9e601b

website/docs/reference/rbac.md Outdated Show resolved Hide resolved
nunogois and others added 5 commits August 9, 2023 09:48
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
@nunogois nunogois deleted the docs-custom-root-roles branch August 10, 2023 07:22
nunogois added a commit that referenced this pull request Aug 10, 2023
This should fix `import` and `access` Cypress e2e tests after recent
changes were introduced:

- `import.spec.ts` - Expected '50%' to be contained in the page, however
now [we are lazy loading the accordion
content](#4454);
- `access.spec.ts` - Expected 'within a project are allowed' to be
visible as a role description, however [we updated the predefined roles
descriptions](#4451);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants