feat(cluster-state-service): add ping endpoint #195

dominicbarnes · 2017-05-12T19:48:01Z

Summary

Adds a "ping" endpoint to cluster-state-service at /v1/ping which behaves exactly like the same endpoint in daemon-scheduler. (fixes #194)

Implementation details

I copy-pasta'd a lot over from daemon-scheduler.

Testing

cluster-state-service binary built locally and unit-tests pass (cd cluster-state-service; make; cd ../)
cluster-state-service build in Docker succeeds (cd cluster-state-service; make release; cd ../)
daemon-scheduler binary built locally and unit-tests pass (cd daemon-scheduler; make; cd ../)
daemon-scheduler build in Docker succeeds (cd daemon-scheduler; make release; cd ../)

New tests cover the changes: yes

Description for the changelog

added a health check endpoint

Licensing

This contribution is under the terms of the Apache 2.0 License: Yes

Before merging

cluster-state-service end-to-end tests pass. Required setup details are listed here.
daemon-scheduler integration tests pass. Required setup details are listed here.
daemon-scheduler end-to-end tests pass. Required setup details are listed here.

dominicbarnes · 2017-05-12T19:48:25Z

I'm having trouble getting this to build locally on my fork because of dependency issues. It looks like go isn't playing nicely with the vendor/ directory and my forked version, since it's technically a different module.

This is also my first contribution to the project, so I may need some help getting this all together.

dominicbarnes · 2017-05-12T20:17:07Z

Tests pass on Travis though! So I think this is in pretty good shape

kylbarnes

Nitpick changes requested.

kylbarnes · 2017-05-17T05:26:34Z

cluster-state-service/handler/api/v1/generalapis.go

+	"github.com/blox/blox/cluster-state-service/handler/store"
+)
+
+// TaskAPIs encapsulates the backend datastore with which the task APIs interact


Can you fix this comment, since this isn't TaskAPIs?

kylbarnes · 2017-05-17T05:27:09Z

cluster-state-service/handler/api/v1/generalapis.go

+
+// TaskAPIs encapsulates the backend datastore with which the task APIs interact
+type GeneralAPIs struct {
+	taskStore store.TaskStore


This probably doesn't need a taskStore object.

kylbarnes · 2017-05-17T05:27:29Z

cluster-state-service/handler/api/v1/generalapis.go

+	taskStore store.TaskStore
+}
+
+// NewTaskAPIs initializes the TaskAPIs struct


Same fix request for this comment.

kylbarnes · 2017-05-17T05:28:27Z

cluster-state-service/handler/api/v1/generalapis_test.go

+	suite.router = suite.getRouter()
+}
+
+// TODO - Add the following test cases once implementation is in place


Can you remove this comment block?

kylbarnes · 2017-05-17T05:33:28Z

Looks good @dominicbarnes, thanks for the commit. I've requested a couple nitpick changes though before I merge it in. Can you update your pull request with the changes requested?

Confirmed unit tests pass. Also verified /v1/ping handler via curl:

$ curl -v http://localhost:3000/v1/ping
< HTTP/1.1 200 OK
< Content-Type: application/json; charset=UTF-8
< Date: Wed, 17 May 2017 05:24:55 GMT
< Content-Length: 0

kylbarnes · 2017-06-23T23:55:35Z

Closing due to inactivity. Please feel free to re-open after addressing PR comments.

feat(cluster-state-service): add ping endpoint

8174717

kylbarnes reviewed May 17, 2017

View reviewed changes

kylbarnes closed this Jun 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cluster-state-service): add ping endpoint #195

feat(cluster-state-service): add ping endpoint #195

dominicbarnes commented May 12, 2017 •

edited

dominicbarnes commented May 12, 2017 •

edited

dominicbarnes commented May 12, 2017

kylbarnes left a comment

kylbarnes May 17, 2017

kylbarnes May 17, 2017

kylbarnes May 17, 2017

kylbarnes May 17, 2017

kylbarnes commented May 17, 2017

kylbarnes commented Jun 23, 2017

feat(cluster-state-service): add ping endpoint #195

feat(cluster-state-service): add ping endpoint #195

Conversation

dominicbarnes commented May 12, 2017 • edited

Summary

Implementation details

Testing

Description for the changelog

Licensing

Before merging

dominicbarnes commented May 12, 2017 • edited

dominicbarnes commented May 12, 2017

kylbarnes left a comment

Choose a reason for hiding this comment

kylbarnes May 17, 2017

Choose a reason for hiding this comment

kylbarnes May 17, 2017

Choose a reason for hiding this comment

kylbarnes May 17, 2017

Choose a reason for hiding this comment

kylbarnes May 17, 2017

Choose a reason for hiding this comment

kylbarnes commented May 17, 2017

kylbarnes commented Jun 23, 2017

dominicbarnes commented May 12, 2017 •

edited

dominicbarnes commented May 12, 2017 •

edited