Skip to content

Conversation

@winstxnhdw
Copy link
Contributor

@winstxnhdw winstxnhdw commented Sep 4, 2023

@winstxnhdw
Copy link
Contributor Author

winstxnhdw commented Sep 4, 2023

Still unsure how to test if a service is critical or warning. I am also having trouble trying to deserialise the AggregatedStatus enum. Is it even possible to deserialise pure strings with ConsulClient?

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still unsure how to test if a service is critical or warning.

It is going to be tricky. I've implemented a test showing how to do it but it doesn't work.
The reason is that local health API returns an unsuccessful status code when at least one check is in a warning or critical state (https://developer.hashicorp.com/consul/api-docs/agent/service#get-local-service-health). Therefore we get an exception instead of a regular response. Not sure how to overcome this API inconsistency.

I am also having trouble trying to deserialise the AggregatedStatus enum. Is it even possible to deserialise pure strings with ConsulClient?

Just use HealthStatus class. I think that a simple enum would work just fine, but we don't need another enum class as the AggregatedStatus has the same possible values as health status, or doesn't it?

@winstxnhdw
Copy link
Contributor Author

The reason is that local health API returns an unsuccessful status code when at least one check is in a warning or critical state (https://developer.hashicorp.com/consul/api-docs/agent/service#get-local-service-health). Therefore we get an exception instead of a regular response. Not sure how to overcome this API inconsistency.

Haha, try-catch maybe?

@marcin-krystianc
Copy link
Contributor

Haha, try-catch maybe?

Well, it is better to avoid exceptions in the regular execution path. I think we have already an example of how to implement a special case for unsuccessful status codes, see

if (!response.IsSuccessStatusCode && (
(response.StatusCode != HttpStatusCode.NotFound && typeof(TOut) != typeof(TxnResponse)) ||
(response.StatusCode != HttpStatusCode.Conflict && typeof(TOut) == typeof(TxnResponse))))

You could implement a similar special case in the https://github.com/G-Research/consuldotnet/blob/master/Consul/Client_GetRequests.cs#L82

@winstxnhdw winstxnhdw marked this pull request as ready for review September 8, 2023 12:52
@winstxnhdw
Copy link
Contributor Author

winstxnhdw commented Sep 8, 2023

Whoops, marked this for review on accident.

@winstxnhdw
Copy link
Contributor Author

Sorry for the long delay! I was really busy with school and exams. Finally got around to working on it. @marcin-krystianc do let me know if I need to make any more changes.

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @winstxnhdw, I've left some comments. I think there was an actual bug in the GetLocalServiceHealthByID method.

@winstxnhdw
Copy link
Contributor Author

winstxnhdw commented Oct 19, 2023

Seems like I misunderstood the docs. I have removed GetWorstLocalHealthByID because querying by ID will query the status of the specific service with that ID. This means that GetLocalHealthByID is already equivalent. GetWorstLocalHealth, however, will find the worst health status among all services with the same name.

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a 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 simple suggestion to rename variable in a test to avoid confusion.

Co-authored-by: Marcin Krystianc <[email protected]>
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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.

Support for "/agent/health/service/*" API methods.

2 participants