-
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
[web] refactor OTBR-WEB #537
base: main
Are you sure you want to change the base?
Conversation
This pull request introduces 4 alerts and fixes 2 when merging 460107c into ecc2570 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 4 alerts and fixes 2 when merging b8c94db into ecc2570 - view on LGTM.com new alerts:
fixed alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #537 +/- ##
==========================================
+ Coverage 76.67% 81.63% +4.95%
==========================================
Files 76 71 -5
Lines 5004 4955 -49
==========================================
+ Hits 3837 4045 +208
+ Misses 1167 910 -257
Continue to review full report at Codecov.
|
This pull request introduces 19 alerts and fixes 2 when merging 66e867a into ecc2570 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 3 alerts and fixes 2 when merging 40adcb6 into e3cbbf7 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 3 alerts and fixes 2 when merging ff5686d into e3cbbf7 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 3 alerts and fixes 2 when merging d364350 into e3cbbf7 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request fixes 2 alerts when merging 8cc43f3 into e3cbbf7 - view on LGTM.com fixed alerts:
|
[cmake] add option for enabling legacy in cmake (openthread#544)
This pull request fixes 2 alerts when merging ccb2e69 into e3cbbf7 - view on LGTM.com fixed alerts:
|
This pull request fixes 2 alerts when merging 46a1fe8 into e3cbbf7 - view on LGTM.com fixed alerts:
|
This pull request fixes 2 alerts when merging 6f8a8fe into e3cbbf7 - view on LGTM.com fixed alerts:
|
This pull request fixes 2 alerts when merging 44142f7 into e3cbbf7 - view on LGTM.com fixed alerts:
|
This pull request fixes 2 alerts when merging d83733c into bc8825d - view on LGTM.com fixed alerts:
|
This pull request fixes 2 alerts when merging 5694906 into bc8825d - view on LGTM.com fixed alerts:
|
8fd94c4
to
3178101
Compare
3178101
to
60a7974
Compare
35094e4
to
518ca34
Compare
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.
I also made some changes (mainly about Nginx & CMake scripts) to make it works for me and this PR LGTM now. Thanks to @tttttangTH, we are seeing a significant coverage increase of more that 5%
!
index index.html; | ||
} | ||
location /v1/ { | ||
proxy_pass http://0.0.0.0:8081; |
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.
allows customize port number?
|
||
private: | ||
typedef void (Resource::*ResourceHandler)(const Request &aRequest, Response &aResponse) const; | ||
typedef void (Resource::*ResourceCallbackHandler)(const Request &aRequest, Response &aResponse); | ||
|
||
// RESTful API entry |
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.
Can we add a document describing the REST APIs?
This PR refactors OTBR web with the new REST APIs:
otbr-web
binary.For the new implementation, HTTP requests for static files are served by Nginx directly while requests for dynamic resources are served by OTBR agent REST APIs (requests are forwarded to OTBR agent by Nginx).