-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Show new/updated plugins list after index updates #593
Show new/updated plugins list after index updates #593
Conversation
* Adjusted the showUpdatedPlugins logic to behave well with multi-index mode * Adjusted the "Updated the local copy of plugin index" message for multi-index mode. (We won't show index name if it's 'default' index.) Signed-off-by: Ahmet Alp Balkan <[email protected]>
afd9394
to
f711cfe
Compare
Thanks for taking care of this! It LGTM, do you think it would be worth modifying this integration test to check for upgrade available for plugins from custom indexes or is that not needed? |
Yeah, I got lazy about tests. I will try to update. |
/lgtm |
Signed-off-by: Ahmet Alp Balkan <[email protected]>
So I fixed this bug and pushed a change. But I'm not touching the tests. That's mostly because I noticed we'd duplicate a lot of test code (as adding custom indexes takes a lot of code right now). Ideally we need an utility like:
but since there's a feature gate around the multi-index features, I'm refraining from developing this pattern. |
/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 |
/hold cancel |
mode. (We won't show index name if it's 'default' index.)
Related issue: #566
/area multi-index
/assign @corneliusweig
/assign @chriskim06