Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add checks config fields and get_checks API #668
Add checks config fields and get_checks API #668
Changes from 21 commits
1ef5861
e76ec56
08ec4ff
38b365c
30722eb
f77f445
ef170f0
6c22f7c
9ea0764
879718f
ec60a5b
752dcee
cbfe6a0
90792db
b355ee0
30c12ac
c1e9988
a0e40ae
01287e0
b454cce
15d96a8
59ef681
6c8c557
d7900ea
48fa314
8fc08db
926e728
42e92ce
2f75e04
68903f4
8e689d4
d7dbd9f
c12ff4a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why the choice to make raw an untyped dict instead of unpacking it?
I would rather have
Check('name', override:str='', level:str='', period:str='', ...)
, makes for a way easier to read initializer, and easier usage of the class.If you really dislike doing
check=Check('name', **my_raw_dict)
, then we should expose an alternative classmethodCheck.from_dict(name:str, dict_:dict)
.If we were on more modern python I could also go for a dataclass or a TypedDict, but until then, this feels like better code.
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.
Actually the whole Check class could very well be a dataclass, and save some eq, repr and to_dict boilerplate
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, this should arguably be done with a
from_dict
. However, I've made this consistent with the other layer-configuration classes, which all do it the same way. If doing it again maybe we could have considered dataclasses or some such (then again, that's 3.7+ andops
still needs to support 3.5 for now). So I'm going to leave as is to be consistent.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 would be nice to get some more detail in this docstring since this is one of the main classes users will interact with for this - or at least a link to a discourse doc that discusses it. Particularly - what is status (i.e. the main thing users should be checking). I looked in the docs you made and couldn't find any mention there of status, UP, DOWN, etc. either.
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.
The main doc is the Pebble SDK doc with a section on health checks: https://juju.is/docs/sdk/pebble#heading--health-checks. I have added a new
model.Container.get_checks
andmodel.Container.get_check
method (to matchget_services
andget_service
) and added a brief doc section on that here -- I'll add that to the Discourse doc as soon as this is merged.I've also added much better docstrings on
CheckInfo
, which will appear in the API documentation (and is linked to from the above).