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

refactor(robot-server): start pulling routers to top-level modules #7632

Merged
merged 3 commits into from
Apr 28, 2021

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Apr 13, 2021

Overview

This PR starts to re-organize the robot-server module a little bit to make various important imports less deep and to reduce time and cognitive load to go from "I need to change out /xyz endpoint works" to actually writing code.

Overall principles:

  • Map endpoint paths to top-level directories
    • e.g. stuff for /heath goes in robot_server/health
  • Organize stuff by domain first
    • e.g. health/router.py + health/models.py is easier than routers/health.py + models/health.py
  • Explicitly export from modules via __all__

Changelog

  • Moved robot_server/service/app.py to robot_server/app.py
    • Also removed unused robot_server/main.py
    • Updated service file, Makefile, and integration test fixture to account for this move
    • Added CORS middleware to the server
  • Moved top-level router from app.py to robot_server/router.py
  • Moved /health logic to robot_server/health/
    • Moving forward, I think we should reconsider classifying the health endpoint as "legacy", given its importance
      • I think tweaking the response is definitely on the table, though
    • Previously, this logic lived in:
      • robot_server/service/legacy/models/health.py
      • robot_server/service/legacy/routers/health.py
  • Moved /system logic to robot_server/system/
    • Previously lived in:
      • robot_server/system/time.py (Time utilities)
      • robot_server/service/system/models.py
      • robot_server/service/system/router.py
  • Deduplicated route dependencies
    • Moved all instances of the version check to the top-level router attachment
    • Combined verify_hardware and get_hardware
  • Added CORS middleware to FastAPI to accept all origins

Review requests

The riskiest changes here are:

  • Moving the main app export
  • Dependency deduping, especially around hardware and version header checking

I'd say we'd want to smoke test:

  • The full OS build for this branch
  • The schema endpoint output
  • The docs endpoint(s) output

Risk assessment

Pretty low, provided we run the smoke tests above. Integration / unit test coverage is good for the endpoints that were moved

@mcous mcous added robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). robot server Affects the `robot-server` project labels Apr 13, 2021
@codecov
Copy link

codecov bot commented Apr 13, 2021

Codecov Report

❗ No coverage uploaded for pull request base (edge@1968164). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #7632   +/-   ##
=======================================
  Coverage        ?   82.63%           
=======================================
  Files           ?      321           
  Lines           ?    21314           
  Branches        ?        0           
=======================================
  Hits            ?    17612           
  Misses          ?     3702           
  Partials        ?        0           

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 1968164...6ba99e5. Read the comment docs.

@mcous mcous marked this pull request as ready for review April 14, 2021 19:07
@mcous mcous requested review from a team as code owners April 14, 2021 19:07
@mcous
Copy link
Contributor Author

mcous commented Apr 28, 2021

Discussed in CPX Slack, got 👍's all around to merge

@mcous mcous merged commit 2b249d9 into edge Apr 28, 2021
@mcous mcous deleted the robot-server_structure-reorg branch April 28, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
robot server Affects the `robot-server` project robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Adding "CORS" policy to opentrons APP
2 participants