feat: swagger.yaml #2818

gregnuj · 2023-11-07T22:46:04Z

Overview

Add cosmos sdk standard scripts/protoc-swagger-gen.sh and supporting config.json file to enable the generation of lcd swagger.yaml file

Checklist

[ *] New and updated code has appropriate documentation
[ *] New and updated code has new and/or updated testing
[ *] Required CI checks are passing
[ *] Visual proof for any user facing features like CLI or documentation updates
[ *] Linked issues closed with keywords

rootulp · 2023-11-08T09:58:22Z

@gregnuj can you provide more context on why we want this PR? Is there an issue this closes? What does the "lcd swagger.yaml file" do?

scripts/protoc-swagger-gen.sh

gregnuj · 2023-11-08T17:01:44Z

The swagger.yaml file describes the available endpoints to your developers and provides them a ui to test api endpoints.
for example:
https://osmosis-api.polkachu.com/swagger/
https://cosmos-api.polkachu.com/swagger/
https://terra-api.polkachu.com/swagger/
https://neutron-api.polkachu.com/swagger/
https://injective-api.polkachu.com/swagger/

This is standard practice in cosmos-sdk which, given the proper path to the file in the code, will provide an interface as linked above when enabled via the app.toml file or via the cli option for example:

[root@celestia-0 bin]# ./celestia-appd start --help |  grep swagger
      --api.swagger                                     Define if swagger documentation should automatically be registered (Note: the API must also be enabled)

gregnuj · 2023-11-08T17:05:40Z

As far as issues I see none open in github, but search your discord for "swagger", multiple inquiries regarding the missing swagger doc.

rootulp · 2023-11-08T20:52:31Z

the generation of lcd swagger.yaml file

What does lcd stand for?

rootulp

Thanks for the contribution! Confirmed Osmosis and Cosmos Hub have something similar:

so I think we want this, but I want to verify the generated config.json before approving. Is there an easy way to do that?

scripts/protoc-swagger-gen.sh

docs/swagger-ui/config.json

gregnuj · 2023-11-09T00:59:10Z

Thanks for the contribution! Confirmed Osmosis and Cosmos Hub have something similar:

https://github.com/osmosis-labs/osmosis/blob/main/client/docs/config.json

https://github.com/cosmos/gaia/blob/be7129bb6bdf3099e000b9786b15b98ec1defe54/client/docs/config.json

so I think we want this, but I want to verify the generated config.json before approving. Is there an easy way to do that?

the scripts generates a file called "swagger.yaml" the config.json file is just some instructions how to structure the output file...

gregnuj · 2023-11-09T01:01:37Z

the generation of lcd swagger.yaml file

What does lcd stand for?

lcd = light client daemon, although technically the name of the rest client in cosmos-sdk has been renamed from lcd to grpc-gateway

gregnuj · 2023-11-09T01:04:38Z

Thanks for the contribution! Confirmed Osmosis and Cosmos Hub have something similar:

https://github.com/osmosis-labs/osmosis/blob/main/client/docs/config.json

https://github.com/cosmos/gaia/blob/be7129bb6bdf3099e000b9786b15b98ec1defe54/client/docs/config.json

so I think we want this, but I want to verify the generated config.json before approving. Is there an easy way to do that?

run the script and you can test it here https://editor.swagger.io

gregnuj · 2023-11-09T01:04:41Z

Thanks for the contribution! Confirmed Osmosis and Cosmos Hub have something similar:

https://github.com/osmosis-labs/osmosis/blob/main/client/docs/config.json

https://github.com/cosmos/gaia/blob/be7129bb6bdf3099e000b9786b15b98ec1defe54/client/docs/config.json

so I think we want this, but I want to verify the generated config.json before approving. Is there an easy way to do that?

run the script and you can test it here https://editor.swagger.io

codecov-commenter · 2023-11-09T10:09:42Z

Codecov Report

Merging #2818 (0c9559e) into main (75f9393) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2818   +/-   ##
=======================================
  Coverage   19.80%   19.80%           
