search

charmed-lma / charm-k8s-grafana

Kubernetes Operator for Grafana
Apache License 2.0
5 stars 3 forks source link

Reconsider the "FrameworkAdapter" infrastructure #11

Open facundobatista opened 4 years ago

facundobatista commented 4 years ago

It's a layer above many things. A quite lightweight one, though, so there's not a lot of work hidden in that layer, but as it's a layer above several other things, to where each property belongs is not easily discoverable.

Most of the FrameworkAdapter attributes are from the Model, so they can always be accessed doing self.framework.model in the charm. But note that the charm already has shortcuts like app or unit (check the docs here).

So, for example you're currently doing...

juju_app = fw_adapter.get_app_name()

...and it's shorter and more accurate to do...

juju_app = self.framework.app.name

It's even absurd to have the separation layer when doing exactly the same thing, forcing you to write...

            self.fw_adapter.observe(event, delegator)

...when it's just doing exactly the same that you would be doing by...

            self.framework.observe(event, delegator)

This in general lowers a lot the learnability of this code. People getting here needs to learn this new abstraction layer instead of just using the Framework, as they are used to do.

An example of that is the developer needing to learn/understand what you're doing in...

    if not fw_adapter.am_i_leader():

...when it's simple (as long you know the Framework, which is a knowledge it's expected to have) to understand what the code does in...

    if not self.unit.is_leader():

It's fine IMO to have some layers/adapters as you have (for building the build_juju_pod_spec, for example).

But in the case of the Framework, which is by design tightly coupled with the Charm (and the Model), I would totally recommend to not have this adapter.

relaxdiego commented 4 years ago

Thanks for the feedback @facundobatista! I have tried to remove FrameworkAdapter from some parts in a topic branch as you suggested. What's missing is my ability to assert if the SUT did exactly as designed. Please see: https://github.com/charmed-lma/charm-k8s-grafana/commit/88006babe566f3dd04d24dc183d3af40f242e82f

More specifically:

https://github.com/charmed-lma/charm-k8s-grafana/blob/88006babe566f3dd04d24dc183d3af40f242e82f/test/charm_test.py#L43-L53

relaxdiego commented 4 years ago

I should also reference the comment that I added on an issue in the Operator Framework which is closely related to the question that I have above: https://github.com/canonical/operator/issues/282#issuecomment-633734214

relaxdiego commented 4 years ago

To be absolutely clear, I'm not using the above as a means to justify the existence of the FrameworkAdapter. Honestly, I would love to not have to maintain that if I can avoid it. But to let go of it, I must have a way to interrogate some other object regarding the side effects that I expect my SUT to cause at certain events.

facundobatista commented 4 years ago

Maybe @jameinel could help us here?

jameinel commented 4 years ago

So if you have to have the individual calls, you could always patch harness.charm.framework to be a mock which would intercept and record the individual calls. I think it makes more sense to test it as "if the pod status is this, show that it sets the unit status to that" and test those as individual steps instead of as a sequence of events.