Add list endpoint to health service #51
Conversation
Motivation: With gRFC A90 a `List` endpoint was added to the health service. Modifications: * Update the health.proto file to the new version * Update the generated files using protoc * Add the new `List` endpoint to the HealthService * Add testing for the list endpoint Result: HealthService is now fully compliant with gRFC A90.
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Thanks @yigityazicilar -- I left some feedback on the tests.
A note for next time: when you're dealing with vendored in and/or generated code it's best to update them in one commit and then make your changes in a separate commit. It just makes the review smaller (sometimes the code gen updates are very large!)
| ) | ||
| } | ||
|
|
||
| for i in 0 ..< 10 { |
There was a problem hiding this comment.
Avoid using direct indexing if possible; not all collections in Swift use integer indexing and those that do aren't required for the indexes to be monotonically increasing.
In this case use enumerated(), something like:
for (index, (descriptor, status)) in testServiceDescriptors.enumerated() {
// ...
}| let statuses = try response.message.statuses | ||
| XCTAssertTrue(statuses.count == i + 1) | ||
|
|
||
| for j in 0 ... i { |
There was a problem hiding this comment.
Avoid using direct indexing again, use:
for (descriptor, status) in testServiceDescriptors.prefix(index + 1) {
// ...
}| testServiceDescriptors[j].1 | ||
| ) | ||
|
|
||
| XCTAssertEqual(receivedStatus!, expectedStatus) |
There was a problem hiding this comment.
In general avoid the ! if possible, where it isn't possible add a brief comment saying why it's okay.
In this instance you can use let status = try XCTUnwrap(receivedStatus) to get the status or exit the test otherwise.
| let receivedServerStatus = statuses[""]?.status | ||
| XCTAssertNotNil(receivedServerStatus) | ||
|
|
||
| XCTAssertEqual(receivedServerStatus!, .serving) |
| let receivedServerStatus = statuses[""]?.status | ||
| XCTAssertNotNil(receivedServerStatus) | ||
|
|
||
| XCTAssertEqual(receivedServerStatus!, .notServing) |
|
The API breakage check failures are a false positive (the symbols were added at |
Co-authored-by: George Barnett <[email protected]>
Co-authored-by: George Barnett <[email protected]>
Co-authored-by: George Barnett <[email protected]>
Co-authored-by: George Barnett <[email protected]>
| // The server may respond with a RESOURCE_EXHAUSTED error if too many services | ||
| // exist. |
There was a problem hiding this comment.
Also, I am not sure about this line in the proto file. Should I add a limit? In the proposal it was limited to 100.
There was a problem hiding this comment.
Yeah, that's a good callout. Go uses the limit of 100, we should do the same.
There was a problem hiding this comment.
Added the limit should be good to merge
* Moved out the empty case to its own test * Removed the usages of `!` * Stopped using direct indexing * Changed to the shorthand API
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Looks great, thanks @yigityazicilar!
2 participants
Motivation:
With gRFC A90 a
Listendpoint was added to the health service.Modifications:
Listendpoint to the HealthServiceResult:
HealthService is now fully compliant with gRFC A90.