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

protoc-gen-swagger: Add ability to specify custom response objects #663

Merged
merged 2 commits into from Jun 20, 2018

Conversation

Projects
None yet
5 participants
@johanbrandhorst
Collaborator

johanbrandhorst commented May 31, 2018

Extends the openapiv2 protofile to allow control over response objects.

Fixes #304

@googlebot googlebot added the cla: yes label May 31, 2018

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst
Collaborator

johanbrandhorst commented May 31, 2018

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 31, 2018

Codecov Report

Merging #663 into master will decrease coverage by 1.36%.
The diff coverage is 13.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
- Coverage   57.78%   56.42%   -1.37%     
==========================================
  Files          30       30              
  Lines        2914     2995      +81     
==========================================
+ Hits         1684     1690       +6     
- Misses       1066     1141      +75     
  Partials      164      164
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 38.52% <13.15%> (-3.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5b66c1...be7eb36. Read the comment docs.

codecov-io commented May 31, 2018

Codecov Report

Merging #663 into master will decrease coverage by 1.36%.
The diff coverage is 13.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
- Coverage   57.78%   56.42%   -1.37%     
==========================================
  Files          30       30              
  Lines        2914     2995      +81     
==========================================
+ Hits         1684     1690       +6     
- Misses       1066     1141      +75     
  Partials      164      164
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 38.52% <13.15%> (-3.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5b66c1...be7eb36. Read the comment docs.

@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 May 31, 2018

Collaborator

I like this PR, I need to look at it in a bigger screen then my phone but it looks like a great additional piece of documentation to include

Collaborator

achew22 commented May 31, 2018

I like this PR, I need to look at it in a bigger screen then my phone but it looks like a great additional piece of documentation to include

key: "admin";
value: "Grants read and write access to administrative information";
}
info: {

This comment has been minimized.

@achew22

achew22 Jun 1, 2018

Collaborator

Out of curiosity, why did you change everything to be tabs? Can you run this through a modern version of clang-format?

@achew22

achew22 Jun 1, 2018

Collaborator

Out of curiosity, why did you change everything to be tabs? Can you run this through a modern version of clang-format?

This comment has been minimized.

@johanbrandhorst

johanbrandhorst Jun 1, 2018

Collaborator

Everything except for the examples added by @ivucica was already tabs, so I just made it consistent as it was driving me mad. I could format this (and the other protofiles) with prototool format 1 if you like, but I haven't touched the others.

@johanbrandhorst

johanbrandhorst Jun 1, 2018

Collaborator

Everything except for the examples added by @ivucica was already tabs, so I just made it consistent as it was driving me mad. I could format this (and the other protofiles) with prototool format 1 if you like, but I haven't touched the others.

This comment has been minimized.

@ivucica

ivucica Jun 4, 2018

Collaborator

(Sorry about this.)

@ivucica

ivucica Jun 4, 2018

Collaborator

(Sorry about this.)

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 4, 2018

Collaborator

Gonna see if I can add automatic references for namespaced messages as well, so please don't merge just yet.

Collaborator

johanbrandhorst commented Jun 4, 2018

Gonna see if I can add automatic references for namespaced messages as well, so please don't merge just yet.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 4, 2018

Collaborator

Actually, go ahead and merge if it's good, I'll make another PR for the additional functionality.

Collaborator

johanbrandhorst commented Jun 4, 2018

Actually, go ahead and merge if it's good, I'll make another PR for the additional functionality.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 4, 2018

Collaborator

Wasn't too hard it turns out :)

Collaborator

johanbrandhorst commented Jun 4, 2018

Wasn't too hard it turns out :)

@ivucica

Mostly in favor. I have just a few nitpicks, and a few requests to evaluate code reuse.

Thanks for this!

Show outdated Hide outdated examples/proto/examplepb/a_bit_of_everything.proto Outdated
Show outdated Hide outdated protoc-gen-swagger/genswagger/template.go Outdated
}
}
func protoExternalDocumentationToSwaggerExternalDocumentation(in *swagger_options.ExternalDocumentation) *swaggerExternalDocumentationObject {

This comment has been minimized.

@ivucica

ivucica Jun 4, 2018

Collaborator

This comment has been minimized.

@johanbrandhorst

johanbrandhorst Jun 5, 2018

Collaborator

I think I squashed these.

@johanbrandhorst

johanbrandhorst Jun 5, 2018

Collaborator

I think I squashed these.

Show outdated Hide outdated protoc-gen-swagger/genswagger/template_test.go Outdated
Show outdated Hide outdated protoc-gen-swagger/options/openapiv2.proto Outdated
Show outdated Hide outdated protoc-gen-swagger/options/openapiv2.proto Outdated
}
if protoSchema.Description != "" {
schema.Description = protoSchema.Description
}

