Add support for the grpc_api_configuration option in the bazel rule. #632

ensonic · 2018-04-30T09:01:23Z

PR #521 added support for grpc service configurations. Add an option to let
users specify this config in the bazel rule.

Also add a simple doc blob to the rule.

PR grpc-ecosystem#521 added support for grpc service configurations. Add an option to let users specify this config in the bazel rule. Also add a simple doc blob to the rule.

codecov-io · 2018-04-30T09:12:45Z

Codecov Report

Merging #632 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #632      +/-   ##
==========================================
+ Coverage   58.88%   58.94%   +0.05%     
==========================================
  Files          30       30              
  Lines        2853     2857       +4     
==========================================
+ Hits         1680     1684       +4     
  Misses       1010     1010              
  Partials      163      163

Impacted Files	Coverage Δ
runtime/marshal_json.go	`83.33% <0%> (ø)`	⬆️
runtime/marshal_jsonpb.go	`70.58% <0%> (+1.2%)`	⬆️

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 87a1b0c...74a397e. Read the comment docs.

achew22

The code looks good to me sans one comment in defs.bzl.

Could you also add a new target in //examples/proto/examplepb that runs with grpc_api_configuration set? Thanks!

achew22 · 2018-04-30T16:12:14Z

protoc-gen-swagger/defs.bzl

@@ -36,6 +48,8 @@ def _run_proto_gen_swagger(direct_proto_srcs, transitive_proto_srcs, actions, pr

 def _proto_gen_swagger_impl(ctx):
    proto = ctx.attr.proto.proto
+    grpc_api_configuration = None


rm unnecessary line

Not quite done?

ensonic · 2018-05-02T08:46:46Z

Could you also add a new target in //examples/proto/examplepb that runs with grpc_api_configuration set? Thanks!
I was trying to add:

protoc_gen_swagger(
    name = "expamplepb_protoc_gen_swagger_with_services",
    grpc_api_configuration = "unannotated_echo_service.yaml",
    proto = ":examplepb_proto",
)

But this will fail with:

bazel build :expamplepb_protoc_gen_swagger_with_services 
ERROR: file 'examples/proto/examplepb/unannotated_echo_service.swagger.json' is generated by these conflicting actions:
Label: //examples/proto/examplepb:expamplepb_protoc_gen_swagger, //examples/proto/examplepb:expamplepb_protoc_gen_swagger_with_services
RuleClass: protoc_gen_swagger rule
Configuration: b93351232485d7aae598752ea518de7e
Mnemonic: SkylarkAction
Action key: 497a439399336d44de55e732fde92588, ca9f888c655373c5b768919ee6510936
Progress message: SkylarkAction examples/proto/examplepb/unannotated_echo_service.swagger.json
PrimaryInput: File:[[<execution_root>]bazel-out/host/bin]external/com_google_protobuf/protoc
PrimaryOutput: File:[[<execution_root>]bazel-out/k8-fastbuild/bin]examples/proto/examplepb/unannotated_echo_service.swagger.json
Owner information: //examples/proto/examplepb:expamplepb_protoc_gen_swagger com.google.devtools.build.lib.skyframe.BuildConfigurationValue$Key@99896fcf false (1448708908), //examples/proto/examplepb:expamplepb_protoc_gen_swagger_with_services com.google.devtools.build.lib.skyframe.BuildConfigurationValue$Key@99896fcf false (431521607)
MandatoryInputs: Previous action contains artifacts not in attempted action (first 5): 
	examples/proto/examplepb/unannotated_echo_service.yaml
Outputs: are equal

If I uncomment the 'expamplepb_protoc_gen_swagger' target it works. I would like to investigate this separately (see #638), if that is okay with you. I am not sure about how to resolve this right now.

achew22 · 2018-05-03T00:07:46Z

Ah, that's totally fair, can you remove that entry from the BUILD file and I'll track doing it in #640

ensonic · 2018-05-03T07:30:22Z

Ah, that's totally fair, can you remove that entry from the BUILD file and I'll track doing it in #640

You mean removing the example that works right now:
https://github.com/grpc-ecosystem/grpc-gateway/blob/master/examples/proto/examplepb/BUILD.bazel#L61

Can we not change it when fixing #640?

achew22 · 2018-05-04T19:41:48Z

I think this is good to go almost. I haven't merged due to https://github.com/grpc-ecosystem/grpc-gateway/pull/632/files#diff-256f5102c31109d1da9e7ebdbde6813cR51 where I asked for a change but I can't see the change. Am I missing something?

googlebot · 2018-05-05T00:31:47Z

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 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 here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

…rpc-ecosystem#632) * Add support for the grpc_api_configuration option in the bazel rule. PR grpc-ecosystem#521 added support for grpc service configurations. Add an option to let users specify this config in the bazel rule.

Add support for the grpc_api_configuration option in the bazel rule.

d395c64

PR grpc-ecosystem#521 added support for grpc service configurations. Add an option to let users specify this config in the bazel rule. Also add a simple doc blob to the rule.

googlebot added the cla: yes label Apr 30, 2018

ensonic mentioned this pull request Apr 30, 2018

feat(bazel): Add rule for generating .swagger.json files #613

Merged

ensonic mentioned this pull request Apr 30, 2018

Generate import mappings. #635

Merged

achew22 requested changes Apr 30, 2018

View reviewed changes

ensonic mentioned this pull request May 2, 2018

bazel rule protoc_gen_swagger creates clashing outputs if used multiple times in the same package #638

Closed

Address reviewer coments

74a397e

googlebot added cla: no and removed cla: yes labels May 5, 2018

achew22 approved these changes May 5, 2018

View reviewed changes

achew22 merged commit dca8693 into grpc-ecosystem:master May 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the grpc_api_configuration option in the bazel rule. #632

Add support for the grpc_api_configuration option in the bazel rule. #632

ensonic commented Apr 30, 2018

codecov-io commented Apr 30, 2018 •

edited

Loading

achew22 left a comment

achew22 Apr 30, 2018

ensonic May 2, 2018

achew22 May 3, 2018

ensonic commented May 2, 2018 •

edited

Loading

achew22 commented May 3, 2018

ensonic commented May 3, 2018

achew22 commented May 4, 2018

googlebot commented May 5, 2018

Add support for the grpc_api_configuration option in the bazel rule. #632

Add support for the grpc_api_configuration option in the bazel rule. #632

Conversation

ensonic commented Apr 30, 2018

codecov-io commented Apr 30, 2018 • edited Loading

Codecov Report

achew22 left a comment

Choose a reason for hiding this comment

achew22 Apr 30, 2018

Choose a reason for hiding this comment

ensonic May 2, 2018

Choose a reason for hiding this comment

achew22 May 3, 2018

Choose a reason for hiding this comment

ensonic commented May 2, 2018 • edited Loading

achew22 commented May 3, 2018

ensonic commented May 3, 2018

achew22 commented May 4, 2018

googlebot commented May 5, 2018

codecov-io commented Apr 30, 2018 •

edited

Loading

ensonic commented May 2, 2018 •

edited

Loading