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: add pgx v5 support (#639) #642

Merged
merged 7 commits into from
Oct 12, 2023
Merged

feat: add pgx v5 support (#639) #642

merged 7 commits into from
Oct 12, 2023

Conversation

annafang-google
Copy link
Contributor

No description provided.

@annafang-google annafang-google marked this pull request as ready for review October 5, 2023 20:47
@annafang-google annafang-google requested a review from a team as a code owner October 5, 2023 20:47
@@ -162,6 +163,48 @@ func TestPostgresHook(t *testing.T) {
}
t.Log(now)
}
pgxv5.RegisterDriver("cloudsql-postgres")
Copy link
Member

Choose a reason for hiding this comment

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

Could we turn this into a table test?

https://semaphoreci.com/blog/table-driven-unit-tests-go

)

// Example shows how to use cloudsqlpostgres dialer
func ExamplePostgresConnection() {
Copy link
Member

Choose a reason for hiding this comment

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

We can omit this change -- it was a mistake to add it for pgxv4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it for both v4 and v5.

Copy link
Member

Choose a reason for hiding this comment

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

thank you

@enocom enocom changed the title chore: bump pgx version to v5 (#639) feat: add pgx v5 support (#639) Oct 11, 2023
@annafang-google
Copy link
Contributor Author

The update of the commit takes a long time to be propagated to the PR page. Will close it and reopen.

Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

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

One small change for named properties. I'll approve since it's a minor change.

IAMAuthN bool
}{
{
"cloudsql-postgres",
Copy link
Member

Choose a reason for hiding this comment

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

We should use named properties here:

{
    driver: "cloudsql-postgres",
    source: ... // etc
}

@annafang-google annafang-google merged commit 8d86d92 into main Oct 12, 2023
13 checks passed
@annafang-google annafang-google deleted the pgvx5 branch October 12, 2023 17:36
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.

None yet

2 participants