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

PostgreSQL dialog URL parser #346

Merged
merged 7 commits into from
Feb 17, 2019
Merged

Conversation

tombiscan
Copy link
Contributor

After some improvements from the #281 proposal have been implemented by @sio4 some nice options to improve the PG dialog have been opened.

lib/pg by default converts any provided URL type of string into specific connection string.

gobuffalo/pop by default parses the URL for PG. In practice, that means any attempt to use connection format string will fail due to the URL parsing something that's not a URL.
Notably, that also affects Google Cloud SQL, which uses the mentioned string format to connect.
Rationale and previous "quick fix" can be seen here: #281 (comment)

This PR:

  • implements URL parser for PostgreSQL by replicating the behaviour from lib/pg (and shamelessly copying few parser funcs from there)
  • the parser now sets all the additional options to ConnectionDetails.Options
  • adds some tests
  • "works for me" on GCP with App Engine Standard and Cloud SQL
  • helps with making Gobuffalo GAE compatible - Make Buffalo GAE compatible  buffalo#213

@tombiscan tombiscan requested a review from a team as a code owner February 8, 2019 17:24
@stanislas-m stanislas-m self-assigned this Feb 17, 2019
Copy link
Member

@stanislas-m stanislas-m left a comment

Choose a reason for hiding this comment

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

LGTM. :)

@stanislas-m stanislas-m merged commit 3e6cf4d into gobuffalo:development Feb 17, 2019
@tombiscan tombiscan deleted the pg-gcp branch February 18, 2019 20:15
@stanislas-m stanislas-m added this to the v4.10.0 milestone Mar 2, 2019
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.

2 participants