Add Pydantic validation for bootc status JSON #4423
Conversation
AthreyVinay
added
area | maintenance
|
|
||
|
|
||
| class BootcHost(BootcMetadataContainer): | ||
| status: Optional[HostStatus] = None |
There was a problem hiding this comment.
Could you revert the order of these definitions for readability? I wonder if we should also put these in a dedicated tmt.model package.
There was a problem hiding this comment.
I kept the definitions ordered from leaf to root intentionally. Did not want to do string-bsaed forward refs (gaining direct type references without forward annotations).
There was a problem hiding this comment.
Yeah, but it can be distracting. If we go with the tmt.model approach I think it would be good in either ordering since it is more compartmentalized then. Also with __future__.annotations the order should also be arbitrary in there.
There was a problem hiding this comment.
This mostly comes down to readability preference. I’m happy to reorder the models if that’s the general preference, otherwise I’m fine keeping the current layout.
| booted = image_status.get('status', {}).get('booted', {}) | ||
| if not booted: | ||
| raise KeyError("'booted' key") | ||
| host = BootcHost.from_json(output) |
There was a problem hiding this comment.
try ... except this for json failures or whatnots.
There was a problem hiding this comment.
i think we should fail hard. Happy to let pydantic and json parsing raise.
There was a problem hiding this comment.
It should fail either way. The question is if you want to have a more friendly error message than what you would get from pydantic.
There was a problem hiding this comment.
Yeah - meant failing hard leaving pydantic handle. Also see the similar question that I had from a similar task: #4356 (comment)
Resolves #4411