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

♻️ catalog API schema models moved to models-library #4553

Merged
merged 27 commits into from
Jul 31, 2023

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Jul 27, 2023

What do these changes do?

Basically, it moves the pydantic models for the catalog services (i.e. models/schemas) to models_library.api_schemas_catalog. This way both server and client can share these models. This PR only affects to catalog and models_library. Changes in the clients (e.g. web-server or api-server will be done in separate PRs).

  • ♻️ Cleanup models-library
    • 🗑️ removes unused modules
    • New single parametrized test to validate all Config.schema_extra exposed examples in models_library. SEE packages/models-library/tests/test__models_examples.py!
    • Previous test detected import simcore_postgres_database.models.cluster.ClusterType in models_library. This PR introduces a duplicated enum and adapts pydantic model to handle it until next PR.
    • ⚗️ See rationale of equivalent enums and str-enums in test__pydantic_models_and_enums.py
    • ✨ Common pydantic validator to parse equivalent enums
  • ♻️ Moves catalog's schemas to models-library.api_schemas_catalog:
    • can be used by any client connecting to the catalog e.g. webserver, api-server
    • 🗑️ removes unused dag API entrypoint -> upgrades services/catalog version: 0.4.0 → 0.5.0
    • Moves tests on schema models to models_library

Next PR will do the same but with the schema models of director-v2, i.e. they will be moved to models_library.

Related issue/s

How to test

Driving tests were:

  • packages/models-library/tests/test__pydantic_models_and_enums.py
  • packages/models-library/tests/test_utils_common_validators.py
  • And all tests from catalog services and models_library

DevOps

None

@pcrespov pcrespov changed the title Is4481/modelslib and catalog ♻️ catalog API schema models moved to models-library Jul 27, 2023
@pcrespov pcrespov self-assigned this Jul 27, 2023
@pcrespov pcrespov added this to the Sundae milestone Jul 27, 2023
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #4553 (cfa1c85) into master (52ca6b4) will increase coverage by 1.6%.
The diff coverage is 84.2%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #4553     +/-   ##
========================================
+ Coverage    86.6%   88.2%   +1.6%     
========================================
  Files         746     898    +152     
  Lines       34139   39706   +5567     
  Branches      647     876    +229     
========================================
+ Hits        29570   35059   +5489     
- Misses       4415    4445     +30     
- Partials      154     202     +48     
Flag Coverage Δ
integrationtests 68.4% <66.6%> (-0.1%) ⬇️
unittests 85.5% <84.2%> (+1.5%) ⬆️

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

Files Changed Coverage Δ
.../src/models_library/api_schemas_webserver/_base.py 90.9% <ø> (ø)
...s/models-library/src/models_library/basic_regex.py 100.0% <ø> (ø)
.../service-library/src/servicelib/fastapi/openapi.py 68.1% <0.0%> (ø)
...es/catalog/src/simcore_service_catalog/api/root.py 100.0% <ø> (ø)
...c/simcore_service_catalog/api/routes/_constants.py 100.0% <ø> (ø)
...ervices/catalog/src/simcore_service_catalog/cli.py 0.0% <0.0%> (ø)
...ry/src/models_library/utils/string_substitution.py 80.7% <20.0%> (ø)
...ages/models-library/src/models_library/services.py 91.3% <46.1%> (ø)
...models_library/function_services_catalog/_utils.py 72.4% <46.6%> (ø)
...imcore_service_catalog/db/repositories/services.py 76.6% <46.6%> (-0.7%) ⬇️
... and 61 more

... and 324 files with indirect coverage changes

@pcrespov pcrespov force-pushed the is4481/modelslib-and-catalog branch 2 times, most recently from 94756b2 to f8ee2fd Compare July 28, 2023 16:02
@pcrespov pcrespov force-pushed the is4481/modelslib-and-catalog branch from 4a7feaa to cfa1c85 Compare July 28, 2023 18:38
@codeclimate
Copy link

codeclimate bot commented Jul 28, 2023

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

View more on Code Climate.

@sonarcloud
Copy link

sonarcloud bot commented Jul 28, 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.0% 0.0% Duplication

@pcrespov pcrespov marked this pull request as ready for review July 28, 2023 18:42
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.

Thanks! 🥇

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Nice, only one doubt about how the validators are registered.
I don't see an advantage, I would say only a disadvantage.

Comment on lines +123 to +125
_from_equivalent_enums = validator("type", allow_reuse=True, pre=True)(
create_enums_pre_validator(ClusterTypeInModel)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I get it, this is a validator. Why is it register like this and not as a decorated function?
I find it less readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

This technique is used to reuse validators. The validators I add in models_library.utils.common_validators are solutions to common pitfalls. The Enum problem is one of them and therefore it is very convenient to have a tested solution in place instead of being reinventing a solution in place.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got that.
My question was more in the likes of why not do something like this:

@validator(allow_reuse=True, pre=True)
@classmethod
def _from_equivalent_enums(cli, values):
    return create_enums_pre_validator(values)

Since this is the format to whom we are normally used to?

@pcrespov pcrespov merged commit f2f6a81 into ITISFoundation:master Jul 31, 2023
50 checks passed
@pcrespov pcrespov deleted the is4481/modelslib-and-catalog branch July 31, 2023 07:06
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Sep 22, 2023
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants