Xabaril
feat: Expected content on UriHealthCheck
Hey @unaizorrilla , are you back from holidays? I'd like to get this through so that my team can do some work off the back of it.
Yeah! Tomorrow I try to spend all day in healthchecks, 3.0 migration history and pending pr
Hey @unaizorrilla, the idea behind the Func<HttpContent, (IsOk, Reason)> is that it enables much more powerful checking scenarios.
Consumers could provide their own Func that accepts HttpContent, and they can use all of the available extension methods off HttpContent like .ReadAsAync<T>() to read out strong types, etc. etc., perform their validation and then also return the reason it failed, which will come out in the HealthCheckResult (another reason for the split ifs).
I definitely agree that a tuple is a bad choice for a public API, I'm more than happy to refactor that, but I'm worried about removing the other functionality as I can see powerful uses for it.
Hi @benmccallum
Well, we can accept two overloads with expectedContent and the lambda but please, remove tuples for public api.
Note: Be carefull because we are migrating to netcoreapp3 and modifying branch version.
If healthcheck is for netcoreapp2 please use 2.2 branch, if is for netcoreapp3 use master branch!!
Hey @unaizorrilla, I've made some changes to the PR to address your concerns, specifically around the option to check the expected content with a Func. The signature is now Func<HttpContent, Task<ContentCheckResult>>. I thought about using a Func<HttpContent, string> and if the string is non-null it's a failure message and the health check should fail, but it seemed too "loose".
I'd gone down that path and described it in the param's description, but in the end went for a typed result, kinda like how MVC does it with ActionResult.
Hope that's OK. Happy to just merge this into master and have it 3.0+. Ready when you are!
Hey @unaizorrilla , sorry to be a pain, I know you're probably super busy, but could we get this merged?
@anthonykeller79, I think we're going to have to just cut out the code into our own code base that we need for this as I'm not sure if this will ever be merged. Unless you wanna fork this into our own org on GitHub and setup a nuget package for it?
@benmccallum Hi. I help maintaining this repo. Rebase onto master if you want to continue working with this PR and I'll review it.
@benmccallum do you need help to finish this pull request? Because I would love to see a completion of this as well, and could potentially take over if you need it.
Hey @sungam3r and @Nisden , feel free to take over.
It's been so long since I did the work on this that I'm out of touch and you'd be just as good to take over as me starting again. What I wanted to achieve is documented in the linked issue.
Happy to review once it's ready.
I'm closing this one in favor of #1199. For anyone interested in - as @benmccallum said
feel free to take over.