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

Add support for sqlite/postgres database backends #96

Merged
merged 9 commits into from
Jun 15, 2023

Conversation

positiveblue
Copy link
Contributor

This PR adds sqlite/postgres support for aperture.

Currently the server uses a couple of interfaces SecretStore and OnionStore that are implemented in this PR. To keep consistency with other code bases aperture uses the same tools sqlc/goang-migrate and the same interfaces than Taproot Assets

Fixes #95

@positiveblue positiveblue force-pushed the sql branch 3 times, most recently from 735c78d to 82ca797 Compare May 29, 2023 01:08
@guggero guggero self-requested a review May 30, 2023 16:04
@lightninglabs-deploy
Copy link
Collaborator

@guggero: review reminder

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Nice work! Happy to see we could re-use so much existing code. Main comments are related to some schema modifications to simplify some of the biz logic, and make unique fields explicit.

aperturedb/sqlc/migrations/000001_secrets.up.sql Outdated Show resolved Hide resolved
aperturedb/sqlc/migrations/000002_onion.up.sql Outdated Show resolved Hide resolved
aperturedb/sqlc/migrations/000001_secrets.up.sql Outdated Show resolved Hide resolved
aperturedb/interface.go Outdated Show resolved Hide resolved
aperturedb/postges.go Outdated Show resolved Hide resolved
aperturedb/secrets.go Show resolved Hide resolved
aperturedb/onion.go Outdated Show resolved Hide resolved
aperturedb/onion.go Show resolved Hide resolved
aperturedb/onion.go Outdated Show resolved Hide resolved
config.go Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot!
Found a couple of smaller things, but those should be relatively easy to address.

aperturedb/sqlc/queries/onion.sql Show resolved Hide resolved
aperturedb/postgres.go Show resolved Hide resolved
aperturedb/test_postgres.go Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Look preeetty good, ran things locally and found an issue with the sqlite file config if one isn't explicitly specified. I think we want to allow people to just say db_backend: sqlite and not have to explicitly set where the file should go (actually use the default).

config.go Outdated Show resolved Hide resolved
aperture.go Show resolved Hide resolved
aperture.go Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

tACK, LGTM 🎉

Tested in the Pool server integration test.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🛺

@Roasbeef
Copy link
Member

Needs a rebase!

@guggero guggero merged commit 369489f into lightninglabs:master Jun 15, 2023
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.

db: add sqlite/postgres as a database backend option
4 participants