From cf26f00143bfc1e1d977c9ac89becd803f5ffed2 Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Fri, 24 Nov 2023 08:19:36 +0100 Subject: [PATCH 1/3] collect-status is now a lifecycle event --- ops/charm.py | 11 +++++++++-- ops/framework.py | 4 ++-- test/test_main.py | 21 +++++++++++++++------ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index cb599863b..58ccc2a4f 100755 --- a/ops/charm.py +++ b/ops/charm.py @@ -34,7 +34,14 @@ from ops import model from ops._private import yaml -from ops.framework import EventBase, EventSource, Framework, Object, ObjectEvents +from ops.framework import ( + EventBase, + EventSource, + Framework, + LifecycleEvent, + Object, + ObjectEvents, +) if TYPE_CHECKING: from typing_extensions import Required, TypedDict @@ -830,7 +837,7 @@ def defer(self) -> None: 'this event until you create a new revision.') -class CollectStatusEvent(EventBase): +class CollectStatusEvent(LifecycleEvent): """Event triggered at the end of every hook to collect statuses for evaluation. If the charm wants to provide application or unit status in a consistent diff --git a/ops/framework.py b/ops/framework.py index df9aa1bb2..39fec20a8 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -906,11 +906,11 @@ def _reemit(self, single_event_path: Optional[str] = None): if single_event_path is None: logger.debug("Re-emitting deferred event %s.", event) elif isinstance(event, LifecycleEvent): - # Ignore Lifecycle events: they are "private" and not interesting. - pass + logger.debug("Emitting lifecycle event %s.", event.handle.kind) elif self._event_name and self._event_name != event.handle.kind: # if the event we are emitting now is not the event being # dispatched, and it also is not an event we have deferred, + # and is also not a lifecycle (framework-emitted) event, # it must be a custom event logger.debug("Emitting custom event %s.", event) diff --git a/test/test_main.py b/test/test_main.py index 25c0ba286..a26d70fd6 100755 --- a/test/test_main.py +++ b/test/test_main.py @@ -625,6 +625,7 @@ def test_collect_metrics(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event collect_metrics.'], ['add-metric', '--labels', 'bar=4.2', 'foo=42'], ['is-leader', '--format=json'], + ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] calls = fake_script_calls(self) @@ -645,6 +646,7 @@ def test_custom_event(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event update_status.'], ['juju-log', '--log-level', 'DEBUG', '--', custom_event_prefix], ['is-leader', '--format=json'], + ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] # Remove the "[key]>" suffix from the end of the event string self.assertRegex(calls[2][-1], re.escape(custom_event_prefix) + '.*') @@ -791,9 +793,13 @@ def _call_event(self, rel_path, env): def test_setup_event_links(self): """Test auto-creation of symlinks caused by initial events.""" - all_event_hooks = [f"hooks/{name.replace('_', '-')}" - for name, event_source in self.charm_module.Charm.on.events().items() - if issubclass(event_source.event_type, ops.LifecycleEvent)] + # Fixme: this is always an empty list, is this test outdated? + all_event_hooks = [ + f"hooks/{name.replace('_', '-')}" for name, + event_source in self.charm_module.Charm.on.events().items() if issubclass( + event_source.event_type, + (ops.CommitEvent, + ops.PreCommitEvent))] initial_events = { EventSpec(ops.InstallEvent, 'install'), @@ -909,9 +915,10 @@ def test_hook_and_dispatch(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], ['is-leader', '--format=json'], + ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] calls = fake_script_calls(self) - self.assertRegex(' '.join(calls.pop(-3)), 'Initializing SQLite local storage: ') + self.assertRegex(' '.join(calls.pop(-4)), 'Initializing SQLite local storage: ') self.assertEqual(calls, expected) def test_non_executable_hook_and_dispatch(self): @@ -929,9 +936,10 @@ def test_non_executable_hook_and_dispatch(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], ['is-leader', '--format=json'], + ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] calls = fake_script_calls(self) - self.assertRegex(' '.join(calls.pop(-3)), 'Initializing SQLite local storage: ') + self.assertRegex(' '.join(calls.pop(-4)), 'Initializing SQLite local storage: ') self.assertEqual(calls, expected) def test_hook_and_dispatch_with_failing_hook(self): @@ -1012,9 +1020,10 @@ def test_hook_and_dispatch_but_hook_is_dispatch_copy(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], ['is-leader', '--format=json'], + ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] calls = fake_script_calls(self) - self.assertRegex(' '.join(calls.pop(-3)), 'Initializing SQLite local storage: ') + self.assertRegex(' '.join(calls.pop(-4)), 'Initializing SQLite local storage: ') self.assertEqual(calls, expected) From 2b17ddecdf447fb4f264d1dba4ee17b0638191ca Mon Sep 17 00:00:00 2001 From: Pietro Pasotti Date: Tue, 30 Jan 2024 10:29:00 +0100 Subject: [PATCH 2/3] pr comments --- ops/framework.py | 3 ++- test/test_main.py | 11 +++-------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/ops/framework.py b/ops/framework.py index 39fec20a8..a5e55517c 100755 --- a/ops/framework.py +++ b/ops/framework.py @@ -906,7 +906,8 @@ def _reemit(self, single_event_path: Optional[str] = None): if single_event_path is None: logger.debug("Re-emitting deferred event %s.", event) elif isinstance(event, LifecycleEvent): - logger.debug("Emitting lifecycle event %s.", event.handle.kind) + # Ignore Lifecycle events: they are "private" and not interesting. + pass elif self._event_name and self._event_name != event.handle.kind: # if the event we are emitting now is not the event being # dispatched, and it also is not an event we have deferred, diff --git a/test/test_main.py b/test/test_main.py index a26d70fd6..6a745c86c 100755 --- a/test/test_main.py +++ b/test/test_main.py @@ -625,7 +625,6 @@ def test_collect_metrics(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event collect_metrics.'], ['add-metric', '--labels', 'bar=4.2', 'foo=42'], ['is-leader', '--format=json'], - ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] calls = fake_script_calls(self) @@ -646,7 +645,6 @@ def test_custom_event(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event update_status.'], ['juju-log', '--log-level', 'DEBUG', '--', custom_event_prefix], ['is-leader', '--format=json'], - ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] # Remove the "[key]>" suffix from the end of the event string self.assertRegex(calls[2][-1], re.escape(custom_event_prefix) + '.*') @@ -915,10 +913,9 @@ def test_hook_and_dispatch(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], ['is-leader', '--format=json'], - ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] calls = fake_script_calls(self) - self.assertRegex(' '.join(calls.pop(-4)), 'Initializing SQLite local storage: ') + self.assertRegex(' '.join(calls.pop(-3)), 'Initializing SQLite local storage: ') self.assertEqual(calls, expected) def test_non_executable_hook_and_dispatch(self): @@ -936,10 +933,9 @@ def test_non_executable_hook_and_dispatch(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], ['is-leader', '--format=json'], - ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] calls = fake_script_calls(self) - self.assertRegex(' '.join(calls.pop(-4)), 'Initializing SQLite local storage: ') + self.assertRegex(' '.join(calls.pop(-3)), 'Initializing SQLite local storage: ') self.assertEqual(calls, expected) def test_hook_and_dispatch_with_failing_hook(self): @@ -1020,10 +1016,9 @@ def test_hook_and_dispatch_but_hook_is_dispatch_copy(self): ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting Juju event install.'], ['is-leader', '--format=json'], - ['juju-log', '--log-level', 'DEBUG', '--', 'Emitting lifecycle event commit.'] ] calls = fake_script_calls(self) - self.assertRegex(' '.join(calls.pop(-4)), 'Initializing SQLite local storage: ') + self.assertRegex(' '.join(calls.pop(-3)), 'Initializing SQLite local storage: ') self.assertEqual(calls, expected) From 245cbb2fbdf90141ef711a9c33bc982dcdc8ba08 Mon Sep 17 00:00:00 2001 From: Ben Hoyt Date: Wed, 31 Jan 2024 10:29:03 +1300 Subject: [PATCH 3/3] Improve formatting, remove fixme comment --- test/test_main.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/test/test_main.py b/test/test_main.py index 6a745c86c..b790474d7 100755 --- a/test/test_main.py +++ b/test/test_main.py @@ -791,13 +791,11 @@ def _call_event(self, rel_path, env): def test_setup_event_links(self): """Test auto-creation of symlinks caused by initial events.""" - # Fixme: this is always an empty list, is this test outdated? all_event_hooks = [ - f"hooks/{name.replace('_', '-')}" for name, - event_source in self.charm_module.Charm.on.events().items() if issubclass( - event_source.event_type, - (ops.CommitEvent, - ops.PreCommitEvent))] + f"hooks/{name.replace('_', '-')}" + for name, event_source in self.charm_module.Charm.on.events().items() + if issubclass(event_source.event_type, (ops.CommitEvent, ops.PreCommitEvent)) + ] initial_events = { EventSpec(ops.InstallEvent, 'install'),