-
Notifications
You must be signed in to change notification settings - Fork 50
Bug #309 unable to set password fix #313
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
Bug #309 unable to set password fix #313
Conversation
src/pytest_postgresql/executor.py
Outdated
) | ||
password_file.write(self.password) | ||
# Without seek(0), PostgreSQL will se password file as empty | ||
password_file.seek(0) |
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.
flush, we don't need to read, so no need to seek
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.
Right, I changed that to flush()
src/pytest_postgresql/executor.py
Outdated
with tempfile.NamedTemporaryFile(mode='w') as password_file: | ||
init_directory += ( | ||
'--pwfile "%s"' % password_file.name, | ||
'-o "--auth=trust --username={} --pwfile={}"'.format( |
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.
you can have many -o outputs, add here only the pwfile
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.
Didn't know about that! Surely that simplifies code. Added.
src/pytest_postgresql/executor.py
Outdated
|
||
if self.password: | ||
with tempfile.NamedTemporaryFile() as password_file: | ||
with tempfile.NamedTemporaryFile(mode='w') as password_file: |
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.
when flushing, the mode=w is not needed.
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.
Yea, I've added that to make write
function accept password as str
. Now i saw that in your pull request you've added password encoding in init so I decided to leave the default mode. If you think it is not a good idea to leave it not working without your pr then tell me about it and I will change.
tests/test_postgresql.py
Outdated
assert postgresql_rand.status == psycopg2.extensions.STATUS_READY | ||
|
||
|
||
def test_rand_postgres_port(postgresql_password): |
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 think I'd love specific executor test, but I already have one on other Pull request. This is great as well!
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.
Just the name overshadows the previous one
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.
Oh, yes, sorry. Didn't see that. Fixed.
src/pytest_postgresql/executor.py
Outdated
self.executable, 'initdb', | ||
'-o "--auth=trust --username=%s"' % self.user, | ||
'-D %s' % self.datadir, | ||
self.executable, 'initdb', '-D {}'.format(self.datadir) |
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.
prefer f strings if migrating to other formatting type.
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.
Sure, changed.
…ry write mode from tempfile, changed output flag to multiple ones
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.
As I see it just the test name to be fixed. Other than that it's all good.
tests/test_postgresql.py
Outdated
assert postgresql_rand.status == psycopg2.extensions.STATUS_READY | ||
|
||
|
||
def test_rand_postgres_port(postgresql_password): |
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.
Just the name overshadows the previous one
@fizyk Could you resolve the merge conflicts please? I see that someone has made some fixed before me and i think it's better if you decide which of the solutions is better as you are the member of a core team. |
@wolkiewiczk I will look at it, just got back from time off :) |
@wolkiewiczk actually both are fine. I tried to rebase but messed something terribly during. So I think I'll leave as it is. Sorry for that. But you did good job anyway. Thank you! |
Fixes #309 .
I was not able to run tests locally because of my postgresql broke from unknown reason. I am trying to fix it but decided to make a pull request anyway so you can check it. There was more to change than i previously thought but hope everything is ok. Please check my code and leave your comments if something is not ok.