-
Notifications
You must be signed in to change notification settings - Fork 26
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃悰 fixed SimcoreServiceSettingLabelEntry parsing #3052
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3052 +/- ##
========================================
- Coverage 80.6% 80.5% -0.1%
========================================
Files 712 712
Lines 30686 30686
Branches 4006 4006
========================================
- Hits 24737 24727 -10
- Misses 5099 5109 +10
Partials 850 850
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 for the hotfix!
Kudos, SonarCloud Quality Gate passed!聽 聽 0 Bugs No Coverage information |
"NanoCPUs": 0, | ||
"MemoryBytes": 0, | ||
"GenericResources": [], | ||
SimcoreServiceSettingLabelEntry.parse_obj( |
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.
as @pcrespov said, using parse_obj is always more secure than the constructor.
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.
not sure in this case makes a big difference though :-)
I think it is secure when your data is in a dict
-like object but here we pass explicitly the values
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.
Anybody an idea why this fails stochastically?
SimcoreServiceSettingLabelEntry.parse_obj( | ||
dict( | ||
name="Resources", | ||
type="Resources", |
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.
-Q: why are you not using the constructor? (curious about your decision)
-TIP: an alternative is to add Config.allow_population_by_field_name = True
to SimcoreServiceSettingLabelEntry
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.
I think that was some very old code. I now Know we should go for the parsing instead of the constructor. I've seen that using the constructor does not trigger the validators (or some of them).
What do these changes do?
iSeg
are not able to start because the parsing failed.Related issue/s
How to test
Checklist