-
Notifications
You must be signed in to change notification settings - Fork 232
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
base: main
Are you sure you want to change the base?
Conversation
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2515 +/- ##
===========================================
- Coverage 55.77% 45.13% -10.65%
===========================================
Files 87 104 +17
Lines 6890 12773 +5883
Branches 0 928 +928
===========================================
+ Hits 3843 5765 +1922
- Misses 3047 6695 +3648
- Partials 0 313 +313 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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:
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! |
There was a problem hiding this 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! 👍
ace5ce2
to
fc76fb5
Compare
@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. |
…d joiner request and a no bufs error on joiner post with 507 and 400 return codes.
…rrors and idempotent handling of duplicate PUT and not found delete requests
…o produce a big endian string
Co-authored-by: Jonathan Hui <[email protected]>
…onJoinerInfo2JoinerInfo`
efacc02
to
8d7051a
Compare
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.
/node/commissioner/state
endpoint allows the user to get/set the commissioner state using GET and PUT requests respectively./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:
Please give me feedback and let me know of any required changes 😄