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

database enhancements #666

Merged
merged 1 commit into from
Aug 19, 2024
Merged

database enhancements #666

merged 1 commit into from
Aug 19, 2024

Conversation

runcom
Copy link
Contributor

@runcom runcom commented Aug 14, 2024

chore: store: db: reuse connection pool

The pool object is meant to be reused and since we're initializing the
store as part of a long running process, we should save the pool object
and pass it around/use it.

chore: store: db: refactor database configuration

This mainly comes from @mmartinv comment https://github.com/fdo-rs/fido-device-onboard-rs/pull/556/files#r1465996442

@runcom runcom requested a review from 7flying August 14, 2024 14:03
@runcom runcom changed the title Db fixes database enhancements Aug 14, 2024
test/fmf/tests/onboarding/run-onboarding.sh Dismissed Show dismissed Hide dismissed
@runcom runcom force-pushed the db_fixes branch 5 times, most recently from f836d05 to afb76ec Compare August 15, 2024 14:38
@nullr0ute
Copy link
Contributor

Is this ready for review? Looking to be ongoing updates so maybe should be a draft until it's ready

test/fmf/tests/onboarding/run-onboarding.sh Dismissed Show dismissed Hide dismissed
@nullr0ute nullr0ute marked this pull request as draft August 16, 2024 09:28
@runcom runcom force-pushed the db_fixes branch 2 times, most recently from 350a333 to 90dd3f5 Compare August 19, 2024 07:51
test/fmf/tests/onboarding/run-onboarding.sh Dismissed Show dismissed Hide dismissed
test/fmf/tests/onboarding/run-onboarding.sh Dismissed Show dismissed Hide dismissed
test/fmf/tests/onboarding/run-onboarding.sh Dismissed Show dismissed Hide dismissed
test/fmf/tests/onboarding/run-onboarding.sh Dismissed Show dismissed Hide dismissed
@runcom runcom marked this pull request as ready for review August 19, 2024 08:26
@runcom runcom requested a review from mmartinv August 19, 2024 08:26
Copy link
Contributor

@7flying 7flying left a comment

Choose a reason for hiding this comment

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

You should move the removal of store/src/db.rs into the first commit since you are already creating two separate files for sqlite and postgres that have the pool reuse modification

@runcom
Copy link
Contributor Author

runcom commented Aug 19, 2024

You should move the removal of store/src/db.rs into the first commit since you are already creating two separate files for sqlite and postgres that have the pool reuse modification

I figured it's probably better to have this enhancement as a single commit (too many moving pieces and I want to avoid to break bisect too)

The pool object is meant to be reused and since we're initializing the
store as part of a long running process, we should save the pool object
and pass it around/use it.
Have a proper db url directly in configuration files instead of using
environment variables.

Signed-off-by: Antonio Murdaca <[email protected]>
@7flying
Copy link
Contributor

7flying commented Aug 19, 2024

You should move the removal of store/src/db.rs into the first commit since you are already creating two separate files for sqlite and postgres that have the pool reuse modification

I figured it's probably better to have this enhancement as a single commit (too many moving pieces and I want to avoid to break bisect too)

That also works for me 👍

Copy link
Contributor

@7flying 7flying left a comment

Choose a reason for hiding this comment

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

LGTM

@7flying 7flying merged commit 839270e into fdo-rs:main Aug 19, 2024
23 of 24 checks passed
@runcom runcom deleted the db_fixes branch August 20, 2024 08:18
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