feat: add WatchResourcesService to v1alpha1 package#11
feat: add WatchResourcesService to v1alpha1 package#11jzelinskie merged 1 commit intoauthzed:mainfrom
v1alpha1 package#11Conversation
|
recheck |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@jzelinskie I think I've resolved the CLA and DCO checks now. I've rebased with |
|
@josephschorr or @jzelinskie could I get an additional set of eyes on these changes? I'd like to get these v1alpha1 APIs merged in for the work going on in authzed/spicedb#255. |
|
@jon-whit can you please rebase on HEAD of main? |
|
@josephschorr alright should be good. It's been rebased. |
jzelinskie
left a comment
There was a problem hiding this comment.
This general strategy looks correct to me, barring typos and maybe some discussion on naming.
|
|
||
| // LookupWatchService is used to receive a stream of updates for resources of a | ||
| // specific (resource type, permission, subject) combination. | ||
| service LookupWatchService { |
There was a problem hiding this comment.
This will eventually live in the v1.WatchService so let's make this parallel that.
There was a problem hiding this comment.
@josephschorr @jzelinskie this is contradictory to what we settled on authzed/spicedb#207.
I remember proposing this in the LookupWatch API proposal. It always made sense to me for this to be part of the Watch API, but I thought we agreed to put it under a difference service specification. Has that decision just changed?
There was a problem hiding this comment.
I'd prefer it being WatchService rather than LookupWatchService. WDYT @josephschorr? When it moves into v1 it certainly should be.
There was a problem hiding this comment.
I prefer we keep it distinct, as I can absolutely imagine scenarios where LookupWatchService is exposed to applications using SpiceDB but Watch is not; for example, an application might use LookupWatchService to track changes directly, but the admin of the SpiceDB cluster may only want "managed" users of WatchService to be exposed (future caches, audit logging, etc)
There was a problem hiding this comment.
I think this makes sense, let's call it "WatchResourcesService" then and call this good to go.
There was a problem hiding this comment.
Cool. I've updated the service name and renamed the file to match it.
2dd1111 to
a824564
Compare
Signed-off-by: Jonathan Whitaker <[email protected]>
v1alpha1 packagev1alpha1 package
This work is part of the effort behind this issue.