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

♻️ Maintenance: reduce code duplication #4982

Merged
merged 18 commits into from
Nov 7, 2023

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Nov 6, 2023

What do these changes do?

The purpose of this PR is to remove some of the duplication reported by sonarcloud after PR #4973.

image

The duplication is removed by moving duplicated parts from services to the libraries. Specifically we move:

  • Default API error model schema to models_library.api_schemas__common/errors.DefaultApiError
  • Tools to create httpx clients and set up in an app to servicelib.fastapi.http_client
  • Tools to handle exceptions to: servicelib.fastapi.exceptions_utils
  • Common parts for application settings to settings_library.application
  • Function duplication in simcore_service_webserver.wallets.payments_handlers

NOTE that I only changed payments, webserver and dynamic-scheduler services. Other services might also need to be refactored. I propose that every service owner will follow up on his corresponding service.

Related issue/s

How to test

Add tests for new modules

  • packages/models-library/tests/test_api_schemas__common.py
  • packages/service-library/tests/fastapi/test_exceptions_utils.py
  • `packages/settings-library/src/settings_library/application.py

DevOps

None

@pcrespov pcrespov self-assigned this Nov 6, 2023
Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #4982 (65b8dd4) into master (a4002f4) will decrease coverage by 0.1%.
The diff coverage is 98.6%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #4982     +/-   ##
========================================
- Coverage    86.8%   86.7%   -0.1%     
========================================
  Files        1239    1157     -82     
  Lines       51114   49459   -1655     
  Branches     1078     941    -137     
========================================
- Hits        44398   42917   -1481     
+ Misses       6479    6333    -146     
+ Partials      237     209     -28     
Flag Coverage Δ
integrationtests 54.1% <14.2%> (-10.8%) ⬇️
unittests 84.9% <98.6%> (+0.3%) ⬆️

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

Files Coverage Δ
...y/src/models_library/api_schemas__common/errors.py 100.0% <100.0%> (ø)
...library/api_schemas_directorv2/dynamic_services.py 96.6% <100.0%> (ø)
...emas_resource_usage_tracker/credit_transactions.py 96.0% <100.0%> (-0.2%) ⬇️
...pi_schemas_resource_usage_tracker/pricing_plans.py 100.0% <100.0%> (ø)
...api_schemas_resource_usage_tracker/service_runs.py 100.0% <100.0%> (ø)
...-library/src/models_library/api_schemas_storage.py 89.5% <100.0%> (ø)
...y/src/models_library/api_schemas_webserver/auth.py 100.0% <100.0%> (ø)
...dels_library/api_schemas_webserver/computations.py 100.0% <100.0%> (ø)
...src/models_library/api_schemas_webserver/groups.py 95.3% <100.0%> (ø)
...rc/models_library/api_schemas_webserver/product.py 100.0% <100.0%> (ø)
... and 14 more

... and 119 files with indirect coverage changes

@pcrespov pcrespov force-pushed the maintenance/duplications branch from 372962a to 5202e14 Compare November 6, 2023 13:49
@pcrespov pcrespov added a:webserver issue related to the webserver service t:maintenance Some planned maintenance work labels Nov 6, 2023
@pcrespov pcrespov added this to the 7peaks milestone Nov 6, 2023
@pcrespov pcrespov changed the title WIP: ♻️ Maintenance/duplications ♻️ Maintenance: reduce code duplication Nov 6, 2023
@pcrespov pcrespov marked this pull request as ready for review November 6, 2023 16:50
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

Very good! thanks a lot for this cleanup action!

@pcrespov pcrespov enabled auto-merge (squash) November 6, 2023 18:32
@pcrespov pcrespov force-pushed the maintenance/duplications branch from dc1cca1 to aaa165b Compare November 7, 2023 09:09
Copy link

codeclimate bot commented Nov 7, 2023

Code Climate has analyzed commit 65b8dd4 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link

sonarqubecloud bot commented Nov 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

Copy link
Contributor

@bisgaard-itis bisgaard-itis 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. Thanks

@pcrespov pcrespov merged commit 420f177 into ITISFoundation:master Nov 7, 2023
@pcrespov pcrespov deleted the maintenance/duplications branch November 7, 2023 13:32
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Nov 23, 2023
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:dynamic-scheduler a:payments payments service a:webserver issue related to the webserver service t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants