Skip to content

Show new/updated plugins list after index updates - #593

Merged
k8s-ci-robot merged 2 commits into
kubernetes-sigs:masterfrom
ahmetb:plugin-update-list
Apr 23, 2020
Merged

Show new/updated plugins list after index updates#593
k8s-ci-robot merged 2 commits into
kubernetes-sigs:masterfrom
ahmetb:plugin-update-list

Conversation

@ahmetb

@ahmetb ahmetb commented Apr 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Member
  • 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.)

Related issue: #566
/area multi-index
/assign @corneliusweig
/assign @chriskim06

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 16, 2020
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/multi-index labels Apr 16, 2020
@chriskim06 chriskim06 mentioned this pull request Apr 16, 2020
Closed
22 tasks done
@ahmetb
Show new/updated plugins list after index updates
f711cfe 
* 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 <ahmetb@google.com>
@ahmetb
ahmetb force-pushed the plugin-update-list branch from afd9394 to f711cfe Compare April 16, 2020 02:03
chriskim06
chriskim06 reviewed Apr 16, 2020
View reviewed changes
Comment thread cmd/krew/cmd/update.go
@chriskim06

chriskim06 commented Apr 16, 2020

Copy link
Copy Markdown
Member

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?

@ahmetb

ahmetb commented Apr 16, 2020

Copy link
Copy Markdown
Member Author

Yeah, I got lazy about tests. I will try to update.

corneliusweig
corneliusweig approved these changes Apr 18, 2020
View reviewed changes
Comment thread cmd/krew/cmd/update.go
@corneliusweig

corneliusweig commented Apr 18, 2020

Copy link
Copy Markdown
Contributor

/lgtm
/approve
/hold
In case you want to follow up in a different PR.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 18, 2020
@ahmetb
Fix bug printing success after error
37f9692 
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2020
@ahmetb

ahmetb commented Apr 19, 2020

Copy link
Copy Markdown
Member Author

nit: this message is shown even if the EnusreUpdated() call failed with an error.

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:

NewTest(t).WithDefaultIndex().WithDefaultIndexMirroredAs("foo").WithCustomIndex(name, src). ...

but since there's a feature gate around the multi-index features, I'm refraining from developing this pattern.

@corneliusweig

corneliusweig commented Apr 23, 2020

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2020
@k8s-ci-robot

k8s-ci-robot commented Apr 23, 2020

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:
  • OWNERS [ahmetb,corneliusweig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@corneliusweig

corneliusweig commented Apr 23, 2020

Copy link
Copy Markdown
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2020
@k8s-ci-robot
k8s-ci-robot merged commit 045262e into kubernetes-sigs:master Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chriskim06 chriskim06 chriskim06 left review comments
@soltysh soltysh Awaiting requested review from soltysh
+1 more reviewer
@corneliusweig corneliusweig corneliusweig approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@corneliusweig corneliusweig

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/multi-index cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ahmetb @chriskim06 @corneliusweig @k8s-ci-robot