-
Notifications
You must be signed in to change notification settings - Fork 27
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 Decimal serialization #6854
🐛 Fix Decimal serialization #6854
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6854 +/- ##
==========================================
+ Coverage 88.33% 89.20% +0.87%
==========================================
Files 1553 1268 -285
Lines 61193 53074 -8119
Branches 2095 874 -1221
==========================================
- Hits 54055 47347 -6708
+ Misses 6806 5591 -1215
+ Partials 332 136 -196
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change affects some models in the web api schema. Can you please, cd web/server
and
- increase minor version
make minor-version
- update openapi specs
make openapi-specs
…/osparc-simcore into fix-decimal-serialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please wait a bit with merging this one? This will break the api-server. It essentially reverts the changes I did in https://github.com/ITISFoundation/osparc-simcore/pull/6658/files in order to make the api-server as "backwards compatible as possible". I am confident your changes should go in. But it would be nice to have a strategy for how to handle this in the api-server before this breaks it. I will discuss with @pcrespov tomorrow how to handle this. Sorry for the inconvenience.
I am a bit scared by the fact that you don't see here that you are breaking backwards compatibility of the api-server. I think I will set up a hook so people see that they are breaking our interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bisgaard-itis @giancarloromeo I will temporary block it to avoid accidental merging. Let me know if this is a problem for you! :-)
@pcrespov @bisgaard-itis The related PR (#6853) is already in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this one can go in for me. I will have to handle the backwards compatibility directly in the api-server. I am half way there and this should be quite easy to handle. @pcrespov I believe you can also approve this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also update the api-server's openapi.json
in this PR?
cd services/api-server
make install-dev
make openapi.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @bisgaard-itis . It does not affect backwards compatibility f the api-server
Quality Gate passedIssues Measures |
What do these changes do?
Pydatic v2 serializes
Decimal
s as strings to avoid loss of precision derived from conversion to float. This PR reverts the partial changes made during the migration.Related issue/s
How to test
Dev-ops checklist