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: DB driver and cache #15

Merged
merged 42 commits into from
Sep 19, 2024
Merged

feat: DB driver and cache #15

merged 42 commits into from
Sep 19, 2024

Conversation

commoddity
Copy link
Contributor

@commoddity commoddity commented Sep 9, 2024

Related to #11

PR to add a Postgres database driver using SQLC to generate Go code from SQL files:
https://sqlc.dev/

This is the first prerequisite to allow associating a service request with a user for the purposes of metering, billing, rate limiting, user settings, etc.

This PR adds the following:

  • db/driver package containing Postgres driver
  • db package containing local cache of user data
  • user package containing the GatewayEndpoint struct

@commoddity commoddity added the enhancement New feature or request label Sep 9, 2024
@commoddity commoddity self-assigned this Sep 9, 2024
@commoddity commoddity linked an issue Sep 9, 2024 that may be closed by this pull request
10 tasks
@commoddity commoddity marked this pull request as ready for review September 9, 2024 19:24
@commoddity commoddity removed a link to an issue Sep 9, 2024
10 tasks
@Olshansk Olshansk added this to the Path MVP milestone Sep 9, 2024
Copy link
Contributor

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@commoddity Great start!

I did a high-level preliminary review (see screenshot below) but just wanted to pause it here because I have some questions w.r.t to the naming decisions.

Lmk if it makes sense or if we need to sync over a call.

Screenshot 2024-09-09 at 3 34 12 PM

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
db/driver/sqlc/query.sql Outdated Show resolved Hide resolved
db/driver/sqlc/schema.sql Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@commoddity REALLY love how this is shaping up. I think we're almost there and this should be my last round of a lot of comments.

We're close and ty for bearing with the iterations 🙏

user/gateway_endpoint.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
config/config_test.go Outdated Show resolved Hide resolved
config/user_data.go Outdated Show resolved Hide resolved
db/cache.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docker/docker-compose.full.yml Outdated Show resolved Hide resolved
docker/docker-compose.full.yml Outdated Show resolved Hide resolved
docker/docker-compose.yml Outdated Show resolved Hide resolved
@commoddity
Copy link
Contributor Author

commoddity commented Sep 13, 2024

@Olshansk Have addressed or responded to all review comments. PTAL. We're almost there. 🙂

Copy link
Contributor

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Not a review - just publishing my comments on previous threads and will dive into a full review shortly.

Makefile Show resolved Hide resolved
db/postgres/sqlc/schema.sql Outdated Show resolved Hide resolved
docker/docker-compose.yml Outdated Show resolved Hide resolved
config/user_data.go Outdated Show resolved Hide resolved
config/config_test.go Outdated Show resolved Hide resolved
config/utils/utils.go Outdated Show resolved Hide resolved
db/driver_mock_test.go Outdated Show resolved Hide resolved
docker/docker-compose.yml Outdated Show resolved Hide resolved
router/router_test.go Outdated Show resolved Hide resolved
db/postgres/testdata/seed-test-db.sql Outdated Show resolved Hide resolved
db/postgres/sqlc/schema.sql Outdated Show resolved Hide resolved
docker/docker-compose.full.yml Outdated Show resolved Hide resolved
db/postgres/sqlc/query.sql Outdated Show resolved Hide resolved
db/postgres/driver.go Outdated Show resolved Hide resolved
@Olshansk
Copy link
Contributor

@adshmh Would appreciate if you could review this as well.

router/router.go Outdated Show resolved Hide resolved
router/router_test.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
db/cache_test.go Outdated Show resolved Hide resolved
db/cache_test.go Outdated Show resolved Hide resolved
cmd/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

LGTM! Let's just get a 👍 from @adshmh as well before merging it in

Copy link
Contributor

@adshmh adshmh left a comment

Choose a reason for hiding this comment

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

Pre-approving with one ask:

Please add TODOs to address the following:

  • Our overall Authentication solution.
  • Alternatives to DBs as source of truth: e.g. config files, API servers, etc.

@commoddity commoddity merged commit bd4e0cf into main Sep 19, 2024
4 checks passed
@commoddity commoddity deleted the db-driver-and-cache branch September 19, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants