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

Use Docker to generate swagger files #7933

Closed
1 of 4 tasks
amaury1093 opened this issue Nov 13, 2020 · 3 comments · Fixed by #9064
Closed
1 of 4 tasks

Use Docker to generate swagger files #7933

amaury1093 opened this issue Nov 13, 2020 · 3 comments · Fixed by #9064
Labels
T: Dev UX UX for SDK developers (i.e. how to call our code)

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Nov 13, 2020

Summary

To not install binaries on the user's local machine, use Docker for proto swagger generation.

Problem Definition

In protoc-swagger-gen.sh, we rely on a globally installed swagger-combine binary to combine all the generated swagger files into one. This may lead to incompatible versions used by different devs on the cosmos-sdk, leading to unwanted file diff in generated files.

# combine swagger files
# uses nodejs package `swagger-combine`.
# all the individual swagger files need to be configured in `config.json` for merging
swagger-combine ./client/docs/config.json -o ./client/docs/swagger-ui/swagger.yaml -f yaml --continueOnConflictingPaths true --includeDefinitions true

Proposal

Following #7332, we should containerize swagger-combine (maybe in the same Dockerfile as buf/protoc?), and update the following Makefile target to use Docker:

cosmos-sdk/Makefile

Lines 380 to 381 in 4b529a4

proto-swagger-gen:
@./scripts/protoc-swagger-gen.sh


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093 amaury1093 added the T: Dev UX UX for SDK developers (i.e. how to call our code) label Nov 13, 2020
@amaury1093 amaury1093 added this to the v0.40.1 milestone Nov 13, 2020
@amaury1093
Copy link
Contributor Author

How should we move forward for dockerizing proto-swagger-gen (which requires installing another npm binary called swagger-combine):

  1. add swagger-combine somewhere in the tendermintdev/sdk-proto-gen dockerfile we use.
  2. create a new dockerfile on the sdk which is tendermintdev/sdk-proto-gen + swagger-combine
  3. Remove swagger-combine from the protoc-swagger-gen.sh file, and use 2 lines for the make command:
proto-swagger-gen:
	$(DOCKER) run --rm -v $(CURDIR):/workspace --workdir /workspace tendermintdev/sdk-proto-gen sh ./scripts/protoc-swagger-gen.sh
	$(DOCKER) run --rm -v $(CURDIR):/workspace --workdir /workspace courtapi/swagger-combine {swagger combine arguments}

(we use an external dockerfile for swagger-combine)

  1. Don't use docker at all for this command.

I have a slight preference for 2 or 3.

@alexanderbez
Copy link
Contributor

Why not (1)? It's not difficult to rebuild the image with that dependency added.

@hallazzang
Copy link
Contributor

I'm working on this issue

@mergify mergify bot closed this as completed in #9064 Apr 7, 2021
@aaronc aaronc removed the in progress label Apr 7, 2021
mergify bot pushed a commit that referenced this issue Apr 7, 2021
* use docker for proto-swagger-gen

fixes #7933

* fix deprecated commands and flags

see https://docs.buf.build/faq/ for detail

* run proto-gen and proto-swagger-gen

* add changelog entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Dev UX UX for SDK developers (i.e. how to call our code)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants