-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 plugin directive #1694
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.
LGTM
P. S. Updating the old PR would have been good enough
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.
Great; just remove the extra line. I trust it's in the right place in the chain?
caddyhttp/httpserver/plugin.go
Outdated
@@ -490,6 +490,8 @@ var directives = []string{ | |||
"hugo", // github.com/hacdias/caddy-hugo | |||
"mailout", // github.com/SchumacherFM/mailout | |||
"awslambda", // github.com/coopernurse/caddy-awslambda | |||
|
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.
Let's take out this extra line.
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.
What is the best way to determine where in the chain are the best place to put the directive?
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.
Where ever it works correctly and doesn't break other middlewares in the chain. We can always move it later, but I always ask to double check and make sure that the author at least considered that the plugin is in the right place. Whatever makes the most logical sense in the order of things done by handlers, really.
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.
Okay. I have left it at the bottom of the chain for now. I tested it along with tls,log and prometheus plugins on my dev machine and it all works fine
Removed whitespace line
Cool, thanks. Just fix the spacing with the comment (gofmt -s) and I'll merge this in! (Check CI failures) |
@pieterlouw You may now deploy your plugin to the Caddy website! |
Thanks @mholt! I just registered it now. What's the steps if there's a new version of the plugin? Redeploy and then users need to build a new download of Caddy? |
@pieterlouw Congrats! And yep. |
1. What does this change do, exactly?
New directive for the http server type called
grpc
2. Please link to the relevant issues.
N/A
3. Which documentation changes (if any) need to be made because of this PR?
No document changes needed. Link to plugin's documentation --> here