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

litrpc: add REST annotations for Litd RPC services #522

Merged
merged 11 commits into from Mar 20, 2023

Conversation

ellemouton
Copy link
Member

@ellemouton ellemouton commented Mar 18, 2023

Add REST annotations for all LitD RPC services and register them with the REST handler. Update all itests accordingly.

Also filled in lots of missing comments & added litcli annotations :)

@jamaljsr
Copy link
Member

Not sure if you want to address it in this PR or separately, but many of the RPC methods are missing comments in the proto files. It would improve the API docs site if each one had a comment which included the CLI command (example) so that the Code Samples Shell tab will display the help output.

@ellemouton
Copy link
Member Author

Great point @jamaljsr! Added them to this PR :)

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice, thanks a lot for the added documentation and REST mapping 💯
Just a few comments about the REST mapping, other than that LGTM 🌮

litrpc/lit-accounts.proto Show resolved Hide resolved
cmd/litcli/autopilot.go Outdated Show resolved Hide resolved
litrpc/firewall.yaml Outdated Show resolved Hide resolved
litrpc/lit-accounts.yaml Show resolved Hide resolved
litrpc/lit-autopilot.yaml Outdated Show resolved Hide resolved
cmd/litcli/autopilot.go Outdated Show resolved Hide resolved
litrpc/firewall.yaml Outdated Show resolved Hide resolved
litrpc/firewall.yaml Outdated Show resolved Hide resolved
litrpc/lit-accounts.yaml Show resolved Hide resolved
litrpc/lit-autopilot.yaml Outdated Show resolved Hide resolved
litrpc/lit-autopilot.yaml Outdated Show resolved Hide resolved
litrpc/proxy.yaml Show resolved Hide resolved
litrpc/lit-autopilot.yaml Outdated Show resolved Hide resolved
@ellemouton ellemouton force-pushed the litREST branch 2 times, most recently from 173376f to 6ed90db Compare March 20, 2023 10:52
@ellemouton ellemouton force-pushed the litREST branch 2 times, most recently from 97990c0 to e203fee Compare March 20, 2023 11:33
@ellemouton ellemouton requested a review from guggero March 20, 2023 11:43
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

litrpc/lit-autopilot.proto Outdated Show resolved Hide resolved
@ellemouton ellemouton force-pushed the litREST branch 2 times, most recently from 25339c3 to 1d52b37 Compare March 20, 2023 12:26
@ellemouton
Copy link
Member Author

also updated the very last commit to add lit-autopilot, firewall and proxy to app/scripts/build-protos.js

Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

tACK 👌

Tested a handful of REST calls across each of the services. Didn't run into any issues.

proto/proxy.proto Outdated Show resolved Hide resolved
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Great update!

@guggero guggero merged commit 163af3d into lightninglabs:master Mar 20, 2023
12 checks passed
@ellemouton ellemouton deleted the litREST branch March 20, 2023 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants