-
Notifications
You must be signed in to change notification settings - Fork 887
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 from TicketProvider
to something like RefreshingTokenProvider
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 like SignedToken
(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 do SignedExpiringToken
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 to SignedToken
and the cookie says coder_devurl_signed_app_token
.
I am cool with w/e name. Ticket -> Token does make us reuse existing terminology, but they are different things. 🤷♂️ |
WIP
Closes #6913