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.

Sign up for GitHub

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

Jump to bottom

[STORM-2626] Provided a template for drpc-auth-acl.yaml #2207

Merged
merged 1 commit into from
Jul 24, 2017
Merged

[STORM-2626] Provided a template for drpc-auth-acl.yaml #2207

merged 1 commit into from
Jul 24, 2017

Conversation

liu-zhaokun
Copy link
Contributor

https://issues.apache.org/jira/browse/STORM-2626

The default value of drpc.authorizer.acl.filename in defaults.yaml is "drpc-auth-acl.yaml",so I think we should provided a template for drpc-auth-acl.yaml.

@liu-zhaokun
Copy link
Contributor Author

@HeartSaVioR
I am so sorry to bother you.Do you have time to help me review it?

@liu-zhaokun
Copy link
Contributor Author

Can one of the admins verify this patch?

@liu-zhaokun
Copy link
Contributor Author

@vesense
I am so sorry to bother you.Do you have time to help me review it?

@liu-zhaokun
Copy link
Contributor Author

@harshach
I am so sorry to bother you.Do you have time to help me review it?

@liu-zhaokun
Copy link
Contributor Author

@HeartSaVioR
Do you have time to help me review this PR,it's about drpc acl?

Ethanlm
Ethanlm reviewed Jul 21, 2017
View reviewed changes
Copy link
Contributor

@Ethanlm Ethanlm left a comment

Choose a reason for hiding this comment

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

I would prefer using "functionName1", "functionName2" instead of "jump" or "partial".
The comment seems mismatch with the configurations: "bob" is a not invocation user

Ethanlm
Ethanlm reviewed Jul 21, 2017
View reviewed changes
conf/drpc-auth-acl.yaml.example Outdated


# For the function "jump", alice can perform client operations, and bob can
# perform invocation operations.User should replace "jump","alice","bob","charlie" with their own value
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I should have stated it here. "bob" is not a invocation user in your example.

@liu-zhaokun
Copy link
Contributor Author

@Ethanlm
Thanks for your reply.I will modify it follow your suggestion.

@liu-zhaokun
Copy link
Contributor Author

@Ethanlm
Hi,could you help me review the new change?

@Ethanlm
Copy link
Contributor

Ethanlm commented Jul 21, 2017

Thanks. @liu-zhaokun . I was just thinking that adding some instructions on SECURITY.md about DRPC is better than adding one more file in the conf directory. It would make me nervous to see some many configuration files if I'm just a user. People don't even need to care about DRPC configuration if they are not using it. I would like to hear other people's opinions about this.

@liu-zhaokun
Copy link
Contributor Author

@Ethanlm
Ok,thank you.

@liu-zhaokun
Copy link
Contributor Author

@revans2
Could you help me review this PR，there are no committer to review it all the time.

revans2
revans2 approved these changes Jul 24, 2017
View reviewed changes
Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

+1 the change looks fine to me.

@revans2
Copy link
Contributor

revans2 commented Jul 24, 2017

I do agree with @Ethanlm that updating SECURITY.md would probably be more helpful, but if you want this to be a first step then I am happy to merge it in.

@liu-zhaokun
Copy link
Contributor Author

@revans2
You are right.I created several PRs which has been mergerd that is related to ACL. I will update SECURITY.md in a new PR with these changes later.Could you help me merge this PR firstly?

@asfgit asfgit merged commit 1deab8d into apache:master Jul 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ethanlm Ethanlm Ethanlm left review comments

@revans2 revans2 revans2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liu-zhaokun @Ethanlm @revans2 @asfgit