Skip to content

Conversation

leeroyrose
Copy link

@leeroyrose leeroyrose commented Jul 14, 2025

Summary

Update role types to reflect documentation and better reflect return values from the api.

Description

Adds string to the ContentModel type so it can accept the value "all". When retrieving roles using the apis, the ContentModel is set to "all" but when creating them, you are forced to use an array - ["all"].

Adds an action type to allow better typing and support for the "access" action. Exports the type as it's quite useful. Renamed it to prevent clashes/make it clear what it's used for.

Also, there is some confusion about the not operator. In the documentation here, it says "You can specify nested constraints for and, or, not". When referencing the table of examples, it states that not can only accept one constraint as an argument. I have personally concluded that not can be used as both a nested operator, used at the top level and as an operator that takes one constraint. Would it be possible to confirm this and possibly update the documentation?

Motivation and Context

This change fixes a bug wth the ContentModel permission not accepting a string and adds access as an action type for writting environment based policies.

Checklist (check all before merging)

  • Both unit and integration tests are passing
  • There are no breaking changes
  • Changes are reflected in the documentation

When adding a new method:

  • The new method is exported through the default and plain CMA client
  • All new public types are exported from ./lib/export-types.ts
  • Added a unit test for the new method
  • Added an integration test for the new method
  • The new method is added to the documentation

@leeroyrose leeroyrose requested a review from a team as a code owner July 14, 2025 20:40
@leeroyrose leeroyrose force-pushed the lr/role-types branch 3 times, most recently from 7f6485f to 233d2df Compare July 22, 2025 18:32
@leeroyrose
Copy link
Author

Is anyone able to give this a review?

@leeroyrose leeroyrose force-pushed the lr/role-types branch 2 times, most recently from f6e4368 to 67fdb54 Compare July 28, 2025 15:36
Copy link

@t4nn t4nn left a comment

Choose a reason for hiding this comment

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

lgtm!

@michaelphamcf
Copy link
Contributor

michaelphamcf commented Aug 5, 2025

Hey @leeroyrose — sorry for the delay, and thanks for opening this PR!

I’ve flagged a few folks who might be impacted to take a look. In the meantime, I’d recommend introducing a new RoleActionType instead of replacing ActionType. I agree that ActionType could eventually be replaced, but it should be mark it as @deprecated first to avoid breaking changes for anyone that may have imported it directly.

@leeroyrose
Copy link
Author

Hi @michaelphamcf

Thank you for your reply. ActionType was not exported in a way that exposed it to the consumers, does that change the approach we need to take?

Only code within the client could have used the export and judging by the builds and a quick search in the codebase, it's not being used anywhere.

In the process of renaming the variable, I added it to the export-types.ts file, which would then expose it to consumers.

@leeroyrose
Copy link
Author

Hey, thank you for the approval.

Before I merge, would it be possible to get some clarification on my question regarding the not operator?

Also, there is some confusion about the not operator...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants