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

feat: add the action's ID to the ActionEvent object #1124

Merged
merged 7 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# 2.11.0

* Added `ActionEvent.id`, exposing the JUJU_ACTION_UUID environment variable.

# 2.10.0

* Added support for Pebble Notices (`PebbleCustomNoticeEvent`, `get_notices`, and so on)
Expand Down
15 changes: 15 additions & 0 deletions ops/charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,16 @@ class ActionEvent(EventBase):
:meth:`log`.
"""

id: str = ""
"""The Juju ID of the action invocation."""

params: Dict[str, Any]
"""The parameters passed to the action."""

def __init__(self, handle: 'Handle', id: Optional[str] = None):
super().__init__(handle)
self.id = id # type: ignore (for backwards compatibility)

def defer(self) -> NoReturn:
"""Action events are not deferrable like other events.

Expand All @@ -144,10 +151,18 @@ def restore(self, snapshot: Dict[str, Any]):

Not meant to be called directly by charm code.
"""
self.id = cast(str, snapshot['id'])
# Params are loaded at restore rather than __init__ because
# the model is not available in __init__.
self.params = self.framework.model._backend.action_get()

def snapshot(self) -> Dict[str, Any]:
"""Used by the framework to serialize the event to disk.

Not meant to be called by charm code.
"""
return {'id': self.id}

def set_results(self, results: Dict[str, Any]):
"""Report the result of the action.

Expand Down
4 changes: 2 additions & 2 deletions ops/framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -1057,10 +1057,10 @@ class BoundStoredState:
if TYPE_CHECKING:
# to help the type checker and IDEs:
@property
def _data(self) -> StoredStateData: ... # noqa, type: ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This, and the next change, are drive-bys. I weirdly get ruff complaining about these but only some of the time that it's run (I haven't dug into why that is). They do not seem to be necessary, so simplest to just remove.

def _data(self) -> StoredStateData: ... # type: ignore

@property
def _attr_name(self) -> str: ... # noqa, type: ignore
def _attr_name(self) -> str: ... # type: ignore

def __init__(self, parent: Object, attr_name: str):
parent.framework.register_type(StoredStateData, parent)
Expand Down
3 changes: 3 additions & 0 deletions ops/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ def _get_event_args(charm: 'ops.charm.CharmBase',
storage = cast(Union[ops.storage.JujuStorage, ops.storage.SQLiteStorage], storage)
storage.location = storage_location # type: ignore
return [storage], {}
elif issubclass(event_type, ops.charm.ActionEvent):
args: List[Any] = [os.environ['JUJU_ACTION_UUID']]
return args, {}
elif issubclass(event_type, ops.charm.RelationEvent):
relation_name = os.environ['JUJU_RELATION']
relation_id = _get_juju_relation_id()
Expand Down
4 changes: 3 additions & 1 deletion ops/testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ def __init__(
self._unit_name: str = f"{self._meta.name}/0"
self._hooks_enabled: bool = True
self._relation_id_counter: int = 0
self._action_id_counter: int = 0
config_ = self._get_config(config)
self._backend = _TestingModelBackend(self._unit_name, self._meta, config_)
self._model = model.Model(self._meta, self._backend)
Expand Down Expand Up @@ -1883,7 +1884,8 @@ def run_action(self, action_name: str,
action_under_test = _RunningAction(action_name, ActionOutput([], {}), params)
handler = getattr(self.charm.on, f"{action_name.replace('-', '_')}_action")
self._backend._running_action = action_under_test
handler.emit()
self._action_id_counter += 1
handler.emit(str(self._action_id_counter))
self._backend._running_action = None
if action_under_test.failure_message is not None:
raise ActionFailed(
Expand Down
11 changes: 6 additions & 5 deletions test/test_charm.py
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ def __init__(self, *args: typing.Any):
def _on_foo_bar_action(self, event: ops.ActionEvent):
self.seen_action_params = event.params
event.log('test-log')
event.set_results({'res': 'val with spaces'})
event.set_results({'res': 'val with spaces', 'id': event.id})
event.fail('test-fail')

def _on_start_action(self, event: ops.ActionEvent):
Expand All @@ -477,12 +477,13 @@ def _on_start_action(self, event: ops.ActionEvent):
self.assertIn('foo_bar_action', events)
self.assertIn('start_action', events)

charm.on.foo_bar_action.emit()
action_id = "1234"
charm.on.foo_bar_action.emit(id=action_id)
self.assertEqual(charm.seen_action_params, {"foo-name": "name", "silent": True})
self.assertEqual(fake_script_calls(self), [
['action-get', '--format=json'],
['action-log', "test-log"],
['action-set', "res=val with spaces"],
['action-set', "res=val with spaces", f"id={action_id}"],
['action-fail', "test-fail"],
])

Expand Down Expand Up @@ -511,7 +512,7 @@ def _on_foo_bar_action(self, event: ops.ActionEvent):
charm.res = bad_res

with self.assertRaises(ValueError):
charm.on.foo_bar_action.emit()
charm.on.foo_bar_action.emit(id='1')

def _test_action_event_defer_fails(self, cmd_type: str):

Expand All @@ -532,7 +533,7 @@ def _on_start_action(self, event: ops.ActionEvent):
charm = MyCharm(framework)

with self.assertRaises(RuntimeError):
charm.on.start_action.emit()
charm.on.start_action.emit(id='2')

def test_action_event_defer_fails(self):
self._test_action_event_defer_fails('action')
Expand Down
4 changes: 2 additions & 2 deletions test/test_framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -1813,7 +1813,7 @@ class CustomEvents(ops.ObjectEvents):

with patch('sys.stderr', new_callable=io.StringIO):
with patch('pdb.runcall') as mock:
publisher.foobar_action.emit()
publisher.foobar_action.emit(id='1')

self.assertEqual(mock.call_count, 1)
self.assertFalse(observer.called)
Expand All @@ -1833,7 +1833,7 @@ class CustomEvents(ops.ObjectEvents):

with patch('sys.stderr', new_callable=io.StringIO):
with patch('pdb.runcall') as mock:
publisher.foobar_action.emit()
publisher.foobar_action.emit(id='2')

self.assertEqual(mock.call_count, 1)
self.assertFalse(observer.called)
Expand Down
27 changes: 17 additions & 10 deletions test/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -586,11 +586,13 @@ def test_multiple_events_handled(self):
'departing_unit': 'remote/42'},
), (
EventSpec(ops.ActionEvent, 'start_action',
env_var='JUJU_ACTION_NAME'),
env_var='JUJU_ACTION_NAME',
set_in_env={'JUJU_ACTION_UUID': '1'}),
{},
), (
EventSpec(ops.ActionEvent, 'foo_bar_action',
env_var='JUJU_ACTION_NAME'),
env_var='JUJU_ACTION_NAME',
set_in_env={'JUJU_ACTION_UUID': '2'}),
{},
), (
EventSpec(ops.PebbleReadyEvent, 'test_pebble_ready',
Expand Down Expand Up @@ -726,19 +728,20 @@ def test_logger(self):
fake_script(typing.cast(unittest.TestCase, self), 'action-get', "echo '{}'")

test_cases = [(
EventSpec(ops.ActionEvent, 'log_critical_action', env_var='JUJU_ACTION_NAME'),
EventSpec(ops.ActionEvent, 'log_critical_action', env_var='JUJU_ACTION_NAME',
set_in_env={'JUJU_ACTION_UUID': '1'}),
['juju-log', '--log-level', 'CRITICAL', '--', 'super critical'],
), (
EventSpec(ops.ActionEvent, 'log_error_action',
env_var='JUJU_ACTION_NAME'),
env_var='JUJU_ACTION_NAME', set_in_env={'JUJU_ACTION_UUID': '2'}),
['juju-log', '--log-level', 'ERROR', '--', 'grave error'],
), (
EventSpec(ops.ActionEvent, 'log_warning_action',
env_var='JUJU_ACTION_NAME'),
env_var='JUJU_ACTION_NAME', set_in_env={'JUJU_ACTION_UUID': '3'}),
['juju-log', '--log-level', 'WARNING', '--', 'wise warning'],
), (
EventSpec(ops.ActionEvent, 'log_info_action',
env_var='JUJU_ACTION_NAME'),
env_var='JUJU_ACTION_NAME', set_in_env={'JUJU_ACTION_UUID': '4'}),
['juju-log', '--log-level', 'INFO', '--', 'useful info'],
)]

