Redefine ops.relation so multi relationships work both ways #603
Conversation
Add .ready() and .unready() to ConsumerBase. multi= is left as an argument to `__init__` for now so we don't break libraries as soon as it's instantiated, but it does not actually do anything anymore. Instead, it can now multiplex in both directions. Add `.relation` to `ConsumerEvents` by destructuring the relation and fetching it back once a snapshot is loaded, so `ConsumerEvents` have `event.relation` like `RelationiEvents` do. Simplify `ConsumerEvents` so they all inherit from a single base class, considering that none of them had invidualized behavior. Remove `TooManyProviders` and add a test to ensure that multiple consumers can work with one provider. Closes canonical#586
rbarry82
requested review from
Abuelodelanada,
balbirthomas,
dstathis and
sed-i
There was a problem hiding this comment.
It might be nice to fix some of the method names before moving the current convention further. It's a bit unfortunate because I'm late to this review, as I didn't see it coming back in July. "ready" and "unready" are not typically used as verbs in this context, and as such are not great options for actions that inflict changes on these objects. They look more like questions ("is it ready?"), and I suspect that's how we'll see existing code using such conventions.
This could instead be something like foo.set_ready(True/False).
Let's talk to see how we can get out of the current situation without breaking code.
|
@niemeyer Fortunately, as far as I know, the o11y team is the only one really using this part of the framework so far, so we can probably rename these without too much fallout |
|
@rbarry82 That's great news. In general we cannot change public APIs after we release them, as the framework is a very important central part of the system. With that said, I've learned that we haven't yet made these changes available in an actual release that would be fetched by charmcraft, so this is probably internal use only. Let's please have a conversation to detail how we can backtrack this in the least disruptive way possible, and also try to get a mechanism in place for us to create strong conventions that will be carried on in the future. |
| data: typing.Optional[typing.Dict] = None): | ||
| super().__init__(handle) | ||
| self.relation = relation | ||
| self.data = data |
There was a problem hiding this comment.
On second thought would it be more consistent if we nested data under relation event.relation.data ? I do appreciate data was not nested previously and it may not be directly related to this PR but if you agree then might as well ...
| @@ -295,54 +290,29 @@ class ConsumerBase(Object): | |||
|
|
|||
| multi: a Boolean flag that indicates if the :class:`ConsumerBase` derived | |||
There was a problem hiding this comment.
I would have expected we could get rid of this multi argument entirely. I mean ops.relation always works in multi mode, i.e. always allowing the relation between providers and consumers to be many to many. If charms want to impose such limits, they can do so with limit in metadata.yaml which is now exposed to charms through a recent merged PR by Michele.
There was a problem hiding this comment.
Or maybe add deprecation warning if we know its fate is doomed.
|
Since you say this closes #586 I take it that the explicit choices are
|
| @@ -81,7 +86,8 @@ class ProviderBase(Object): | |||
| """ | |||
| _stored = StoredState() | |||
|
|
|||
| def __init__(self, charm, name, service, version=None): | |||
| def __init__(self, charm: CharmBase, name: str, | |||
There was a problem hiding this comment.
charm: CharmBase
Is this intentional ("restrict" to only CharmBase methods) or you actually intended to somehow use typing.Generic?
| """Set provider state to unready.""" | ||
| logger.debug("Provider is not ready") | ||
| self._stored.ready = False | ||
| self._notify_consumers() | ||
|
|
||
| def _provider_data(self): | ||
| def _provider_data(self) -> typing.Dict: |
There was a problem hiding this comment.
Without using generics, there is no difference between dict and typing.Dict. Also, in newer python versions dict gains additional typing capabilities.
https://www.python.org/dev/peps/pep-0585/
| raised in response to a relation joined event for each additional | ||
| unit of the same or any additional provider. | ||
| """ | ||
| pass | ||
|
|
||
|
|
||
| class ConsumerEvents(CharmEvents): |
There was a problem hiding this comment.
For consistency maybe also match with class ProviderEvents(CharmEvents)?
| @@ -295,54 +290,29 @@ class ConsumerBase(Object): | |||
|
|
|||
| multi: a Boolean flag that indicates if the :class:`ConsumerBase` derived | |||
There was a problem hiding this comment.
Or maybe add deprecation warning if we know its fate is doomed.
|
|
||
| def _meets_requirements(self, provides, consumes): | ||
| def _meets_requirements(self, provides: typing.Dict, consumes: typing.Dict) -> bool: |
There was a problem hiding this comment.
I'd replace bare typing.Dict with dict.
| return True | ||
| return False | ||
| @property | ||
| def is_ready(self) -> bool: |
There was a problem hiding this comment.
Isn't by convention methods with is_ prefix are never properties?
|
See #606 |
Add .ready() and .unready() to ConsumerBase.
multi=is left as an argument to__init__for now so we don't break libraries as soon as it's instantiated, but it does not actually do anything anymore. Instead, it can now multiplex in both directions.Add
.relationtoConsumerEventsby destructuring the relation and fetching it back once a snapshot is loaded, soConsumerEventshaveevent.relationlikeRelationiEventsdo.Simplify
ConsumerEventsso they all inherit from a single base class, considering that none of them had invidualized behavior.Remove
TooManyProvidersand add a test to ensure that multiple consumers can work with one provider.Add type hinting everywhere.
Closes #586