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

feat: add WatchResourcesService to v1alpha1 package #11

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

jon-whit
Copy link
Contributor

This work is part of the effort behind this issue.

@jzelinskie
Copy link
Member

recheck

@github-actions
Copy link

github-actions bot commented Oct 28, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@jon-whit
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@jon-whit
Copy link
Contributor Author

@jzelinskie I think I've resolved the CLA and DCO checks now. I've rebased with --signoff to satisfy that.

@jon-whit
Copy link
Contributor Author

@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.

@josephschorr
Copy link
Member

@jon-whit can you please rebase on HEAD of main?

@jon-whit
Copy link
Contributor Author

@josephschorr alright should be good. It's been rebased.

Copy link
Member

@jzelinskie jzelinskie left a 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 {
Copy link
Member

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.

Copy link
Contributor Author

@jon-whit jon-whit Dec 8, 2021

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?

Copy link
Member

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.

Copy link
Member

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)

Copy link
Member

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.

Copy link
Contributor

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.

authzed/api/v1alpha1/lookupwatch_service.proto Outdated Show resolved Hide resolved
authzed/api/v1alpha1/lookupwatch_service.proto Outdated Show resolved Hide resolved
authzed/api/v1alpha1/lookupwatch_service.proto Outdated Show resolved Hide resolved
Signed-off-by: Jonathan Whitaker <jon.b.whitaker@gmail.com>
@jon-whit jon-whit changed the title feat: add LookupWatchService to v1alpha1 package feat: add WatchResourcesService to v1alpha1 package Dec 14, 2021
@jzelinskie jzelinskie merged commit 041a426 into authzed:main Dec 15, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants