- Notifications
You must be signed in to change notification settings - Fork1k
chore: ticket provider interface#6915
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderd/workspaceapps/provider.go Outdated
| // TicketProvider provides workspace app tickets. | ||
| // | ||
| // write a funny comment that says a ridiculous amount of fees will be incurred: | ||
| // | ||
| // Please keep in mind that all transactions incur a service fee, handling fee, | ||
| // order processing fee, delivery fee, |
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 might rename this fromTicketProvider to something likeRefreshingTokenProvider or something. Just because we already use the token wording.
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.
We call the session token a token and we already call the ticket a ticket. I'd rather use a different term than token to avoid people thinking it's the same thing as a session token.
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 is also invisible to the end user so the naming of it doesn't matter that much. I like TicketProvider more than RefreshingTokenProvider
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.
Well, this is really a fast-expiring token, so I'm just trying to reduce the number of terms people need to consume to understand what's going on.
I'm proposing renaming Ticket to something likeSignedToken (or something else token related, so that it doesn't confuse people).
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.
SignedToken is a valid name imo.
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.
Maybe we doSignedExpiringToken to make it verbose but super obvious!
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.
SignedExpiringToken isn't obvious because it doesn't say what it's for. SignedAppToken would be better
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.
Renamed toSignedToken and the cookie sayscoder_devurl_signed_app_token.
Uh oh!
There was an error while loading.Please reload this page.
I am cool with w/e name. Ticket -> Token does make us reuse existing terminology, but they are different things. 🤷♂️ |
WIP
Closes#6913