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

♻️🔨 Unexpected mypy upgrade revealed configuration and code failures #6527

Merged
merged 14 commits into from
Oct 15, 2024

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Oct 14, 2024

What do these changes do?

The issue was caused by our CI pipeline not using a pinned version of mypy; instead, it was always fetching the latest version. This happened because requirements/ci.txt did not include requirements/tools.txt, which specifies the version for mypy.

As a result, mypy was updated between the CI passing for this PR and its merge into the master branch (commit da15add).

The new version of mypy introduced additional type checks that uncovered issues in some parts of the code. Specifically, we had to make further changes to the section handling invoice attachments in emails to resolve these newly identified issues.

Highlights

  • ♻️ payments service:
    • Proper attachment and better error handling to invoice attachment.
    • Adapts tests
    • @matusdrobuliak66 please pay special attention to the changes i did of your code in
      - services/payments/src/simcore_service_payments/services/notifier_email.py
      - services/payments/tests/unit/test_services_notifier_email.py
  • ♻️ ensures create_troubleshotting_log_message does not raise
  • 🔨 uses uv run to run script

Related issue/s

How to test

Dev-ops checklist

NOne

@pcrespov pcrespov self-assigned this Oct 14, 2024
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.1%. Comparing base (cafbf96) to head (7b3df84).
Report is 628 commits behind head on master.

Files with missing lines Patch % Lines
...imcore_service_payments/services/notifier_email.py 91.6% 3 Missing ⚠️
...models_library/utils/_original_fastapi_encoders.py 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6527      +/-   ##
=========================================
+ Coverage    84.5%   88.1%    +3.5%     
=========================================
  Files          10    1548    +1538     
  Lines         214   63351   +63137     
  Branches       25    2059    +2034     
=========================================
+ Hits          181   55845   +55664     
- Misses         23    7188    +7165     
- Partials       10     318     +308     
Flag Coverage Δ
integrationtests 64.7% <ø> (?)
unittests 86.1% <90.6%> (+1.5%) ⬆️

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

Files with missing lines Coverage Δ
...es/service-library/src/servicelib/logging_utils.py 77.8% <100.0%> (ø)
packages/service-library/src/servicelib/utils.py 91.6% <100.0%> (ø)
...es/service-library/src/servicelib/utils_secrets.py 92.3% <100.0%> (ø)
...models_library/utils/_original_fastapi_encoders.py 51.2% <0.0%> (ø)
...imcore_service_payments/services/notifier_email.py 95.1% <91.6%> (ø)

... and 1493 files with indirect coverage changes

@pcrespov pcrespov added t:maintenance Some planned maintenance work a:models-library labels Oct 14, 2024
@pcrespov pcrespov marked this pull request as ready for review October 14, 2024 14:36
Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

👍

@pcrespov pcrespov requested a review from GitHK as a code owner October 14, 2024 14:42
@pcrespov pcrespov changed the title 🔨 Unexpected mypy failure in models-library ♻️🔨 Unexpected mypy upgrade surfaced some failures in configuration and code Oct 14, 2024
@pcrespov pcrespov added this to the MartinKippenberger milestone Oct 14, 2024
@pcrespov pcrespov changed the title ♻️🔨 Unexpected mypy upgrade surfaced some failures in configuration and code ♻️🔨 Unexpected mypy upgrade revealed configuration and code failures Oct 14, 2024
@pcrespov pcrespov enabled auto-merge (squash) October 14, 2024 17:37
Copy link

Copy link
Contributor

@jsaq007 jsaq007 left a comment

Choose a reason for hiding this comment

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

👍

@pcrespov pcrespov merged commit b25d613 into ITISFoundation:master Oct 15, 2024
57 checks passed
@pcrespov pcrespov deleted the mai/fix-typecheck-master branch October 15, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:models-library t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants