Show updated plugin manifests and available upgrades #457

artmello · 2020-01-04T00:29:32Z

Closes #443

k8s-ci-robot · 2020-01-04T00:29:40Z

Welcome @artmello!

It looks like this is your first PR to kubernetes-sigs/krew 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

corneliusweig · 2020-01-04T13:51:35Z

First of all, thanks for taking a shot at this!
The function ensureIndexUpdated is actually called in three places: install, update, and upgrade. I'm not sure if it really makes sense to show this for each of these cases.
Also, I wonder if we can get away without scanning the manifest index each time. We could also use git diff --name-only to restrict the number of scanned manifests. WDYT?

artmello · 2020-01-05T13:38:38Z

Hi @corneliusweig, thanks for taking a look on this!
On the original issue, it was mentioned that for other commands (i.e install) we could have a short version of the updated plugins output. Maybe we can have either a verbose/quiet option to control when that is displayed or not. WDYT?

I agree that we should avoid scanning index when not necessary. I tried to implement suggested approach using diff --name-only let me know if it does look any better.

This is current output:

$ cd -
~/.krew/index

$ git reset --hard dc075b6355c02e30aa0a43bebf4587bab2eafb9d                                                                                     HEAD is now at dc075b6 update neat to v1.1.0 (#399)

$ cd -
~/code/krew

$ ./out/bin/krew-darwin_amd64 update
Updated the local copy of plugin index.
  New plugins available:
    * tree v0.2.0
  The following plugins have new version:
    * pod-dive v0.1.3 -> v0.1.4 (!)
    * fleet v0.1.3 -> v0.1.4
    * open-svc v2.2.0 -> v2.3.0
    * view-allocations v0.6.0 -> v0.6.2
    * view-serviceaccount-kubeconfig v2.0.3 -> v2.1.1

$ ./out/bin/krew-darwin_amd64 update
Updated the local copy of plugin index.

ahmetb · 2020-01-05T18:26:49Z

For now let’s just show the same output for all (install/update/upgrade). I initially proposed we only show a summary (items combined into single line). Maybe it’s not worth it. Let’s get a feel for it.

aimbot31 · 2020-01-14T09:37:36Z

Hello, would like to help if it's possible, still ongoing ?

corneliusweig

Sorry for my response time. I must have lost the notification and only stumbled over this today.

You did a great job integrating git into this logic. But I think you can use git even more to simplify the logic (see my inline comment).

In general, I like the index summary and would love to see that merged at some point. In order to get there fast, please add tests as soon as you can. This will also force you to structure the code in a testable way.

Lastly, I think we should switch to a oneline report for starters (maybe one line per added/updated but not more :) ). Otherwise we take up too much space and people will hate the feature, because they don't see what is most relevant to them anymore.

corneliusweig · 2020-01-14T21:40:48Z

internal/gitutil/git.go

+	return modifiedFiles, nil
+}
+
+func exec(pwd string, args ...string) (string, error) {


Can you add an exec and execOut variant, so that the out return argument does not have to be ignored in so many places?

corneliusweig · 2020-01-14T21:46:29Z

internal/gitutil/git.go

+
+	var modifiedFiles []string
+	for _, f := range strings.Split(output, "\n") {
+		if len(f) > 0 {


Simpler: if f == "" {

corneliusweig · 2020-01-14T21:48:50Z

internal/gitutil/git.go

+	}
+
+	var modifiedFiles []string
+	for _, f := range strings.Split(output, "\n") {


The only empty line is the last one, right? Then you can do this simpler as

modifiedFiles := strings.Split(strings.Trim(output), "\n")

corneliusweig · 2020-01-14T22:10:57Z

internal/gitutil/git.go

@@ -69,7 +70,27 @@ func EnsureUpdated(uri, destinationPath string) error {
 	return updateAndCleanUntracked(destinationPath)
 }

-func exec(pwd string, args ...string) error {
+// ListModifiedFiles will fetch origin and list modified files
+func ListModifiedFiles(uri, destinationPath string) ([]string, error) {


ListModifiedFiles is a slight misnomer, because no files are actually modified. What about ListUpstreamChanges?

corneliusweig · 2020-01-14T22:12:46Z

cmd/krew/cmd/update.go

+	m := make(map[string]string, len(names))
+	for _, n := range names {
+		plugin, err := indexscanner.LoadPluginByName(paths.IndexPluginsPath(), n)
+		if err != nil {


You should only continue for os.IsNoExist(err). All other errors should abort the function.

corneliusweig · 2020-01-14T22:19:16Z

cmd/krew/cmd/update.go

+		if !ok {
+			newPluginList = append(newPluginList, newPlugin{
+				name:    name,
+				version: version,
+			})
+			continue
+		}


This could also be a

newVersionPlugin{ ..., oldVersion: "", installed: false, }

That could simplify things a little. WDYT?

corneliusweig · 2020-01-14T22:21:57Z

cmd/krew/cmd/update.go

+	if len(newPluginList) > 0 {
+		fmt.Fprintln(os.Stderr, "  New plugins available:")
+		for _, np := range newPluginList {
+			fmt.Fprintf(os.Stderr, "    * %s %s\n", np.name, np.version)


I think a TabWriter would produce more legible output here.

But as far as I understand, a one-line report would be preferable anyways.

cmd/krew/cmd/update.go

corneliusweig · 2020-01-14T22:26:22Z

cmd/krew/cmd/update.go

+	}
+
+	var oldMap map[string]string
+	if len(updatedPlugins) > 0 {


If len(updatedPlugins) == 0, you can return early here, because this means the index is already up-to-date.

corneliusweig · 2020-01-14T22:36:57Z

internal/gitutil/git.go

+		return []string{}, errors.Wrapf(err, "fetch index at %q failed", destinationPath)
+	}
+
+	output, err := exec(destinationPath, "diff", "--name-only", "@{upstream}", "--", ".")


Sorry if I come up with those ideas bit by bit. But you can also use git diff --name-status which will add a mark each change as modified/added/deleted...

You can use that to further simplify the logic you need to build the report. I suggest that you play around with a toy git repo to see how this works. Try to offload as much as possible to git :)

corneliusweig · 2020-01-14T22:45:35Z

Hi @aimbot31, only @artmello can answer that :).

artmello · 2020-01-15T14:31:16Z

Hey @corneliusweig thanks a lot for reviewing it. I will work on your comments and let you know when it is good for another round.

Hey @aimbot31, thanks for the interest. I plan to work on Cornelius comments and on adding tests (as mentioned this may need some refactor). Not sure how we could split it, but let me know if you have any suggestion :)

ahmetb · 2020-01-16T01:11:43Z

cmd/krew/cmd/update.go

+func retrieveInstalledPluginMap() (map[string]bool, error) {
+	plugins, err := installation.ListInstalledPlugins(paths.InstallReceiptsPath())
+	if err != nil {
+		return map[string]bool{}, err


you can use nil here.
consider wrapping err. (e.g. "failed during listing installed plugins")

ahmetb · 2020-01-16T01:12:51Z

cmd/krew/cmd/update.go

+	return m
+}
+
+func retrieveInstalledPluginMap() (map[string]bool, error) {


why's the return type a map here?
you only need to just return names, i.e. []string?

ahmetb · 2020-01-16T01:14:19Z

cmd/krew/cmd/update.go

+
+func showUpdatedPlugins(newPluginList []newPlugin, newVersionList []newVersionPlugin) {
+	if len(newPluginList) > 0 {
+		fmt.Fprintln(os.Stderr, "  New plugins available:")


it would be better if you just accept out io.Writer here, so that you don't repeat os.Stderr everywhere.

ahmetb · 2020-01-16T01:16:20Z

cmd/krew/cmd/update.go

+	return plugins, nil
+}
+
+func retrievePluginNameVersionMap(names []string) map[string]string {


I'm not convinced this should be map[string]string.
It's not great for extensibility or readability.

Feel free to either,

just create a type pluginInfo struct {name, version string }, or

just return the []*Plugin objects.

actually it seems like you have a type newPlugin struct, so let's follow suit here. either create a new type or find a way of reusing.

ahmetb · 2020-01-16T01:17:49Z

cmd/krew/cmd/update.go

+	sort.Slice(newPluginList, func(i, j int) bool {
+		return newPluginList[i].name < newPluginList[j].name
+	})
+
+	sort.Slice(newVersionList, func(i, j int) bool {
+		if newVersionList[i].installed && !newVersionList[j].installed {
+			return true
+		}
+
+		if !newVersionList[i].installed && newVersionList[j].installed {
+			return false
+		}
+
+		return newVersionList[i].name < newVersionList[j].name
+	})


pro-tip:

if you don't do maps (which I don't see a reason why you'd need maps here), you don't need to worry about sorting at all.

all the methods we have already are returning *index.Plugin types sorted by name. :)

ahmetb · 2020-01-16T01:18:49Z

cmd/krew/cmd/update.go

+		return newVersionList[i].name < newVersionList[j].name
+	})
+
+	return newPluginList, newVersionList


shout-out to my comment above, this looks odd.

just create an internal type (struct) to carry these.

ahmetb · 2020-01-16T01:19:53Z

cmd/krew/cmd/update.go

+	var newPluginList []newPlugin
+	var newVersionList []newVersionPlugin


something I noticed throughout your coding style is how you prefix things with their types (e.g. xxxMap xxxList). let's avoid that. :)

you can simply use newPlugins, updatedPlugins here.

ahmetb · 2020-01-16T01:21:01Z

cmd/krew/cmd/update.go

+	version string
+}
+
+type newVersionPlugin struct {


rename: updatedPlugin

ahmetb · 2020-01-16T01:24:39Z

cmd/krew/cmd/update.go

 	"sigs.k8s.io/krew/pkg/constants"
 )

+type newPlugin struct {


I feel like you can simply have 1 type to carry out everything

type pluginInfo struct { name, version string }

If you simply return []pluginInfo from everywhere, when the time comes to printing, just diff two []pluginInfo slices to find the "new" and "updated" plugins.

Correct me if I'm wrong but this can even help reduce some of the code.

internal/gitutil/git.go

artmello · 2020-01-21T05:02:55Z

@ahmetb / @corneliusweig

Thanks a lot for the detailed review. I tried to address all your comments, please, let me know if I missed some.
I reverted gitutil changes and used suggested approach of executing a diff on indexes from pre/pos update. Also I moved to use a less verbose output as suggested.

Here is a sample output:

$ cd -
~/.krew/index

$ git reset --hard dc075b6355c02e30aa0a43bebf4587bab2eafb9d
HEAD is now at dc075b6 update neat to v1.1.0 (#399)

$ cd -
~/dump/krew

$ ./out/bin/krew-darwin_amd64 update
Updated the local copy of plugin index.
  New plugins available: advise-psp v1.4.6, deprecations v0.2.4, duck v0.0.1, net-forward v1.0.3, profefe v0.9.0, resource-snapshot v0.1.2, spy v0.3.1, tree v0.3.1
  Upgrades available: pod-dive v0.1.3 -> v0.1.4

ahmetb · 2020-01-21T06:13:30Z

Can I just say: I LOVEEEEE how small this PR is.

I have only 2 concerns atm:

when first ever “update” is done, does it print everything as new (ideally should not)
it seems like the indentation in the output isnt that useful if the plugin list is gonna just flow over to the next line. I wonder if we can do something like print max 4-5 on each line and just add \n\t after each 5?

I am on the fence about whether we should add integration tests for this? Probably not in this PR since it’s not critical functionality.

ahmetb · 2020-01-23T18:38:42Z

Also it seems like there are a couple lint and test errors.

ahmetb · 2020-01-25T21:02:08Z

@artmello pinging if you've still got time to pursue. it seems like we're pretty close :)

- Do not raise an error if no index was found before an update - Do not print updated plugins output if no index was found before an update - Split output lines on chuncks of 5 plugins - Fix complains from linter

codecov-io · 2020-01-26T12:27:47Z

Codecov Report

Merging #457 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #457   +/-   ##
=======================================
  Coverage   56.52%   56.52%           
=======================================
  Files          19       19           
  Lines         927      927           
=======================================
  Hits          524      524           
  Misses        350      350           
  Partials       53       53

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2bb438...93529ee. Read the comment docs.

artmello · 2020-01-26T12:39:04Z

Hi @ahmetb, once again thanks for reviewing it and sorry for the sparse answers. I believe all your last comments are addressed with latest commits. Output has changed, now we are splitting plugins list on chuncks of 5.

$ cd -
~/.krew/index

$ git reset --hard dc075b6355c02e30aa0a43bebf4587bab2eafb9d
HEAD is now at dc075b6 update neat to v1.1.0 (#399)

$ cd -
~/dump/krew

$ ./out/bin/krew-darwin_amd64 update
Updated the local copy of plugin index.
  New plugins available: advise-psp v1.4.6, deprecations v0.2.4, duck v0.0.1, net-forward v1.0.3, profefe v0.9.0
	resource-snapshot v0.1.2, spy v0.3.3, tree v0.4.0

ahmetb · 2020-01-26T17:41:01Z

/lgtm
/approve
Thanks a bunch. This was cool work.

I might write a few integration tests for this.

k8s-ci-robot · 2020-01-26T17:41:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, artmello

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:

~~OWNERS~~ [ahmetb]

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

corneliusweig · 2020-01-26T20:59:28Z

This is great! Thanks for your contribution @artmello

artmello · 2020-01-26T21:00:41Z

Thanks a lot @ahmetb and @corneliusweig for all the help on it. Looking forward to contribute again soon :)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 4, 2020

k8s-ci-robot requested review from corneliusweig and soltysh January 4, 2020 00:29

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 4, 2020

Show information about updated plugin during update command

9e65c53

artmello force-pushed the show_updated_plugins branch from 27f50e0 to 9e65c53 Compare January 5, 2020 13:31

corneliusweig suggested changes Jan 14, 2020

View reviewed changes

ahmetb changed the title ~~Show information about updated plugin during update command~~ Show updated plugin manifests and available upgrades Jan 16, 2020

ahmetb reviewed Jan 16, 2020

View reviewed changes

cmd/krew/cmd/update.go Outdated

version string

}

type newVersionPlugin struct {

Copy link

Member

ahmetb Jan 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename: updatedPlugin

ahmetb reviewed Jan 16, 2020

View reviewed changes

internal/gitutil/git.go Outdated Show resolved Hide resolved

Merge branch 'master' into show_updated_plugins

a880a40

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 21, 2020

Refactor code following suggestions from review

d2bb438

artmello force-pushed the show_updated_plugins branch from bea76ce to d2bb438 Compare January 21, 2020 04:58

artmello added 2 commits January 26, 2020 08:37

Merge branch 'master' into show_updated_plugins

1c61c37

Improve code following suggestions from review

93529ee

- Do not raise an error if no index was found before an update - Do not print updated plugins output if no index was found before an update - Split output lines on chuncks of 5 plugins - Fix complains from linter

k8s-ci-robot assigned ahmetb Jan 26, 2020

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2020

k8s-ci-robot merged commit 99b9cdf into kubernetes-sigs:master Jan 26, 2020

This was referenced Jan 27, 2020

updated plugin list is should be wrapped more aggressively #472

Closed

Add integration tests about printing new plugins/available upgrades #474

Closed

Proposal: occassionally show available upgrades for Krew #480

Closed

		var newPluginList []newPlugin
		var newVersionList []newVersionPlugin

Show updated plugin manifests and available upgrades #457

Show updated plugin manifests and available upgrades #457

Conversation

artmello commented Jan 4, 2020

k8s-ci-robot commented Jan 4, 2020

corneliusweig commented Jan 4, 2020

artmello commented Jan 5, 2020

ahmetb commented Jan 5, 2020

aimbot31 commented Jan 14, 2020 • edited Loading

corneliusweig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corneliusweig commented Jan 14, 2020

artmello commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artmello commented Jan 21, 2020

ahmetb commented Jan 21, 2020

ahmetb commented Jan 23, 2020

ahmetb commented Jan 25, 2020

codecov-io commented Jan 26, 2020 • edited Loading

Codecov Report

artmello commented Jan 26, 2020

ahmetb commented Jan 26, 2020

k8s-ci-robot commented Jan 26, 2020

corneliusweig commented Jan 26, 2020

artmello commented Jan 26, 2020

aimbot31 commented Jan 14, 2020 •

edited

Loading

codecov-io commented Jan 26, 2020 •

edited

Loading