canonical / data-integrator

Juju charm to provide DB access for apps outside of Juju.
https://charmhub.io/data-integrator
Apache License 2.0
3 stars 4 forks source link

[DPE-1928] Update lib #32

Closed zmraul closed 1 year ago

zmraul commented 1 year ago

Update with the changes from data_platform_libs on KafkaRequires and add an integration test on wildcard usage.

Only the test needs review.

deusebio commented 1 year ago

Uhm, could we handle the error and rather put the charm into blocked state (with a sensible message) should the validation for the topic name not pass?

zmraul commented 1 year ago

Uhm, could we handle the error and rather put the charm into blocked state (with a sensible message) should the validation for the topic name not pass?

We can do that, but I was letting it fail on purpose as a way of also stopping all events from triggering.

deusebio commented 1 year ago

But the relation would be created anyway in the juju model, right? I mean, you would only stop anything from happening at the data-integrator side I suppose. We could always put the charm into a blocked status and then allow the user to change the configuration (topic_name) and if the user then provides sensible values (with juju config and handling config_changed events), then everything solves by itself, since the relations are already in place

zmraul commented 1 year ago

But the relation would be created anyway in the juju model, right? I mean, you would only stop anything from happening at the data-integrator side I suppose.

Not exactly, KafkaRequires is invoked in the init method. Since we don't update relation databag, the config information is not being used on kafka side either. But I see that this is creating some unexpected outcomes, as I don't think setting status on init is a good option.

deusebio commented 1 year ago

Changes look all right, but let's trigger the discussion of handling validation inside and outside DataPlatform to understand the long-term solution. But fine to be merged for the time being.

There are a number of failing integration tests although, that needs to be fixed before merging.