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(auth): add subscriptions table to auth, add rds quota to claim limits #1529

Merged

Conversation

oddgrd
Copy link
Contributor

@oddgrd oddgrd commented Jan 19, 2024

Description of change

Add a subscriptions table to auth to support multiple types of subscriptions, and update the claim limits with an rds quota.

How has this been tested? (if applicable)

Existing tests pass without modification, but new tests will be needed as we add functionality to add more than a pro subscription.

@oddgrd oddgrd changed the title Feature/engn 53 add subscriptions table to auth update jwt feat(auth): add subscriptions table to auth, add rds quota to claim limits Jan 21, 2024
@oddgrd oddgrd marked this pull request as ready for review January 22, 2024 12:50
Copy link
Contributor

@chesedo chesedo left a comment

Choose a reason for hiding this comment

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

I left some comments. Just confirm which changes are valid and I'll get to them 😄

auth/migrations/0001_sync_updated_at.sql Outdated Show resolved Hide resolved
auth/migrations/0002_subscriptions.sql Show resolved Hide resolved
common/src/limits.rs Show resolved Hide resolved
Provisioner is the only service using this field via the `Claim` since
it and auth will be released at the same time, it is safe(ish) to remove
this field.
Old runtimes will fail to deserialize if this field is not present. So
marking it as deprecated will allow us to phase it out.
Copy link
Contributor Author

@oddgrd oddgrd left a comment

Choose a reason for hiding this comment

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

On minor comment, but LGTM!

@@ -10,32 +10,44 @@ pub struct Limits {
/// The amount of projects this user can create.
project_limit: u32,
/// Whether this user has permission to provision RDS instances.
#[deprecated(
since = "0.38.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Should it be 0.37? The current version is 0.36.

Suggested change
since = "0.38.0",
since = "0.37.0",

Copy link
Contributor

Choose a reason for hiding this comment

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

I assumed this PR will only make it into the next release (if we release today) 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

We can update this if it is released earlier 😄

@chesedo chesedo merged commit 02d68a5 into main Jan 23, 2024
35 checks passed
@chesedo chesedo deleted the feature/engn-53-add-subscriptions-table-to-auth-update-jwt branch January 23, 2024 08:55
jonaro00 pushed a commit that referenced this pull request Jan 24, 2024
…imits (#1529)

* feat(auth): subscriptions table migration

* feat(auth): update existing code to work with sub table

* feat(common): add rds_quota to claim limits

* feat(auth): refactor to two queries for user,

* misc(auth): naming and migration comment

* refactor: remove `rds_access`

Provisioner is the only service using this field via the `Claim` since
it and auth will be released at the same time, it is safe(ish) to remove
this field.

* refactor: improve comment

* feat: added `create_at` data field

* refactor: prevent uses of `rds_access`

Old runtimes will fail to deserialize if this field is not present. So
marking it as deprecated will allow us to phase it out.

---------

Co-authored-by: chesedo <[email protected]>
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.

3 participants