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 spec for namespace configs #883

Merged
merged 1 commit into from
Jun 29, 2022
Merged

Conversation

hperl
Copy link
Collaborator

@hperl hperl commented Jun 15, 2022

This PR adds the first version of the Ory Permission language spec to the repo.

Two (last) open issues:

  • Type unions
  • Type representation of subject sets

@hperl hperl marked this pull request as ready for review June 20, 2022 18:31
@hperl hperl force-pushed the feat/namespace-config-spec branch 3 times, most recently from bf3b305 to 411a86a Compare June 22, 2022 08:09
@hperl hperl requested a review from aeneasr June 22, 2022 13:00
@hperl
Copy link
Collaborator Author

hperl commented Jun 22, 2022

@zepatrik @aeneasr I'm now done with the spec. We landed on a design that is a strict subset of TypeScript, which will enable us to use a whole lot of tooling, starting with type information to guide the user, linting (with eslint) to detect common problems etc.

The configuration language is still easy enough to map directly to the Zanzibar AST, as I have documented in the spec.

Once the spec is merged, I will add a diagram through mermaid.js. I already started, but keeping the diagram up to date with the spec changes was tedious during the draft.

/cc @vinckr

@hperl
Copy link
Collaborator Author

hperl commented Jun 22, 2022

/cc @kevgo

Copy link
Contributor

@kevgo kevgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this up! This is really cool, I'm very excited to start playing with this idea more! I consider this idea of using TS syntax pre-alpha stage, so here comes relatively fundamental feedback. Curious what you think!

docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
@hperl hperl self-assigned this Jun 23, 2022
@hperl hperl force-pushed the feat/namespace-config-spec branch from bfd4a57 to d37c63a Compare June 23, 2022 10:34
@hperl
Copy link
Collaborator Author

hperl commented Jun 23, 2022

@kevgo thank you for your comments. I addressed your suggestions, and especially liked the improved wordings for related and permits.

Some of the things you mentioned (adding keywords, builtins), can be moved to a next version of the spec, as they will be backwards-compatible.

The only thing that is still open is the discussion about == vs ===. I find comparison irritating in JS as well and would rather define simple semantics in our language. If we just pick ===, then it feels strange to not have ==. Having both (with same or different semantics) would be confusing as well. I would argue for just having == and forbidding ===.

@aeneasr @zepatrik WDYT?

@hperl hperl force-pushed the feat/namespace-config-spec branch from 7c76024 to 9ce5f05 Compare June 23, 2022 10:45
@hperl hperl requested review from zepatrik and kevgo June 23, 2022 12:41
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kevgo kevgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome collaboration on this, thanks for all the thoughful comments! I totally agree with keeping this as simple as possible. Another round of feedback after sleeping over it again. This is coming along nicely!

docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
@hperl hperl requested review from kevgo and aeneasr June 24, 2022 08:11
@hperl
Copy link
Collaborator Author

hperl commented Jun 24, 2022

Awesome collaboration on this, thanks for all the thoughful comments! I totally agree with keeping this as simple as possible. Another round of feedback after sleeping over it again. This is coming along nicely!

Agreed! I just incorporated the latest comments and am quite happy with the result. I noticed that we can sidestep the equality discussion because we are only testing for set membership for now anyways (and there are good arguments for it staying that way).

Copy link
Contributor

@kevgo kevgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next round of feedback. I limit the amount of ideas in each review to keep them manageable. What do you think?

docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@hperl hperl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

  • remove metadata
  • parens in boolean expr

Copy link
Collaborator Author

@hperl hperl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

address review comments

@hperl hperl force-pushed the feat/namespace-config-spec branch from 6c4ff84 to 572c0b9 Compare June 28, 2022 12:59
@hperl hperl dismissed aeneasr’s stale review June 28, 2022 14:22

All comments addressed :)

Copy link
Contributor

@kevgo kevgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, that's all the feedback I have from my end. Next steps after the last changes of the spec are done:

  • polish of the language using grammarly.com (I can do this, want to wait after all other edits are done since this will cause lots of conflicts with concurrent edits)
  • format this file using Prettier

docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
docs/namespace_config_spec.md Outdated Show resolved Hide resolved
@hperl hperl force-pushed the feat/namespace-config-spec branch 2 times, most recently from 0523960 to 1881793 Compare June 28, 2022 14:38
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice 👍

Copy link
Contributor

@kevgo kevgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the suggestions from grammarly.com. Mostly removing passive voice. Please run Prettier again after you add these changes.

docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
@hperl
Copy link
Collaborator Author

hperl commented Jun 28, 2022

Excellent, that's all the feedback I have from my end. Next steps after the last changes of the spec are done:

  • polish of the language using grammarly.com (I can do this, want to wait after all other edits are done since this will cause lots of conflicts with concurrent edits)
  • format this file using Prettier

Thanks, @kevgo! I (think) I applied all of your suggestions.

Copy link
Contributor

@kevgo kevgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has turned out really nice! Can't wait to build some permission sets in it!

docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
docs/ory_permission_language_spec.md Outdated Show resolved Hide resolved
Co-authored-by: Kevin Goslar <kevin.goslar@gmail.com>
Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>
@hperl hperl force-pushed the feat/namespace-config-spec branch from a83b720 to 1586af5 Compare June 29, 2022 12:22
@hperl hperl enabled auto-merge (rebase) June 29, 2022 12:25
@hperl hperl merged commit 3d61b1c into master Jun 29, 2022
@hperl hperl deleted the feat/namespace-config-spec branch June 29, 2022 12:28
@hperl hperl mentioned this pull request Jul 4, 2022
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants