protoc_gen_swagger: Add attr for allow_merge

[prestonvanloon]

load("@grpc_ecosystem_grpc_gateway//protoc-gen-swagger:defs.bzl", "protoc_gen_swagger")

proto_library(
    name = "ethereum_eth_v1_proto",
    srcs = [
        "example_a.proto",
        "example_b.proto",
    ],
    visibility = ["//visibility:public"],
)

protoc_gen_swagger(
    name = "swagger",
    proto = ":my_proto",
    visibility = ["//visibility:public"],
    allow_merge = True,
)

This would output apidocs.swagger.json rather than two files of example_a.swagger.json and example_b.swagger.json.

Opening this for early review on my approach. I'll request full review soon after I've used this a bit.

[codecov-io]

Codecov Report

Merging #944 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #944   +/-   ##
=======================================
  Coverage   53.79%   53.79%           
=======================================
  Files          40       40           
  Lines        3969     3969           
=======================================
  Hits         2135     2135           
  Misses       1638     1638           
  Partials      196      196

---

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 e6f18d3...d2666a7. Read the comment docs.

[johanbrandhorst]

@drigz

[drigz]

Seems like a good approach, thanks for the contribution! You could create an output file called ctx.attr.name + ".swagger.json" instead of hardcoding apidocs, which would allow multiple rules with allow_merge = True in the same package. You could also consider using a function to reduce the duplication between the two code paths.

[prestonvanloon]

@drigz Thanks for that! I updated with the suggestion to use the target name. With regards to refactoring, I agree but I think it's OK for now. I'm not much of a python / starlark person so I don't fully trust my refactoring abilities in best practices :)

[drigz]

LGTM, thanks! One more suggestion: if you add an invocation with allow_merge = True, eg //examples/proto/examplepb:examplepb_merged, this is slightly less likely to get broken in future.

[prestonvanloon]

Good idea. Thanks

[achew22]

is allow_merge the best name for this? allow_merge makes me think that it is possible but not necessary to merge the output files. What would you think about merge_files or single_output?

[prestonvanloon]

The rationale here is that is the name of the flag provided to the protoc-gen-swagger.

I don't feel very strongly about this either way, but let me know what you think if it makes sense to match the flag name or use a more idiomatic name.

[prestonvanloon]

@achew22 I'd be happy to change allow_merge to whatever we can do to merge this into master.

Let me know so this doesn't rot. Thanks!

[achew22]

Isn't the flag that actually does it merge_file_name?

[prestonvanloon]

You need both. merge_file_name to specify the file name and allow_merge to specify that you want to allow merging of files. That’s how I have understood and implemented the flag usage here.

[achew22]

I think you're 100% right. We are exposing a new abstraction layer here, and since the user of the Bazel rule sets the name for the multiple output by setting the name of the target, I would like to make the arg that does the merging be slightly more obvious.

If you renamed it to merge_files or single_output I will merge this as soon as I see it.

[prestonvanloon]

@achew22 done. PTAL

[achew22]

Looks good to me! Merging

[achew22]

Looks like we need to update rules_go. One moment please

[prestonvanloon]

@achew22 just pulled in latest master, maybe that will help

[achew22]

fatal error: error in backend: IO failure on output stream: Cannot allocate memory is the error I see on CircleCI. Looking around to see if there is anything about this

[achew22]

The only theory I have on how to fix this is to buy more CircleCI quota or to use the Bazel CI system, which I assume would handle this tidily... seeking suggestions if you have one

[prestonvanloon]

@achew22 There are some helpful suggestions here about lowering bazel memory: https://github.com/bazelbuild/rules_go#how-do-i-run-bazel-on-travis-ci

4gb of memory seems like plenty. You could try adding --local_ram_resources=HOST_RAM*.5 which would limit to half of whatever is available. The default is to use 67% of host RAM.

Bazel's CI uses their own runners in buildkite and they are quite nice. :)

[johanbrandhorst]

Build succeeded on rerun; @achew22 I'll leave it to you to merge.