I’m not clear why that’s being done in
__init__ though. It would seem to me this is going to cause the event to be emitted every time this interface is initialized by the charm after the relation data has been setup. That means every status, config-changed, etc will be emitting this event again.
Since the data might be used (once available) on any possible hook depending on the needs of the charm using the API class, the most natural thing to do felt like inspecting the model during
__init__ and setting up attributes with the data and whether it was complete. After that, since the data was already read and setup there, it then also felt natural to go ahead and check if it had changed there as well. (Note that the event is only emitted if the data is newly complete or if it had changed since becoming complete, so it wouldn’t be emitted every time as you thought.)
Actually, the original implementation tried to only read the data during the relation joined / changed hook but that led to thinking it needed to be cached in stored state to be available at other times and then race conditions keeping it in sync, which is what originally led to the current approach.
That said, as you point out, the relation joined / changed events are really the only point at which the data could possibly have changed, so it would be reasonable to do a hybrid approach of inspecting the model during
__init__ (or even making the attributes into properties which inspect the model, to avoid asking for relation data when it might not be needed yet) and then only doing the changed check in the joined / changed handler and emitting the event there would work. My main argument against that approach, if you can really call it that, is that, and this mostly applies to K8s charms, you end up with a charm that consists of an
__init__ with a slew of
observes that all point to the same handler which then checks if preconditions are met and, if so, sets the pod spec. With that, a common source of error that I’ve already seen is not thinking of one or more hook events that you should include in the list and thus end up with the charm not doing the right thing at odd times. Just doing the checks and pod spec directly in
__init__ leads to easier to follow code and avoids that source of error. But, of course, it also doesn’t encourage you to think about and understand the underlying life-cycle events which are happening, so once you do move on to more complex use-cases, you might get blindsided by the additional learning curve, and you can end up doing unnecessary work if you’re not careful, as well.
It’s also true that the expectation that events emitted in a component not be dropped seems like it will be a common and understandable one, so if we don’t want to actually allow for that in the framework, we might want to at least document it well and possibly even have it emit some sort of warning or even a fatal error (maybe just having the testing harness consider it fatal but not the runtime would be better).
Anyway, I think that the hybrid approach is the best solution for this, and just doing the data collection in
__init__ is sufficient to avoid the timing issues that @dominik.f ran into in the first pass.