From 3319fb5822bea338d4723ecaed702f6b377fb220 Mon Sep 17 00:00:00 2001 From: Chelsea Holland Komlo Date: Mon, 14 Aug 2017 13:34:31 +0000 Subject: [PATCH] fixups from code review --- api/contexts/contexts.go | 2 +- api/search.go | 10 ++-- command/agent/search_endpoint_test.go | 22 ++++---- command/alloc_status.go | 2 +- nomad/search_endpoint.go | 75 +++++++++++++-------------- nomad/structs/structs.go | 2 +- 6 files changed, 55 insertions(+), 58 deletions(-) diff --git a/api/contexts/contexts.go b/api/contexts/contexts.go index 17200ef94..f505ec561 100644 --- a/api/contexts/contexts.go +++ b/api/contexts/contexts.go @@ -1,6 +1,6 @@ package contexts -// Context is a type which is searchable via a unique identifier. +// Context defines the scope in which a search for Nomad object operates type Context string const ( diff --git a/api/search.go b/api/search.go index 964dd6932..376c12844 100644 --- a/api/search.go +++ b/api/search.go @@ -1,7 +1,7 @@ package api import ( - c "github.com/hashicorp/nomad/api/contexts" + "github.com/hashicorp/nomad/api/contexts" ) type Search struct { @@ -15,7 +15,7 @@ func (c *Client) Search() *Search { // PrefixSearch returns a list of matches for a particular context and prefix. If a // context is not specified, matches for all contexts are returned. -func (s *Search) PrefixSearch(prefix string, context c.Context) (*SearchResponse, error) { +func (s *Search) PrefixSearch(prefix string, context contexts.Context) (*SearchResponse, error) { var resp SearchResponse req := &SearchRequest{Prefix: prefix, Context: context} @@ -29,11 +29,11 @@ func (s *Search) PrefixSearch(prefix string, context c.Context) (*SearchResponse type SearchRequest struct { Prefix string - Context c.Context + Context contexts.Context } type SearchResponse struct { - Matches map[c.Context][]string - Truncations map[c.Context]bool + Matches map[contexts.Context][]string + Truncations map[contexts.Context]bool QueryMeta } diff --git a/command/agent/search_endpoint_test.go b/command/agent/search_endpoint_test.go index fa430a9d4..468959766 100644 --- a/command/agent/search_endpoint_test.go +++ b/command/agent/search_endpoint_test.go @@ -7,11 +7,11 @@ import ( "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" - a "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/assert" ) func TestHTTP_SearchWithIllegalMethod(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) t.Parallel() httpTest(t, nil, func(s *TestAgent) { req, err := http.NewRequest("DELETE", "/v1/search", nil) @@ -24,7 +24,7 @@ func TestHTTP_SearchWithIllegalMethod(t *testing.T) { } func createJobForTest(jobID string, s *TestAgent, t *testing.T) { - assert := a.New(t) + assert := assert.New(t) job := mock.Job() job.ID = jobID @@ -36,7 +36,7 @@ func createJobForTest(jobID string, s *TestAgent, t *testing.T) { } func TestHTTP_Search_POST(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) testJob := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89706" testJobPrefix := "aaaaaaaa-e8f7-fd38" @@ -68,7 +68,7 @@ func TestHTTP_Search_POST(t *testing.T) { } func TestHTTP_Search_PUT(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) testJob := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89706" testJobPrefix := "aaaaaaaa-e8f7-fd38" @@ -100,7 +100,7 @@ func TestHTTP_Search_PUT(t *testing.T) { } func TestHTTP_Search_MultipleJobs(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) testJobA := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89706" testJobB := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89707" @@ -140,7 +140,7 @@ func TestHTTP_Search_MultipleJobs(t *testing.T) { } func TestHTTP_Search_Evaluation(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) t.Parallel() httpTest(t, nil, func(s *TestAgent) { @@ -176,7 +176,7 @@ func TestHTTP_Search_Evaluation(t *testing.T) { } func TestHTTP_Search_Allocations(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) t.Parallel() httpTest(t, nil, func(s *TestAgent) { @@ -209,7 +209,7 @@ func TestHTTP_Search_Allocations(t *testing.T) { } func TestHTTP_Search_Nodes(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) t.Parallel() httpTest(t, nil, func(s *TestAgent) { @@ -242,7 +242,7 @@ func TestHTTP_Search_Nodes(t *testing.T) { } func TestHTTP_Search_NoJob(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) t.Parallel() httpTest(t, nil, func(s *TestAgent) { @@ -265,7 +265,7 @@ func TestHTTP_Search_NoJob(t *testing.T) { } func TestHTTP_Search_AllContext(t *testing.T) { - assert := a.New(t) + assert := assert.New(t) testJobID := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89706" testJobPrefix := "aaaaaaaa-e8f7-fd38" diff --git a/command/alloc_status.go b/command/alloc_status.go index 1265151ad..e566d231a 100644 --- a/command/alloc_status.go +++ b/command/alloc_status.go @@ -8,7 +8,7 @@ import ( "strings" "time" - humanize "github.com/dustin/go-humanize" + "github.com/dustin/go-humanize" "github.com/mitchellh/colorstring" "github.com/hashicorp/nomad/api" diff --git a/nomad/search_endpoint.go b/nomad/search_endpoint.go index 6bbf37481..01efff776 100644 --- a/nomad/search_endpoint.go +++ b/nomad/search_endpoint.go @@ -2,22 +2,23 @@ package nomad import ( "fmt" + "strings" memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/nomad/nomad/state" - st "github.com/hashicorp/nomad/nomad/structs" + "github.com/hashicorp/nomad/nomad/structs" ) -// truncateLimit is the maximum number of matches that will be returned for a -// prefix for a specific context const ( + // truncateLimit is the maximum number of matches that will be returned for a + // prefix for a specific context truncateLimit = 20 ) -// allContexts are the available contexts which are searched to find matches -// for a given prefix var ( - allContexts = []st.Context{st.Allocs, st.Jobs, st.Nodes, st.Evals} + // allContexts are the available contexts which are searched to find matches + // for a given prefix + allContexts = []structs.Context{structs.Allocs, structs.Jobs, structs.Nodes, structs.Evals} ) // Search endpoint is used to look up matches for a given prefix and context @@ -25,10 +26,6 @@ type Search struct { srv *Server } -func isSubset(prefix, id string) bool { - return id[0:len(prefix)] == prefix -} - // getMatches extracts matches for an iterator, and returns a list of ids for // these matches. func (s *Search) getMatches(iter memdb.ResultIterator, prefix string) ([]string, bool) { @@ -42,20 +39,20 @@ func (s *Search) getMatches(iter memdb.ResultIterator, prefix string) ([]string, var id string switch t := raw.(type) { - case *st.Job: - id = raw.(*st.Job).ID - case *st.Evaluation: - id = raw.(*st.Evaluation).ID - case *st.Allocation: - id = raw.(*st.Allocation).ID - case *st.Node: - id = raw.(*st.Node).ID + case *structs.Job: + id = raw.(*structs.Job).ID + case *structs.Evaluation: + id = raw.(*structs.Evaluation).ID + case *structs.Allocation: + id = raw.(*structs.Allocation).ID + case *structs.Node: + id = raw.(*structs.Node).ID default: s.srv.logger.Printf("[ERR] nomad.resources: unexpected type for resources context: %T", t) continue } - if !isSubset(prefix, id) { + if !strings.HasPrefix(id, prefix) { continue } @@ -67,15 +64,15 @@ func (s *Search) getMatches(iter memdb.ResultIterator, prefix string) ([]string, // getResourceIter takes a context and returns a memdb iterator specific to // that context -func getResourceIter(context st.Context, prefix string, ws memdb.WatchSet, state *state.StateStore) (memdb.ResultIterator, error) { +func getResourceIter(context structs.Context, prefix string, ws memdb.WatchSet, state *state.StateStore) (memdb.ResultIterator, error) { switch context { - case st.Jobs: + case structs.Jobs: return state.JobsByIDPrefix(ws, prefix) - case st.Evals: + case structs.Evals: return state.EvalsByIDPrefix(ws, prefix) - case st.Allocs: + case structs.Allocs: return state.AllocsByIDPrefix(ws, prefix) - case st.Nodes: + case structs.Nodes: return state.NodesByIDPrefix(ws, prefix) default: return nil, fmt.Errorf("context must be one of %v; got %q", allContexts, context) @@ -84,8 +81,8 @@ func getResourceIter(context st.Context, prefix string, ws memdb.WatchSet, state // If the length of a prefix is odd, return a subset to the last even character // This only applies to UUIDs, jobs are excluded -func roundUUIDDownIfOdd(prefix string, context st.Context) string { - if context == st.Jobs { +func roundUUIDDownIfOdd(prefix string, context structs.Context) string { + if context == structs.Jobs { return prefix } @@ -98,30 +95,30 @@ func roundUUIDDownIfOdd(prefix string, context st.Context) string { // PrefixSearch is used to list matches for a given prefix, and returns // matching jobs, evaluations, allocations, and/or nodes. -func (s *Search) PrefixSearch(args *st.SearchRequest, - reply *st.SearchResponse) error { - reply.Matches = make(map[st.Context][]string) - reply.Truncations = make(map[st.Context]bool) +func (s *Search) PrefixSearch(args *structs.SearchRequest, + reply *structs.SearchResponse) error { + reply.Matches = make(map[structs.Context][]string) + reply.Truncations = make(map[structs.Context]bool) // Setup the blocking query opts := blockingOptions{ queryMeta: &reply.QueryMeta, - queryOpts: &st.QueryOptions{}, + queryOpts: &structs.QueryOptions{}, run: func(ws memdb.WatchSet, state *state.StateStore) error { - iters := make(map[st.Context]memdb.ResultIterator) + iters := make(map[structs.Context]memdb.ResultIterator) contexts := allContexts - if args.Context != st.All { - contexts = []st.Context{args.Context} + if args.Context != structs.All { + contexts = []structs.Context{args.Context} } - for _, e := range contexts { - iter, err := getResourceIter(e, roundUUIDDownIfOdd(args.Prefix, args.Context), ws, state) + for _, ctx := range contexts { + iter, err := getResourceIter(ctx, roundUUIDDownIfOdd(args.Prefix, args.Context), ws, state) if err != nil { return err } - iters[e] = iter + iters[ctx] = iter } // Return matches for the given prefix @@ -134,8 +131,8 @@ func (s *Search) PrefixSearch(args *st.SearchRequest, // Set the index for the context. If the context has been specified, it // will be used as the index of the response. Otherwise, the // maximum index from all resources will be used. - for _, e := range contexts { - index, err := state.Index(string(e)) + for _, ctx := range contexts { + index, err := state.Index(string(ctx)) if err != nil { return err } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 463314897..b2743b9c6 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -89,7 +89,7 @@ const ( GetterModeDir = "dir" ) -// Context is a type which is searchable via a unique identifier. +// Context defines the scope in which a search for Nomad object operates type Context string const (