grpc-gateway/generator: respect full package #462
Conversation
|
Tested on our CI and it works as expected. Take into account that @tmc PTAL. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
do you agree @notbdu? :) |
|
Fine with me. Thanks @glerchundi. |
|
Wouldn't this change the output path for every file generated, not just the ones using go_package? |
|
@achew22 yep, it will. It's a breaking change. I can refactor to switch on a parameter or whatever and keep the old behaviour but the question is, what is desired? IMHO this is what end-user expects , me included. I can create another PR after this one to override WDYT? |
|
I don't think this is related to the PR itself...Flaky tests? IWOMM... |
|
@tmc what do you think about this? From my vantage it appears to be the correct behavior but I would like a 2nd set of eyes before we merge a breaking change. Also, once we merge this let's do a release. @glerchundi, could you add some tests to verify the behavior and prevent regression? |
|
@glerchundi, I just tagged a release which means now is a GREAT time to introduce breaking changes like this. Please update the PR with some tests and let's get it merged. |
|
Ok, i don’t have much time but give me a day and will try to do ASAP.
…On Wed, 8 Nov 2017 at 03:38, Andrew Z Allen ***@***.***> wrote:
@glerchundi <https://github.com/glerchundi>, I just tagged a release
which means now is a GREAT time to introduce breaking changes like this.
Please update the PR with some tests and let's get it merged.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#462 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACIPlotoPayi6JIiqEIYeSb0VzgclbOwks5s0RQYgaJpZM4PXHi5>
.
|
glerchundi
changed the title
|
@achew22 PTAL |
| func newExampleFileDescriptor() *descriptor.File { | ||
| return newExampleFileDescriptorWithGoPkg( | ||
| &descriptor.GoPackage{ | ||
| Path: "example.com/path/to/example/example.pb", |
There was a problem hiding this comment.
Shouldn't the file name end in .go?
There was a problem hiding this comment.
I don't think so.
I migrated an already existing example into a customisable function without doing any change to it but as you can observe in the two cases that I've implemented i used this descriptor as a 1-1 representation of the go_package proto option.
I tried to touch as less as possible :)
|
I have manually verified CLAs on glerchundi@ and notbdu@. Merging |
|
👏🏻👏🏻
…On Wed, 8 Nov 2017 at 21:29, Andrew Z Allen ***@***.***> wrote:
Merged #462 <#462>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#462 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACIPlmhVXbtS8Dsw-vbAF40DOU5v_UM2ks5s0g8TgaJpZM4PXHi5>
.
|
|
How can I disable this behavior? it broke my existing code :/ |
|
@c4milo can you give us more details? Example proto file which was working and not now, as well as an expected behaviour explanation. Although it should work as it was behaving before with just explicitly specifying the same |
|
I should have clarified a bit better. I have no issues with the generated code itself. I had code relying on the Swagger gateway generated code being placed in the same location as the code generated by |
grpc-gateway v1.3 doesn't contain grpc-ecosystem/grpc-gateway#462 which fixes a bug that prevents grpc-gateway from fully respecting the go_path option in proto files.
If you provide a go_package, your wishes will be reflected in the grpc-gateway output.
Rebased on top of master, done by @notbdu.
Superseeds #417.