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

Add grpc.default_authority option for authorization #78

Merged
merged 6 commits into from
Oct 5, 2020
Merged

Add grpc.default_authority option for authorization #78

merged 6 commits into from
Oct 5, 2020

Conversation

jcai
Copy link
Contributor

@jcai jcai commented Oct 3, 2020

Some hostie server don't use different port for service,It can use the authority to identify user.

@CLAassistant
Copy link

CLAassistant commented Oct 3, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

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

Thank you very much for adding this grpc.default_authority option for authorization!

Could you please create an issue and explain your design in detail about why you want to add this new feature, and how to use this new feature?

Especially, does this new feature will introduce breaking changes from our previous version?

Thank you very much!

src/client/puppet-hostie.ts Outdated Show resolved Hide resolved
@jcai
Copy link
Contributor Author

jcai commented Oct 4, 2020

I can use the authority to identify current user in gRPC server.

@huan
Copy link
Member

huan commented Oct 5, 2020

Yes, I understand that we can use the authority to identify the current user in the gRPC server.

Could you please confirm that: does this new feature will introduce any breaking changes from our previous version?

@jcai
Copy link
Contributor Author

jcai commented Oct 5, 2020

Yes, I understand that we can use the authority to identify the current user in the gRPC server.

Could you please confirm that: does this new feature will introduce any breaking changes from our previous version?

Because the default gRPC server ignore the option, it can't break any previous version.

@jcai jcai requested a review from huan October 5, 2020 09:23
@huan
Copy link
Member

huan commented Oct 5, 2020

Thank your very much for the explanation, and it is great to know that this new feature will not break any previous code!

I'd love to merge this great new feature to the grpc service for testing. However, could you please add some documentation to our README before we can merge it, so that other developers will be able to learn this new feature easily?

We will merge this PR after it is documentated, appreciate for your effort for make this module better!

@jcai
Copy link
Contributor Author

jcai commented Oct 5, 2020

I had added some documentation for version 0.10.4.

Copy link
Member

@huan huan left a comment

Choose a reason for hiding this comment

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

LGTM

@huan huan merged commit 450a326 into wechaty:master Oct 5, 2020
@huan
Copy link
Member

huan commented Oct 5, 2020

Merged. Cheers!

@jcai
Copy link
Contributor Author

jcai commented Oct 5, 2020

@huan Thank you very much!Now bot can use the version 0.10.4 to connect our new gRPC server.The examples/ding-dong-bot works well!!!! Cheers!

@huan
Copy link
Member

huan commented Oct 8, 2020

Your are welcome!

BTW: it will be better if we can create an issue for any pull requests, and then link all related issues/prs to that issue, instead link to a Pull Request.

Because the issue is created for discussing the feature/problem, but the PR is created for discussing specific code changes.

Link to #124

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.

None yet

3 participants