canonical / data-platform-libs

A collection of charm libraries curated by the Data Platform Team
https://charmhub.io/data-platform-libs
Apache License 2.0
10 stars 9 forks source link

[ISSUE-42;ISSUE-43] is_resource_created improvements and fixes #44

Closed deusebio closed 1 year ago

deusebio commented 1 year ago

Addressing #42 and #43

beliaev-maksim commented 1 year ago

@deusebio @welpaolo @delgod so, you removed in this PR check for relation broken. But then it is not really clear what user did wrong. eg without reading docstring of the function you will never know

could this be improved?

This will avoid the raise of the runtime exception if the relation-broken event is not related to the database.

was not check checking the name of the database relation to ensure it is only related to database relations?

deusebio commented 1 year ago

I'm not sure I fully understand what you mean. IIRC, here the issue is that there may exist multiple relations under the same relation name, and fetch_relation_data fetches the databags for all of them, indexed by the relation id.

Now, previously in the relation_broken_event, we were raising an exception in any relation broken event, which it is a bit too general/broad, as people may still want to access the data for the relations other than the one tearing down. Indeed, this was a use case that some people stumbled upon (OSM team if I recall correctly). I remembered that there was also a somewhat inconsistent behaviour for accessing relation data in integration tests vs unittests (see MM thread here), but this might be a different story.

So, here we basically just fetch the data that can be fetched. If the databag of the relation broken is not available, the dictionary would just NOT have the corresponding relation id key rather than raising an exception. I don't think the users would do anything wrong to use fetch_relation_data in relation_broken_event. Just there may not be some data there. Also, If they try to self.fetch_relation_data()[relation_id] for relations that had been removed, they will get an exception anyway.

beliaev-maksim commented 1 year ago

@deusebio knowing the context it makes sense :) however, during execution it might be confusing for users :)