-
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
Standalone gateway package #1239
Conversation
There was a problem hiding this 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?
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 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 ℹ️ Googlers: Go here for more info. |
* refactoring * add notes to readme.md
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. |
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 |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_package
s to be correct anyway. Can we remove import_prefix
if you update the go_package
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no problem
Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Could we also import the generated package in one of the tests and make sure that it still works? |
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 |
…from generator to registry
Great work Setin, thanks for this contribution! |
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