-
Notifications
You must be signed in to change notification settings - Fork 44
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
use provided dname if available for _noproc
#660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you possibly also provide a test configuration that will test your assumption? Not sure if possible to do in github actions though.
@@ -67,7 +67,7 @@ def version(self) -> Any: | |||
# could be called before self.dbname will be created. | |||
# Use default postgres database | |||
with psycopg.connect( | |||
dbname="postgres", | |||
dbname=self.dbname or "postgres", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but dbname will never be empty 🤔 And we'll attempt to create the database in question.... most probably prior to it's creation 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. Indeed, _noproc
tries to create a database, and fails. I'd like to be able to use it with a basic database account, but the problem goes deeper than just connecting to the postgres
database :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With test file test.py
:
import pytest
from pytest_postgresql import factories
def test_hello(postgresql_noproc):
assert True
Run test against an existing database:
# create a basic user
createuser -W pytest # enter password: pytest
# with a database
createdb -O pytest pytest
# run test
pytest --postgresql-user=pytest --postgresql-password=pytest --postgresql-dbname=pytest --postgresql-use-database test.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be dependant on the new flag you've added. rather than the self.dbname being null which it will not 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. This means that the option has somehow two effects, but why not.
I've updated the patch.
- use provided dbname if available, instead of "postgres" - --postgresql-use-database skip creating/dropping the database
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll also need either a test case or test run that will exploit your use case, and a readme updated in the end.
@@ -44,13 +45,16 @@ def __init__( | |||
defaults to server's default | |||
:param connection_timeout: how long to retry connection before | |||
raising a TimeoutError | |||
:param use_database: whether to use an existing database | |||
(may imply manual cleanup) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes the janitor effectively using only a loader method that can do something 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay for now, just thinking out loud whether this should be extracted to something separate for this single responsibility later now 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Indeed this parameter makes the janitor out of a job, but I do not think it is worth changing the code structure to avoid it, I'd just let it be.
@@ -67,7 +67,7 @@ def version(self) -> Any: | |||
# could be called before self.dbname will be created. | |||
# Use default postgres database | |||
with psycopg.connect( | |||
dbname="postgres", | |||
dbname=self.dbname or "postgres", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be dependant on the new flag you've added. rather than the self.dbname being null which it will not 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zx80 the user still needs permissions to create template databases and regular databases to use this plugin then... And now I think that the use_database is the wrong approach here. We should use some parameter instead pointing to existing one it'll use to connect to perform maintenance tasks, but still the permissions to create databases is kind of required.
Addressed as part of #891 |
Fixes #653.
Changes proposed.