=======================================
  Files         144      144           
  Lines       17398    17398           
=======================================
  Hits         3446     3446           
  Misses      13631    13631           
  Partials      321      321

rootulp · 2023-11-09T14:50:13Z

When I put https://github.com/gregnuj/celestia-app/blob/main/docs/swagger-ui/swagger.yaml into https://editor.swagger.io/ I observe errors:

…-gen

rootulp · 2023-11-15T15:46:37Z

Converting to draft until this error is resolved ^

coderabbitai · 2023-11-16T16:45:36Z

Walkthrough

The changes involve updates to scripts used for generating Swagger documentation from Protoc files. A Python script merge_swagger.py has been introduced to merge individual Swagger JSON files into a single YAML file, with functionality to handle duplicate keys and retrieve project versions. The Bash script protoc-swagger-gen.sh has been updated to include error handling, temporary directory management, and the use of the Python script to merge Swagger files into a final JSON file.

Changes

File Path	Change Summary
`scripts/merge_swagger.py`	Introduced a Python script for merging Swagger JSON files into a YAML file, handling duplicates, and version retrieval from `go.mod`.
`scripts/protoc-swagger-gen.sh`	Updated Bash script to generate Swagger files with error handling, directory management, and integration of the Python merging script.

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

gregnuj · 2023-11-16T16:51:36Z

When I put https://github.com/gregnuj/celestia-app/blob/main/docs/swagger-ui/swagger.yaml into https://editor.swagger.io/ I observe errors:

I added a title, I cannot resolve the other error without removing valid endpoints
the swagger editor considers these equivalent paths since the parameter is a string in either case

/cosmos/auth/v1beta1/bech32/{address_bytes}:
/cosmos/auth/v1beta1/bech32/{address_string}:

coderabbitai

Review Status

Actionable comments generated: 7

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 7f3e4ec and 477e825.

Files ignored due to filter (1)

docs/swagger-ui/swagger.yaml

Files selected for processing (2)

scripts/merge_swagger.py (1 hunks)
scripts/protoc-swagger-gen.sh (1 hunks)

scripts/protoc-swagger-gen.sh

coderabbitai · 2023-11-28T00:05:20Z

scripts/merge_swagger.py

+    parser.add_argument('-d', '--directory', help='Directory containing swagger files')
+    parser.add_argument('-t', '--title', help='Title for the swagger file')
+    parser.add_argument('-v', '--version', help='Version for the swagger file')
+    parser.add_argument('-o', '--output', help='Output file')


The script should validate that all required command-line arguments are provided and handle cases where they are not, to prevent runtime errors.

coderabbitai · 2023-11-28T00:05:20Z

scripts/merge_swagger.py

+    with open(os.path.join(project_root, "go.mod"), "r") as f:
+        for line in f.readlines():
+            if line.startswith("module"):
+                version = line.split("/")[-1].strip()
+                break
+
+    if not version:
+        print("Could not find version in go.mod")
+        exit(1)


Consider adding exception handling around file operations to gracefully handle cases where the go.mod file does not exist or cannot be read due to permissions or other issues.

scripts/protoc-swagger-gen.sh

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

coderabbitai · 2023-12-01T20:39:11Z

scripts/protoc-swagger-gen.sh

+# This script generates the swagger.yaml documentation for the rest API on port 1317
+#
+# Prior to running this script, please install the following:
+# - Install Go 1.21+  


