Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: use ops._main._Manager in Scenario #1467

Closed

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Nov 27, 2024

The ops._main._Manager class started out as the Ops class in Scenario, but went through significant modifications before it was added in ops. This PR merges those changes back into Scenario, so that it's also using the _Manager class. Other than generally simplifying the code, this also means that Scenario is even more consistent with using ops in production.

This includes some small refactoring in ops to make this cleaner:

  • ops._main._emit_charm_event is only used by _Manager, so logically belongs inside that class. This also makes it much simpler to have a subclass override the behaviour.
  • Allow passing a JujuContext to _Manager, so that it can be populated by something other than os.environ.
  • Allow _Manager subclasses to override how the charm metadata is loaded.
  • Allow _Manager subclasses to execute code after committing but before the framework is closed.
  • Support a dispatch path that uses _ rather than - in event names (Scenario consistently uses _, even though this would always be - from Juju - feel free to push back on this, requiring Scenario to undo the _ conversion when populating the context).

The Scenario runtime and ops_main_mock modules are renamed to properly indicate that everything inside of them is an implementation detail and should be considered private by Scenario users. This is a breaking change but we're considering it a bug fix and not one that requires a major release, given that the documentation and top-level namespace were already clear on this.

Significant Scenario refactoring:

  • UnitStateDB is moved from runtime to ops_main_mock, and it no longer (re) opens the SQLiteStorage - it instead takes a reference to the one that's in the framework, so that we're consistently using the same underlying storage, without relying on the filename being consistent (this eases moving to :memory: in the future).
  • As far as I can tell, runtime._OpsMainContext is completely unused and missed being removed in previous refactoring, so it's entirely deleted.
  • Loading and storing deferred events and stored state into the state database is moved from the runtime to the manager subclass.
  • The Ops class survives, but is now a subclass of the ops._main._Manager class. A reasonable amount of functionality is now removed (because the parent handles it), and the remaining code is specifically for handling the testing framework.

A few type hints are added in some tests where I was working on fixing the test and it helped to do that.

Fixes #1425

@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review November 27, 2024 05:48
@tonyandrewmeyer
Copy link
Contributor Author

This maybe broke the recent variadic type change, since ops._main._Manager now sets the .charm attribute and the change didn't extend that far. I'll look into that.

Copy link
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a bunch of changes have come in while I'm reviewing, I'll just leave these comments for now and finish the review later.

@@ -482,7 +466,7 @@ def _make_framework(self, dispatcher: _Dispatcher):
# If we are in a RelationBroken event, we want to know which relation is
# broken within the model, not only in the event's `.relation` attribute.

if self._juju_context.dispatch_path.endswith('-relation-broken'):
if self._juju_context.dispatch_path.endswith(('-relation-broken', '_relation_broken')):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If snake case is only being produced by scenario, it feels kind of odd having this one special case in ops._main.

Changing scenario to produce kebab-case instead of snake_case seems like it could be a pain though -- and would it be a breaking a change for users? So probably not worth doing that.

Would it be nice to have a standard way of handling snake vs kebab?

if kebabify(self._juju_context.dispatch_path).endswith('-relation-broken'):

I don't really love this either.

I don't think this topic should really be a blocker either way.

ops/_main.py Show resolved Hide resolved
ops/_main.py Show resolved Hide resolved
ops/_main.py Show resolved Hide resolved
testing/src/scenario/_consistency_checker.py Show resolved Hide resolved
testing/src/scenario/_runtime.py Show resolved Hide resolved
testing/src/scenario/_ops_main_mock.py Show resolved Hide resolved
@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Nov 27, 2024

Looks like a bunch of changes have come in while I'm reviewing, I'll just leave these comments for now and finish the review later.

The changes are only (meant to be!) merging in the charm type PR (well, rebasing to get that, but because I rename the file here it was a bit messy). No functional changes, although as noted above I wonder if I break that feature here and need to get the generic type over to main somehow.

@tonyandrewmeyer tonyandrewmeyer marked this pull request as draft November 27, 2024 23:24
tonyandrewmeyer added a commit that referenced this pull request Dec 2, 2024
This PR fixes the version of ops-scenario used when building the docs.

Previously, the published version of ops-scenario was used for
generating the documentation, and the generated requirements file was
used for local builds but not for read-the-docs.

That breaks if there are local incompatible changes (such as renaming a
module), and is generally wrong anyway.

With this PR, the local version is loaded, and read-the-docs uses the
same requirements file as building locally. A few minor type annotations
were adjusted to remove remaining warnings (I don't know why Sphinx
couldn't figure out where `Path` was coming from, but keeping it in the
`pathlib` namespace is generally nicer anyway).

Split out from #1467.
@tonyandrewmeyer tonyandrewmeyer deleted the use-ops-manager-take-2 branch December 4, 2024 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove testing.Ops, use ops.main.Manager instead
2 participants