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

fix(charm): make collect-status a LifeCycleEvent to avoid logging it #1080

Merged
merged 4 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions ops/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions ops/framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
PietroPasotti marked this conversation as resolved.
Show resolved Hide resolved
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)

Expand Down
21 changes: 15 additions & 6 deletions test/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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) + '.*')
Expand Down Expand Up @@ -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'),
Expand Down Expand Up @@ -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):
Expand All @@ -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):
Expand Down Expand Up @@ -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)

Expand Down