commit 33c24105 broke hvac.api.system_backend.Health.read_health_status()

[cgaspar-deshaw]

read_health_status now sometimes returns Response objects instead of dicts due to the change to use JSONAdapter, which returns Response objects for all non-200 results. This breaks callers expecting a dict, as any node which is not an initialized, unsealed, leader node returns a non-200 response.

[jeffwecan]

@cgaspar-deshaw: Yeah our apologies for that. The implicated change most likely should have been released with a major version bump at the very least in hindsight (though it was called out in the changelog for what that is worth). That said, the updated behavior was intended to more closely track the behavior of Vault API responses. I.e., a similar request sent via the Vault API also would not return a JSON object / dict as part of the response body.

Are you suggesting a change as part of opening this issue?

[cgaspar-deshaw]

"Note: GH-537 changes some methods' return types from None to a request.Response instance" That changelog entry says nothing about this breaking change. I also just got a report of a write failing as it returned a non-200 2XX code and a Response object. This is a massive API change and making it in a 3rd digit minor release is insane. I'm also fairly certain that returning a Response instead of a dict on all non-200 codes is just wrong. So yes, I'm suggest this insanity be reverted until a major release, and reconsidered if it should be done this way at all.

[jeffwecan]

So yes, I'm suggest this insanity be reverted until a major release, and reconsidered if it should be done this way at all.

As I mentioned in that last comment, I certainly agree that it should have gone out as part of a major release in the first place. Also I would be hesitant to revert the change immediately at this point since the release that included it is four months old at this point (and we would also like to ensure we don't break code using that, or subsequent, releases). I'll try to come up with a solution that best accommodates all involved and will post any updates along those lines on this issue. In the meantime, you can of course pin your hvac installation to 0.10.0 and we'd of course also welcome any PR that seeks to accomplish your suggested course of action.

[cgaspar-deshaw]

I can't imagine any sane way to deal with read_health_status() given your current adapter design and defaults (it really needs to return all Response objects or all dicts). The current mish-mash is just crazy, and I don't see a way of avoiding it without violating the adapter layering you've adopted, changing the default adapter, or changing the behaviour of the JSON adapter to stop special casing result codes. Not knowing why you wanted to make the (completely gratuitous and breaking) None -> Response change in the first place, I don't know what to propose to fix this. I just wrapped all hvac calls with isinstance() checks to revert to the previous behaviour expected by our callers no matter what random thing y'all decide to return to any given call.

[jeffwecan]

For context, the impetus for the change was to reduce the amount of code in individual API route methods. The previous response handling also would return a different type (based on the response status code or if the response body could be parsed as json depending). This PR's discussion also covers some of that background: #537

More to the matter at hand, it would seem that returning instances of requests.Resppnse across the board is the best course forward in terms of both avoiding unexpected / astonishing behavior as well as reducing the maintenance load of keeping up to date with the varied type of responses coming back from vault. So I'll aim to sketch out a patch that provides more consistent return types and subsequently publish it as part of a major release (assuming the updated implementation looks reasonable). Does that seem like a sane way to reset things to you @cgaspar-deshaw? (Also thanks for bringing up this issue in the first place as well entertaining this discussion!)