From d219e31db879efa7a4a0a2d5e909faa928a889e5 Mon Sep 17 00:00:00 2001 From: Freddy Date: Tue, 16 Jul 2019 14:47:45 -0600 Subject: [PATCH 1/2] Update retries that weren't using retry.R (#6146) --- agent/consul/catalog_endpoint_test.go | 2 +- agent/coordinate_endpoint_test.go | 30 ++++++------- agent/remote_exec_test.go | 59 ++++++++++++++------------ api/acl_test.go | 6 +-- api/catalog_test.go | 22 +++++----- api/lock_test.go | 6 +-- api/operator_autopilot_test.go | 12 +++--- command/connect/proxy/register_test.go | 9 ++-- 8 files changed, 75 insertions(+), 71 deletions(-) diff --git a/agent/consul/catalog_endpoint_test.go b/agent/consul/catalog_endpoint_test.go index 5c11dc9e84..b031391187 100644 --- a/agent/consul/catalog_endpoint_test.go +++ b/agent/consul/catalog_endpoint_test.go @@ -1049,7 +1049,7 @@ func TestCatalog_ListNodes_StaleRead(t *testing.T) { retry.Run(t, func(r *retry.R) { if err := msgpackrpc.CallWithCodec(codec, "Catalog.ListNodes", &args, &out); err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } found := false diff --git a/agent/coordinate_endpoint_test.go b/agent/coordinate_endpoint_test.go index 6e912a2fbd..208b05551e 100644 --- a/agent/coordinate_endpoint_test.go +++ b/agent/coordinate_endpoint_test.go @@ -84,17 +84,17 @@ func TestCoordinate_Nodes(t *testing.T) { retry.Run(t, func(r *retry.R) { obj, err := a.srv.CoordinateNodes(resp, req) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } // Check that coordinates are empty before registering a node coordinates, ok := obj.(structs.Coordinates) if !ok { - t.Fatalf("expected: structs.Coordinates, received: %+v", obj) + r.Fatalf("expected: structs.Coordinates, received: %+v", obj) } if len(coordinates) != 0 { - t.Fatalf("coordinates should be empty, received: %v", coordinates) + r.Fatalf("coordinates should be empty, received: %v", coordinates) } }) @@ -141,17 +141,17 @@ func TestCoordinate_Nodes(t *testing.T) { retry.Run(t, func(r *retry.R) { obj, err := a.srv.CoordinateNodes(resp, req) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } coordinates, ok := obj.(structs.Coordinates) if !ok { - t.Fatalf("expected: structs.Coordinates, received: %+v", obj) + r.Fatalf("expected: structs.Coordinates, received: %+v", obj) } if len(coordinates) != 2 || coordinates[0].Node != "bar" || coordinates[1].Node != "foo" { - t.Fatalf("expected: bar, foo recieved: %v", coordinates) + r.Fatalf("expected: bar, foo recieved: %v", coordinates) } }) // Filter on a nonexistent node segment @@ -160,15 +160,15 @@ func TestCoordinate_Nodes(t *testing.T) { retry.Run(t, func(r *retry.R) { obj, err := a.srv.CoordinateNodes(resp, req) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } coordinates, ok := obj.(structs.Coordinates) if !ok { - t.Fatalf("expected: structs.Coordinates, received: %+v", obj) + r.Fatalf("expected: structs.Coordinates, received: %+v", obj) } if len(coordinates) != 0 { - t.Fatalf("coordinates should be empty, received: %v", coordinates) + r.Fatalf("coordinates should be empty, received: %v", coordinates) } }) // Filter on a real node segment @@ -177,15 +177,15 @@ func TestCoordinate_Nodes(t *testing.T) { retry.Run(t, func(r *retry.R) { obj, err := a.srv.CoordinateNodes(resp, req) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } coordinates, ok := obj.(structs.Coordinates) if !ok { - t.Fatalf("expected: structs.Coordinates, received: %+v", obj) + r.Fatalf("expected: structs.Coordinates, received: %+v", obj) } if len(coordinates) != 1 || coordinates[0].Node != "foo" { - t.Fatalf("expected: foo received: %v", coordinates) + r.Fatalf("expected: foo received: %v", coordinates) } }) // Make sure the empty filter works @@ -194,15 +194,15 @@ func TestCoordinate_Nodes(t *testing.T) { retry.Run(t, func(r *retry.R) { obj, err := a.srv.CoordinateNodes(resp, req) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } coordinates, ok := obj.(structs.Coordinates) if !ok { - t.Fatalf("expected: structs.Coordinates, received: %+v", obj) + r.Fatalf("expected: structs.Coordinates, received: %+v", obj) } if len(coordinates) != 1 || coordinates[0].Node != "bar" { - t.Fatalf("expected: bar received: %v", coordinates) + r.Fatalf("expected: bar received: %v", coordinates) } }) } diff --git a/agent/remote_exec_test.go b/agent/remote_exec_test.go index 60ac949351..74103ad699 100644 --- a/agent/remote_exec_test.go +++ b/agent/remote_exec_test.go @@ -157,7 +157,9 @@ func testRemoteExecGetSpec(t *testing.T, hcl string, token string, shouldSucceed t.Fatalf("err: %v", err) } key := "_rexec/" + event.Session + "/job" - setKV(t, a.Agent, key, buf, token) + if err := setKV(a.Agent, key, buf, token); err != nil { + t.Fatalf("err: %v", err) + } var out remoteExecSpec if shouldSucceed != a.remoteExecGetSpec(event, &out) { @@ -243,26 +245,26 @@ func testRemoteExecWrites(t *testing.T, hcl string, token string, shouldSucceed } key := "_rexec/" + event.Session + "/" + a.Config.NodeName + "/ack" - d := getKV(t, a.Agent, key, token) - if d == nil || d.Session != event.Session { + d, err := getKV(a.Agent, key, token) + if d == nil || d.Session != event.Session || err != nil { t.Fatalf("bad ack: %#v", d) } key = "_rexec/" + event.Session + "/" + a.Config.NodeName + "/out/00000" - d = getKV(t, a.Agent, key, token) - if d == nil || d.Session != event.Session || !bytes.Equal(d.Value, output) { + d, err = getKV(a.Agent, key, token) + if d == nil || d.Session != event.Session || !bytes.Equal(d.Value, output) || err != nil { t.Fatalf("bad output: %#v", d) } key = "_rexec/" + event.Session + "/" + a.Config.NodeName + "/out/0000a" - d = getKV(t, a.Agent, key, token) - if d == nil || d.Session != event.Session || !bytes.Equal(d.Value, output) { + d, err = getKV(a.Agent, key, token) + if d == nil || d.Session != event.Session || !bytes.Equal(d.Value, output) || err != nil { t.Fatalf("bad output: %#v", d) } key = "_rexec/" + event.Session + "/" + a.Config.NodeName + "/exit" - d = getKV(t, a.Agent, key, token) - if d == nil || d.Session != event.Session || string(d.Value) != "1" { + d, err = getKV(a.Agent, key, token) + if d == nil || d.Session != event.Session || string(d.Value) != "1" || err != nil { t.Fatalf("bad output: %#v", d) } } @@ -284,14 +286,16 @@ func testHandleRemoteExec(t *testing.T, command string, expectedSubstring string } buf, err := json.Marshal(spec) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } key := "_rexec/" + event.Session + "/job" - setKV(t, a.Agent, key, buf, "") + if err := setKV(a.Agent, key, buf, ""); err != nil { + r.Fatalf("err: %v", err) + } buf, err = json.Marshal(event) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } msg := &UserEvent{ ID: generateUUID(), @@ -303,24 +307,24 @@ func testHandleRemoteExec(t *testing.T, command string, expectedSubstring string // Verify we have an ack key = "_rexec/" + event.Session + "/" + a.Config.NodeName + "/ack" - d := getKV(t, a.Agent, key, "") - if d == nil || d.Session != event.Session { - t.Fatalf("bad ack: %#v", d) + d, err := getKV(a.Agent, key, "") + if d == nil || d.Session != event.Session || err != nil { + r.Fatalf("bad ack: %#v", d) } // Verify we have output key = "_rexec/" + event.Session + "/" + a.Config.NodeName + "/out/00000" - d = getKV(t, a.Agent, key, "") + d, err = getKV(a.Agent, key, "") if d == nil || d.Session != event.Session || - !bytes.Contains(d.Value, []byte(expectedSubstring)) { - t.Fatalf("bad output: %#v", d) + !bytes.Contains(d.Value, []byte(expectedSubstring)) || err != nil { + r.Fatalf("bad output: %#v", d) } // Verify we have an exit code key = "_rexec/" + event.Session + "/" + a.Config.NodeName + "/exit" - d = getKV(t, a.Agent, key, "") - if d == nil || d.Session != event.Session || string(d.Value) != expectedReturnCode { - t.Fatalf("bad output: %#v", d) + d, err = getKV(a.Agent, key, "") + if d == nil || d.Session != event.Session || string(d.Value) != expectedReturnCode || err != nil { + r.Fatalf("bad output: %#v", d) } }) } @@ -371,7 +375,7 @@ func destroySession(t *testing.T, a *Agent, session string, token string) { } } -func setKV(t *testing.T, a *Agent, key string, val []byte, token string) { +func setKV(a *Agent, key string, val []byte, token string) error { write := structs.KVSRequest{ Datacenter: a.config.Datacenter, Op: api.KVSet, @@ -385,11 +389,12 @@ func setKV(t *testing.T, a *Agent, key string, val []byte, token string) { } var success bool if err := a.RPC("KVS.Apply", &write, &success); err != nil { - t.Fatalf("err: %v", err) + return err } + return nil } -func getKV(t *testing.T, a *Agent, key string, token string) *structs.DirEntry { +func getKV(a *Agent, key string, token string) (*structs.DirEntry, error) { req := structs.KeyRequest{ Datacenter: a.config.Datacenter, Key: key, @@ -399,10 +404,10 @@ func getKV(t *testing.T, a *Agent, key string, token string) *structs.DirEntry { } var out structs.IndexedDirEntries if err := a.RPC("KVS.Get", &req, &out); err != nil { - t.Fatalf("err: %v", err) + return nil, err } if len(out.Entries) > 0 { - return out.Entries[0] + return out.Entries[0], nil } - return nil + return nil, nil } diff --git a/api/acl_test.go b/api/acl_test.go index dff29bba7c..714ed16be2 100644 --- a/api/acl_test.go +++ b/api/acl_test.go @@ -643,10 +643,10 @@ func TestAPI_RulesTranslate_FromToken(t *testing.T) { // This relies on the token upgrade loop running in the background // to assign an accessor - retry.Run(t, func(t *retry.R) { + retry.Run(t, func(r *retry.R) { token, _, err := acl.TokenReadSelf(nil) - require.NoError(t, err) - require.NotEqual(t, "", token.AccessorID) + require.NoError(r, err) + require.NotEqual(r, "", token.AccessorID) accessor = token.AccessorID }) acl.c.config.Token = "root" diff --git a/api/catalog_test.go b/api/catalog_test.go index 4eee62a37a..9aa101b04b 100644 --- a/api/catalog_test.go +++ b/api/catalog_test.go @@ -342,10 +342,10 @@ func TestAPI_CatalogService_SingleTag(t *testing.T) { retry.Run(t, func(r *retry.R) { services, meta, err := catalog.Service("foo", "bar", nil) - require.NoError(t, err) - require.NotEqual(t, meta.LastIndex, 0) - require.Len(t, services, 1) - require.Equal(t, services[0].ServiceID, "foo1") + require.NoError(r, err) + require.NotEqual(r, meta.LastIndex, 0) + require.Len(r, services, 1) + require.Equal(r, services[0].ServiceID, "foo1") }) } @@ -380,23 +380,23 @@ func TestAPI_CatalogService_MultipleTags(t *testing.T) { retry.Run(t, func(r *retry.R) { services, meta, err := catalog.ServiceMultipleTags("foo", []string{"bar"}, nil) - require.NoError(t, err) - require.NotEqual(t, meta.LastIndex, 0) + require.NoError(r, err) + require.NotEqual(r, meta.LastIndex, 0) // Should be 2 services with the `bar` tag - require.Len(t, services, 2) + require.Len(r, services, 2) }) // Test searching with two tags (one result) retry.Run(t, func(r *retry.R) { services, meta, err := catalog.ServiceMultipleTags("foo", []string{"bar", "v2"}, nil) - require.NoError(t, err) - require.NotEqual(t, meta.LastIndex, 0) + require.NoError(r, err) + require.NotEqual(r, meta.LastIndex, 0) // Should be exactly 1 service, named "foo2" - require.Len(t, services, 1) - require.Equal(t, services[0].ServiceID, "foo2") + require.Len(r, services, 1) + require.Equal(r, services[0].ServiceID, "foo2") }) } diff --git a/api/lock_test.go b/api/lock_test.go index a0d9776725..027cb1c28d 100644 --- a/api/lock_test.go +++ b/api/lock_test.go @@ -109,10 +109,10 @@ func TestAPI_LockForceInvalidate(t *testing.T) { // Should work leaderCh, err := lock.Lock(nil) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } if leaderCh == nil { - t.Fatalf("not leader") + r.Fatalf("not leader") } defer lock.Unlock() @@ -127,7 +127,7 @@ func TestAPI_LockForceInvalidate(t *testing.T) { select { case <-leaderCh: case <-time.After(time.Second): - t.Fatalf("should not be leader") + r.Fatalf("should not be leader") } }) } diff --git a/api/operator_autopilot_test.go b/api/operator_autopilot_test.go index 531e3e9d91..0d1c02404f 100644 --- a/api/operator_autopilot_test.go +++ b/api/operator_autopilot_test.go @@ -46,10 +46,10 @@ func TestAPI_OperatorAutopilotCASConfiguration(t *testing.T) { operator := c.Operator() config, err := operator.AutopilotGetConfiguration(nil) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } if !config.CleanupDeadServers { - t.Fatalf("bad: %v", config) + r.Fatalf("bad: %v", config) } // Pass an invalid ModifyIndex @@ -60,10 +60,10 @@ func TestAPI_OperatorAutopilotCASConfiguration(t *testing.T) { } resp, err := operator.AutopilotCASConfiguration(newConf, nil) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } if resp { - t.Fatalf("bad: %v", resp) + r.Fatalf("bad: %v", resp) } } @@ -75,10 +75,10 @@ func TestAPI_OperatorAutopilotCASConfiguration(t *testing.T) { } resp, err := operator.AutopilotCASConfiguration(newConf, nil) if err != nil { - t.Fatalf("err: %v", err) + r.Fatalf("err: %v", err) } if !resp { - t.Fatalf("bad: %v", resp) + r.Fatalf("bad: %v", resp) } } }) diff --git a/command/connect/proxy/register_test.go b/command/connect/proxy/register_test.go index 153b26e10f..e07f1378cd 100644 --- a/command/connect/proxy/register_test.go +++ b/command/connect/proxy/register_test.go @@ -37,7 +37,6 @@ func TestRegisterMonitor_good(t *testing.T) { func TestRegisterMonitor_heartbeat(t *testing.T) { t.Parallel() - require := require.New(t) a := agent.NewTestAgent(t, t.Name(), ``) defer a.Shutdown() @@ -49,11 +48,11 @@ func TestRegisterMonitor_heartbeat(t *testing.T) { retry.Run(t, func(r *retry.R) { // Get the check and verify that it is passing checks, err := client.Agent().Checks() - require.NoError(err) - require.Contains(checks, m.checkID()) - require.Equal("passing", checks[m.checkID()].Status) + require.NoError(r, err) + require.Contains(r, checks, m.checkID()) + require.Equal(r, "passing", checks[m.checkID()].Status) // Purposely fail the TTL check, verify it becomes healthy again - require.NoError(client.Agent().FailTTL(m.checkID(), "")) + require.NoError(r, client.Agent().FailTTL(m.checkID(), "")) }) retry.Run(t, func(r *retry.R) { From 95dbb7f2f1b9fc3528a16335201e2324f1b388bd Mon Sep 17 00:00:00 2001 From: Alvin Huang Date: Tue, 16 Jul 2019 18:52:24 -0400 Subject: [PATCH 2/2] add lint-consul-retry tool (#6139) * add lint-consul-retry tool * lint consul retry for forks too --- .circleci/config.yml | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 98ce9c5d8d..e9a9d56209 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -23,6 +23,14 @@ references: S3_ARTIFACT_BUCKET: consul-dev-artifacts jobs: + # lint consul tests + lint-consul-retry: + docker: + - image: *GOLANG_IMAGE + steps: + - checkout + - run: go get -u github.com/hashicorp/lint-consul-retry && lint-consul-retry + # Runs go fmt and go vet go-fmt-and-vet: docker: @@ -445,7 +453,10 @@ workflows: version: 2 build-distros: jobs: - - go-fmt-and-vet + - lint-consul-retry + - go-fmt-and-vet: + requires: + - lint-consul-retry - build-386: &require-go-fmt-vet requires: - go-fmt-and-vet