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

[web] refactor OTBR-WEB #537

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

Conversation

tttttangTH
Copy link
Contributor

@tttttangTH tttttangTH commented Aug 12, 2020

This PR refactors OTBR web with the new REST APIs:

  • Remove standalone otbr-web binary.
  • Add REST APIs that are required by the OTBR web page.
  • Host the OTBR web page with Nginx.
  • Remove unused third-party libraries.

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).

@lgtm-com
Copy link

lgtm-com bot commented Aug 12, 2020

This pull request introduces 4 alerts and fixes 2 when merging 460107c into ecc2570 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 13, 2020

This pull request introduces 4 alerts and fixes 2 when merging b8c94db into ecc2570 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

fixed alerts:

  • 2 for Useless assignment to local variable

@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #537 (cc2ef9e) into master (ea8fe65) will increase coverage by 4.95%.
The diff coverage is 81.55%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/agent/main.cpp 93.13% <ø> (ø)
src/rest/connection.hpp 100.00% <ø> (ø)
src/rest/parser.cpp 100.00% <ø> (+13.15%) ⬆️
src/rest/request.cpp 86.66% <ø> (+13.33%) ⬆️
src/rest/request.hpp 100.00% <ø> (ø)
src/rest/response.hpp 100.00% <ø> (ø)
src/rest/rest_web_server.cpp 88.04% <ø> (-5.44%) ⬇️
src/rest/connection.cpp 74.80% <16.66%> (+0.92%) ⬆️
src/rest/resource.cpp 85.89% <80.52%> (-4.87%) ⬇️
src/rest/json.cpp 82.39% <86.45%> (+4.62%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea8fe65...cc2ef9e. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Aug 13, 2020

This pull request introduces 19 alerts and fixes 2 when merging 66e867a into ecc2570 - view on LGTM.com

new alerts:

  • 12 for Unused variable, import, function or class
  • 7 for Missing variable declaration

fixed alerts:

  • 2 for Useless assignment to local variable

@tttttangTH tttttangTH changed the title [web] Refactor OTBR-WEB [web] refactor OTBR-WEB Aug 14, 2020
@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2020

This pull request introduces 3 alerts and fixes 2 when merging 40adcb6 into e3cbbf7 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class
  • 1 for Expression has no effect

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2020

This pull request introduces 3 alerts and fixes 2 when merging ff5686d into e3cbbf7 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class
  • 1 for Expression has no effect

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2020

This pull request introduces 3 alerts and fixes 2 when merging d364350 into e3cbbf7 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class
  • 1 for Expression has no effect

fixed alerts:

  • 2 for Useless assignment to local variable

@tttttangTH tttttangTH marked this pull request as ready for review August 19, 2020 17:58
@lgtm-com
Copy link

lgtm-com bot commented Aug 19, 2020

This pull request fixes 2 alerts when merging 8cc43f3 into e3cbbf7 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

[cmake] add option for enabling legacy in cmake (openthread#544)
@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2020

This pull request fixes 2 alerts when merging ccb2e69 into e3cbbf7 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2020

This pull request fixes 2 alerts when merging 46a1fe8 into e3cbbf7 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2020

This pull request fixes 2 alerts when merging 6f8a8fe into e3cbbf7 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2020

This pull request fixes 2 alerts when merging 44142f7 into e3cbbf7 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2020

This pull request fixes 2 alerts when merging d83733c into bc8825d - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 26, 2020

This pull request fixes 2 alerts when merging 5694906 into bc8825d - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@tttttangTH
Copy link
Contributor Author

Have rebased it with newly merged #519, I will also have a self review these two days. @wgtdkp

script/bootstrap Outdated Show resolved Hide resolved
script/setup Outdated Show resolved Hide resolved
src/CMakeLists.txt Show resolved Hide resolved
src/rest/connection.cpp Show resolved Hide resolved
src/rest/frontend/index.html Outdated Show resolved Hide resolved
src/rest/frontend/index.html Outdated Show resolved Hide resolved
src/rest/frontend/index.html Outdated Show resolved Hide resolved
src/rest/frontend/index.html Outdated Show resolved Hide resolved
src/rest/frontend/res/js/app.js Show resolved Hide resolved
src/rest/json.hpp Outdated Show resolved Hide resolved
src/rest/json.hpp Outdated Show resolved Hide resolved
src/rest/json.hpp Outdated Show resolved Hide resolved
src/rest/json.hpp Outdated Show resolved Hide resolved
src/rest/request.hpp 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.

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%!

@bukepo @jwhui Any comments?

Base automatically changed from master to main March 8, 2021 21:50
index index.html;
}
location /v1/ {
proxy_pass http://0.0.0.0:8081;
Copy link
Member

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
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants