Skip to content

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

Merged
merged 5 commits into from
Apr 4, 2023
Merged

chore: ticket provider interface #6915

merged 5 commits into from
Apr 4, 2023

Conversation

deansheather
Copy link
Member

WIP

Closes #6913

Comment on lines 30 to 35
// 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,
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member

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).

Copy link
Member

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.

Copy link
Member

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!

Copy link
Member Author

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

Copy link
Member Author

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.

@deansheather deansheather marked this pull request as ready for review April 3, 2023 21:44
@deansheather deansheather requested a review from Emyrk April 3, 2023 21:45
@Emyrk
Copy link
Member

Emyrk commented Apr 3, 2023

I am cool with w/e name. Ticket -> Token does make us reuse existing terminology, but they are different things. 🤷‍♂️
Either way for me.

@deansheather deansheather enabled auto-merge (squash) April 3, 2023 23:58
@deansheather deansheather merged commit 34593e3 into main Apr 4, 2023
@deansheather deansheather deleted the dean/ticket-provider branch April 4, 2023 00:59
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.

Refactor app ticket code to use interfaces for getting tickets
3 participants