-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 plugin param 'allow_delete_body' #280
Conversation
… legacy services that inapproriately use HTTP DELETE with body
@msample thank you for your contribution. This code looks good -- what are your thoughts on if this flag should be commented? |
@tmc hopefully most people won't need it, but I'd be happy to update wherever flags are externally documented if you want. Please point me at right thing to edit. Alternatively, we could upgrade the error message that is currently printed when you try to use a DELETE with a body to mention this option (gets to be a bit of a drag if we add more cases that care if DELETE has a body though). |
I like it as an undocumented feature. @msample could you add a couple of tests to verify that it does what you need when the flag is set? |
@achew22 sounds good. I'll try to get to it this weekend. |
…aram parsing and in registry service generation. Required mild refactoring of protoc-gen-swagger cmd line parsing to make testable
Two things I left as-is that seem suspect in protoc-gen-swagger/main.go. Some might depend on this behaviour:
|
@msample that does appear suspect, thanks for pointing that out. Can you comment again to kick googlebot? |
CLA check kick. |
@msample I like the kvvar refactor but can we separate that into a separate pr? |
@tmc sure, not a problem. Should probably be applied to both protoc_gen_* anyway. |
… impl from utilities pkg
@tmc kvvar removed. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
last commit under my work email. Have added CLA to that. |
CLAs look good, thanks! |
@msample thanks! |
* added plugin parameter 'allow_delete_body' to allow REST mappings for legacy services that inapproriately use HTTP DELETE with body * added comment to pubic registy method so lint doesn't complain * added tests for allow_delete_body both on command line/code-gen-req-param parsing and in registry service generation. Required mild refactoring of protoc-gen-swagger cmd line parsing to make testable * fixed copy paste oops on using param vs global flag.Commandline global * kvvar ignores whitespace & empty string Set() input now * removed protoc-gen-swagger pkg_map cmd line flag and KVVar flag.Value impl from utilities pkg
Allows GRPC REST mappings for legacy services that inappropriately use HTTP DELETE with a body
Issue #279