-
Notifications
You must be signed in to change notification settings - Fork 24
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
Update SDK to support sibling acls #3041
Conversation
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@codahq/packs-sdk", | |||
"version": "1.7.11", | |||
"version": "1.7.12", |
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.
since you'll be releasing this immediately, this is fine, but for future reference, the ideal flow is to make this 1.7.12-prerelease.1
and then as a part of doing the release process, it gets to drop the prerelease.
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'll update this
Another way I thought about doing this is that we could let a sync table schema have a But I think that could live side-by-side with this, so no objection from me. |
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.
Yeah we can't release this right away I think? Need to update coda
to add support to recognize support for this and support backwards compatibility, then need to have a PR read to fast merge to update every pack that uses permissions? Unless we change the SDK to be backwards compatible.
schema.ts
Outdated
* | ||
* TODO(sam): Unhide this | ||
* @hidden | ||
*/ | ||
export interface Permission { | ||
export interface DirectPermission { | ||
permissionType: PermissionType.Direct; |
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.
Can we make this for now permissionType?: PermissionType.Direct?
Why not just make the SDK backwards compatible for now (Direct permissions can have no type there and we assume direct?) |
good point. |
9fa5c21
to
4b5c403
Compare
4b5c403
to
06b206e
Compare
* @hidden | ||
*/ | ||
export enum PermissionType { | ||
Delegated = 'delegated', |
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.
Can you include some basic JSDoc describing what the delegated and direct mean, when to use them, etc?
06b206e
to
a3f3498
Compare
a3f3498
to
44584ba
Compare
Make sure to fast follow with |
My bad still trying to figure out how this whole ecosystem works with the sdk. Will be mindful going forward. Sam and I are working on the fast follow rn. |
No need to apologize, there's lots of unwritten best practices to navigate, just spreading awareness. |
Part of the work for sibling ACLs. Updating permissions to have a new delegated type.