From e63d3a1275153e2a6ac9a928544c8242a97c690d Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Sat, 23 Apr 2016 16:01:59 -0700 Subject: [PATCH 1/4] Update Check API to use constants Use constants where appropriate to advocate their use. Also add a deprecation notice re: `updateTTL`. --- api/agent.go | 36 ++++++++++++++++++------- api/agent_test.go | 40 ++++++++++++++-------------- api/catalog_test.go | 2 +- command/agent/agent_endpoint_test.go | 10 +++---- command/agent/check_test.go | 40 ++++++++++++++-------------- command/agent/health_endpoint.go | 5 ++-- testutil/README.md | 6 ++--- testutil/server.go | 13 ++++----- watch/funcs_test.go | 5 ++-- 9 files changed, 88 insertions(+), 69 deletions(-) diff --git a/api/agent.go b/api/agent.go index 67855c67fb..1211cc6fd4 100644 --- a/api/agent.go +++ b/api/agent.go @@ -198,27 +198,42 @@ func (a *Agent) ServiceDeregister(serviceID string) error { return nil } -// PassTTL is used to set a TTL check to the passing state +// PassTTL is used to set a TTL check to the passing state. +// +// DEPRECATION NOTICE: This interface is deprecated in favor of UpdateTTL(). +// The client interface will be removed in 0.8 or changed to use +// UpdateTTL()'s endpoint and the server endpoints will be removed in 0.9. func (a *Agent) PassTTL(checkID, note string) error { return a.updateTTL(checkID, note, "pass") } -// WarnTTL is used to set a TTL check to the warning state +// WarnTTL is used to set a TTL check to the warning state. +// +// DEPRECATION NOTICE: This interface is deprecated in favor of UpdateTTL(). +// The client interface will be removed in 0.8 or changed to use +// UpdateTTL()'s endpoint and the server endpoints will be removed in 0.9. func (a *Agent) WarnTTL(checkID, note string) error { return a.updateTTL(checkID, note, "warn") } -// FailTTL is used to set a TTL check to the failing state +// FailTTL is used to set a TTL check to the failing state. +// +// DEPRECATION NOTICE: This interface is deprecated in favor of UpdateTTL(). +// The client interface will be removed in 0.8 or changed to use +// UpdateTTL()'s endpoint and the server endpoints will be removed in 0.9. func (a *Agent) FailTTL(checkID, note string) error { return a.updateTTL(checkID, note, "fail") } // updateTTL is used to update the TTL of a check. This is the internal -// method that uses the old API that's present in Consul versions prior -// to 0.6.4. Since Consul didn't have an analogous "update" API before it -// seemed ok to break this (former) UpdateTTL in favor of the new UpdateTTL -// below, but keep the old Pass/Warn/Fail methods using the old API under the -// hood. +// method that uses the old API that's present in Consul versions prior to +// 0.6.4. Since Consul didn't have an analogous "update" API before it seemed +// ok to break this (former) UpdateTTL in favor of the new UpdateTTL below, +// but keep the old Pass/Warn/Fail methods using the old API under the hood. +// +// DEPRECATION NOTICE: This interface is deprecated in favor of UpdateTTL(). +// The client interface will be removed in 0.8 and the server endpoints will +// be removed in 0.9. func (a *Agent) updateTTL(checkID, note, status string) error { switch status { case "pass": @@ -240,8 +255,9 @@ func (a *Agent) updateTTL(checkID, note, status string) error { // checkUpdate is the payload for a PUT for a check update. type checkUpdate struct { - // Status us one of the structs.Health* states, "passing", "warning", or - // "critical". + // Status us one of the structs.Health* states: HealthPassing + // ("passing"), HealthWarning ("warning"), or HealthCritical + // ("critical"). Status string // Output is the information to post to the UI for operators as the diff --git a/api/agent_test.go b/api/agent_test.go index a612451493..b188d7ce3f 100644 --- a/api/agent_test.go +++ b/api/agent_test.go @@ -76,7 +76,7 @@ func TestAgent_Services(t *testing.T) { } // Checks should default to critical - if chk.Status != "critical" { + if chk.Status != HealthCritical { t.Fatalf("Bad: %#v", chk) } @@ -97,7 +97,7 @@ func TestAgent_Services_CheckPassing(t *testing.T) { Port: 8000, Check: &AgentServiceCheck{ TTL: "15s", - Status: "passing", + Status: HealthPassing, }, } if err := agent.ServiceRegister(reg); err != nil { @@ -121,7 +121,7 @@ func TestAgent_Services_CheckPassing(t *testing.T) { t.Fatalf("missing check: %v", checks) } - if chk.Status != "passing" { + if chk.Status != HealthPassing { t.Fatalf("Bad: %#v", chk) } if err := agent.ServiceDeregister("foo"); err != nil { @@ -320,47 +320,47 @@ func TestAgent_SetTTLStatus(t *testing.T) { if err := agent.WarnTTL("service:foo", "foo"); err != nil { t.Fatalf("err: %v", err) } - verify("warning", "foo") + verify(HealthWarning, "foo") if err := agent.PassTTL("service:foo", "bar"); err != nil { t.Fatalf("err: %v", err) } - verify("passing", "bar") + verify(HealthPassing, "bar") if err := agent.FailTTL("service:foo", "baz"); err != nil { t.Fatalf("err: %v", err) } - verify("critical", "baz") + verify(HealthCritical, "baz") if err := agent.UpdateTTL("service:foo", "foo", "warn"); err != nil { t.Fatalf("err: %v", err) } - verify("warning", "foo") + verify(HealthWarning, "foo") if err := agent.UpdateTTL("service:foo", "bar", "pass"); err != nil { t.Fatalf("err: %v", err) } - verify("passing", "bar") + verify(HealthPassing, "bar") if err := agent.UpdateTTL("service:foo", "baz", "fail"); err != nil { t.Fatalf("err: %v", err) } - verify("critical", "baz") + verify(HealthCritical, "baz") - if err := agent.UpdateTTL("service:foo", "foo", "warning"); err != nil { + if err := agent.UpdateTTL("service:foo", "foo", HealthWarning); err != nil { t.Fatalf("err: %v", err) } - verify("warning", "foo") + verify(HealthWarning, "foo") - if err := agent.UpdateTTL("service:foo", "bar", "passing"); err != nil { + if err := agent.UpdateTTL("service:foo", "bar", HealthPassing); err != nil { t.Fatalf("err: %v", err) } - verify("passing", "bar") + verify(HealthPassing, "bar") - if err := agent.UpdateTTL("service:foo", "baz", "critical"); err != nil { + if err := agent.UpdateTTL("service:foo", "baz", HealthCritical); err != nil { t.Fatalf("err: %v", err) } - verify("critical", "baz") + verify(HealthCritical, "baz") if err := agent.ServiceDeregister("foo"); err != nil { t.Fatalf("err: %v", err) @@ -390,7 +390,7 @@ func TestAgent_Checks(t *testing.T) { if !ok { t.Fatalf("missing check: %v", checks) } - if chk.Status != "critical" { + if chk.Status != HealthCritical { t.Fatalf("check not critical: %v", chk) } @@ -409,7 +409,7 @@ func TestAgent_CheckStartPassing(t *testing.T) { reg := &AgentCheckRegistration{ Name: "foo", AgentServiceCheck: AgentServiceCheck{ - Status: "passing", + Status: HealthPassing, }, } reg.TTL = "15s" @@ -425,7 +425,7 @@ func TestAgent_CheckStartPassing(t *testing.T) { if !ok { t.Fatalf("missing check: %v", checks) } - if chk.Status != "passing" { + if chk.Status != HealthPassing { t.Fatalf("check not passing: %v", chk) } @@ -580,7 +580,7 @@ func TestServiceMaintenance(t *testing.T) { for _, check := range checks { if strings.Contains(check.CheckID, "maintenance") { found = true - if check.Status != "critical" || check.Notes != "broken" { + if check.Status != HealthCritical || check.Notes != "broken" { t.Fatalf("bad: %#v", checks) } } @@ -627,7 +627,7 @@ func TestNodeMaintenance(t *testing.T) { for _, check := range checks { if strings.Contains(check.CheckID, "maintenance") { found = true - if check.Status != "critical" || check.Notes != "broken" { + if check.Status != HealthCritical || check.Notes != "broken" { t.Fatalf("bad: %#v", checks) } } diff --git a/api/catalog_test.go b/api/catalog_test.go index 691c05618d..3a3395ae6e 100644 --- a/api/catalog_test.go +++ b/api/catalog_test.go @@ -157,7 +157,7 @@ func TestCatalog_Registration(t *testing.T) { CheckID: "service:redis1", Name: "Redis health check", Notes: "Script based health check", - Status: "passing", + Status: HealthPassing, ServiceID: "redis1", } diff --git a/command/agent/agent_endpoint_test.go b/command/agent/agent_endpoint_test.go index 2a3b14844b..d11842fc1a 100644 --- a/command/agent/agent_endpoint_test.go +++ b/command/agent/agent_endpoint_test.go @@ -526,9 +526,9 @@ func TestHTTPAgentUpdateCheck(t *testing.T) { } cases := []checkUpdate{ - checkUpdate{"passing", "hello-passing"}, - checkUpdate{"critical", "hello-critical"}, - checkUpdate{"warning", "hello-warning"}, + checkUpdate{structs.HealthPassing, "hello-passing"}, + checkUpdate{structs.HealthCritical, "hello-critical"}, + checkUpdate{structs.HealthWarning, "hello-warning"}, } for _, c := range cases { @@ -564,7 +564,7 @@ func TestHTTPAgentUpdateCheck(t *testing.T) { } update := checkUpdate{ - Status: "passing", + Status: structs.HealthPassing, Output: strings.Repeat("-= bad -=", 5*CheckBufSize), } req.Body = encodeReq(update) @@ -623,7 +623,7 @@ func TestHTTPAgentUpdateCheck(t *testing.T) { } update := checkUpdate{ - Status: "passing", + Status: structs.HealthPassing, } req.Body = encodeReq(update) diff --git a/command/agent/check_test.go b/command/agent/check_test.go index 053eb5148a..e327be32dd 100644 --- a/command/agent/check_test.go +++ b/command/agent/check_test.go @@ -235,25 +235,25 @@ func TestCheckHTTPCritical(t *testing.T) { server := mockHTTPServer(150) fmt.Println(server.URL) - expectHTTPStatus(t, server.URL, "critical") + expectHTTPStatus(t, server.URL, structs.HealthCritical) server.Close() // 2xx - 1 server = mockHTTPServer(199) - expectHTTPStatus(t, server.URL, "critical") + expectHTTPStatus(t, server.URL, structs.HealthCritical) server.Close() // 2xx + 1 server = mockHTTPServer(300) - expectHTTPStatus(t, server.URL, "critical") + expectHTTPStatus(t, server.URL, structs.HealthCritical) server.Close() server = mockHTTPServer(400) - expectHTTPStatus(t, server.URL, "critical") + expectHTTPStatus(t, server.URL, structs.HealthCritical) server.Close() server = mockHTTPServer(500) - expectHTTPStatus(t, server.URL, "critical") + expectHTTPStatus(t, server.URL, structs.HealthCritical) server.Close() } @@ -261,25 +261,25 @@ func TestCheckHTTPPassing(t *testing.T) { var server *httptest.Server server = mockHTTPServer(200) - expectHTTPStatus(t, server.URL, "passing") + expectHTTPStatus(t, server.URL, structs.HealthPassing) server.Close() server = mockHTTPServer(201) - expectHTTPStatus(t, server.URL, "passing") + expectHTTPStatus(t, server.URL, structs.HealthPassing) server.Close() server = mockHTTPServer(250) - expectHTTPStatus(t, server.URL, "passing") + expectHTTPStatus(t, server.URL, structs.HealthPassing) server.Close() server = mockHTTPServer(299) - expectHTTPStatus(t, server.URL, "passing") + expectHTTPStatus(t, server.URL, structs.HealthPassing) server.Close() } func TestCheckHTTPWarning(t *testing.T) { server := mockHTTPServer(429) - expectHTTPStatus(t, server.URL, "warning") + expectHTTPStatus(t, server.URL, structs.HealthWarning) server.Close() } @@ -323,7 +323,7 @@ func TestCheckHTTPTimeout(t *testing.T) { t.Fatalf("should have at least 2 updates %v", mock.updates) } - if mock.state["bar"] != "critical" { + if mock.state["bar"] != structs.HealthCritical { t.Fatalf("should be critical %v", mock.state) } } @@ -397,7 +397,7 @@ func TestCheckTCPCritical(t *testing.T) { ) tcpServer = mockTCPServer(`tcp`) - expectTCPStatus(t, `127.0.0.1:0`, "critical") + expectTCPStatus(t, `127.0.0.1:0`, structs.HealthCritical) tcpServer.Close() } @@ -407,11 +407,11 @@ func TestCheckTCPPassing(t *testing.T) { ) tcpServer = mockTCPServer(`tcp`) - expectTCPStatus(t, tcpServer.Addr().String(), "passing") + expectTCPStatus(t, tcpServer.Addr().String(), structs.HealthPassing) tcpServer.Close() tcpServer = mockTCPServer(`tcp6`) - expectTCPStatus(t, tcpServer.Addr().String(), "passing") + expectTCPStatus(t, tcpServer.Addr().String(), structs.HealthPassing) tcpServer.Close() } @@ -579,27 +579,27 @@ func expectDockerCheckStatus(t *testing.T, dockerClient DockerClient, status str } func TestDockerCheckWhenExecReturnsSuccessExitCode(t *testing.T) { - expectDockerCheckStatus(t, &fakeDockerClientWithNoErrors{}, "passing", "output") + expectDockerCheckStatus(t, &fakeDockerClientWithNoErrors{}, structs.HealthPassing, "output") } func TestDockerCheckWhenExecCreationFails(t *testing.T) { - expectDockerCheckStatus(t, &fakeDockerClientWithCreateExecFailure{}, "critical", "Unable to create Exec, error: Exec Creation Failed") + expectDockerCheckStatus(t, &fakeDockerClientWithCreateExecFailure{}, structs.HealthCritical, "Unable to create Exec, error: Exec Creation Failed") } func TestDockerCheckWhenExitCodeIsNonZero(t *testing.T) { - expectDockerCheckStatus(t, &fakeDockerClientWithExecNonZeroExitCode{}, "critical", "") + expectDockerCheckStatus(t, &fakeDockerClientWithExecNonZeroExitCode{}, structs.HealthCritical, "") } func TestDockerCheckWhenExitCodeIsone(t *testing.T) { - expectDockerCheckStatus(t, &fakeDockerClientWithExecExitCodeOne{}, "warning", "output") + expectDockerCheckStatus(t, &fakeDockerClientWithExecExitCodeOne{}, structs.HealthWarning, "output") } func TestDockerCheckWhenExecStartFails(t *testing.T) { - expectDockerCheckStatus(t, &fakeDockerClientWithStartExecFailure{}, "critical", "Unable to start Exec: Couldn't Start Exec") + expectDockerCheckStatus(t, &fakeDockerClientWithStartExecFailure{}, structs.HealthCritical, "Unable to start Exec: Couldn't Start Exec") } func TestDockerCheckWhenExecInfoFails(t *testing.T) { - expectDockerCheckStatus(t, &fakeDockerClientWithExecInfoErrors{}, "critical", "Unable to inspect Exec: Unable to query exec info") + expectDockerCheckStatus(t, &fakeDockerClientWithExecInfoErrors{}, structs.HealthCritical, "Unable to inspect Exec: Unable to query exec info") } func TestDockerCheckDefaultToSh(t *testing.T) { diff --git a/command/agent/health_endpoint.go b/command/agent/health_endpoint.go index e4a36f6a6a..b252a4126a 100644 --- a/command/agent/health_endpoint.go +++ b/command/agent/health_endpoint.go @@ -1,9 +1,10 @@ package agent import ( - "github.com/hashicorp/consul/consul/structs" "net/http" "strings" + + "github.com/hashicorp/consul/consul/structs" ) func (s *HTTPServer) HealthChecksInState(resp http.ResponseWriter, req *http.Request) (interface{}, error) { @@ -126,7 +127,7 @@ func (s *HTTPServer) HealthServiceNodes(resp http.ResponseWriter, req *http.Requ } // Filter to only passing if specified - if _, ok := params["passing"]; ok { + if _, ok := params[structs.HealthPassing]; ok { out.Nodes = filterNonPassing(out.Nodes) } diff --git a/testutil/README.md b/testutil/README.md index ebbbade5dc..d47e76788d 100644 --- a/testutil/README.md +++ b/testutil/README.md @@ -48,13 +48,13 @@ func TestMain(t *testing.T) { }) // Create a service - srv1.AddService("redis", "passing", []string{"master"}) + srv1.AddService("redis", structs.HealthPassing, []string{"master"}) // Create a service check - srv1.AddCheck("service:redis", "redis", "passing") + srv1.AddCheck("service:redis", "redis", structs.HealthPassing) // Create a node check - srv1.AddCheck("mem", "", "critical") + srv1.AddCheck("mem", "", structs.HealthCritical) // The HTTPAddr field contains the address of the Consul // API on the new test server instance. diff --git a/testutil/server.go b/testutil/server.go index 5574f668ce..8a88a8b1c7 100644 --- a/testutil/server.go +++ b/testutil/server.go @@ -25,6 +25,7 @@ import ( "strings" "sync/atomic" + "github.com/hashicorp/consul/consul/structs" "github.com/hashicorp/go-cleanhttp" ) @@ -444,11 +445,11 @@ func (s *TestServer) AddService(name, status string, tags []string) { s.put("/v1/agent/check/register", payload) switch status { - case "passing": + case structs.HealthPassing: s.put("/v1/agent/check/pass/"+chkName, nil) - case "warning": + case structs.HealthWarning: s.put("/v1/agent/check/warn/"+chkName, nil) - case "critical": + case structs.HealthCritical: s.put("/v1/agent/check/fail/"+chkName, nil) default: s.t.Fatalf("Unrecognized status: %s", status) @@ -472,11 +473,11 @@ func (s *TestServer) AddCheck(name, serviceID, status string) { s.put("/v1/agent/check/register", payload) switch status { - case "passing": + case structs.HealthPassing: s.put("/v1/agent/check/pass/"+name, nil) - case "warning": + case structs.HealthWarning: s.put("/v1/agent/check/warn/"+name, nil) - case "critical": + case structs.HealthCritical: s.put("/v1/agent/check/fail/"+name, nil) default: s.t.Fatalf("Unrecognized status: %s", status) diff --git a/watch/funcs_test.go b/watch/funcs_test.go index 2e0e345a5c..78f93c90a8 100644 --- a/watch/funcs_test.go +++ b/watch/funcs_test.go @@ -6,6 +6,7 @@ import ( "time" consulapi "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/consul/structs" ) var consulAddr string @@ -299,7 +300,7 @@ func TestChecksWatch_State(t *testing.T) { Node: "foobar", CheckID: "foobar", Name: "foobar", - Status: "warning", + Status: structs.HealthWarning, }, } catalog.Register(reg, nil) @@ -363,7 +364,7 @@ func TestChecksWatch_Service(t *testing.T) { Node: "foobar", CheckID: "foobar", Name: "foobar", - Status: "passing", + Status: structs.HealthPassing, ServiceID: "foobar", }, } From f1873c21d7ad2e99686c4d54aed61b270b790b98 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Sat, 23 Apr 2016 16:06:58 -0700 Subject: [PATCH 2/4] consul/ uses structs.Health*, the api uses api.Health* --- api/agent.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/agent.go b/api/agent.go index 1211cc6fd4..24404a3b96 100644 --- a/api/agent.go +++ b/api/agent.go @@ -255,7 +255,7 @@ func (a *Agent) updateTTL(checkID, note, status string) error { // checkUpdate is the payload for a PUT for a check update. type checkUpdate struct { - // Status us one of the structs.Health* states: HealthPassing + // Status us one of the api.Health* states: HealthPassing // ("passing"), HealthWarning ("warning"), or HealthCritical // ("critical"). Status string From 4255a0826d1c7a25a658b4f55e2fd4793e933336 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Sat, 23 Apr 2016 20:18:19 -0700 Subject: [PATCH 3/4] Correct a small typo --- api/agent.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/agent.go b/api/agent.go index 24404a3b96..3df013cc5b 100644 --- a/api/agent.go +++ b/api/agent.go @@ -255,7 +255,7 @@ func (a *Agent) updateTTL(checkID, note, status string) error { // checkUpdate is the payload for a PUT for a check update. type checkUpdate struct { - // Status us one of the api.Health* states: HealthPassing + // Status is one of the api.Health* states: HealthPassing // ("passing"), HealthWarning ("warning"), or HealthCritical // ("critical"). Status string From a3254522fef3e0bdbf06eb49e6d9b012f76ec582 Mon Sep 17 00:00:00 2001 From: Sean Chittenden Date: Sat, 23 Apr 2016 20:18:45 -0700 Subject: [PATCH 4/4] Clean up the test example in README This works without an import cycle and has been `go fmt`'ified --- testutil/README.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/testutil/README.md b/testutil/README.md index d47e76788d..dd953343c8 100644 --- a/testutil/README.md +++ b/testutil/README.md @@ -13,14 +13,16 @@ from Consul's core and API client, meaning it can be easily imported and used in external unit tests for various applications. It works by invoking the Consul CLI, which means it is a requirement to have Consul installed in the `$PATH`. -Following is some example usage: +Following is an example usage: ```go -package main +package my_program import ( - "github.com/hashicorp/consul/testutil" "testing" + + "github.com/hashicorp/consul/consul/structs" + "github.com/hashicorp/consul/testutil" ) func TestMain(t *testing.T) {