Expand Down Expand Up @@ -779,7 +782,8 @@ def test_sets_model_name(self):
state = self._simulate_event(EventSpec(
ops.ActionEvent, 'get_model_name_action',
env_var='JUJU_ACTION_NAME',
model_name='test-model-name'))
model_name='test-model-name',
set_in_env={'JUJU_ACTION_UUID': '1'}))
assert isinstance(state, ops.BoundStoredState)
self.assertEqual(state._on_get_model_name_action, ['test-model-name'])

Expand All @@ -791,7 +795,8 @@ def test_has_valid_status(self):
"""echo '{"status": "unknown", "message": ""}'""")
state = self._simulate_event(EventSpec(
ops.ActionEvent, 'get_status_action',
env_var='JUJU_ACTION_NAME'))
env_var='JUJU_ACTION_NAME',
set_in_env={'JUJU_ACTION_UUID': '1'}))
assert isinstance(state, ops.BoundStoredState)
self.assertEqual(state.status_name, 'unknown')
self.assertEqual(state.status_message, '')
Expand All @@ -801,7 +806,8 @@ def test_has_valid_status(self):
"""echo '{"status": "blocked", "message": "help meeee"}'""")
state = self._simulate_event(EventSpec(
ops.ActionEvent, 'get_status_action',
env_var='JUJU_ACTION_NAME'))
env_var='JUJU_ACTION_NAME',
set_in_env={'JUJU_ACTION_UUID': '1'}))
assert isinstance(state, ops.BoundStoredState)
self.assertEqual(state.status_name, 'blocked')
self.assertEqual(state.status_message, 'help meeee')
Expand Down Expand Up @@ -1169,7 +1175,8 @@ def test_crash_action(self):
with self.assertRaises(subprocess.CalledProcessError):
self._simulate_event(EventSpec(
ops.ActionEvent, 'keyerror_action',
env_var='JUJU_ACTION_NAME'))
env_var='JUJU_ACTION_NAME',
set_in_env={'JUJU_ACTION_UUID': '1'}))
self.stderr.seek(0)
stderr = self.stderr.read()
self.assertIn('KeyError', stderr)
Expand Down
1 change: 1 addition & 0 deletions test/test_testing.py
Original file line number Diff line number Diff line change
Expand Up @@ -5445,6 +5445,7 @@ def __init__(self, framework: ops.Framework):
def _on_simple_action(self, event: ops.ActionEvent):
"""An action that doesn't generate logs, have any results, or fail."""
self.simple_was_called = True
assert isinstance(event.id, str)

def _on_fail_action(self, event: ops.ActionEvent):
event.fail("this will be ignored")
Expand Down