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(api): module update process bug #14572

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Feb 29, 2024

Closes RQA-2417

Overview

Python 3.10 removed the loop param of create_subprocess_exec. This instance in firmware update process got left out and was not caught by the linter because the loop was being passed as a kwarg.

Test Plan

Test updating an arduino AVR module (temp/ mag) arduino SAMD module (thermocycler gen1) and an STM32 module (TC GEN2/ Heater-shaker).
I was able to successfully update the H/S with this change.

Review requests

  • usual stuff

@sanni-t sanni-t marked this pull request as ready for review February 29, 2024 21:02
@sanni-t sanni-t requested a review from a team as a code owner February 29, 2024 21:02
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.50%. Comparing base (d6d9416) to head (43bd7db).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14572      +/-   ##
==========================================
- Coverage   67.50%   67.50%   -0.01%     
==========================================
  Files        2514     2514              
  Lines       72376    72375       -1     
  Branches     9317     9317              
==========================================
- Hits        48857    48856       -1     
  Misses      21314    21314              
  Partials     2205     2205              
Flag Coverage Δ
app 63.99% <ø> (ø)
components 48.40% <ø> (ø)
g-code-testing 92.43% <ø> (ø)
labware-library 41.11% <ø> (ø)
protocol-designer 37.90% <ø> (ø)
step-generation 86.88% <ø> (ø)

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

Files Coverage Δ
...i/src/opentrons/hardware_control/modules/update.py 27.69% <ø> (-1.10%) ⬇️
...ver/robot_server/service/legacy/routers/modules.py 98.36% <ø> (ø)

@sfoster1
Copy link
Member

I sort of scooped you on this one sorry: 6c5a6dc

@sanni-t
Copy link
Member Author

sanni-t commented Feb 29, 2024

I have more stuff in here than your PR @sfoster1 😛
Surprised it's not giving a merge conflict on the removal of loop kwarg, but I've fully removed the loop arg from the function def & call as well. So this can still be merged instead of closing.

@sfoster1
Copy link
Member

I have more stuff in here than your PR @sfoster1 😛 Surprised it's not giving a merge conflict on the removal of loop kwarg, but I've fully removed the loop arg from the function def & call as well. So this can still be merged instead of closing.

That's the best part about kwargs, it's a javascript like experience free of pesky runtime checks

@sanni-t sanni-t changed the base branch from chore_release-7.2.0 to edge February 29, 2024 22:37
@sanni-t sanni-t requested review from a team as code owners February 29, 2024 22:37
@sanni-t sanni-t requested a review from a team February 29, 2024 22:37
@sanni-t sanni-t requested a review from a team as a code owner February 29, 2024 22:37
@sanni-t sanni-t requested review from smb2268 and removed request for a team February 29, 2024 22:37
@sanni-t sanni-t added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Feb 29, 2024
@sanni-t
Copy link
Member Author

sanni-t commented Feb 29, 2024

To be rebased and merged in edge after 7.0.2 mergeback

@sfoster1 sfoster1 removed the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Mar 7, 2024
@sanni-t sanni-t force-pushed the api-fix_module_update_process_loop_bug branch from 6dc0d13 to 43bd7db Compare March 7, 2024 15:49
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 to me, thank you!

@sanni-t sanni-t merged commit b533692 into edge Mar 7, 2024
44 of 45 checks passed
@sanni-t sanni-t deleted the api-fix_module_update_process_loop_bug branch March 7, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants