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

Standalone gateway package #1239

Merged
merged 13 commits into from
Apr 25, 2020
Merged

Conversation

ssetin
Copy link
Contributor

@ssetin ssetin commented Apr 23, 2020

This feature makes developers able to generate gateway package separately from protobuf .pb files and imports them as outer packages.
It's useful when you have one inner protocol but want to share different api/gateways for different clients

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Setin, thanks for the PR! This looks nice, but to test it a little, could we try generating a new standalone package from one of the existing protofiles? Also I would ideally see this merged against v2, since adding features to both is more work for everyone. Could we also add a little documentation blurb about how to use this, and why?

protoc-gen-grpc-gateway/main.go Outdated Show resolved Hide resolved
@ssetin ssetin changed the base branch from master to v2 April 23, 2020 21:01
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@johanbrandhorst
Copy link
Collaborator

Hi Setin, I don't think that merge worked out the way you wanted it to. Maybe you should reopen this PR against v2 from the start? I could try and fix it if you want otherwise but it might be easiest.

@ssetin
Copy link
Contributor Author

ssetin commented Apr 24, 2020

Hello Johan, my mistake. I've made rebase to v2 now. I got little problem importing packages in examples because it's placed in the internal folder. I'm solving it

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

README.md Outdated Show resolved Hide resolved
Makefile Outdated
@@ -173,6 +173,7 @@ $(RUNTIME_TEST_SRCS): $(GO_PLUGIN) $(RUNTIME_TEST_PROTO)
$(EXAMPLE_GWSRCS): ADDITIONAL_GW_FLAGS:=$(ADDITIONAL_GW_FLAGS),grpc_api_configuration=examples/internal/proto/examplepb/unannotated_echo_service.yaml
$(EXAMPLE_GWSRCS): $(GATEWAY_PLUGIN) $(EXAMPLES)
protoc -I $(PROTOC_INC_PATH) -I. -I$(GOOGLEAPIS_DIR) --plugin=$(GATEWAY_PLUGIN) --grpc-gateway_out=logtostderr=true,allow_repeated_fields_in_body=true,$(PKGMAP)$(ADDITIONAL_GW_FLAGS):. $(EXAMPLES)
protoc -I $(PROTOC_INC_PATH) -I. -I$(GOOGLEAPIS_DIR) --plugin=$(GATEWAY_PLUGIN) --grpc-gateway_out=logtostderr=true,allow_repeated_fields_in_body=true,paths=source_relative,import_prefix="github.com/grpc-ecosystem/grpc-gateway/v2/",standalone=true$(ADDITIONAL_GW_FLAGS):examples/internal/proto/standalone $(EXAMPLES)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generates way too many files 😂. Lets just choose one of the packages (a simple one) and generate a standalone package for that instead of all of them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the import_prefix parameter do here? I've never had to use it before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point to generate all examples was to check that my feature accept all edge cases described in .proto. Looks like it does, so ok, i leave only one example.
The way you can pass external package path is to set it inside package in go_package option, but I didn't want to change code of the examples, so I decide to simulate such behavior with import_prefix, which adds prefix to go package paths for imported proto files. PS: Sorry for my English.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is v2, I'd like to change all of these go_packages to be correct anyway. Can we remove import_prefix if you update the go_package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem

ssetin and others added 2 commits April 24, 2020 13:17
Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>
@codecov-io
Copy link

Codecov Report

Merging #1239 into v2 will decrease coverage by 0.17%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v2    #1239      +/-   ##
==========================================
- Coverage   54.15%   53.97%   -0.18%     
==========================================
  Files          42       42              
  Lines        4406     4426      +20     
==========================================
+ Hits         2386     2389       +3     
- Misses       1759     1775      +16     
- Partials      261      262       +1     
Impacted Files Coverage Δ
protoc-gen-grpc-gateway/descriptor/types.go 38.12% <0.00%> (-2.96%) ⬇️
...-gen-grpc-gateway/internal/gengateway/generator.go 37.14% <0.00%> (-1.48%) ⬇️
...c-gen-grpc-gateway/internal/gengateway/template.go 80.58% <ø> (ø)
protoc-gen-grpc-gateway/descriptor/registry.go 57.63% <66.66%> (-0.35%) ⬇️
protoc-gen-grpc-gateway/descriptor/services.go 76.14% <100.00%> (+0.12%) ⬆️

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 f60488d...8b9f8ea. Read the comment docs.

@johanbrandhorst
Copy link
Collaborator

Could we also import the generated package in one of the tests and make sure that it still works?

@johanbrandhorst
Copy link
Collaborator

@ssetin ssetin marked this pull request as draft April 24, 2020 14:56
@ssetin ssetin marked this pull request as ready for review April 24, 2020 15:34
@ssetin
Copy link
Contributor Author

ssetin commented Apr 24, 2020

I've added tests for echo service, which handles another api routes. That was tricky to import gw from internal package, so I choose easiest way to get around this issue

@johanbrandhorst johanbrandhorst merged commit e6d0536 into grpc-ecosystem:v2 Apr 25, 2020
@johanbrandhorst
Copy link
Collaborator

Great work Setin, thanks for this contribution!

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

Successfully merging this pull request may close these issues.

4 participants