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: ops eval hook #1450

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
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
30 changes: 30 additions & 0 deletions ops/_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import subprocess
import sys
import warnings
from optparse import Option
Copy link
Contributor

Choose a reason for hiding this comment

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

OT: unused, afaics

from pathlib import Path
from typing import Any, Dict, List, Optional, Tuple, Type, Union, cast

Expand All @@ -32,6 +33,7 @@
from ops.log import setup_root_logging

CHARM_STATE_FILE = '.unit-state.db'
CHARM_EVAL_EXPR_ENVVAR = 'CHARM_EVAL_EXPR'


logger = logging.getLogger()
Expand Down Expand Up @@ -523,15 +525,43 @@ def _commit(self):
"""Commit the framework and gracefully teardown."""
self.framework.commit()

def _eval(self, expr: str):
"""Eval an expression in the context of the charm and print the result to stdout."""
import json

globs = {'self': self.charm, 'ops': ops, 'json': json}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel a slippery slope here: start with self, add ops, then json.dumps() because surely it's useful, then maybe yaml? pathlib? os? stdlib operator?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I agree. self seems fine as you kinda need the charm instance, ops seems reasonable, but anything beyond that I'd see if we can find a different way to import stuff. If we go the "exec" route (rather than "eval"), I guess you can do import json; json.dumps(x).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be fair, jhack also has a sister command, jhack script, that allows loading in a python file with a main(charm:Charm) entrypoint for the kind of 'more complex' scenarios where you may need to import stuff and the like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eval is mostly for inspection, not so much mutation. json felt like a universal, low-barrier addition. Yaml I wouldn't object to, but pathlib, os, operator feel too much. It's not about allowing you to golf complex operations into a string, it's about inspecting live data

try:
globs = {'self': self.charm, 'ops': ops, 'json': json}
out = eval(expr, globs)
except Exception:
logger.exception(
f'failure evaluating expression {expr!r} ' f'given available globs ({globs})'
)
return
logger.debug(f'expression {expr!r} evaluated to {out!r}')
print(out)

def run(self):
"""Emit and then commit the framework."""
try:
if eval_expr := _is_eval_set():
return self._eval(eval_expr)
self._emit()
self._commit()
finally:
self.framework.close()


def _is_eval_set() -> Optional[str]:
"""Return the value of the `CHARM_EVAL_EXPR_ENVVAR` envvar, if set.

This allows the admin to run, for example,
`CHARM_EVAL_EXPR="self.foo()" juju exec mycharm/0`
and see the result printed to stdout.
"""
return os.getenv(CHARM_EVAL_EXPR_ENVVAR)


def main(charm_class: Type[ops.charm.CharmBase], use_juju_for_storage: Optional[bool] = None):
"""Set up the charm and dispatch the observed event.

Expand Down
57 changes: 55 additions & 2 deletions test/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import abc
import importlib.util
import io
import json
import logging
import os
import re
Expand All @@ -30,10 +31,9 @@
import pytest

import ops
from ops._main import _should_use_controller_storage
from ops._main import _should_use_controller_storage, CHARM_EVAL_EXPR_ENVVAR
from ops.jujucontext import _JujuContext
from ops.storage import SQLiteStorage

from .charms.test_main.src.charm import MyCharmEvents
from .test_helpers import FakeScript

Expand Down Expand Up @@ -1491,3 +1491,56 @@ def test_not_if_already_local(self):
with patch.dict(os.environ, {'JUJU_VERSION': '2.8'}), tempfile.NamedTemporaryFile() as fd:
juju_context = _JujuContext.from_dict(os.environ)
assert not _should_use_controller_storage(Path(fd.name), meta, juju_context)


class TestCharmEval:
def _check(
self,
charm_class: typing.Type[ops.CharmBase],
*,
extra_environ: typing.Optional[typing.Dict[str, str]] = None,
**kwargs: typing.Any,
):
"""Helper for below tests."""

fake_environ = {
'JUJU_UNIT_NAME': 'test_main/0',
'JUJU_MODEL_NAME': 'mymodel',
'JUJU_VERSION': '2.7.0',
}
if extra_environ is not None:
fake_environ.update(extra_environ)

with tempfile.TemporaryDirectory() as tmpdirname:
fake_environ.update({'JUJU_CHARM_DIR': tmpdirname})
with patch.dict(os.environ, fake_environ):
tmpdirname = Path(tmpdirname)
fake_metadata = tmpdirname / 'metadata.yaml'
with fake_metadata.open('wb') as fh:
fh.write(b'name: test')

ops.main(charm_class, **kwargs)

def test_eval_charm_stmt(self, fake_script: FakeScript):
fake_script.write('juju-log', 'exit 0')

for expr, expected_result in (
('type(self).__name__', 'CharmBase'),
('ops.StatusBase.__name__', 'StatusBase'),
('json.dumps({1:2})', json.dumps({1: 2})),
('type(self.framework).__name__', 'Framework'),
('2 + 2', 4),
):
with patch.dict(os.environ, {CHARM_EVAL_EXPR_ENVVAR: expr}):
self._check(ops.CharmBase)

expected = [
'juju-log',
'--log-level',
'DEBUG',
'--',
f'expression {expr!r} evaluated to {expected_result!r}',
]

calls = fake_script.calls()
assert expected in calls
Loading