feat!: unify run() and run_action() and simplify context managers

[tonyandrewmeyer]

The run_action() method (both standalone and in the context manager) are removed. This means that all events, including action events, are emitted with the same run() call, and both return the output state.

To get access to the results of actions, new action_results and action_logs attributes are added to the Context. For example:

# Scenario 6
out = ctx.run_action(Action("backup", params={"output": "data.tar.gz"}), State())
assert out.results == {"size": 1234}
assert out.logs = [..., ...]
assert out.state...

# Scenario 7
state = ctx.run(ctx.on.action("backup", params={"output": "data.tar.gz"}), State())
assert ctx.action_results == {"size": 1234}
assert ctx.action_logs = [..., ...]
assert state...

When the charm calls event.fail(), this raises an exception, in the same way that Harness does. For example:

# Scenario 6
out = ctx.run_action("bad-action", State())
assert out.failure == "couldn't do the thing"

# Scenario 7
with pytest.raises(ActionFailed) as exc_info:
    ctx.run(ctx.on.action("bad-action"), State())
assert exc_info.value.message == "couldn't do the thing"

The advantage of this is that tests that the action is successful do not need to have assertions of such, which is easy to miss.

In addition, the Context.manager and Context.action_manager methods are replaced by the ability to use the Context object itself as a context manager.

For example:

ctx = Context(MyCharm)
with ctx(ctx.on.start(), State()) as event:
    event.charm.prepare()
    state = event.run()
    assert event.charm...

The same code is also used (with ctx.on.action()) for action events.

Advantages:

• Slightly shorter code (no ".manager" or ".action_manager")
• Avoids naming complications with "manager", "action_manager" and the various alternatives proposed in #115.

The .output property of the context manager is also removed. The context manager will still handle running the event if it's not done explicitly, but if that's the case then the output is not available. We want to encourage people to explicitly run the event, not rely on the automated behaviour - although I think it does make sense that it does run, rather than raise or end in a weird situation where the event never ran.

This replaces #115 and #118, being a combination of ideas/discussion from both, plus incorporating the unification of run/run_action discussed here, and the "action fail raises" discussed elsewhere.

Also, as drive-by changes while names are being made consistent:

• _Port becomes Port
• _RelationBase becomes (again) RelationBase

[dimaqq]

To summarise, with this change the charmer has two options:

out = ctx.run(event, state)

or

with ctx(event, state) as event: out = event.run()

Did I get that right?

Having ctx as callable in one place and an object in another is tad confusing, but I can't think of better syntax...

P.S. @tonyandrewmeyer pls update the PR description, I guess it's .run() now and not .run_action(), is it?

P.P.S. perhaps we could clarify this in the docs like this:

with ctx(event, state) as event: # bound event?

or

with ctx(event_source? event_generator?, state) as event:?

[dimaqq]

Off-topic I really wish there was a better name than "Context" for ctx. Frankly, I'm still unclear what sort of context it is, what gets contextualised by it... Given that it stands for "charm execution context", I kinda with it was called "Charm". The code could look like:

charm = scenario.Charm(MyRealCharm, meta=...)
state_out = charm.run(charm.on.startup(), state_in)

That would be easier to understand, wouldn't it?

[PietroPasotti]

Off-topic I really wish there was a better name than "context" for ctx. Frankly, I'm still unclear what sort of context it is, what gets contextualised by it...

In my head it's the charm's execution context. The filesystem it is situated in, the envvars the python process runs with (encapsulated further in the Event), the juju version...

[tonyandrewmeyer]

P.S. @tonyandrewmeyer pls update the PR description, I guess it's .run() now and not .run_action(), is it?

I'm reworking this PR to rename run_action as suggested and as discussed by charm-tech. I'll update the description to match when the code is changed.

[dimaqq]

scenario.CharmContext then? 🤔

[tonyandrewmeyer]

To summarise, with this change the charmer has two options:

out = ctx.run(event, state)

or

with ctx(event, state) as event: out = event.run()

Did I get that right?

Yes. This was the case before as well, it's just the names that have changed (and that you can now use the same API for action events as other events). state_out = ctx.run(event, state_in) is unchanged, and with ctx.manager(event, state_in) as mgr: state_out = mgr.run() is now the simpler with ctx(event, state_in) as event: state_out = event.run().

Note that the "event" that's the first argument would hardly ever be an object named "event", it would almost always be ctx.on.something(possible_args), like ctx.on.start() or ctx.on.relation(joined_relation=rel). This is a Event object, but it's roughly the same as an ops.EventSource.

The "event" that is the manager that you get back can obviously be named anything the charmer likes. If people like to stick with "mgr" (or "manager") that would be ok. It only has one attribute (.charm) and one method (.run()). I think of it as the event (and therefore the charm that's instantiated to handle the event, and .run() "runs" (emits) the event) so I like "event" (or "evt"). The ideal would be that it gave you the charm and so you'd call it "charm", but you need to be able to do the run call. See the discussion above and in the linked PRs.

(One possibility I only thought of while doing the latest changes is:

with ctx(ctx.on.start(), State()) as charm:
    charm.prepare_function()
    ctx.run(charm)
    assert charm...

But I still prefer what we have to that).

Having ctx as callable in one place and an object in another is tad confusing, but I can't think of better syntax...

Having a method instead (like we have now in Scenario 6) and either leaving it named "manager" or renaming it was the original option, but I do prefer this version (again there's lots of discussion in the links PRs). If feedback on the beta is really negative, it wouldn't be hard to change it back.

P.P.S. perhaps we could clarify this in the docs like this:

with ctx(event, state) as event: # bound event?

bound_event is a reasonable name, but it feels long.

or

with ctx(event_source? event_generator?, state) as event:?

If someone is going to put a name on the first argument, instead of doing an inline ctx.on, then I think we leave it to them to label it (I don't think this needs to be anywhere in the examples in the docs). event_source is probably reasonable.

[tonyandrewmeyer]

Off-topic

Let's move that discussion to the ops development channel or a separate issue - these PRs tend to get pretty long as-is, even when staying on topic.

[tonyandrewmeyer]

Ok @benhoyt, @PietroPasotti , @dimaqq this is ready for another round, with the much enlarged scope (I've updated the description to cover it all). I'm pretty happy with the current state, so looking forward to hearing what you all think!