This comment has been minimized.

@johanbrandhorst

johanbrandhorst Jun 5, 2018

Collaborator

I added these because I need them :P

@johanbrandhorst

johanbrandhorst Jun 5, 2018

Collaborator

I added these because I need them :P

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 6, 2018

Collaborator

@ivucica are you happy with the changes? @achew22 could you rerun the failure please, seems like a github TLS timeout error?

Collaborator

johanbrandhorst commented Jun 6, 2018

@ivucica are you happy with the changes? @achew22 could you rerun the failure please, seems like a github TLS timeout error?

@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 Jun 6, 2018

Collaborator

Retriggered

Collaborator

achew22 commented Jun 6, 2018

Retriggered

@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 Jun 6, 2018

Collaborator

I think the error comes from an update to the protobuf generator. Could you update your protoc and go plugin and regenerate with make examples then upload again?

I expect it should add the following

index cb6fb3a..d416004 100644
--- a/examples/clients/abe/examplepb_a_bit_of_everything.go
+++ b/examples/clients/abe/examplepb_a_bit_of_everything.go
@@ -14,6 +14,7 @@ import (
 	"time"
 )
 
+// Intentionaly complicated message type to cover much features of Protobuf.
 type ExamplepbABitOfEverything struct {
 
 	SingleNested ABitOfEverythingNested `json:"single_nested,omitempty"`
Collaborator

achew22 commented Jun 6, 2018

I think the error comes from an update to the protobuf generator. Could you update your protoc and go plugin and regenerate with make examples then upload again?

I expect it should add the following

index cb6fb3a..d416004 100644
--- a/examples/clients/abe/examplepb_a_bit_of_everything.go
+++ b/examples/clients/abe/examplepb_a_bit_of_everything.go
@@ -14,6 +14,7 @@ import (
 	"time"
 )
 
+// Intentionaly complicated message type to cover much features of Protobuf.
 type ExamplepbABitOfEverything struct {
 
 	SingleNested ABitOfEverythingNested `json:"single_nested,omitempty"`
@ivucica

ivucica approved these changes Jun 6, 2018

@ivucica

This comment has been minimized.

Show comment
Hide comment
@ivucica

ivucica Jun 6, 2018

Collaborator

No objections on my part as long as the tests are fixed.

Collaborator

ivucica commented Jun 6, 2018

No objections on my part as long as the tests are fixed.

@achew22

achew22 approved these changes Jun 6, 2018

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 6, 2018

Collaborator

Cool, I think I should have fixed the failing test, lets get this in :D.

Collaborator

johanbrandhorst commented Jun 6, 2018

Cool, I think I should have fixed the failing test, lets get this in :D.

@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 Jun 6, 2018

Collaborator

Looks like there are a few remaining issues. Can you give it another whack?

Collaborator

achew22 commented Jun 6, 2018

Looks like there are a few remaining issues. Can you give it another whack?

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 6, 2018

Collaborator

@achew22 I can't seem to get swagger-codegen installed - I hate to be a bother but any chance you could just clone and regenerate the examples? I'll take another stab tomorrow at work but haven't got time now.

Collaborator

johanbrandhorst commented Jun 6, 2018

@achew22 I can't seem to get swagger-codegen installed - I hate to be a bother but any chance you could just clone and regenerate the examples? I'll take another stab tomorrow at work but haven't got time now.

@ivucica

This comment has been minimized.

Show comment
Hide comment
@ivucica

ivucica Jun 7, 2018

Collaborator
Collaborator

ivucica commented Jun 7, 2018

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 7, 2018

Collaborator

@ivucica thanks I will take a look through the logs. I tried using their docker image and it didn't work.

Collaborator

johanbrandhorst commented Jun 7, 2018

@ivucica thanks I will take a look through the logs. I tried using their docker image and it didn't work.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 7, 2018

Collaborator

Alright I should've gotten them this time. Sure is fiddly 🤔.

Collaborator

johanbrandhorst commented Jun 7, 2018

Alright I should've gotten them this time. Sure is fiddly 🤔.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 7, 2018

Collaborator

OK so I had some time to look into recursive resolution of manually referenced types - I got it working but I can't help but feel like it's a bit hacky. I'd be happy with any tips on improving it, but basically it required me to keep a separate set of referenced definitions and render them after everything else, recursively.

Collaborator

johanbrandhorst commented Jun 7, 2018

OK so I had some time to look into recursive resolution of manually referenced types - I got it working but I can't help but feel like it's a bit hacky. I'd be happy with any tips on improving it, but basically it required me to keep a separate set of referenced definitions and render them after everything else, recursively.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 11, 2018

Collaborator

Bump - any thoughts on the additions?

Collaborator

johanbrandhorst commented Jun 11, 2018

Bump - any thoughts on the additions?

@ivucica

A few nits to correct and I will approve.

As they're tiny issues, I would be willing to approve even as is.

Show outdated Hide outdated examples/clients/abe/examplepb_a_bit_of_everything.go Outdated
Show outdated Hide outdated examples/proto/examplepb/a_bit_of_everything.proto Outdated
Show outdated Hide outdated protoc-gen-swagger/genswagger/template.go Outdated
Show outdated Hide outdated protoc-gen-swagger/genswagger/template.go Outdated
protoc-gen-swagger: Add ability to specify custom response objects
Extends the openapiv2 protofile to allow control over response objects.

Fixes #304
@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 15, 2018

Collaborator

Made changes as requested

Collaborator

johanbrandhorst commented Jun 15, 2018

Made changes as requested

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 15, 2018

Collaborator

@tmc @achew22 I think the test failure was a flaky test, could one of you rerun it please?

Collaborator

johanbrandhorst commented Jun 15, 2018

@tmc @achew22 I think the test failure was a flaky test, could one of you rerun it please?

@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 Jun 16, 2018

Collaborator

Rerun. I'll try to check on it in 20 but I'll probably forget. If I don't do something by tomorrow morning please reply here, it means I forgot (sorry in advance)

Collaborator

achew22 commented Jun 16, 2018

Rerun. I'll try to check on it in 20 but I'll probably forget. If I don't do something by tomorrow morning please reply here, it means I forgot (sorry in advance)

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 16, 2018

Collaborator

@achew22 thanks for retriggering it, seems like it passed ;). I appreciate the time you put into maintaining this project, so no worries about forgetting!

Collaborator

johanbrandhorst commented Jun 16, 2018

@achew22 thanks for retriggering it, seems like it passed ;). I appreciate the time you put into maintaining this project, so no worries about forgetting!

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 16, 2018

