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.

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: change payload for healthchecks to text #607

Merged
merged 4 commits into from
Dec 10, 2024

Conversation

herobrauni
Copy link
Contributor

Fixes #552 in my tests.
image

I can barely write go code and have done only minimal checks, so viewers discretion is advised.

In any case thank you a lot for the great tool

@CLAassistant
Copy link

CLAassistant commented Dec 9, 2024

CLA assistant check
All committers have signed the CLA.

@herobrauni herobrauni marked this pull request as ready for review December 9, 2024 18:06
@garethgeorge garethgeorge changed the title change payload for healthchecks to text fix: change payload for healthchecks to text Dec 9, 2024
@garethgeorge
Copy link
Owner

First, thanks for contributing this fix! Generally looks great.

Question about healthchecks.io -- is there a feature of healthchecks that depends on the JSON text field / @inode64 is there a reason you originally used json payloads with a text field set?

Will this switch break an existing use case?

@inode64
Copy link
Contributor

inode64 commented Dec 9, 2024

When copying another hook I used the same structures that used json, then I noticed the problem that they were not displayed correctly, I tried using a json to text converter internally in healthchecks, but in the end I think it is better that backrest send the body correctly

@garethgeorge
Copy link
Owner

garethgeorge commented Dec 9, 2024

Sgtm, thanks for the context! The JSON text field just happens to be the payload structure for two of the messaging services used -- makes sense that it confusingly looked like a pattern to follow. Given that, I think I feel reasonably comfortable making this change without a migration / providing a toggle. We can just switch over.

I’ll mark this as a “feat” as it’s slightly breaking but should be an improvement for most users. It deserves to be tracked as a minor version change.

@garethgeorge garethgeorge changed the title fix: change payload for healthchecks to text feat: change payload for healthchecks to text Dec 9, 2024
@garethgeorge garethgeorge merged commit a1e3a70 into garethgeorge:main Dec 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

healthchecks.io hook should send a plain text message as its status
4 participants