-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: support starting and stopping Pebble checks, and the checks enabled field #1560
base: main
Are you sure you want to change the base?
feat: support starting and stopping Pebble checks, and the checks enabled field #1560
Conversation
def _render_services(self) -> Dict[str, pebble.Service]: | ||
services: Dict[str, pebble.Service] = {} | ||
for key in sorted(self._layers.keys()): | ||
layer = self._layers[key] | ||
for name, service in layer.services.items(): | ||
# TODO: merge existing services https://github.com/canonical/operator/issues/1112 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive-by: that ticket is closed, and it looks like this is already done.
The pack failure is:
I assume this is a temporary problem with the store - I'll retry the job later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good work -- thanks especially for updating the testing stuff (and testing the testing stuff!).
A few minor comments, some debatable, and one that would require a Pebble tweak.
@@ -1475,6 +1516,7 @@ def __repr__(self): | |||
'CheckInfo(' | |||
f'name={self.name!r}, ' | |||
f'level={self.level}, ' | |||
f'startup={self.startup}, ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ben, repeat after me: "dataclasses are good, use them next time". :-)
@@ -2126,7 +2168,9 @@ def autostart_services(self, timeout: float = 30.0, delay: float = 0.1) -> Chang | |||
return self._services_action('autostart', [], timeout, delay) | |||
|
|||
def replan_services(self, timeout: float = 30.0, delay: float = 0.1) -> ChangeID: | |||
"""Replan by (re)starting changed and startup-enabled services and wait for them to start. | |||
"""Replan by (re)starting changed and startup-enabled services and checks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a pity I called this replan_services
, but oh well, at least it's called just replan
for the Container
method. Not worth deprecating/aliasing here, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was my thinking as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me know, thanks!
Add support for:
startup
field in Pebble checks.start_checks
andstop_checks
Pebble API calls.Harness (and Scenario, mostly via re-using the Harness implementation) is adjusted to more closely simulate the Changes implementation of Pebble Checks, so that the 'if the change ID is the empty string, the check is inactive' behaviour can be simulated.
A subtle bug with notices and check-infos is also fixed: previously the mocked Pebble would gather all notices and check-infos from all containers in the state, instead of only those that are in the correct container.
Pebble PR