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

examples: add an example to illustrate authorization (authz) support #5920

Merged
merged 18 commits into from
Mar 2, 2023

Conversation

KenxinKun
Copy link
Contributor

@KenxinKun KenxinKun commented Jan 10, 2023

Partially addresses #5900 using a hard coded string.

RELEASE NOTES:

  • examples: add an example to illustrate authorization (authz) support

examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
@KenxinKun
Copy link
Contributor Author

FYI pushed a few changes, not finished with all the feedback yet :)

@KenxinKun
Copy link
Contributor Author

@easwars Do you prefer to resolve the comments yourself on review, or should I mark them msyelf? I've addressed them all but the one on the stream wrapper

@easwars
Copy link
Contributor

easwars commented Jan 11, 2023

@easwars Do you prefer to resolve the comments yourself on review, or should I mark them msyelf? I've addressed them all but the one on the stream wrapper

Thanks for asking. Yes, we prefer to resolve the comments ourselves, because if the author marks it as resolved, then the reviewer will have to go through the extra step of clicking through each of those comments to see if they were resolved appropriately.

Also, please let me know once you have addresses all comments and I will make another pass.

@KenxinKun
Copy link
Contributor Author

Hi @easwars just pushed my last changes and have addressed all comments, please go ahead for another review pass :)

examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/client/main.go Outdated Show resolved Hide resolved
easwars
easwars previously approved these changes Jan 17, 2023
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
@easwars easwars assigned KenxinKun and unassigned easwars Jan 17, 2023
@easwars easwars dismissed their stale review January 20, 2023 00:19

Approved by mistake. There are still comments to be addressed.

@easwars easwars changed the title Add authz example examples: add an example to illustrate authorization (authz) support Jan 20, 2023
@KenxinKun
Copy link
Contributor Author

Hi @easwars sorry for the delay, I've addressed all your points (left a question in #5920 (comment))
I think I've illustrated the behaviours you requested (and removed the unnecessary bits), let me know what you think :)

examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/client/main.go Show resolved Hide resolved
examples/features/authz/client/main.go Show resolved Hide resolved
examples/features/authz/client/main.go Show resolved Hide resolved
examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/client/main.go Outdated Show resolved Hide resolved
examples/features/authz/server/main.go Outdated Show resolved Hide resolved
examples/features/authz/token/token.go Outdated Show resolved Hide resolved
examples/features/authz/token/token.go Show resolved Hide resolved
@easwars
Copy link
Contributor

easwars commented Jan 25, 2023

I think we are quite close to having a good example now. Just some minor nits this time around. Thanks.

@easwars easwars self-assigned this Jan 25, 2023
@dfawley
Copy link
Member

dfawley commented Jan 25, 2023

cc @temawi

@arvindbr8 arvindbr8 modified the milestones: 1.53 Release, 1.54 Release Jan 26, 2023
@easwars
Copy link
Contributor

easwars commented Feb 2, 2023

@dfawley : Could you please take a look.

@dfawley dfawley self-requested a review February 7, 2023 18:34
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Thanks for adding the explanatory text! LGTM except for the two small nits.

examples/features/authz/README.md Outdated Show resolved Hide resolved
examples/features/authz/README.md Outdated Show resolved Hide resolved
@dfawley dfawley removed their assignment Feb 15, 2023
@dfawley
Copy link
Member

dfawley commented Feb 15, 2023

cc @temawi FYI

Copy link
Contributor

@easwars easwars 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 your contribution.

@easwars
Copy link
Contributor

easwars commented Feb 21, 2023

This looks good, but I'm unable to merge this at this point in time because @dfawley "requested changes" in his most recent review. Although you have addressed those changes, @dfawley is out this week. So, once he come back and marks the requested changes as addressed, we will be able to merge this.

Thanks again.

@easwars easwars assigned dfawley and unassigned easwars Feb 21, 2023
examples/features/authz/README.md Outdated Show resolved Hide resolved
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Thanks!

@dfawley dfawley merged commit 20141c2 into grpc:master Mar 2, 2023
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants