canonical / ops-scenario

State-transition testing SDK for Operator Framework Juju charms.
Apache License 2.0
10 stars 7 forks source link

feat: add support for Pebble custom notices #108

Closed tonyandrewmeyer closed 3 months ago

tonyandrewmeyer commented 5 months ago

Adds support for Pebble custom notices:

It seems like having the custom_notice_event property on the notice (and corresponding changes, like Event would have a notice attribute) would be nicer, but (pebble) Notices don't know which container they are in, but PebbleCustomNoticeEvent does have the container as an attribute. We can't have the PebbleNotice know which container it is in and also have the container be passed the list of notices because that's circular. A PebbleNotice could take the Container as an argument, and could 'register' itself on the container, but that doesn't seem like the Scenario way.

This also forces the sugar to be for the most recent notice (more precisely: the last one in the list) in the container, but that does seem reasonable - it seems like this would be the behaviour in Juju as well.

Another option would to not have the sugar (and Event could take a notice), but it does seem like it's both nicer and more Scenario if it is there. Happy to get feedback on this!

The required version of ops is bumped to 2.10 to get the Pebble custom notices support there (particularly in the mock Pebble client).

There are a few small adjustments to support a second type of pebble/workload event.

I had hoped to have PebbleNotice inherit from ops.pebble.Notice (and _DCBase) They are both frozen dataclasses, but in Scenario it's better if there are suitable default values for everything except the key, but the key isn't the first argument for a pebble Notice, so this generates errors if done simply. Nothing else in Scenario is currently doing this, so it seems ok, but let me know if you want me to explore that more.

tonyandrewmeyer commented 5 months ago

@benhoyt it would be great to get your feedback on this as well, since you're the Pebble King (tm) and also did the custom notices in Juju/ops/Harness.

tonyandrewmeyer commented 5 months ago

By the way, I have every expectation that this might take a few rounds of review since it's my first non-trivial contribution to Scenario :smile:.

benhoyt commented 5 months ago

Thanks @tonyandrewmeyer. Something came up so I have a busy day tomorrow, but I hope to be able to review this Thursday or Friday.

PietroPasotti commented 3 months ago

Shall we get this moving forward again?

tonyandrewmeyer commented 3 months ago

Shall we get this moving forward again?

Yes! Ben was keen on pausing until we had progressed the 7.0 discussions, but I think we are far enough along with those now.

If this aligns with the 7.0 style, then it would be:

notice1 = Notice(...)
notice2 = Notice(...)
container = Container("my-container", can_connect=True, notices={notice1, notice2})
state_in = State(containers={container})

state_out = ctx.run(ctx.on.my_container_pebble_custom_notice(notice=notice2), state_in)

# Presumably you would not ever want to get existing notices, since the charm can't change them, but
# could in theory call `notify()`, although it seems like the workload ought to be doing that, not the charm.
new_notice = state_out.get_container(container).get_notice(key=...)

But for the current main, then it would be:

notice1 = Notice(...)
notice2 = Notice(...)
container = Container("my-container", can_connect=True, notices=[notice1, notice2])
state_in = State(containers=[container])

state_out = ctx.run(container.get_notice(notice2.key), state_in)

new_notice = state_out.containers["my-container].get_notice(...)

I think it would be worth having this PR implement the latter (there are a couple of fixes I need to make, most of which I have locally but haven't pushed), and then a separate PR for the 7.0 branch (probably the same one that changes everything else).

tonyandrewmeyer commented 3 months ago

@PietroPasotti could you please give this a fresh review?