-
Notifications
You must be signed in to change notification settings - Fork 122
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
refactor!: use ops._main._Manager in Scenario #1467
Conversation
a01e286
to
8d662c4
Compare
This maybe broke the recent variadic type change, since |
There was a problem hiding this 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')): |
There was a problem hiding this comment.
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.
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. |
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.
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.JujuContext
to_Manager
, so that it can be populated by something other thanos.environ
._Manager
subclasses to override how the charm metadata is loaded._Manager
subclasses to execute code after committing but before the framework is closed._
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
andops_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:
runtime
toops_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).runtime._OpsMainContext
is completely unused and missed being removed in previous refactoring, so it's entirely deleted.Ops
class survives, but is now a subclass of theops._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