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

[rest] add commissioner api #2515

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

OmegaRelay
Copy link

@OmegaRelay OmegaRelay commented Sep 30, 2024

Added endpoints to the REST server to expose commissioner functionality over the REST API

These changes intend to implement commissioner functionality in the same style as the current rest API offering.

  • The /node/commissioner/state endpoint allows the user to get/set the commissioner state using GET and PUT requests respectively.
  • The /node/commissioner/joiner allows the user to manage joiners accepted by the commissioner, supporting POST, GET and DELETE request methods to add, get and remove a joiner respectively.

All added endpoints and their usage have been documented in the openapi.yaml

Example usage:
image

Please give me feedback and let me know of any required changes 😄

@marijnacht
Copy link

Will this PR be picked up as we kind of need this functionality. I also see another PR #2514 but that is really large change.

Copy link

codecov bot commented Oct 5, 2024

Codecov Report

Attention: Patch coverage is 0.72727% with 273 lines in your changes missing coverage. Please review.

Project coverage is 44.49%. Comparing base (2b41187) to head (3756476).
Report is 886 commits behind head on main.

Files with missing lines Patch % Lines
src/rest/resource.cpp 1.21% 162 Missing ⚠️
src/rest/json.cpp 0.00% 96 Missing ⚠️
src/common/api_strings.cpp 0.00% 13 Missing ⚠️
src/utils/hex.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2515       +/-   ##
===========================================
- Coverage   55.77%   44.49%   -11.29%     
===========================================
  Files          87      106       +19     
  Lines        6890    12438     +5548     
  Branches        0      931      +931     
===========================================
+ Hits         3843     5534     +1691     
- Misses       3047     6606     +3559     
- Partials        0      298      +298     

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

@OmegaRelay OmegaRelay changed the title Rest commissioner api [rest] commissioner api Oct 7, 2024
Copy link
Member

@wgtdkp wgtdkp left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Overall loooks good to me, a few suggestions below:

src/rest/openapi.yaml Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
src/rest/resource.cpp Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
src/utils/hex.cpp Outdated Show resolved Hide resolved
@OmegaRelay
Copy link
Author

OmegaRelay commented Oct 24, 2024

Hey @wgtdkp, I've made the requested changes, do you want me to resolve the comments?

@jwhui jwhui requested a review from wgtdkp October 24, 2024 10:24
@wgtdkp
Copy link
Member

wgtdkp commented Oct 24, 2024

Hey @wgtdkp, I've made the requested changes, do you want me to resolve the comments?

@OmegaRelay It's typically the PR author to resolve the comments if the suggests in the comments are followed. If you have different thoughts, please reply to the comment and leave it open, the reviewers will take a look again and close it when a consensus is reached.

I did take a glance and it seems my comments are not resolved. Could you check the comments again? thank you!

src/rest/openapi.yaml Outdated Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
Copy link
Member

@wgtdkp wgtdkp left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thank you! 👍

src/rest/resource.cpp Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
@wgtdkp wgtdkp changed the title [rest] commissioner api [rest] add commissioner api Oct 29, 2024
@wgtdkp wgtdkp requested a review from jwhui October 29, 2024 13:52
src/rest/json.cpp Outdated Show resolved Hide resolved
src/rest/json.cpp Outdated Show resolved Hide resolved
src/rest/json.cpp Outdated Show resolved Hide resolved
src/rest/json.cpp Outdated Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
src/rest/resource.cpp Outdated Show resolved Hide resolved
@martinzi
Copy link
Contributor

@OmegaRelay A Joiner drops from the joiner table after timeout, but also after successfully joining the network. Could your API differentiate between these two scenarios?

@OmegaRelay
Copy link
Author

@OmegaRelay A Joiner drops from the joiner table after timeout, but also after successfully joining the network. Could your API differentiate between these two scenarios?

Hi @martinzi, this PR is intended to be a simple extension of the current stateless API and as such implementing this joiner status feature feels outside the scope of this PR.

On a side note, I am currently looking at implementing server-sent events which can also cover this use case and keep the API itself stateless, I do have an open feature request to discuss this but it hasn't had much interaction.

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.

6 participants