Fixes bug 1137879 add support for postgres:// or postgresql:// URLs #2647
Conversation
|
|
||
| from configman.config_manager import RequiredConfig | ||
| from configman import Namespace | ||
|
|
||
| urlparse.uses_netloc.append('postgres') | ||
| urlparse.uses_netloc.append('postgresql') | ||
|
|
There was a problem hiding this comment.
These might require a comment. Why is it needed? Also, the first part of a URI is usually called the "scheme" E.g. "ftp" but this talks about "netloc" which is usually the alias for "hostname".
|
Two things:
|
|
I see a potential problem. the PG connection context will likely be embedded in namespaces, perhaps several deep. that would mean that in order for configman to grab the value of the URL from the environment, the environment var would have to reflect that same nesting. Do you want to have this PG URL behavior for all the Socorro apps, or do you have it in mind just for one in particular? |
|
@twobraids we need support for this in all apps. I had originally made a similar change in In my AWS instance, I'm using the integration test command-line invocations that use |
|
Note from IRC conversation: |
| default='postgres://test:aPassword@localhost:5432/socorro_integration_test', | ||
| doc='Name of database to manage', | ||
| ) | ||
| required_config.add_option( |
There was a problem hiding this comment.
Why keep these old options then?
There was a problem hiding this comment.
I was trying not to make the heroku-specific stuff be mandatory. Having the password embedded in the URL might be an issue for some users as well.
There was a problem hiding this comment.
Using a URL isn't Heroku-specific. I use it whenever I can when I do non-django. The reason I like it is because it files into one environment variable instead of 5.
Also, wherever you type in database_password you'd also type in database_url.
Either way, I'm not convinced. But I am convinced that you know better. :)
My comment was just a question after all :)
There was a problem hiding this comment.
I think it's worth exploring the rest of the changes in another PR. It's just a lot of code change that touches a bunch of things like integration tests, so I'd like to take this step now, and rip out the rest of the stuff later.
That said.. I'll try it right now and see how many things need to be altered to support it.
There was a problem hiding this comment.
Exploration revealed depedencies on socorro.database.database in our tests and middleware. Going back to just supporting the minimal change of adding support for database_url without deprecating DSN, pending deprecation of socorro.database.database.
selenamarie
force-pushed
the
nobug-support-heroku-in-ext-postgres
branch
2 times, most recently
from
2911d61
to
516b799
Compare
| @@ -8,23 +8,27 @@ JENKINS_CONF=jenkins.py.dist | |||
|
|
|||
| ENV=env | |||
|
|
|||
| PYTHONPATH="." | |||
| PYTHONPATH='.' | |||
There was a problem hiding this comment.
Why this change? Why quotation at all?
There was a problem hiding this comment.
That's debugging cruft. There's no need for the quotes, so I'll just remove them when I add comments about deprecation.
|
r+ with some optional nits |
* remove testing change to database username * Move urlparse to Database object for commonconfig * Fix defaults for database_url and PEP8 * Use kwargs if available for postgres context manager * Updates rabbitmq integration test for database_url * Create a better URL based on --database_url * Set default for database_url in connection pooling to None * Add resource.postgresql.database_url to crontabber setup
selenamarie
force-pushed
the
nobug-support-heroku-in-ext-postgres
branch
from
9702b0a
to
9075998
Compare
|
I am replacing this PR with one that does not suffer override ambiguities. |
No description provided.