Replace PostgresqlConnector with ConnectionStringProvider#18
Replace PostgresqlConnector with ConnectionStringProvider#18andrew-corbalt wants to merge 13 commits intomainfrom
Conversation
jonahb
left a comment
jonahb
left a comment
There was a problem hiding this comment.
This looks good. I like the new approach. I guess it'd also be worth thinking about whether we want to tweak things per Slack thread and how this change might fit into that.
andrew-corbalt
changed the title
hundt-corbalt
left a comment
hundt-corbalt
left a comment
There was a problem hiding this comment.
Code LGTM! But PR description still refers to an older version of the code. Can you update that?
done |
andrew-corbalt
dismissed
jonahb’s stale review
Code has been rewritten since the requested change was submitted. Feel free to re-review the new version that was made with Chris' input!
pgutils/connector.go
Outdated
| // | ||
| // IAM example 2 (cross-account): | ||
| // | ||
| // postgres+rds-iam://user@host:5432/dbname?assume_role_arn=...&assume_role_session_name=... |
There was a problem hiding this comment.
replace host:5432 with <rds-endpoint>?
to be consistent with the README in professor-mac repo
pgutils/connector.go
Outdated
| // | ||
| // IAM example 1: | ||
| // | ||
| // postgres+rds-iam://user@host:5432/dbname |
There was a problem hiding this comment.
replace host:5432 with <rds-endpoint> to be consistent with professor-mac README?
| port = defaultPostgresPort | ||
| } | ||
|
|
||
| // Match libpq/psql defaulting: if dbname isn't specified, dbname defaults to username. |
There was a problem hiding this comment.
What does this mean: Match libpq/psql defaulting. Sounds cryptic to me.
Also, why set a default? Why not force the user to set this?
There was a problem hiding this comment.
the words after the ':' describe in more details what that means. if dbname isn't specified, dbname defaults to username. Is that confusing?
I mean we historically have relied on this all over the place. We have lots of connection strings that don't have /postgres at the end to specify the database, instead we rely on lib/psql behavior where if the username is 'postgres' then the database will be set to 'postgres' if you don't set it.
|
|
||
| // ConnectDB opens a connection using the connector and verifies it with a ping | ||
| func ConnectDB(conn driver.Connector) (*sqlx.DB, error) { | ||
| sqlDB := sql.OpenDB(conn) |
There was a problem hiding this comment.
This is less code:
func ConnectDB(conn driver.Connector) (*sqlx.DB, error) {
db := sqlx.NewDb(sql.OpenDB(conn), "postgres")
if err := db.Ping(); err != nil {
db.Close()
return nil, err
}
return db, nil
}
You don't need the intermediate variable sqlDB.
| }, | ||
| return &rdsIAMConnectionStringProvider{ | ||
| Region: awsCfg.Region, | ||
| RDSEndpoint: net.JoinHostPort(host, port), |
There was a problem hiding this comment.
Here you have Endpoint including port.
You mentioned that RDSEndpoint does not include port.
4 participants
PR Description
To simplify handling both RDS IAM auth and conventional password auth postgres connections, PostgresqlConnector is replaced with ConnectionStringProvider which adds the concept of a custom DSN
postgres+rds-iam://<user>@<host>[:<port>]/<dbname>for IAM auth. Alternatively, a normal postgres DSN can be used with conventional password auth.Tests:
Updated the ProfessorMAC app and microservices to use ConnectionStringProvider and verified function
PR Checklist
Examples:
To provide feedback on this template, visit https://docs.google.com/document/d/1YfTv7Amyop5G_8w1c2GJ_Mu-70L0KkZHhm9f9umDi3U/edit