Collaborator

@ivucica anymore comments?

Collaborator

johanbrandhorst commented Jun 16, 2018

@ivucica anymore comments?

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 18, 2018

Collaborator

Friendly Monday bump @achew22 @ivucica

Collaborator

johanbrandhorst commented Jun 18, 2018

Friendly Monday bump @achew22 @ivucica

@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 Jun 18, 2018

Collaborator

LGTM. Let's give @ivucica 24 hours from your ping before merging. I will rebase tomorrow around this time. If I don't please reping and I'll merge when I get to a computer.

PS: Sorry this is takling forever, it's a great feature and I am REALLY looking forward to using it. Taking a little extra time is definitely worth it.

Collaborator

achew22 commented Jun 18, 2018

LGTM. Let's give @ivucica 24 hours from your ping before merging. I will rebase tomorrow around this time. If I don't please reping and I'll merge when I get to a computer.

PS: Sorry this is takling forever, it's a great feature and I am REALLY looking forward to using it. Taking a little extra time is definitely worth it.

@johanbrandhorst

This comment has been minimized.

Show comment
Hide comment
@johanbrandhorst

johanbrandhorst Jun 20, 2018

Collaborator

At the risk of becoming a tyrant on my first day, I'm going to commandeer and this and merge as @ivucica has been given ample time to add any more opinions and he conceded that he was happy with the PR even without the minor changes, that have been addressed.

Collaborator

johanbrandhorst commented Jun 20, 2018

At the risk of becoming a tyrant on my first day, I'm going to commandeer and this and merge as @ivucica has been given ample time to add any more opinions and he conceded that he was happy with the PR even without the minor changes, that have been addressed.

@johanbrandhorst johanbrandhorst merged commit 9b9677c into grpc-ecosystem:master Jun 20, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@johanbrandhorst johanbrandhorst deleted the johanbrandhorst:add-swagger-response-configuration branch Jun 20, 2018

@ivucica

This comment has been minimized.

Show comment
Hide comment
@ivucica

ivucica Jun 20, 2018

Collaborator
Collaborator

ivucica commented Jun 20, 2018

@achew22

This comment has been minimized.

Show comment
Hide comment
@achew22

achew22 Jun 20, 2018

Collaborator

@johanbrandhorst I approve of the action and the attitude. Thanks for merging

Collaborator

achew22 commented Jun 20, 2018

@johanbrandhorst I approve of the action and the attitude. Thanks for merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment