-
Notifications
You must be signed in to change notification settings - Fork 22
feat: add broken permissions service #150
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: main
Are you sure you want to change the base?
Conversation
|
The latest Buf updates on your PR. Results from workflow Lint / lint (pull_request).
|
Co-authored-by: Víctor Roldán Betancort <[email protected]>
vroldanbet
left a comment
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.
LGTM, just nits
| option java_multiple_files = true; | ||
| option java_package = "com.authzed.api.materialize.v0"; | ||
|
|
||
| service BrokenPermissionsService { |
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.
Should this be BrokenWatchedPermissionsService? I'm also not opposed to leaving it as-is
| service BrokenPermissionsService { | |
| service BrokenWatchedPermissionsService { |
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.
all cycles detected during the hydration process.
BrokenWatchedPermissionsService
These don't line up; I think we should just use BrokenPermissionsService, since these will be found during LPS
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.
What do you mean? They could also happen during updates, wouldn't they?
Co-authored-by: Víctor Roldán Betancort <[email protected]>
Co-authored-by: Víctor Roldán Betancort <[email protected]>
Co-authored-by: Víctor Roldán Betancort <[email protected]>
Co-authored-by: Víctor Roldán Betancort <[email protected]>
|
|
||
| message ReadBrokenWatchedPermissionsRequest { | ||
| // optional_at_revision defines the specific revision at which the broken watched permissions should be evaluated. | ||
| // At this time, it is only compared against the revision of the provided backing store snapshot. |
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.
I'm not sure I follow this. The first line is sufficient to explain the semantics, I'd drop it.
Also please clarify that if no argument is provided, the server will use the latest available revision.
| // resource_type is the type of the resource to watch for changes. | ||
| string resource_type = 1; | ||
| // permission is the permission to watch for changes. | ||
| string permission = 2; |
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.
The part that gives me a pause is the fact you could technically use a relation here too.
Perhaps I'd rename it to resource_relation, so it also looks symmetric when we eventually add subject_type and subject_relation.
Or you could use resource_permission_or_relation but that seems loooong
Description
This PR adds extends the materialize API with a
BrokenSetsServicecontaining aListBrokenSetsrpc method.The method returns a list of all the broken sets detected during the hydration process up to a specific revision.