-
Notifications
You must be signed in to change notification settings - Fork 121
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 pebble.CheckInfo.change_id field #1197
Conversation
This is new with health checks being implemented as changes and tasks (canonical/pebble#409). We shouldn't merge this before that Pebble PR is merged, just in case anything changes.
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 - just one small question (about the drive-by).
I'm not sure I totally understand the use-case for knowing the change ID in ops - is there one? Or is this more for the Python Pebble API and completeness?
) | ||
|
||
def __repr__(self): | ||
return ('CheckInfo(' | ||
f'name={self.name!r}, ' | ||
f'level={self.level!r}, ' | ||
f'level={self.level}, ' |
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 seems like it's better if level is a CheckLevel
and worse if it's a str
. Maybe both should be explicitly handled?
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.
True. However, realistically this is always going to be a CheckLevel
(UP or DOWN). I think I'll leave this as is for now, as explicitly handling both is messy. We can update them all if we want in a later PR. But right now we don't test repr()
and don't guarantee that eval(repr(x))
works, so I think it's probably fine as is.
This is new with health checks being implemented as changes and tasks (canonical/pebble#409).
Ops/charms will likely not use this new field. Even once we have the new
change-updated
event, that has a.get_change()
method to get the change directly. But we're adding it to the Pebble client/types for completeness.We shouldn't merge this before that Pebble PR is merged, just in case anything changes.