Skip to content

Conversation

ataft
Copy link
Contributor

@ataft ataft commented Nov 1, 2017

Description

Related issues

  • connect to <link_to_referenced_issue>

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide
@slnode
Copy link

slnode commented Nov 1, 2017

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Nov 1, 2017

Can one of the admins verify this patch?

1 similar comment
@slnode
Copy link

slnode commented Nov 1, 2017

Can one of the admins verify this patch?

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ataft Thank you for your contribution. I've left two comments which are not quite related to your PR, but would be great if you could fix them. If not, we could do them in a separate PR.

README.md Outdated
"host": "mydbhost",
"port": 5432,
"url": "postgres://admin:admin@myhost/db",
"url": "postgres://admin:admin@myhost/db?ssl=false",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not related to your change, but could you add the port in the url string and fix myhost and db to be mydbhost and db1 respectively? Here is an example complete url string:

const connectionString = 'postgresql://dbuser:secretpassword@database.server.com:3211/mydb' 
README.md Outdated
"host": "mydbhost",
"port": 5432,
"url": "postgres://admin:admin@myhost/db",
"url": "postgres://admin:admin@myhost/db?ssl=false",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. Could you please fix up the url string as I mentioned in my other comment?

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign the CLA :-)

@ataft
Copy link
Contributor Author

ataft commented Nov 1, 2017

Fixes made and CLA signed

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@b-admike
Copy link
Contributor

b-admike commented Nov 2, 2017

@slnode test please

@b-admike
Copy link
Contributor

b-admike commented Nov 2, 2017

@ataft Please squash your commits into one with the commit message of your first commit, and we should be able to land.

@ataft
Copy link
Contributor Author

ataft commented Nov 2, 2017

I can't figure out how to squash them. Can you just "Squash and Merge" when you do the merge? Thanks

@b-admike
Copy link
Contributor

b-admike commented Nov 6, 2017

@ataft No worries, I have squashed them for you. Here are the commands I ran for your future reference:

git remote add upstream https://github.com/strongloop/loopback-connector-postgresql git fetch -p upstream git rebase -i upstream/master git push origin +patch-1 

If you want detailed logs for the commands, I've put them in a gist here. Thank you again for your contribution! 🎉

@b-admike
Copy link
Contributor

b-admike commented Nov 6, 2017

@slnode test please

8 similar comments
@b-admike
Copy link
Contributor

b-admike commented Nov 6, 2017

@slnode test please

@rmg
Copy link
Contributor

rmg commented Nov 6, 2017

@slnode test please

@b-admike
Copy link
Contributor

b-admike commented Nov 7, 2017

@slnode test please

@b-admike
Copy link
Contributor

b-admike commented Nov 8, 2017

@slnode test please

@b-admike
Copy link
Contributor

b-admike commented Nov 9, 2017

@slnode test please

@b-admike
Copy link
Contributor

@slnode test please

@b-admike
Copy link
Contributor

@slnode test please

@b-admike
Copy link
Contributor

@slnode test please

@b-admike b-admike force-pushed the patch-1 branch 2 times, most recently from 82fd7b2 to 5f7a059 Compare November 14, 2017 21:13
@b-admike
Copy link
Contributor

@slnode test please

@b-admike b-admike merged commit 6caab41 into loopbackio:master Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants