-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: add WatchResourcesService to v1alpha1
package
#11
Conversation
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will eventually live in the v1.WatchService so let's make this parallel that.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, let's call it "WatchResourcesService" then and call this good to go.
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.
Cool. I've updated the service name and renamed the file to match it.
2dd1111
to
a824564
Compare
Signed-off-by: Jonathan Whitaker <jon.b.whitaker@gmail.com>
v1alpha1
packagev1alpha1
package
This work is part of the effort behind this issue.