-
-
Notifications
You must be signed in to change notification settings - Fork 65
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: initial support for healthchecks.io #480
Conversation
Include event restore launch Add specific signals Fix readme.md Fix message send
internal/hook/types/healthchecks.go
Outdated
event == v1.Hook_CONDITION_ANY_ERROR || | ||
event == v1.Hook_CONDITION_CHECK_ERROR || | ||
event == v1.Hook_CONDITION_PRUNE_ERROR || | ||
event == v1.Hook_CONDITION_SNAPSHOT_ERROR { |
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.
perhaps my only nit is it may be worth declaring some utilities or constants for this e.g. isStartCondition, isErrorCondition, isSuccessCondition so that we can be consistent about this going forward.
These blocks probably also should have handling for CONDITION_SNAPSHOT_SUCCESS
, CONDITION_PRUNE_SUCCESS
, and CONDITION_CHECK_SUCCESS
This is something that could be added to protoutil e.g. protoutil/conditions.go . It would be good to think about how to add a test that asserts that the functions are updated whenever the enum definition changes, but that's also something I can followup with :)
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.
Yes, I already thought about it,
I can create an array with the constants and the function or, a function only using switch like this example
func isStartCondition(event v1.Hook_Condition) bool { switch event { case v1.Hook_CONDITION_SNAPSHOT_START, v1.Hook_CONDITION_PRUNE_START, v1.Hook_CONDITION_CHECK_START: return true default: return false } }
What do you see better?
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.
I think a couple of arrays and slices.Contains
as the check would be a good way to define this.
The test I'm thinking of would look somewhat similar to https://github.com/garethgeorge/backrest/blob/main/internal/hook/hook_test.go#L10-L16 e.g. just assert that each enum value exists in at least one of the declared arrays categorizing them.
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.
I've been looking for a more elegant solution, see what you 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.
Dictionary approach looks good to me! Great work & thanks for adding this hook. I think it's a very intuitive behavior.
Thanks for contributing this! Looks great and I think is very reasonable, my understanding is that one of the most commonly used command hooks is to ping healthchecks.io -- native support is a good idea. Left a comment re: a more scalable approach to handling the conditions, but overall looks great :) |
Going ahead and marking as approved, I'm going to hold off on merging until the weekend as this makes sense for Backrest's next feature release (v1.6.0) which will be middle of next month. I'm aiming to put out a bug patch release over the weekend that will include the bug fixes in restic 0.17.1 and then start merging the new features for the next minor version release. |
Released in 1.6.0 :) |
https://healthchecks.io/docs/http_api/