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

Adding support for Redis 6 ACL - Issue #2035 #2077

Merged
merged 17 commits into from
Mar 14, 2020
Merged

Adding support for Redis 6 ACL - Issue #2035 #2077

merged 17 commits into from
Mar 14, 2020

Conversation

tgrall
Copy link
Contributor

@tgrall tgrall commented Oct 13, 2019

This pull request adds support so ACL to Jedis (issue #2035), the following commands have been added:

  • ACL LIST (jedis.aclList())
  • ACL USERS (jedis.aclUsers())
  • ACL SETUSER (jedis.setUser(name, option1, options2, ...))
  • ACL GETUSER (jedis.aclGetUser(name))
  • ACL SETUSER (jedis.aclDelUser(name))
  • ACL CAT (jedis.aclCat())
  • ACL CAT (jedis.aclCat(cat))
  • ACL GENPASS (jedis.aclGenPass())
  • ACL WHOAMI (jedis.aclWhoAmI())

Not Implemented: ACL LOAD / ACL SAVE

Tested have been written to be ignored when the redis_version is smaller than 6.

I have duplicated the tests JedisPoolTest, JedisSentinelPoolTest, JedisTest, ShardedJedisPoolTest, SSLJedisTest, adding WithCompleteCredentials in the classname.

  • Maybe not the best approach please suggest if you have a better idea.

Also, I think we should, if people are using Jedis to create user and set ACL, create a small DSL/or set of Classes that would help the developers to use the setUser method.

Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Please see the comments.

@tgrall tgrall requested a review from sazzad16 October 16, 2019 07:02
@tgrall tgrall requested a review from sazzad16 October 18, 2019 19:21
@sazzad16 sazzad16 added this to the 3.2.0 milestone Oct 19, 2019
@sazzad16
Copy link
Collaborator

@tgrall Great!

@gkorland Do we really need this many new tests?

@sazzad16 sazzad16 requested a review from gkorland October 19, 2019 05:11
@hradilf
Copy link

hradilf commented Nov 12, 2019

@gkorland
Hello, any idea when this feature can be merged and v3.2.0 released? We would to use it. Thank you.

@hradilf
Copy link

hradilf commented Nov 22, 2019

Hi, @tgrall please, do you know when this PR can be merged and when new Jedis with the feature will be released? Thanks.

@gkorland
Copy link
Contributor

@tgrall can you please resolve the conflicts?

@tgrall
Copy link
Contributor Author

tgrall commented Dec 19, 2019

@tgrall can you please resolve the conflicts?

I will work on it during the hollidays

@tgrall
Copy link
Contributor Author

tgrall commented Dec 30, 2019

@gkorland I have solved the conflict and merge master to the PR branch

src/main/java/redis/clients/jedis/Protocol.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/Protocol.java Outdated Show resolved Hide resolved
@sazzad16
Copy link
Collaborator

@gkorland

Do we really need this many new tests?

WDYT?

@tgrall
Copy link
Contributor Author

tgrall commented Dec 31, 2019

@sazzad16 @gkorland :
the reason I have *duplicated" the test is mostly to keep all the existing test working without any changes on Redis 5, and have new test for 6 that are ignored when running on 5

@sazzad16
Copy link
Collaborator

@tgrall I understand. I was just asking @gkorland for his opinion about so many new tests. IMO, We don't need to test almost all existing features twice. Only some of the new tests. i.e. about 5%-10% of those should be enough to test the new feature.

@hradilf
Copy link

hradilf commented Jan 21, 2020

Hi @gkorland, please, could you add your review/comment to be able to move forward? PR is opened quite long. I would really appreciate to have available the feature soon. Thank you

@gkorland
Copy link
Contributor

gkorland commented Feb 2, 2020

@tgrall notice the use case of URI connection is not handled

@tgrall
Copy link
Contributor Author

tgrall commented Feb 2, 2020 via email

@tgrall
Copy link
Contributor Author

tgrall commented Feb 4, 2020

@gkorland I have added the support for URL auth.

@hradilf
Copy link

hradilf commented Feb 25, 2020

I think Redis 6 RC is available. When the new Jedis with ACL support will be available?

@gkorland gkorland requested a review from sazzad16 March 5, 2020 15:35
@sazzad16
Copy link
Collaborator

sazzad16 commented Mar 6, 2020

@gkorland @tgrall I'm mostly okay with the implementation. But,

  1. [An Objection] I don't think we should use the word Complete (in phrase WithCompleteCredentials).
  2. [Just an Idea] There are many new test classes focusing same objective. Can we move those in a separate package?

@sazzad16 sazzad16 dismissed their stale review March 6, 2020 12:49

Requests addressed.

@gkorland
Copy link
Contributor

gkorland commented Mar 14, 2020

@sazzad16

[An Objection] I don't think we should use the word Complete (in phrase WithCompleteCredentials).

Is it still relevant? Do you mean the names of the tests?

@sazzad16
Copy link
Collaborator

@gkorland

Is it still relevant? Do you mean the names of the tests?

Yes and yes.

But I'm merging it now and will try to change it myself.

@gkorland
Copy link
Contributor

@tgrall Thanks for the great contribution!

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

Successfully merging this pull request may close these issues.

4 participants