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

Adds ACL categories to each command #1291

Closed
wants to merge 1 commit into from
Closed

Adds ACL categories to each command #1291

wants to merge 1 commit into from

Conversation

itamarhaber
Copy link
Member

No description provided.

@oranagra
Copy link
Member

@itamarhaber i took a quick look at the changes, the diff is very hard to review, some commands seem to have moved to a different place in the file, makes it hard to review, but also introduces unnecessary conflicts for this and other PRs.

is it just about specifying the ACL categories for each command? or does this PR include other changes?
about the ACL categories, i'm not certain we wanna document them, same as command flags (ok-loading and such), they might change and we'll need to keep updating the docs.
instead there's a simple command that let's the people get the categories from a the actual server.

@zuiderkwast
Copy link
Contributor

Would we instead want to list the categories automatically by querying the local Redis instance? That would eliminate the need to update the docs when ACL categories change.

@oranagra
Copy link
Member

oranagra commented Aug 1, 2021

I don't think that's needed. I think what we have now is enough. If someone wants the exact details, depending on the version he uses, he should get that from Redis..
Like Salvatore said in the (now deleted) docs: listing them would be boring...

@zuiderkwast zuiderkwast added the to-be-closed should probably be dismissed sooner or later label Aug 1, 2021
@itamarhaber
Copy link
Member Author

Closing as this was implemented elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-be-closed should probably be dismissed sooner or later
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants