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/removing-old-json-schemas 🚨 #3966

Conversation

matusdrobuliak66
Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 commented Mar 13, 2023

What do these changes do?

  • ♻️ These changes replace old manually created json schemas node-meta-v0.0.1.json and project-v0.0.1.json by it's pydantic counterparts node-meta-v0.0.1-pydantic.json and project-v0.0.1-pydantic.json. (Old ones are still used in the director v0)

Motivation:

  • when comparing the current json schema with pydantic generated json schema it is not possible to compare them visually as they differ a lot. The only option is to compare via json-schema-diff tool which outputs might not be so useful, here ex.: Breaking change after pydantic version upgrade #3905
  • we are trying to consolidate the open API specs and corresponding pydantic models that we use

Tried two approaches:
Currently, project-v0.0.1.json json schema is converted to project-v0.0.1-converted.yaml open api spec that is used as a reference in the final openapi.yaml (to be precise in project.yaml -> openapi-project.yaml -> openapi.yaml)

  1. Approach (use FastApi generated openapi specs)
  • This was problematic because swagger cli validation was complaining about many things. Ex. fastapi generated openapi has problem with generating x-pattern properties (example in our code: Workbench = dict[NodeIDStr, Node], where NodeIDStr is constrained string.
  1. Approach (use Pydantic model generated json schema to create openapi specs)
  • Similarity as we do it now, we replace the manually created old json schema with the Pydantic generated schema. This generated schema is then converted to the openapi specs with the json-schema-diff tool.
  • There was problem with inline references, therefore I dereferenced the JSON schema, and afterward needed to remove the "definition" field as it was not needed anymore as it was already dereferenced and it caused problems later on in the JSON schema validation tool we use.
  • Therefore now the flow is as follows: (generate dereferenced pydantic json schema) project-v0.0.1-pydantic.json --> (convert it to openapi schema via json-schema-diff) project-v0.0.1-pydantic-converted.yaml --> (removing the "definition" field) project-v0.0.1-pydantic-converted-clean.yaml

ISSUE found on the way:

Related issue/s

How to test

Checklist

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 37729 lines exceeds the maximum allowed for the inline comments feature.

…matusdrobuliak66/osparc-simcore into maintenance/upgrade-of-pydantic-library
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 37725 lines exceeds the maximum allowed for the inline comments feature.

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Merging #3966 (d873cb7) into master (0850c25) will increase coverage by 2.3%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3966     +/-   ##
========================================
+ Coverage    81.1%   83.4%   +2.3%     
========================================
  Files         836     918     +82     
  Lines       35248   40342   +5094     
  Branches      957     957             
========================================
+ Hits        28587   33661   +5074     
- Misses       6442    6475     +33     
+ Partials      219     206     -13     
Flag Coverage Δ
integrationtests 72.3% <ø> (+10.5%) ⬆️
unittests 82.6% <100.0%> (+5.3%) ⬆️

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

Impacted Files Coverage Δ
...ls-library/src/models_library/projects_nodes_io.py 97.1% <ø> (ø)
...2/src/simcore_service_director_v2/core/settings.py 98.4% <ø> (ø)
...mcore_service_webserver/projects/project_models.py 92.8% <ø> (-3.3%) ⬇️
...ages/models-library/src/models_library/projects.py 95.5% <100.0%> (ø)
...dels-library/src/models_library/projects_access.py 100.0% <100.0%> (ø)
...odels-library/src/models_library/projects_nodes.py 95.5% <100.0%> (+0.1%) ⬆️
...s/models-library/src/models_library/services_ui.py 100.0% <100.0%> (ø)

... and 273 files with indirect coverage changes

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 37732 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 37724 lines exceeds the maximum allowed for the inline comments feature.

@matusdrobuliak66 matusdrobuliak66 self-assigned this Mar 14, 2023
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 41648 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 37700 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 37696 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 37699 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 35127 lines exceeds the maximum allowed for the inline comments feature.

@matusdrobuliak66 matusdrobuliak66 changed the title WIP: ♻️ Maintenance/testing-removing-old-json-schemas WIP: ♻️ Maintenance/removing-old-json-schemas Mar 14, 2023
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 35127 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 22116 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 22116 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Thx !
Did you manage to check 3 (there is a Make recipe in package/postgres-database to import e.g. master db and use it with the deployment?

Please make sure to check that everything is working nicely in master when you get this pr deployed.

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 nice step. I still would remove the old crap as soon as possible (since we do not use them anymore, and that includes the tests between the said old crap and newest cleaner json schemas).
good job!

@sanderegg sanderegg modified the milestones: Mithril, The Next Milestone Mar 31, 2023
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 22116 lines exceeds the maximum allowed for the inline comments feature.

…matusdrobuliak66/osparc-simcore into maintenance/upgrade-of-pydantic-library
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 24606 lines exceeds the maximum allowed for the inline comments feature.

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.

OK with me

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 21003 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 21003 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 21003 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 21003 lines exceeds the maximum allowed for the inline comments feature.

@codeclimate
Copy link

codeclimate bot commented Apr 4, 2023

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

View more on Code Climate.

@sonarcloud
Copy link

sonarcloud bot commented Apr 4, 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants