Better password handling using pgpass

mbryzek / schema-evolution-manager

Schema Evolution Manager makes it very simple for engineers to contribute schema changes to a postgresql database, managing the schema evolutions as proper source code. Schema changes are deployed as gzipped tarballs named with the corresponding git tag.

Apache License 2.0

268 stars 47 forks source link

Better password handling using pgpass #32

Closed yonigoldberg closed 8 years ago

yonigoldberg commented 8 years ago

Inspired by #14

mbryzek commented 8 years ago

Love this change! Thanks for taking the time to put together the PR. This is going to be incredibly useful.

I think main pieces for us all to align on are param naming, making sure we never touch ~/.pgass, and maybe some more testing around going from the connection url to the pgpass file format.

yonigoldberg commented 8 years ago

Thanks for the prompt review and for the great feedback.

I think that I have addressed most of your concerns. Two open points:

I think that the updated approach on .pgpass is more reliable (won't nuke it anymore) and recommended (based on postgres docs) than using the EV. But if both of you (@mbryzek and @Seanstoppable) still think that I should go down the EV path I will make the change.
I am not sure why you are using the helper in Library.with_temp_file, since Ruby's Tempfile seems to do the same. I actually think that it will be cleaner to replace the helper with Ruby's Tempfile.

Thoughts?

yonigoldberg commented 8 years ago

Ok my last change is using the PGPASSFILE (not PGPASSWORD) and it doesn't touch at all the ~/.pgpass

@mbryzek - what else do you want me to address?

Seanstoppable commented 8 years ago

Yeah, my bad, I was trying to refer to PGPASSFILE. Seems like the best way to go here. :+1:

Some minor README language comments. I will leave you and @mbryzek to discuss the pros and cons about using tempfile.

mbryzek commented 8 years ago

I've taken a pass at refactoring:

https://github.com/mbryzek/schema-evolution-manager/pull/34

While many of the changes are largely subjective - this is the general style I would like to proceed in.

I've installed locally and would like to test locally over the next few days before releasing.

comments welcome