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

feat(cmd): match upcoming node version with API #3318

Merged
merged 6 commits into from
May 9, 2024

Conversation

vgonkivs
Copy link
Member

Resolves #2549

docgen cmd moved to the root celestia cmd folder, so it will be a part of the c-node binary to get the correct Semantic version.

@vgonkivs vgonkivs added kind:feat Attached to feature PRs area:cli labels Apr 18, 2024
@vgonkivs vgonkivs requested a review from jcstein April 18, 2024 14:32
@vgonkivs vgonkivs self-assigned this Apr 18, 2024
@vgonkivs
Copy link
Member Author

Снимок экрана 2024-04-18 в 17 42 48

ramin
ramin previously approved these changes Apr 18, 2024
renaynay
renaynay previously approved these changes Apr 18, 2024
Wondertan
Wondertan previously approved these changes Apr 18, 2024
@jcstein
Copy link
Member

jcstein commented Apr 18, 2024

will this PR make it possible to generate the spec json file on releases? to be stored as artifacts on the release as suggested in #2611?

@jcstein
Copy link
Member

jcstein commented Apr 18, 2024

imagefromabove

it looks like this just outputs the version info? but not the actual api specs? am i missing something here?

Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now it is not possible to generate the api specs?

make openrpc-gen
--> Generating OpenRPC spec
stat /Users/joshstein/celestia-node/cmd/docgen: directory not found
make: *** [openrpc-gen] Error 1

however, the version (and no actual api specs) show when you run:

$ celestia docgen
{
    "openrpc": "1.2.6",
    "info": {
        "title": "Celestia Node API",
        "description": "The Celestia Node API is the collection of RPC methods that can be used to interact with the services provided by Celestia Data Availability Nodes.",
        "version": "v0.13.2-13-gc57e39bc"
    },
    "externalDocs": {
        "description": "Celestia Node GitHub",
        "url": "https://github.com/celestiaorg/celestia-node"
    },
    "methods": []
}%               

i am quite confused at what this PR accomplishes

@jcstein
Copy link
Member

jcstein commented Apr 18, 2024

flagging that I do not think this PR should be merged. it
(1) makes it impossible to generate rpc docs, which is counterproductive
(2) introduces celestia docgen which outputs the description of the docs with the api version, which is sort of productive, except for all the missing api docs that should come along with this spec generation

@vgonkivs
Copy link
Member Author

Let me fix the spec generation. Sorry, did not test it properly

@vgonkivs vgonkivs dismissed stale reviews from Wondertan, renaynay, and ramin via 8b569ad April 19, 2024 10:00
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 44.77%. Comparing base (2469e7a) to head (b886feb).
Report is 42 commits behind head on main.

Files Patch % Lines
cmd/celestia/docgen.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3318      +/-   ##
==========================================
- Coverage   44.83%   44.77%   -0.06%     
==========================================
  Files         265      273       +8     
  Lines       14620    15253     +633     
==========================================
+ Hits         6555     6830     +275     
- Misses       7313     7628     +315     
- Partials      752      795      +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Wondertan
Wondertan previously approved these changes Apr 19, 2024
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@jcstein
Copy link
Member

jcstein commented Apr 19, 2024

has this been tested?

@vgonkivs
Copy link
Member Author

Yeap. I have tested it and it worked for me

@jcstein
Copy link
Member

jcstein commented Apr 19, 2024

will this PR make it possible to generate the spec json file on releases? to be stored as artifacts on the release as suggested in #2611?

thank you, and does this help accomplish generation of specs for releases in #2611 ?

@jcstein
Copy link
Member

jcstein commented Apr 19, 2024

it works for me. but why do we need 2 commands to generate the spec? shouldn't celestia docgen handle this for us?

also, in the output I get when testing make openrpc-gen, there is still --> Generating OpenRPC spec which will need to be filtered out to handle #2611

@ramin
Copy link
Contributor

ramin commented Apr 19, 2024

will this PR make it possible to generate the spec json file on releases? to be stored as artifacts on the release as suggested in #2611?

thank you, and does this help accomplish generation of specs for releases in #2611 ?

I will do that next in a separate PR @jcstein

@jcstein
Copy link
Member

jcstein commented Apr 19, 2024

I understand that. I'm trying to understand if it helps accomplish that goal, or of it is going to add more work, that is all

@Wondertan Wondertan enabled auto-merge (squash) April 25, 2024 16:35
@renaynay renaynay requested a review from jcstein April 25, 2024 19:58
Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, also especially if it also helps to accomplish #2611 (as opposed to being unproductive toward that issue i.e. introducing more work)

@jcstein
Copy link
Member

jcstein commented Apr 25, 2024

my only feedback would be that the celestia docgen command is a little misleading, as it doesn't actually generate the docs. the command to do that is make openrpc-gen which AFAIU uses celestia docgen?

@vgonkivs
Copy link
Member Author

my only feedback would be that the celestia docgen command is a little misleading, as it doesn't actually generate the docs. the command to do that is make openrpc-gen which AFAIU uses celestia docgen?

make openrpc-gen calls docgen under the hood, passing all available modules. so to make it work, you should call
celestia docgen da fraud share etc....

But I'd remove the parameters and generate docs for all modules.

wdyt @jcstein @distractedm1nd , @renaynay, @Wondertan

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utack

@Wondertan Wondertan merged commit 93dfedf into celestiaorg:main May 9, 2024
51 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:cli kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: match upcoming node version with API
8 participants