fix: support Pydantic MISSING sentinel in ops.Relation.save#2306
fix: support Pydantic MISSING sentinel in ops.Relation.save#2306dimaqq merged 8 commits intocanonical:mainfrom
Conversation
james-garner-canonical
left a comment
There was a problem hiding this comment.
The core code change looks good to me, as does the to-do list.
james-garner-canonical
left a comment
There was a problem hiding this comment.
The implementation looks good based on our team discussion.
The addition of a class using this sentinel to the tests is great, ensuring that we don't crash on MISSING anymore. I think it would be a good idea to add a test that explicitly asserts that MISSING doesn't get serialized as well.
Are you still planning to add some real Juju tests, per the checklist in the PR description? I'm not sure if they're necessary, but I'm definitely not opposed to us adding more integration tests.
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
I remain unconvinced that there's a need to support this use-case, and also that "MISSING" should mean "REMOVE", but the PR does implement the team decision.
I do think we should explicitly document the behaviour in a comment - maybe also in the reference doc as well (it feels like that should be expected if we want charms to rely on this behaviour).
james-garner-canonical
left a comment
There was a problem hiding this comment.
I'm happy with the new behaviour and with the test coverage.
benhoyt
left a comment
There was a problem hiding this comment.
Seems reasonable to me, thanks.
Support Pydantic's experimental
MISSINGsentinel field types and values.ops.Relation.load()does the right thing already.ops.Relation.save()needs a small fix to erase missing fieldsCheck list:
Fixes #2299