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(update-server): Type update-server more strictly #10458

Merged
merged 24 commits into from
May 26, 2022

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented May 25, 2022

Overview

This PR enhances static type-checking in update-server.

Changelog

  • Update mypy from 0.910 to 0.940. I needed to do this to resolve perplexing errors where mypy didn't think the aiohttp.web.Request class existed.
  • Turn on mypy's strict mode, like we do in the api and robot-server packages.
    • Resolve as many of the resultant typing errors as I could today.
    • Exclude the rest in mypy.ini and leave a # TODO comment for them. This follows the pattern set by api and robot-server.
  • Solve one actual silly bug that the type-checker caught..

Review requests

Go over the code and make sure that the changes are only to type annotations, unless I've noted otherwise.

I've tested this on a real OT-2 by making sure that it can still apply an update file and that it can still accept robot renames. I don't have an OT-3 environment to test on.

Risk assessment

Low as long as I haven't accidentally affected the runtime behavior of anything.

@SyntaxColoring SyntaxColoring added robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). chore refactor labels May 25, 2022
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #10458 (1670d8b) into edge (69fc92b) will decrease coverage by 0.00%.
The diff coverage is 75.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #10458      +/-   ##
==========================================
- Coverage   73.64%   73.64%   -0.01%     
==========================================
  Files        2134     2135       +1     
  Lines       57366    57376      +10     
  Branches     5754     5754              
==========================================
+ Hits        42249    42253       +4     
- Misses      13913    13919       +6     
  Partials     1204     1204              
Flag Coverage Δ
update-server 72.50% <75.36%> (-0.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
update-server/otupdate/buildroot/__main__.py 0.00% <0.00%> (ø)
update-server/otupdate/common/systemd.py 0.00% <0.00%> (ø)
update-server/otupdate/openembedded/__init__.py 85.41% <ø> (ø)
update-server/otupdate/openembedded/__main__.py 0.00% <0.00%> (ø)
update-server/otupdate/openembedded/updater.py 76.22% <ø> (ø)
update-server/otupdate/common/update_actions.py 74.19% <33.33%> (-11.11%) ⬇️
update-server/otupdate/common/handler_type.py 80.00% <80.00%> (ø)
update-server/otupdate/common/name_management.py 35.38% <85.71%> (ø)
update-server/otupdate/common/update.py 80.00% <87.50%> (ø)
update-server/otupdate/buildroot/__init__.py 87.17% <100.00%> (ø)
... and 6 more

hostname = loop.run_until_complete(name_management.setup_hostname())
hostname = loop.run_until_complete(name_management.set_up_static_hostname())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is intentional. setup_hostname() got renamed to set_up_static_hostname() in #10219.

@@ -53,9 +53,9 @@ test:

.PHONY: lint
lint:
$(python) -m mypy otupdate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my experience, running mypy first makes for the nicest dev experience because its errors tend to be the ones that matter most.

@SyntaxColoring SyntaxColoring marked this pull request as ready for review May 25, 2022 21:04
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner May 25, 2022 21:04
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Changes look great to me! We should get embedded signoff before moving forward. I won't officially approve until I've actually tested this on hardware, to be safe

update-server/otupdate/buildroot/update_actions.py Outdated Show resolved Hide resolved
update-server/otupdate/common/update_actions.py Outdated Show resolved Hide resolved
@mcous mcous added the embedded label May 26, 2022
@mcous mcous requested a review from a team May 26, 2022 15:43
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the changes on the openembedded side!

@SyntaxColoring
Copy link
Contributor Author

Tested again after that last commit that update-server starts up, responds to name requests, and accepts update files.

@SyntaxColoring SyntaxColoring merged commit faacde2 into edge May 26, 2022
@SyntaxColoring SyntaxColoring deleted the update_server_strict_typing branch May 26, 2022 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore embedded refactor 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