-
Notifications
You must be signed in to change notification settings - Fork 233
Add an auth read-replica database client #1759
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
base: main
Are you sure you want to change the base?
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ebcccb8f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| github.com/aws/aws-sdk-go-v2/service/s3 v1.79.3 | ||
| github.com/bits-and-blooms/bitset v1.22.0 | ||
| github.com/dchest/uniuri v1.2.0 | ||
| github.com/e2b-dev/infra/packages/db v0.0.0-20251013083250-eb6cd250d671 |
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 wanted to remove the shared dependency on db (it was circular)
| } | ||
| } | ||
|
|
||
| func FromDB(build queries.EnvBuild) MachineInfo { |
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.
moved to db package
dobrac
left a comment
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, jsut few nits
| authDB, err := authdb.NewClient( | ||
| ctx, | ||
| config.AuthDBConnectionString, | ||
| config.AuthDBReadReplicaConnectionString, |
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.
do I understand correctly that if this is "" the AuthDBConnectionString will be used instead?
|
|
||
| func NewClient(ctx context.Context, databaseURL, replicaURL string, options ...pool.Option) (*Client, error) { | ||
| if strings.TrimSpace(databaseURL) == "" { | ||
| logger.L().Error(ctx, "POSTGRES_CONNECTION_STRING is required") |
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.
do we need this log line? Also, is the POSTGRES_CONNECTION_STRING valid requirement here?
| readPool, err = pool.New(ctx, replicaURL, options...) | ||
| if err != nil { | ||
| writePool.Close() | ||
| logger.L().Error(ctx, "Unable to create read connection pool", zap.Error(err)) |
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 probably don't need this log line either
| package: "authqueries" | ||
| out: "pkg/auth/queries/" | ||
| sql_package: "pgx/v5" | ||
| overrides: |
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.
do we need all of these override in the auth db?
Note
Adds a separate auth database client with optional read-replica and refactors services to use it.
packages/db/pkg/authclient exposingRead/Writequery sets; sharedpoolfor PG connections;client.NewClientsignature now requires a connection stringauthdb(Readfor lookups,Writefor mutations); updates config to supportAUTH_DB_CONNECTION_STRINGandAUTH_DB_READ_REPLICA_CONNECTION_STRINGwith pool sizingpostgres-read-replica-connection-stringsecret and plumbs it through modulesauthdb; tests and seed scripts updated to use new clients; addedpackages/dbseed-dbtarget and referenced from self-host docspackages/db/pkg/builds; adjust imports and go.mod entriesWritten by Cursor Bugbot for commit 72ac884. This will update automatically on new commits. Configure here.