dwyl / hapi-postgres-connection

:zap: Creates a PostgreSQL Connection (Pool) available anywhere in your Hapi App

GNU General Public License v2.0

40 stars 13 forks source link

Make connection string compatible with pg-pool #29

Closed ddrager closed 7 years ago

ddrager commented 7 years ago

The update to make hapi-postgres-connection compatible with latest postgres module broke for me. As far as I can tell, if you are using pg-pool you can no longer pass a single connection url string to have it connect, per https://github.com/brianc/node-pg-pool/blob/master/README.md

Adding the database connection url to the connectionString parameter seems like the simple fix to me.

Tested using the latest pg 6.4.1 and pg-pool 1.8.

codecov[bot] commented 7 years ago

Codecov Report

Merging #29 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #29   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          32     33    +1     
=====================================
+ Hits           32     33    +1

Impacted Files	Coverage Δ
index.js	`100% <100%> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update dbbdbde...744ea8c. Read the comment docs.

naazy commented 7 years ago

Thanks for this @ddrager! Assign over when ready to be reviewed :smile:

ddrager commented 7 years ago

After I upgraded hapi-postgres-connection I started getting postgres connection issues, I believe this has to do with the attempted upgrade to make this npm compatible with pg 7.x and switching to pg.Pool.

In addition to updating the connection string there were some other areas of incompatibility with pg version 7 which are now fixed up.

Tests were failing but that was because travis was set to an old version of nodejs and pg 7 was incompatible. Updated to the latest versions so it will now test across nodejs versions 4/6/8.

ddrager commented 7 years ago

@naazy This is ready to go but I can't seem to assign the pull request over to you, or add tags. Also it needs version bump for npm.

Thanks!

nelsonic commented 7 years ago

@naazy & @roryc89 please check and confirm no changes required for OA if this PR is merged. thanks! 👍

naazy commented 7 years ago

Seems to be fine for OA @nelsonic :smile:

ddrager commented 7 years ago

Just wanted to bump this because I believe npm version 6.2.0 is actually broken, I had to manually specify hapi-postgres-connection@6.1.0 in order to get postgres connecting again.

roryc89 commented 7 years ago

Looks great. Thanks @ddrager ! merging

naazy commented 7 years ago

@ddrager @nelsonic is currently away but have assigned him https://github.com/dwyl/hapi-postgres-connection/issues/31 with a note to publish to npm on return 👍 . Will let you know when this is available on npm

ddrager commented 7 years ago

Thanks all! @naazy @nelsonic @roryc89