Refactor upgrade machinery to use semver versions #278

ahmetb · 2019-07-18T20:33:34Z

This change starts using semver version of plugin manifests to:

determine if upgrade is needed
determine if a plugin is installed

by looking at receipts installation directory. It doesn't help with other commands just yet.

This fixes #166, and helps with #277 partially.

Other commands are probably not yet using receipts entirely.

This change starts using semver version of plugin manifests to: - determine if upgrade is needed - determine if a plugin is installed by looking at receipts installation directory. Other commands are probably not yet using receipts entirely. Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

codecov-io · 2019-07-18T20:44:07Z

Codecov Report

Merging #278 into master will decrease coverage by 0.42%.
The diff coverage is 24.48%.

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
- Coverage   55.34%   54.91%   -0.43%     
==========================================
  Files          19       19              
  Lines         880      874       -6     
==========================================
- Hits          487      480       -7     
- Misses        337      341       +4     
+ Partials       56       53       -3

Impacted Files	Coverage Δ
pkg/installation/upgrade.go	`0% <0%> (ø)`	⬆️
pkg/download/verifier.go	`100% <100%> (ø)`	⬆️
pkg/installation/install.go	`34.31% <17.39%> (+2.31%)`	⬆️
pkg/installation/util.go	`41.37% <87.5%> (-7.51%)`	⬇️

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 5969272...aac4bb3. Read the comment docs.

corneliusweig

Well.. this is not pretty, but it looks mostly fine. I have only found minor things.

However, 24% diff coverage is really sad. I can see that these changes are not testable at the moment, but we need to think what to do about it. Ideally, we should add more integration tests, as they are reasonably fast. Then we can follow up with some more refactorings.

pkg/download/verifier.go

corneliusweig · 2019-07-19T21:19:49Z

pkg/installation/install.go

@@ -41,7 +41,7 @@ const (
 	krewPluginName = "krew"
 )

-func downloadAndMove(version, uri string, fos []index.FileOperation, downloadPath, installPath, forceDownloadFile string) (dst string, err error) {
+func downloadAndMove(version, sha256sum, uri string, fos []index.FileOperation, downloadPath, installPath, forceDownloadFile string) (dst string, err error) {


7 arguments? I hope this is a very very private function. Can you add a todo so we get reminded that this needs to be refactored?

The TODO is already at getDownloadTarget (see that method's diff). Once that's fixed we'd be just passing the returned struct here to simplify.

corneliusweig · 2019-07-19T21:24:39Z

pkg/installation/install.go

-		return err
-	}
-	if ok {
+	_, err := receipt.Load(p.PluginInstallReceiptPath(plugin.Name))


As you are discarding the return value, could you also do a simpler Lstat?

I think we need a receipt.Exists. Let's add that.

On a second thought, I'm thinking if an empty file named plugin.yaml exists instead of the actual receipt, despite being harmless today, it might complicate things.

This doesn't have a lot of cost. Can we keep?

corneliusweig · 2019-07-19T21:26:47Z

pkg/installation/util.go

-
-func getDownloadTarget(index index.Plugin) (version, uri string, fos []index.FileOperation, bin string, err error) {
+func getDownloadTarget(index index.Plugin) (version, sha256sum, uri string, fos []index.FileOperation, bin string, err error) {
+	// TODO(ahmetb): We have many return values from this method, indicating


corneliusweig · 2019-07-19T21:29:53Z

pkg/installation/install.go

-	if !installed {
-		return ErrIsNotInstalled
+
+	if _, err := receipt.Load(p.PluginInstallReceiptPath(name)); err != nil {


Again, a simpler Lstat would do.

I'm inclined to keep this as is. If receipt cannot be parsed during uninstallation, that points to a problem and we should probably stop.

In the future we might introduce a field like status.originIndex (or like status.installPath) to determine the installation path. So we'll actually need to use the receipt, soon.

Yeah, that makes sense. Then let's keep it as is.

corneliusweig · 2019-07-19T21:31:21Z

pkg/installation/install.go

@@ -137,10 +140,10 @@ func Uninstall(p environment.Paths, name string) error {
 	if err := os.RemoveAll(pluginInstallPath); err != nil {
 		return errors.Wrapf(err, "could not remove plugin directory %q", pluginInstallPath)
 	}
-	PluginInstallReceiptPath := p.PluginInstallReceiptPath(name)


Woops.. I should have caught this in the review.

pkg/installation/util.go

pkg/installation/install.go

corneliusweig · 2019-07-20T22:07:58Z

/lgtm
/approve

k8s-ci-robot · 2019-07-20T22:08:05Z

[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:

~~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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 18, 2019

k8s-ci-robot requested review from corneliusweig and juanvallejo July 18, 2019 20:33

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 18, 2019

ahmetb mentioned this pull request Jul 18, 2019

pkg/receipt: move under pkg/installation #280

Merged

corneliusweig suggested changes Jul 19, 2019

View reviewed changes

ahmetb mentioned this pull request Jul 20, 2019

pkg/index: validate sha256 values #281

Merged

k8s-ci-robot assigned corneliusweig Jul 20, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2019

k8s-ci-robot merged commit f346eba into kubernetes-sigs:master Jul 20, 2019

corneliusweig mentioned this pull request Oct 21, 2019

Refactor all commands to determine they use plugin receipts #277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor upgrade machinery to use semver versions #278

Refactor upgrade machinery to use semver versions #278

ahmetb commented Jul 18, 2019 •

edited

Loading

codecov-io commented Jul 18, 2019 •

edited

Loading

corneliusweig left a comment •

edited

Loading

corneliusweig Jul 19, 2019

ahmetb Jul 20, 2019

corneliusweig Jul 19, 2019

ahmetb Jul 20, 2019

ahmetb Jul 20, 2019

corneliusweig Jul 19, 2019

corneliusweig Jul 19, 2019

ahmetb Jul 20, 2019

corneliusweig Jul 20, 2019

corneliusweig Jul 19, 2019

corneliusweig commented Jul 20, 2019

k8s-ci-robot commented Jul 20, 2019

Refactor upgrade machinery to use semver versions #278

Refactor upgrade machinery to use semver versions #278

Conversation

ahmetb commented Jul 18, 2019 • edited Loading

codecov-io commented Jul 18, 2019 • edited Loading

Codecov Report

corneliusweig left a comment • edited Loading

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

Choose a reason for hiding this comment

corneliusweig commented Jul 20, 2019

k8s-ci-robot commented Jul 20, 2019

ahmetb commented Jul 18, 2019 •

edited

Loading

codecov-io commented Jul 18, 2019 •

edited

Loading

corneliusweig left a comment •

edited

Loading