Thought: make Context read metadata from __scenario_metadata__

[PietroPasotti]

In many, many tests (e.g. https://github.com/canonical/tempo-k8s-operator/blob/main/tests/scenario/test_charm_tracing.py#L43), I do something like this:

class MyCharm(CharmBase):
    META = {"name": "pietro"}
    ...
    
def test_foo():
    ctx = scenario.Context(MyCharm, meta=MyCharm.META)
    ...

As suggested by Leon in canonical/cos-lib#44 (comment) , it could be nice to teach this behaviour to scenario so that you don't have to explicitly pass the metadata to Context, but Context will first try to read it from charm_type.__scenario_metadata__ (or something similar).

This would only be useful for tests where the charm instance is not 'The charm instance', but a throwaway one only intended to test, typically, a charm-lib-provided object attached to it.

[tonyandrewmeyer]

I like this idea - having the metadata closer to the charm is better than close to the Context object. Maybe we solve #156 with this as well, rather than making changes there?

I think the name doesn't need to (and shouldn't) have "scenario" in it. It's not Scenario metadata, it's the charm metadata that Scenario happens to use. Other tools in the future could choose to make use of it as well.

Given that charms are meant to think in terms of the unified charmcraft.yaml rather than the individual metadata, actions, config yaml, I assume the new __metadata__ attribute would handle a combined object too, so you could load the yaml from a file and assign the resulting dict to the attribute without needing to split it into three, and without needing to have three attributes.

Would we keep the ability to pass the metadata to the Context, or deprecate that? I like having a single way to do this, and it seems like the main advantage of being able to do it via Context args would be that you can easily use the same charm class with different meta, which doesn't seem particularly useful.

[PietroPasotti]

I do that

I like this idea - having the metadata closer to the charm is better than close to the Context object. Maybe we solve #156 with this as well, rather than making changes there?

I think the name doesn't need to (and shouldn't) have "scenario" in it. It's not Scenario metadata, it's the charm metadata that Scenario happens to use. Other tools in the future could choose to make use of it as well.

+1, although we have to be wary of the risk that technically all dunders are sort of reserved and might at some point in the future be broken without warning by python or other tooling, so, the more weird we make it, the less the chance.

Given that charms are meant to think in terms of the unified charmcraft.yaml rather than the individual metadata, actions, config yaml, I assume the new __metadata__ attribute would handle a combined object too, so you could load the yaml from a file and assign the resulting dict to the attribute without needing to split it into three, and without needing to have three attributes.

+1

Would we keep the ability to pass the metadata to the Context, or deprecate that? I like having a single way to do this, and it seems like the main advantage of being able to do it via Context args would be that you can easily use the same charm class with different meta, which doesn't seem particularly useful.

I do that a lot in tests for 'meta' objects such as charm libs or objects intended to be subclassed by charms. See for example https://github.com/canonical/cos-lib/blob/fix-worker-py8/tests/test_coordinated_workers/test_worker.py

Also another use case for it is repositories that for whatever reason don't have a standard structure (multiple charms, for example, or metadata that gets generated on the fly).
If we deprecate passing metadata as an arg to Context, then if you want to test such a charm you'll be forced to actually go and change the charm code to add __metadata__, which feels bad.

This is the main reason why I was wary of this approach when it came to mind first some time ago

[tonyandrewmeyer]

+1, although we have to be wary of the risk that technically all dunders are sort of reserved and might at some point in the future be broken without warning by python or other tooling, so, the more weird we make it, the less the chance.

Fair point, and I guess "metadata" is the sort of name that a Python feature might want to use. Maybe __charm_meta__?

If we deprecate passing metadata as an arg to Context, then if you want to test such a charm you'll be forced to actually go and change the charm code to add __metadata__, which feels bad.

This is the main reason why I was wary of this approach when it came to mind first some time ago

Hmm, I think it's ok to add new ways to specify the metadata, but ideally there would be clear, non-overlapping, reasons to use each one, and hopefully also some obviousness about what metadata 'wins' when there's more than one. I'm not sure I yet understand when someone would choose __metadata__ vs meta=.

[PietroPasotti]

yeah, and there's also an argument for "Explicit is better than implicit"...

Overall I'm not convinced this is a good change, but let's wait for @sed-i and see if he has stronger arguments :)

[PietroPasotti]

closing this for now, we could reconsider in the future.