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

fix(robot-server): exempt /logs and RPC from version header requirement #7927

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Jun 10, 2021

Overview

This PR fixes an unreleased bug with robot logs downloads caused by #7632. It exempts certain endpoints (/logs included) to fix robot logs downloading.

Closes #7924.

Changelog

  • fix(robot-server): exempt /logs and RPC from version header requirement

Review requests

  • Can download logs with this build installed on a robot

Risk assessment

Low. Revert of bad change

@mcous mcous added the robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). label Jun 10, 2021
@mcous mcous requested a review from a team as a code owner June 10, 2021 17:19
@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #7927 (5177290) into release_4.4.0 (3563656) will increase coverage by 12.74%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           release_4.4.0    #7927       +/-   ##
==================================================
+ Coverage          80.70%   93.44%   +12.74%     
==================================================
  Files                223      130       -93     
  Lines              16530     5083    -11447     
==================================================
- Hits               13340     4750     -8590     
+ Misses              3190      333     -2857     
Impacted Files Coverage Δ
robot-server/robot_server/app.py 91.42% <100.00%> (ø)
robot-server/robot_server/router.py 93.10% <100.00%> (ø)
...er/robot_server/service/legacy/routers/__init__.py 100.00% <100.00%> (ø)
opentrons/protocol_engine/clients/__init__.py
opentrons/protocols/execution/json_dispatchers.py
opentrons/drivers/serial_communication.py
opentrons/__init__.py
opentrons/commands/types.py
opentrons/hardware_control/emulation/tempdeck.py
opentrons/tools/args_handler.py
... and 346 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 3563656...5177290. Read the comment docs.

@@ -33,6 +34,7 @@
router.include_router(
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't leave a comment in the right place because GitHub is dumb, but:

Can we leave a comment on the router.include_router(router=legacy_routes, ...) above, to say that it's deliberately excluded from check_version_header? (You've already done this for the sub-routers within legacy_routes.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That feels a little inaccurate to me, since many of the legacy_routesare very much not excluded from the header check requirement

@mcous mcous mentioned this pull request Jun 10, 2021
@nusrat813 nusrat813 self-requested a review June 10, 2021 17:28
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Confirmed with the reproduction steps in #7924 that this fixes that bug.

Other than my optional nitpick above, LGTM. 🎉

Copy link

@nusrat813 nusrat813 left a comment

Choose a reason for hiding this comment

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

Tested, LGTM

@mcous mcous merged commit 58eb503 into release_4.4.0 Jun 10, 2021
@mcous mcous deleted the release_4.4.0-fix-logs-header-reqment branch June 10, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants