feat: allow testing.State.get_relation to accept relation objects#2359
Conversation
21d791c to
74c1e66
Compare
74c1e66 to
4e0af23
Compare
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
I'm +1 on this, and now -1 on both the alternatives. Thanks for taking the time to work through the options.
I'm not sure that checking the (often empty) interface and endpoint adds value, but I'm not opposed to including that.
Please do update the PR description (or commit body) for merging.
| if isinstance(relation, RelationBase) and ( | ||
| (type(state_relation), state_relation.endpoint) | ||
| != (type(relation), relation.endpoint) |
There was a problem hiding this comment.
Is it the case that we don't care about the interface, because realistically we infer it from charm metadata, so it can't really change?
There was a problem hiding this comment.
I think validating interfaces here might come back to haunt us in the future, as there are behind the scenes shenanigans involved. See other comment (relevant bit quoted below).
PS: I'm dropping validating the interface from this PR, because I'm worried that it may lead to unexpected issues in future. The documentation threatens that it will be auto-populated from the charm's metadata if not specified, but I think this only happens with
State.from_context(where you don't specify anything manually). But if we ever did start auto-populating this duringContext.runor similar it would be easy to get an unexpected mismatch here (manually createRelationwithoutinterfacename, then query post-runStateand get mismatch).
This PR updates
testing.State.get_relationto accept a relation object. The means that users can writestate_out.get_relation(rel_in)instead ofstate_out.get_relation(rel_in.id), and if they do so, the returned type is guaranteed to match the relation type passed in (peer, subordinate, or regular) -- both statically and at runtime.Resolves: #2243
Alternative to:
For context on why
testing.State.get_relationonly acceptsintcurrently, see discussion on the 2024 PR where this was designed: canonical/ops-scenario#134