Skip to content
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

added username+password auth support #310

Merged
merged 2 commits into from
Jun 3, 2022
Merged

added username+password auth support #310

merged 2 commits into from
Jun 3, 2022

Conversation

OldManFroggy
Copy link
Contributor

Fixes issue #300

You can now use redis client with cloud hosted redis (redis enterprise/cloud stack) or any other redis server that has user+password authentication.

PR is not breaking, but has changed RedisConnectionOptions to support an optional username: string property.

Tests not included since, only tests that make sense require a redis instances setting up with:

  • password only (auth token only)
  • username + password
  • no auth

I did not see mock redis server for this in tests, and existing test for authentication is only checking if a parameter has been passed and is valid. Which exception handling catches anyway.

ps. been a while since I did a PR. (also note, I have this code in production, be nice to use official repo for this. )

@uki00a uki00a linked an issue Jun 3, 2022 that may be closed by this pull request
Copy link
Member

@uki00a uki00a left a comment

Choose a reason for hiding this comment

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

@scott-ling Thanks for working on this!

I just left one comment, other than that it looks good overall! 👍

@@ -24,6 +24,7 @@ export interface RedisConnectionOptions {
tls?: boolean;
db?: number;
password?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Could you run deno fmt on this file?

$ deno fmt connection.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done and fixed, pushing shortly darn deno fmt bit me again...

@uki00a uki00a added the enhancement New feature or request label Jun 3, 2022
@OldManFroggy
Copy link
Contributor Author

there ya go, sorry about that

@OldManFroggy OldManFroggy requested a review from uki00a June 3, 2022 13:38
Copy link
Member

@uki00a uki00a left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution! 👍

I'll release a new version in the coming days

@uki00a uki00a merged commit 84184f8 into denodrivers:master Jun 3, 2022
@OldManFroggy
Copy link
Contributor Author

OldManFroggy commented Jun 3, 2022 via email

@uki00a
Copy link
Member

uki00a commented Jun 4, 2022

Released in v0.26.0!

@OldManFroggy
Copy link
Contributor Author

OldManFroggy commented Jun 4, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to connect to Redis with correct credentials
2 participants