multi-index: Add default index if none exists #595
Conversation
k8s-ci-robot
added
area/multi-index
cncf-cla: yes
k8s-ci-robot
added
approved
cmd/krew/cmd/update.go
Outdated
| "failed to add default plugin index in absence of no indexes") | ||
| } | ||
|
|
||
| // ensureDefaultIndexIfNoneExist iterates over all indexes and updates them |
There was a problem hiding this comment.
Should be ensureIndexesUpdated
There was a problem hiding this comment.
Hey just wanted to check in on this, it LGTM except this minor typo in the godoc.
There was a problem hiding this comment.
Ah fixing now. I'll make sure cornelius reviews too.
| ensureIndexListHasDefaultIndex(t, string(test.Krew("index", "list").RunOrFailOutput())) | ||
| } | ||
|
|
||
| func ensureIndexListHasDefaultIndex(t *testing.T, output string) { |
There was a problem hiding this comment.
Seems like you could just pass *ITest here as the param.
There was a problem hiding this comment.
🤔 why? Only testing.T seems sufficient?
For now I prefer we do list exec outside this.
There was a problem hiding this comment.
Ok, I only mentioned since the string(test.Krew("index", "list").RunOrFailOutput()) is being passed to each one. I thought it might be better to put that in the ensureIndexListHasDefaultIndex function rather than relying on the caller to pass the desired output.
There was a problem hiding this comment.
yeah got it now. I think that's fine. this way we can actually see the error from RunOrFail at the correct line number.
|
|
||
| func ensureIndexListHasDefaultIndex(t *testing.T, output string) { | ||
| t.Helper() | ||
| if !regexp.MustCompile(`(?m)^default\b`).MatchString(output) { |
There was a problem hiding this comment.
Nit: isn't it better to put this at the package level so that the regular expression is only parsed once instead of for each time this function is called?
There was a problem hiding this comment.
since it's only part of integration tests, I figured it doesn't matter a whole lot.
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
There was a problem hiding this comment.
/lgtm
/approve
/hold
These tests cover the new behavior quite nicely. My only concern is that nearly all our integration tests use a cached repository to avoid slow network operations. However, most of the added tests clone from remote. We should keep an eye on the test speed. Maybe we can make DefaultIndexURI configurable for tests via an env variable, to avoid the network?
k8s-ci-robot
added
lgtm
When user runs krew {install,update,upgrade} for the first time, if there are
no indexes, default index (with krew-index repo) is now automatically added.
Right now this code path only runs behind multi-index feature gate.
Signed-off-by: Ahmet Alp Balkan <[email protected]>
Signed-off-by: Ahmet Alp Balkan <[email protected]>
k8s-ci-robot
removed
lgtm
|
@corneliusweig please re-lgtm. |
|
/lgtm |
k8s-ci-robot
added
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 |
When user runs krew {install,update,upgrade} for the first time, if there are
no indexes, default index (with krew-index repo) is now automatically added.
Right now this code path only runs behind multi-index feature gate.
Related issue: #566
/assign @chriskim06
/area multi-index