The Go version specified in the comment is 1.21+, which does not exist. Please correct it to a valid Go version, such as 1.21 (if it's a typo) or the actual minimum required version.

coderabbitai · 2023-12-01T20:39:11Z

scripts/protoc-swagger-gen.sh

+
+set -eo pipefail
+
+work_dir="$(dirname "$(dirname "$(realpath "$0")")")"


The realpath command is not available by default on all systems, such as macOS. Consider using a more portable alternative or ensure that realpath is installed as part of the prerequisites.

coderabbitai · 2023-12-01T20:39:11Z

scripts/protoc-swagger-gen.sh

+go build -o $tmp_dir/protoc-gen-swagger protoc-gen-swagger/main.go
+cd $tmp_dir
+
+PATH=./:$PATH


Adding the current directory to the PATH can be a security risk. Consider specifying the full path to the protoc-gen-swagger binary instead of modifying the PATH.

rootulp

When I run this script locally, I observe changes to the generated swagger.yaml. See https://pastebin.com/K05t1qbr

I would expect no changes. Can we update the generator or swagger.yaml so that subsequent invocations to ./scripts/protoc-swagger-gen.sh don't cause a dirty git diff.

Even though it's late in the review process, I think a GitHub issue with more context on what this PR is attempting to address would help. Ideally the GitHub issue would contain context, a problem statement, a proposal for how to resolve, etc.

scripts/merge_swagger.py

rootulp · 2023-12-01T20:44:32Z

scripts/protoc-swagger-gen.sh

+#
+# Prior to running this script, please install the following:
+# - Install Go 1.21+  
+# - Install pyyaml: `pip3 install pyyaml`


Is there anyway to do this via Go or bash so that maintainers don't have to set up Python to run this? AFAIK no other files in this repo use Python so it would be preferable to not introduce a new language / environment set up.

Co-authored-by: Rootul P <[email protected]>

coderabbitai · 2023-12-07T03:50:52Z

scripts/merge_swagger.py

+        for key in data["definitions"]:
+            definitions[key] = data["definitions"][key]


The script still merges definitions without checking for duplicates, which could lead to data loss if two definitions with the same key but different contents exist. Consider adding a check similar to the one for paths to handle duplicate definitions appropriately.

+ if key in definitions and definitions[key] != data["definitions"][key]: + print(f"Duplicate definition found for {key}, which may lead to unexpected behavior.") + continue + definitions[key] = data["definitions"][key]

Committable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change

for key in data["definitions"]:

definitions[key] = data["definitions"][key]

for key in data["definitions"]:

if key in definitions and definitions[key] != data["definitions"][key]:

print(f"Duplicate definition found for {key}, which may lead to unexpected behavior.")

continue

definitions[key] = data["definitions"][key]

rootulp · 2023-12-28T04:34:44Z

Closing because stale. If this is still something we want, please re-open the PR or create a GH issue.

gregnuj added 3 commits November 7, 2023 16:33

add protoc-swagger-gen script and supporting files

1f9c4d6

add swagger.yaml

255ae0b

fix version string

71ce7b5

gregnuj requested review from liamsi, adlerjohn, evan-forbes, rootulp and cmwaters as code owners November 7, 2023 22:46

celestia-bot requested a review from a team November 7, 2023 22:46

github-actions bot added the external label Nov 7, 2023

fix version string

4b6c5c1

rootulp assigned gregnuj Nov 8, 2023

rootulp reviewed Nov 8, 2023

View reviewed changes

scripts/protoc-swagger-gen.sh Outdated Show resolved Hide resolved

scripts/protoc-swagger-gen.sh Outdated Show resolved Hide resolved

scripts/protoc-swagger-gen.sh Outdated Show resolved Hide resolved

Merge branch 'main' into feat/protoc-swagger-gen

798f413

celestia-bot requested a review from a team November 8, 2023 16:55

implement requested changes

9a16d47

rootulp reviewed Nov 8, 2023

View reviewed changes

scripts/protoc-swagger-gen.sh Outdated Show resolved Hide resolved

scripts/protoc-swagger-gen.sh Outdated Show resolved Hide resolved

docs/swagger-ui/config.json Outdated Show resolved Hide resolved

rootulp changed the title ~~add protoc-swagger-gen script and supporting files~~ feat: swagger config.json Nov 8, 2023

celestia-bot requested a review from a team November 9, 2023 01:13

rootulp changed the title ~~feat: swagger config.json~~ feat: swagger.yaml Nov 9, 2023

gregnuj added 3 commits November 14, 2023 11:33

update swagger generation

f81db7d

merge changes

488fd78

Merge remote-tracking branch 'upstream/main' into feat/protoc-swagger…

b8abe8e

…-gen

rootulp marked this pull request as draft November 15, 2023 15:46

use python to generate swagger.yaml directly

c917e5c

gregnuj force-pushed the feat/protoc-swagger-gen branch from c5c566e to c917e5c Compare November 16, 2023 16:45

use python to generate yaml directly

246f55c

revert mod to go.mod

477e825

gregnuj marked this pull request as ready for review November 28, 2023 00:03

Merge branch 'main' into feat/protoc-swagger-gen

e514c0c

coderabbitai bot reviewed Nov 28, 2023

View reviewed changes

rootulp reviewed Nov 28, 2023

View reviewed changes

scripts/protoc-swagger-gen.sh Show resolved Hide resolved

rootulp and others added 2 commits December 1, 2023 15:35

Merge branch 'main' into feat/protoc-swagger-gen

986f77f

Update scripts/protoc-swagger-gen.sh

3b1b839

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

coderabbitai bot reviewed Dec 1, 2023

View reviewed changes

rootulp requested changes Dec 1, 2023

View reviewed changes

Update scripts/merge_swagger.py

398eadd

Co-authored-by: Rootul P <[email protected]>

celestia-bot requested a review from a team December 7, 2023 03:49

coderabbitai bot reviewed Dec 7, 2023

View reviewed changes

Merge branch 'main' into feat/protoc-swagger-gen

9343b60

rootulp marked this pull request as draft December 7, 2023 16:29

rootulp closed this Dec 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: swagger.yaml #2818

feat: swagger.yaml #2818

gregnuj commented Nov 7, 2023 •

edited by coderabbitai bot

Loading

rootulp commented Nov 8, 2023

gregnuj commented Nov 8, 2023

gregnuj commented Nov 8, 2023

rootulp commented Nov 8, 2023

rootulp left a comment

gregnuj commented Nov 9, 2023

gregnuj commented Nov 9, 2023

gregnuj commented Nov 9, 2023

gregnuj commented Nov 9, 2023

codecov-commenter commented Nov 9, 2023

rootulp commented Nov 9, 2023

rootulp commented Nov 15, 2023

coderabbitai bot commented Nov 16, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

gregnuj commented Nov 16, 2023

coderabbitai bot left a comment

coderabbitai bot Nov 28, 2023

coderabbitai bot Nov 28, 2023

coderabbitai bot Dec 1, 2023

coderabbitai bot Dec 1, 2023

coderabbitai bot Dec 1, 2023

rootulp left a comment

rootulp Dec 1, 2023

coderabbitai bot Dec 7, 2023

rootulp commented Dec 28, 2023


		set -eo pipefail

		work_dir="$(dirname "$(dirname "$(realpath "$0")")")"

		for key in data["definitions"]:
		definitions[key] = data["definitions"][key]

feat: swagger.yaml #2818

feat: swagger.yaml #2818

Conversation

gregnuj commented Nov 7, 2023 • edited by coderabbitai bot Loading

Overview

Checklist

rootulp commented Nov 8, 2023

gregnuj commented Nov 8, 2023

gregnuj commented Nov 8, 2023

rootulp commented Nov 8, 2023

rootulp left a comment

Choose a reason for hiding this comment

gregnuj commented Nov 9, 2023

gregnuj commented Nov 9, 2023

gregnuj commented Nov 9, 2023

gregnuj commented Nov 9, 2023

codecov-commenter commented Nov 9, 2023

Codecov Report

rootulp commented Nov 9, 2023

rootulp commented Nov 15, 2023

coderabbitai bot commented Nov 16, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

gregnuj commented Nov 16, 2023

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Nov 28, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 28, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 1, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 1, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 1, 2023

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rootulp Dec 1, 2023

Choose a reason for hiding this comment

coderabbitai bot Dec 7, 2023

Choose a reason for hiding this comment

rootulp commented Dec 28, 2023

gregnuj commented Nov 7, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 16, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)