cmd: make krew search work with multiple indexes #574
Conversation
k8s-ci-robot
added
approved
k8s-ci-robot
added
the
cncf-cla: yes
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
cmd/krew/cmd/search.go
Outdated
| keys := func(v map[string]pluginEntry) []string { | ||
| out := make([]string, 0, len(v)) | ||
| for k := range v { | ||
| out = append(out, k) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
There's only a single use of this lambda. Have you considered inlining it?
A proper function would also make sense because this does not capture any variables.
cmd/krew/cmd/search.go
Outdated
| var status string | ||
| if _, ok := installed[name]; ok { | ||
| if index := installed[v.p.Name]; index == v.indexName { |
There was a problem hiding this comment.
This is a slight abuse of map[string]string for a map of a pair of properties. Iow, instead of {"plugin": "index"} we should have {"index/plugin": true}.
You already do this for pluginMap, why not for installed too?
There was a problem hiding this comment.
Yeah, that'd work to let me refactor.
cmd/krew/cmd/search.go
Outdated
| return nil | ||
| } | ||
|
|
||
| var rows [][]string | ||
| cols := []string{"NAME", "DESCRIPTION", "INSTALLED"} | ||
| for _, name := range matchNames { | ||
| plugin := pluginMap[name] | ||
| for _, name := range searchResults { |
There was a problem hiding this comment.
| for _, name := range searchResults { | |
| for _, canonicalName := range searchResults { |
would make it clearer that this name includes the index.
| @@ -46,3 +50,52 @@ func TestKrewSearchOne(t *testing.T) { | |||
| t.Errorf("The first match should be krew") | |||
| } | |||
| } | |||
|
|
|||
| func TestKrewSearchMultiIndex(t *testing.T) { | |||
Mostly just refactoring. Added test to make sure search output is alphabetically sorted (even in case of multiple indexes). Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
|
Nice work! |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Mostly just refactoring. Added test to make sure search output is
alphabetically sorted (even in case of multiple indexes).
Related issue: #566, #483
/assign @chriskim06
/assign @corneliusweig
/area multi-index