feat: enable swagger by RafilxTenfen · Pull Request #1203 · umee-network/umee

RafilxTenfen · 2022-08-02T17:58:21Z

Description

closes: #957

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

RafilxTenfen · 2022-08-02T17:59:03Z

@toteki @adamewozniak could you guys check if the module specifics seem correct?

adamewozniak

Just a few questions - also @RafilxTenfen should we ignore the generated files here and maybe add it to make build, or something similar?

These files will likely become expired if they're not generated each time queries are updated

app/app.go

Makefile

app/app.go

adamewozniak · 2022-08-03T01:06:38Z

client/docs/config.json

@@ -0,0 +1,26 @@
+{


Could we move this into a folder swagger instead of client/docs maybe?

this is what other projects are using, but I agree - docs/swagger would be probably better place.

RafilxTenfen · 2022-08-03T01:43:30Z

Just a few questions - also @RafilxTenfen should we ignore the generated files here and maybe add it to make build, or something similar?

These files will likely become expired if they're not generated each time queries are updated

Make sense, maybe we could put it together with proto-gen? since it will be modified it time the proto files change

toteki · 2022-08-03T03:10:26Z

Makefile

+        echo "\033[91mSwagger docs are out of sync!!!\033[0m";\
+        exit 1;\


Are we taking the approach where the swagger docs are always gitignored, or will they be committed?

And if they're being committed - this is most of the ingredients to a CI check that we could make, which would check it they're out of sync on any PR

toteki

client/docs/statik/statik.go is so large that my git clone time for the repository seems to have doubled from this commit.

Because it's all in 13 lines, I also fear that it cannot diff efficiently, so our repo size would increase by the full 10+ MB every time it's generated.

Thus, we need to go the .gitignore route

Co-authored-by: Adam Wozniak <29418299+adamewozniak@users.noreply.github.com>

robert-zaremba

Maybe we can check with the ecosystem if someone else has better setup rather than bundling the swagger docs into the chain app.

Makefile

robert-zaremba · 2022-08-03T14:04:09Z

Makefile

+runsim: $(RUNSIM)
+$(RUNSIM):
+	@echo "Installing runsim..."
+	@go install github.com/cosmos/tools/cmd/runsim@v1.0.0


I don't think runsim nor statik is needed for swagger. Statik can be used to pack static files in a go library. Not sure if this is the best solution though. I know SDK and Regen are using Statik to serve doc files directly from the app. Ideally I would serve documentation from other app, rather than bundling it into the chain. What do you think?

robert-zaremba · 2022-08-03T14:10:11Z

client/docs/config.json

@@ -0,0 +1,26 @@
+{


this is what other projects are using, but I agree - docs/swagger would be probably better place.

Makefile

RafilxTenfen · 2022-08-03T14:43:08Z

@adamewozniak @toteki @robert-zaremba
Okay, forget the first implementation, can you guys take a look again?
I used how Juno did to make ours

To re-review later

adamewozniak

Couple of questions but this looks much cleaner 🥳

app/app.go

swagger/swagger.yaml

toteki

Doc file size (swagger.yaml) is much more manageable now (80 kB) and its format would allow for efficient diff.

Remaining question is whether we should

.gitignore it anyway (so it's only generated locally when needed, and can't be out of sync)
commit it (and either manually or through CI, make sure it doesn't desync)

RafilxTenfen · 2022-08-03T18:02:32Z

Doc file size (swagger.yaml) is much more manageable now (80 kB) and its format would allow for efficient diff.

Remaining question is whether we should

.gitignore it anyway (so it's only generated locally when needed, and can't be out of sync)

commit it (and either manually or through CI, make sure it doesn't desync)

In my opinion, we could commit that, in the same way we commit the generated structs from proto files

adamewozniak · 2022-08-03T18:04:28Z

Doc file size (swagger.yaml) is much more manageable now (80 kB) and its format would allow for efficient diff.
Remaining question is whether we should

.gitignore it anyway (so it's only generated locally when needed, and can't be out of sync)

commit it (and either manually or through CI, make sure it doesn't desync)

In my opinion, we could commit that, in the same way we commit the generated structs from proto files

Maybe we can add a CI task (later) where we check to see if swagger is out of date?
We could hash the existing swagger file & then compare that hash to a newly generated file.

…e-swagger

robert-zaremba · 2022-08-03T22:11:49Z

We could hash the existing swagger file & then compare that hash to a newly generated file.

Or check if proto was modified: whenever proto was modified, swagger should be checked if it should be modified as well.

robert-zaremba · 2022-08-03T22:13:31Z

swagger/proto-config-gen.json

+  },
+  "apis": [
+    {
+      "url": "./tmp-swagger-gen/umee/leverage/v1/query.swagger.json",


why tmp prefix? It looks weird.

it is just the name of the temp dir /tmp-swagger-gen used in contrib/scripts/protoc-swagger-gen.sh to generate the ./swagger/swagger.yaml

robert-zaremba

long term we should consider serving swagger docs not from the chain app.

…e-swagger

RafilxTenfen added 3 commits August 2, 2022 14:47

add swagger to umeed

88648ed

swagger-ui

2f3ab97

add swagger to readme and deleted unused files

cc6f79d

RafilxTenfen added C:API C:Docs labels Aug 2, 2022

RafilxTenfen requested a review from dreamcodez August 2, 2022 17:58

RafilxTenfen self-assigned this Aug 2, 2022

RafilxTenfen requested review from a team as code owners August 2, 2022 17:58

RafilxTenfen requested review from adamewozniak and toteki August 2, 2022 17:58

RafilxTenfen added 2 commits August 2, 2022 15:06

fix lint

182254a

fix imports

194dedd

adamewozniak reviewed Aug 3, 2022

View reviewed changes

toteki reviewed Aug 3, 2022

View reviewed changes

toteki previously requested changes Aug 3, 2022

View reviewed changes

RafilxTenfen and others added 2 commits August 3, 2022 11:06

Update app/app.go

6bfc12e

Co-authored-by: Adam Wozniak <29418299+adamewozniak@users.noreply.github.com>

removed runsim

09118d8

robert-zaremba reviewed Aug 3, 2022

View reviewed changes

RafilxTenfen added 4 commits August 3, 2022 11:14

removed unused vars

470ff21

moved froms statik to openapi

2054e3a

moved swagger package

be9e71f

undo tools-stamp

7546d67

RafilxTenfen requested review from adamewozniak, robert-zaremba and toteki August 3, 2022 14:43

add #1203 to changelog

fd0a0b7

adamewozniak approved these changes Aug 3, 2022

View reviewed changes

app/app.go Outdated Show resolved Hide resolved

swagger/swagger.yaml Show resolved Hide resolved

toteki reviewed Aug 3, 2022

View reviewed changes

moved RegisterSwaggerAPI

d1f7c30

Merge branch 'main' of github.com:umee-network/umee into rafilx/enabl…

71ed32b

…e-swagger

robert-zaremba reviewed Aug 3, 2022

View reviewed changes

robert-zaremba approved these changes Aug 4, 2022

View reviewed changes

Merge branch 'main' of github.com:umee-network/umee into rafilx/enabl…

ed65791

…e-swagger

RafilxTenfen added the A:Automerge label Aug 4, 2022

mergify bot merged commit c09c9f1 into main Aug 4, 2022

mergify bot deleted the rafilx/enable-swagger branch August 4, 2022 13:34

		echo "\033[91mSwagger docs are out of sync!!!\033[0m";\
		exit 1;\

Conversation

RafilxTenfen commented Aug 2, 2022

Description

Author Checklist

Reviewers Checklist

Uh oh!

RafilxTenfen commented Aug 2, 2022

Uh oh!

adamewozniak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamewozniak Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

robert-zaremba Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

RafilxTenfen commented Aug 3, 2022

Uh oh!

toteki Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

toteki left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robert-zaremba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robert-zaremba Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

robert-zaremba Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RafilxTenfen commented Aug 3, 2022

Uh oh!

adamewozniak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

toteki left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RafilxTenfen commented Aug 3, 2022

Uh oh!

adamewozniak commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robert-zaremba commented Aug 3, 2022

Uh oh!

robert-zaremba Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

RafilxTenfen Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

robert-zaremba left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

toteki left a comment •

edited

Loading

toteki left a comment •

edited

Loading

adamewozniak commented Aug 3, 2022 •

edited

Loading