-
Notifications
You must be signed in to change notification settings - Fork 232
Initial draft of tokensource readme docs #1681
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
base: main
Are you sure you want to change the base?
Conversation
|
size-limit report 📦
|
|
|
||
| |Mechanism: | using pre-generated credentials | via a http request to a url | via fully custom logic | | ||
| |-------------|--|--|--| | ||
| |Fixed | [`TokenSource.literal`](#tokensourceliteral) | — | [`TokenSource.literal(async () => { /* ... */ })`](#tokensourceliteral) | | ||
| |Configurable | — | [`TokenSource.endpoint`](#tokensourceendpoint) or [`TokenSource.sandboxTokenServer`](#tokensourceendpoint) | [`TokenSource.custom`](#tokensourcecustom) | | ||
|
|
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.
Just wanted to call out this table - not sure if it's in the right place exactly in the flow of the broader content, but IMO it's super useful to visualize them all like this and it would be great to have this in the final docs pages as well!
| default, a `POST` request with a `Content-Type: application/json` header is made, and the request | ||
| body is expected to follow the [standard token format](FIXME: add docs link here!). If | ||
| credentials generation is successful, the endpoints returns a 2xx status code with a body following | ||
| the [standard token response format](FIXME: add docs link here!). |
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.
Wanted to call out: there are some placeholder links that I think would be good to point to a docs page talking about the standard token format.
In effect I think this could contain a more wordsmithed version of https://github.com/livekit-examples/token-server-status/pull/33, probably also linking to https://cloud.livekit.io/projects/p_/sandbox/templates/token-server somewhere in there too.
I can take a pass at this if desired but I think it's probably pretty self explanatory (the content in the pull request body plus maybe an example or two). One small detail not in the pull request that would be good to include is that there are typescript types that can be used if writing a node server endpoint here and it would be good to make those known.
README.md
Outdated
|
|
||
| There are two types of `TokenSource`'s - fixed and configurable. Configurable token sources can be | ||
| passed options as part of the generation process, allowing your to customize the token that they | ||
| generate. Fixed token sources generate their credentials fully indepentantly and don't allow for |
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.
indepentantly -> independently
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 also wonder if this will be slightly more clear:
Fixed token sources generate static or pre-generated credentials and do not accept parameters when fetching
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.
Good point, I went for something somewhere in between.
| A pre-implemented set of credentials fetching utilities. Once one is constructed, call `fetch` on | ||
| to generate a new set of credentials. | ||
|
|
||
| There are two types of `TokenSource`'s - fixed and configurable. Configurable token sources can 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 wonder if that makes sense to mention which use cases fixed tokenSources and configurable tokenSource are used for ? assuming they should be used by different use cases.
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 was kinda thinking the table would accomplish that, but maybe it would be worth having an explicit paragraph / callout with some examples?
| ``` | ||
|
|
||
| |Mechanism: | using pre-generated credentials | via a http request to a url | via fully custom logic | | ||
| |-------------|--|--|--| |
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.
remove this line ?
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.
It has to be there for the markdown table formatting to work properly. I'll make myself a note before merging to add the proper number of dashes though (not important for markdown to parse it properly, but I think this might be what you are getting at)
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 am ok with either option.
README.md
Outdated
|
|
||
| #### TokenSource.Literal | ||
| A fixed token source which returns a static set of credentials or a computed set of credentials | ||
| with no external input on each call. |
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.
nit, with no external input required on each call
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.
Made the change, but reading it through I'm still not super pleased with the wording on this one, I'l going to think some more about it.
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.
lgtm
|
lgtm again. thanks for addressing the comments. |
|
Note that right now, there are a few link placeholders still in here expecting to point to docs pages: #1681 (comment) I think either I need to extract these out or some docs pages need to be put together before this is merged. |
|
Any update on this PR? |
|
@xianshijing-lk I left a note a the comment above:
That is the only blocker remaining. |
Added a section to the readme about the
TokenSourceand how to use it currently.I'll admit I'm not sure if all this really should be in the README - it might make more sense to move most of this to docs pages and just link to it directly. But I'll leave thoughts about exactly how this is formatted up to the docs team!