-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 example to illustrate the use of file watcher interceptor #7226
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good. Just some minor nits. Thanks for doing this.
@easwars Comments addressed. Please take another look when you get time. Thanks. |
@gtcooke94 Can you take a second pass on this review? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! A few comments
examples/features/authz/README.md
Outdated
``` | ||
|
||
The client will first hit `codes.PermissionDenied` error when invoking | ||
`UnaryEcho` although a legit username (`super-user`) is associated. This is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: UnaryEcho
although a legitimate username (super-user
) is associated with the RPC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
) | ||
|
||
var ( | ||
port = flag.Int("port", 50051, "the port to serve on") | ||
port = flag.Int("port", 50051, "the port to serve on") | ||
authzOpt = flag.String("authz-option", authzOptStatic, "the authz option (static or file watcher)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: file watcher
-> filewatcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
switch *authzOpt { | ||
case authzOptStatic: | ||
// Create an authorization interceptor using a static policy. | ||
staticInt, err := authz.NewStatic(authzPolicy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer the full name Interceptor
rather than shortening to Int
, please change throughout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
} | ||
unaryAuthzInt, streamAuthzInt = fileWatcherInt.UnaryInterceptor, fileWatcherInt.StreamInterceptor | ||
default: | ||
log.Fatalf("Invalid authz option: %s", *authzOpt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this example server is specifically for authz, let's put this check earlier (after parsing flags), then error log and exit the process if an invalid authz option is specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@gtcooke94 Comments addressed. Please take another look when you get time. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the PR!
Summary of changes:
Fixes #5900
RELEASE NOTES: none