-
Notifications
You must be signed in to change notification settings - Fork 253
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
auth: store state in postgres instance #1420
auth: store state in postgres instance #1420
Conversation
b512b07
to
5fb2848
Compare
9332bda
to
fcc27fc
Compare
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.
Nice work, didn't expect to see this that fast 💪 Maybe switch the PR as draft to make sure we don't inadvertently merge it ?
5fce012
to
051b472
Compare
4333ca3
to
7716b4c
Compare
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! Just a couple of questions and minor comments.
test-utils/src/lib.rs
Outdated
} | ||
} | ||
|
||
impl PostgresDockerInstance { |
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.
No need to change now that it is done, but we might want to look into https://testcontainers.com/ to avoid doing all that work ourselves.
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 just saw it today, looks promising.
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.
Oh, I didn't know about that, cool!
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.
Opened #1479. It shouldn't be difficult and might be a nice OSS contribution. What do you think?
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 haven't looked closely at test containers, but it does seem like it would simplify this a lot. For the GitHub issue we might want to expand a bit on what should change, and where, since we use this strategy in logger, provisioner and now auth. Provisioner does not use the code you moved to common-tests, but maybe we don't need to share code in common-tests if we use test containers.
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 might want to suggest that the contributor breaks it up into separate PRs, as well.
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.
Thanks @oddgrd ! I added more details under the definition of done for the associated issue. I missed that we have the docker tests in the provisioner
too. In common-tests
we created API to create/get unique conn strings, referring to unique databases, that can be used in parallel by test cases. I think this is the outstanding part shared by logger
and auth
soon. Not sure if the provisioner
needs this functionality for its integration tests, and if not, it can probably avoid using common-tests
, integrating directly with testcontainers
.
cc29418
to
04dc9ef
Compare
04dc9ef
to
de5e417
Compare
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! 🥳
Also removed the build & push approval, since it is not used for the production images.
Description of change
Store the auth service state inside Postgres.
Note: This PR should NOT be merged before we set up the migration logic for the existing SQLite-based state to Postgres.
How has this been tested? (if applicable)