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.

Sign up for GitHub

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

Add support for exec "service-context" #957

Merged
merged 11 commits into from
Jul 17, 2023
Merged

Add support for exec "service-context" #957

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0294f7d
Add support for exec "context"
benhoyt Jun 26, 2023
8f68a98
Comment out grafana-k8s-operator CI till type checking issue is fixed
benhoyt Jun 27, 2023
31926c4
Bump up to latest Pyright version while we're at it
benhoyt Jun 27, 2023
7561a98
Wording tweak; undo commented-out CI test
benhoyt Jun 28, 2023
c38729e
Merge branch 'main' into exec-context
benhoyt Jul 10, 2023
273330e
Rename "context" to "service_context"
benhoyt Jul 11, 2023
e1545ee
Merge branch 'main' into exec-context
benhoyt Jul 11, 2023
15831c2
Error if exec used with service_context on Juju that doesn't support it
benhoyt Jul 12, 2023
3432745
Merge branch 'main' into exec-context
benhoyt Jul 12, 2023
d024ce8
Oh wait, 3.2.1 is already out, it'll be 3.2.2
benhoyt Jul 12, 2023
001b16a
Simplify per John's feedback
benhoyt Jul 17, 2023
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
4 changes: 4 additions & 0 deletions ops/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2360,6 +2360,7 @@ def exec( # noqa
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2381,6 +2382,7 @@ def exec( # noqa
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2400,6 +2402,7 @@ def exec(
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2420,6 +2423,7 @@ def exec(
"""
return self._pebble.exec(
command,
service_context=service_context,
environment=environment,
working_dir=working_dir,
timeout=timeout,
Expand Down
33 changes: 30 additions & 3 deletions ops/pebble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
'user-id': Optional[int],
'group': str,
'group-id': Optional[int],
'working-dir': str,
'on-success': str,
'on-failure': str,
'on-check-failure': Dict[str, Any],
Expand All @@ -88,9 +89,24 @@
},
total=False)

HttpDict = typing.TypedDict('HttpDict', {'url': str})
TcpDict = typing.TypedDict('TcpDict', {'port': int})
ExecDict = typing.TypedDict('ExecDict', {'command': str})
HttpDict = typing.TypedDict('HttpDict',
{'url': str,
'headers': Dict[str, str]},
total=False)
TcpDict = typing.TypedDict('TcpDict',
{'port': int,
'host': str},
total=False)
ExecDict = typing.TypedDict('ExecDict',
{'command': str,
'service-context': str,
'environment': Dict[str, str],
'user-id': Optional[int],
'user': str,
'group-id': Optional[int],
'group': str,
'working-dir': str},
total=False)
Copy link
Member

Choose a reason for hiding this comment

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

How did these get so out of sync with what you can put into them? Presumably because they are just dicts (so you could have always passed additional information there).
But that also implies that we are missing test cases that the extra fields have any impact on the requests.
I suppose mostly these are just curried to Pebble, and all the testing should happen at that level.
It does feel like we are missing something about the contract with an external system
(for example, someone might spell it service_context vs service-context, and while the typeddict will catch with a linter, it would be nice to have some form of validation that it matches what pebble actually expects)

What if pebble had a "validate these arguments" mode, where it didn't actually run it, but at least validated the content, and then an ops test that if it doesn't see pebble in the path, just gets skipped, but otherwise asks to validate args that we want to pass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These types only just became public (in ops 2.4.0), so that's probably the main reason. I wanted them to be public so people could use them to type check their pebble.LayerDicts in their charms. If people are using this new type annotations for their dicts, it will check them (though they don't have to).

Separately, we're planning to add a --dry mode for validating layer config to Pebble (canonical/pebble#214). But I think additional work here is out of scope, at least for now.


CheckDict = typing.TypedDict('CheckDict',
{'override': str,
Expand Down Expand Up @@ -800,6 +816,7 @@ def __init__(self, name: str, raw: Optional['ServiceDict'] = None):
self.user_id = dct.get('user-id')
self.group = dct.get('group', '')
self.group_id = dct.get('group-id')
self.working_dir = dct.get('working-dir', '')
self.on_success = dct.get('on-success', '')
self.on_failure = dct.get('on-failure', '')
self.on_check_failure = dict(dct.get('on-check-failure', {}))
Expand All @@ -823,6 +840,7 @@ def to_dict(self) -> 'ServiceDict':
('user-id', self.user_id),
('group', self.group),
('group-id', self.group_id),
('working-dir', self.working_dir),
('on-success', self.on_success),
('on-failure', self.on_failure),
('on-check-failure', self.on_check_failure),
Expand Down Expand Up @@ -2136,6 +2154,7 @@ def exec( # noqa
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2157,6 +2176,7 @@ def exec( # noqa
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2176,6 +2196,7 @@ def exec(
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand Down Expand Up @@ -2260,6 +2281,11 @@ def exec(
Args:
command: Command to execute: the first item is the name (or path)
of the executable, the rest of the items are the arguments.
service_context: If specified, run the command in the context of
this service. Specifically, inherit its environment variables,
user/group settings, and working directory. The other exec
options will override the service context; ``environment``
will be merged on top of the service's.
environment: Environment variables to pass to the process.
working_dir: Working directory to run the command in. If not set,
Pebble uses the target user's $HOME directory (and if the user
Expand Down Expand Up @@ -2325,6 +2351,7 @@ def exec(

body = {
'command': command,
'service-context': service_context,
'environment': environment or {},
'working-dir': working_dir,
'timeout': _format_timeout(timeout) if timeout is not None else None,
Expand Down
2 changes: 1 addition & 1 deletion requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ flake8-builtins==2.1.0
pyproject-flake8==4.0.1
pep8-naming==0.13.2
pytest==7.2.1
pyright==1.1.313
pyright==1.1.316
pytest-operator==0.23.0
coverage[toml]==7.0.5
typing_extensions==4.2.0
Expand Down
2 changes: 2 additions & 0 deletions test/pebble_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def main():
p.add_argument('name', help='check name(s) to filter on', nargs='*')

p = subparsers.add_parser('exec', help='execute a command')
p.add_argument('--context', help='service context')
p.add_argument('--env', help='environment variables to set', action='append',
metavar='KEY=VALUE')
p.add_argument('--working-dir', help='working directory to run command in')
Expand Down Expand Up @@ -196,6 +197,7 @@ def main():

process = client.exec(
args.exec_command,
service_context=args.context,
environment=environment,
working_dir=args.working_dir,
timeout=args.timeout,
Expand Down
2 changes: 2 additions & 0 deletions test/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1781,6 +1781,7 @@ def test_exec(self):
self.pebble.responses.append('fake_exec_process')
p = self.container.exec(
['echo', 'foo'],
service_context='srv1',
environment={'K1': 'V1', 'K2': 'V2'},
working_dir='WD',
timeout=10.5,
Expand All @@ -1796,6 +1797,7 @@ def test_exec(self):
)
self.assertEqual(self.pebble.requests, [
('exec', ['echo', 'foo'], dict(
service_context='srv1',
environment={'K1': 'V1', 'K2': 'V2'},
working_dir='WD',
timeout=10.5,
Expand Down
6 changes: 5 additions & 1 deletion test/test_pebble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,7 @@ def _assert_empty(self, service, name):
self.assertIs(service.user_id, None)
self.assertEqual(service.group, '')
self.assertIs(service.group_id, None)
self.assertEqual(service.working_dir, '')
self.assertEqual(service.on_success, '')
self.assertEqual(service.on_failure, '')
self.assertEqual(service.on_check_failure, {})
Expand Down Expand Up @@ -711,6 +712,7 @@ def test_dict(self):
'user-id': 1000,
'group': 'staff',
'group-id': 2000,
'working-dir': '/working/dir',
'on-success': 'restart',
'on-failure': 'ignore',
'on-check-failure': {'chk1': 'halt'},
Expand All @@ -732,6 +734,7 @@ def test_dict(self):
self.assertEqual(s.user_id, 1000)
self.assertEqual(s.group, 'staff')
self.assertEqual(s.group_id, 2000)
self.assertEqual(s.working_dir, '/working/dir')
self.assertEqual(s.on_success, 'restart')
self.assertEqual(s.on_failure, 'ignore')
self.assertEqual(s.on_check_failure, {'chk1': 'halt'})
Expand Down Expand Up @@ -2639,10 +2642,11 @@ def add_responses(self, change_id, exit_code, change_err=None):
return (stdio, stderr, control)

def build_exec_data(
self, command, environment=None, working_dir=None, timeout=None,
self, command, service_context=None, environment=None, working_dir=None, timeout=None,
user_id=None, user=None, group_id=None, group=None, combine_stderr=False):
return {
'command': command,
'service-context': service_context,
'environment': environment or {},
'working-dir': working_dir,
'timeout': f'{timeout:.3f}s' if timeout is not None else None,
Expand Down