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 plugin param 'allow_delete_body' #280

Merged
merged 6 commits into from
Jan 9, 2017
Merged

Conversation

msample
Copy link
Contributor

@msample msample commented Dec 11, 2016

Allows GRPC REST mappings for legacy services that inappropriately use HTTP DELETE with a body
Issue #279

… legacy services that inapproriately use HTTP DELETE with body
@tmc
Copy link
Collaborator

tmc commented Jan 5, 2017

@msample thank you for your contribution. This code looks good -- what are your thoughts on if this flag should be commented?

@msample
Copy link
Contributor Author

msample commented Jan 5, 2017

@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).

@tmc
Copy link
Collaborator

tmc commented Jan 5, 2017

I'm fine with leaving it undocumented. Other thoughts @achew22, @yugui, @t-yuki?

@achew22
Copy link
Collaborator

achew22 commented Jan 7, 2017

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?

@msample
Copy link
Contributor Author

msample commented Jan 7, 2017

@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
@msample
Copy link
Contributor Author

msample commented Jan 9, 2017

Two things I left as-is that seem suspect in protoc-gen-swagger/main.go. Some might depend on this behaviour:

if *file != "stdin" {
		f, _ = os.Open("input.txt") // <---" input.txt"? not *file?
	}
  1. support of pkg map parameters. "Mfoo/bar/baz.proto=github.com/proj/with/gen/baz-pb-go" seems unneeded for generating swagger. Could break some makefiles if it becomes a param error tho.

@tmc
Copy link
Collaborator

tmc commented Jan 9, 2017

@msample that does appear suspect, thanks for pointing that out. Can you comment again to kick googlebot?

@msample
Copy link
Contributor Author

msample commented Jan 9, 2017

CLA check kick.

@tmc
Copy link
Collaborator

tmc commented Jan 9, 2017

@msample I like the kvvar refactor but can we separate that into a separate pr?

@msample
Copy link
Contributor Author

msample commented Jan 9, 2017

@tmc sure, not a problem. Should probably be applied to both protoc_gen_* anyway.

@msample
Copy link
Contributor Author

msample commented Jan 9, 2017

@tmc kvvar removed.

@googlebot
Copy link

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.

@msample
Copy link
Contributor Author

msample commented Jan 9, 2017

last commit under my work email. Have added CLA to that.

@googlebot
Copy link

CLAs look good, thanks!

@tmc
Copy link
Collaborator

tmc commented Jan 9, 2017

@msample thanks!

@tmc tmc merged commit 2dbcd36 into grpc-ecosystem:master Jan 9, 2017
flisky added a commit to flisky/grpc-gateway that referenced this pull request Feb 28, 2017
flisky added a commit to flisky/grpc-gateway that referenced this pull request Mar 1, 2017
@tamalsaha tamalsaha mentioned this pull request Mar 30, 2017
1 task
tmc pushed a commit that referenced this pull request Apr 25, 2017
tmc pushed a commit to tmc/grpc-gateway that referenced this pull request May 12, 2017
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* 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
adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
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.

5 participants