search

grpc-ecosystem / grpc-gateway

gRPC to JSON proxy generator following the gRPC HTTP spec
https://grpc-ecosystem.github.io/grpc-gateway/
BSD 3-Clause "New" or "Revised" License
18.09k stars 2.23k forks source link

Update or remove swagger-codegen #254

Open tmc opened 7 years ago

tmc commented 7 years ago

We're on 2.1.6 while 2.2.0 is latest: https://github.com/swagger-api/swagger-codegen/releases

As it stands this creates a barrier to contribution as folks will run into errors.

achew22 commented 7 years ago

@tmc, Do we need to do more to upgrade than change the hardcoded compatibility string? I didn't see anything in there listed as a breaking change

tmc commented 7 years ago

@achew22 afaict it dumps out code with different package names into the same directory.

tmc commented 7 years ago

I pushed https://travis-ci.org/tmc/grpc-gateway/builds/177507616 to demonstrate

tmc commented 7 years ago

with 2.2.0 or 2.2.1 I get:

go test -race github.com/grpc-ecosystem/grpc-gateway/...
can't load package: package github.com/grpc-ecosystem/grpc-gateway/examples/clients: found packages abe (a_bit_of_everything_nested.go) and echo (echo_service_api.go) in /Users/tmc/go/src/github.com/grpc-ecosystem/grpc-gateway/examples/clients
make: *** [test] Error 1``

Thoughts on removing  or disabling the swagger-codegen calls? It's a bit tangential and it will be a contribution barrier.
tmc commented 7 years ago

looking more closely this appears to be the --additional-properties packageName=echo parameter to swagger-codegen not affecting the output directory anymore.

achew22 commented 7 years ago

It feels like a test that is testing something outside the control of this project. It does, however, add a nice e2e test to the repo. Honestly I don't even remember this commit going by which explains my total lack of understanding in my earlier comment.

Let's ask ourselves the question "What is this testing and can it be tested in another way" specifically without bringing in a dependency on swagger-codegen

tmc commented 7 years ago

If were were actually exercising the swagger-codegen-generated code in test I'd be more inclined to keep it around. Right now it's basically testing well-formedness of the generated swagger output.

tmc commented 7 years ago

@yugui thoughts here?

dmitris commented 5 years ago

the current version of swagger-codegen (that you get with brew install for example) is 2.3.1 - CONTRIBUTING.md says "use swagger-codegen 2.2.2, not newer versions". @johanbrandhorst - would it be possible to make grpc-gateway to work with 2.3.1? Thanks.

johanbrandhorst commented 5 years ago

With the new CI system proposed in #772 this will be trivial to accomplish, and I'm not sure what if anything stands in our way to do it, but lets get back to this once #772 has been merged.

johanbrandhorst commented 5 years ago

@tmc @achew22 I think we could probably remove it altogether as well - personally when I'm generating swagger files I don't use swagger-codegen as the Go code is next to useless anyway. Again, this can easily be accomplished after #772 has been merged. I don't think it adds any extra confidence as it is.

dmitris commented 5 years ago

772 has been merged now, can we remove swagger-codegen now?

johanbrandhorst commented 5 years ago

Yes :). I don't think it adds anything. Would you like to create a new PR with it removed? We can remove it from the Dockerfile as well.

johanbrandhorst commented 5 years ago

Ok after some discussion a requirement has presented itself; if we remove swagger-codegen, we must replace it with some other method by which we can validate that our output swagger files are sensible. I have some personal experience with using https://github.com/go-swagger/go-swagger, which seems to do the job. If anyone wants to replace swagger-codegen we should consider it at the same time.

johanbrandhorst commented 5 years ago

As mentioned in https://github.com/grpc-ecosystem/grpc-gateway/pull/795#issuecomment-435618237, the use of swagger-codegen is now leading to slightly annoying travis files appearing after every generation. We can fix this with a simple .gitignore rule, but it's just another thing we shouldn't have to do.