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

Fix postgres connection string #102

Merged
merged 2 commits into from
Feb 18, 2023

Conversation

ralfonso
Copy link
Contributor

This addresses the bug reported on the previous changeset that moved from a URI to a PostgreSQL specific space-delimited connection string.

Since this postgres storage type is perfectly fine for usage with CockroachDB, I've opted to revert back to the URI format since CockroachDB does not support to support the single quote form described in the libpq docs.

This change takes advantage of net/url's ability to escape characters in a URL's userinfo.

@ralfonso ralfonso requested a review from a team as a code owner February 16, 2023 00:10
want string
wantErr bool
}{
{
Copy link
Contributor

Choose a reason for hiding this comment

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

you missed your special use case with an empty password in the tests here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've amended the second commit and also used subtests to consume the name field from the test cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot reproduce the same test failure (tcp read).

On my Thinkpad, the tests pass via make test.

On my desktop, the tests time out, even on the master branch, even after extending the timeout to 20m.

Do you have any tips for reproducing this test environment or providing a way for me to re-run the Docker Build Action here on Github?

https://gist.github.com/ralfonso/d00c6a32feaae3dae83ca36f85152a53

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running docker system prune seems to have resolved the timeouts on my desktop. The only place this is failing is in GH Actions.

This allows us to take advantage of net/url's ability to escape
characters in username/password. It does not expose this feature via
exported functions in the vein of EscapePath and EscapeQuery. The rules
for escaping userinfo is slightly different.
@ralfonso ralfonso force-pushed the fix-postgres-connection-string branch from 9ce0f0b to 3618201 Compare February 16, 2023 17:14
@codecov-commenter
Copy link

Codecov Report

Merging #102 (97b4ecd) into master (161f882) will decrease coverage by 0.20%.
The diff coverage is 80.00%.

❗ Current head 97b4ecd differs from pull request most recent head 3618201. Consider uploading reports for the commit 3618201 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
- Coverage   70.69%   70.49%   -0.20%     
==========================================
  Files          10       10              
  Lines        1126     1132       +6     
==========================================
+ Hits          796      798       +2     
- Misses        210      212       +2     
- Partials      120      122       +2     
Impacted Files Coverage Δ
postgres.go 71.42% <80.00%> (-1.91%) ⬇️
memory.go 90.00% <0.00%> (-3.34%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@majst01 majst01 merged commit 94f5222 into metal-stack:master Feb 18, 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.

3 participants