Add support for username and password authentication #213

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@sixfeetover
Contributor

sixfeetover commented Jun 16, 2016

Fixes #200, and maintains support for auth keys. Also added tests for parsing URIs with and without usernames.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jun 16, 2016

Owner

Hey Jeff, thanks for the PR :)

I've modified your commit with the following:

  • Simplified the logic with password vs auth_key in the implementation (the driver is backward compatible with the auth_key) (and no need for ifs)
  • Remove the tests you wrote (they don't really do much: e.g. if the rethinkdb driver changes its implementation we will still go green -- and that's really what we need to test, but we won't because it involves reconfiguring the DB)
  • Added the environment variables RETHINKDB_USER and RETHINKDB_PASSWORD and made some tests for that

The commit is 0b3ca76

Let me know if you approve as the commit is in your name :)

Owner

nviennot commented Jun 16, 2016

Hey Jeff, thanks for the PR :)

I've modified your commit with the following:

  • Simplified the logic with password vs auth_key in the implementation (the driver is backward compatible with the auth_key) (and no need for ifs)
  • Remove the tests you wrote (they don't really do much: e.g. if the rethinkdb driver changes its implementation we will still go green -- and that's really what we need to test, but we won't because it involves reconfiguring the DB)
  • Added the environment variables RETHINKDB_USER and RETHINKDB_PASSWORD and made some tests for that

The commit is 0b3ca76

Let me know if you approve as the commit is in your name :)

@sixfeetover

This comment has been minimized.

Show comment
Hide comment
@sixfeetover

sixfeetover Jun 17, 2016

Contributor

Looks good to me! I wasn't sure how important it was to preserve auth key, so I took the conservative approach. Thanks for the quick review. :shipit:

Contributor

sixfeetover commented Jun 17, 2016

Looks good to me! I wasn't sure how important it was to preserve auth key, so I took the conservative approach. Thanks for the quick review. :shipit:

@nviennot nviennot closed this in 914bafa Jun 17, 2016

@nviennot

This comment has been minimized.

Show comment
Hide comment

meenie added a commit to MatchbookLabs/nobrainer that referenced this pull request Nov 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment