Skip to content
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.

Sign up for GitHub

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

Refactor upgrade machinery to use semver versions #278

Merged
merged 1 commit into from
Jul 20, 2019
Merged

Refactor upgrade machinery to use semver versions #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/krew/cmd/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ Remarks:
fmt.Fprintf(os.Stderr, "Installing plugin: %s\n", plugin.Name)
err := installation.Install(paths, plugin, *forceDownloadFile)
if err == installation.ErrIsAlreadyInstalled {
glog.Warningf("Skipping plugin %s, it is already installed", plugin.Name)
glog.Warningf("Skipping plugin %q, it is already installed", plugin.Name)
continue
}
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion pkg/download/verifier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func (v sha256Verifier) Verify() error {
if bytes.Equal(v.wantedHash, v.Sum(nil)) {
return nil
}
return errors.Errorf("checksum does not match, wantReader: %x, got %x", v.wantedHash, v.Sum(nil))
return errors.Errorf("checksum does not match, want: %x, got %x", v.wantedHash, v.Sum(nil))
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
}

var _ Verifier = trueVerifier{}
Expand Down
49 changes: 26 additions & 23 deletions pkg/installation/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

glog.V(3).Infof("Creating download dir %q", downloadPath)
if err = os.MkdirAll(downloadPath, 0755); err != nil {
return "", errors.Wrapf(err, "could not create download path %q", downloadPath)
Expand All @@ -53,7 +53,7 @@ func downloadAndMove(version, uri string, fos []index.FileOperation, downloadPat
fetcher = download.NewFileFetcher(forceDownloadFile)
}

verifier := download.NewSha256Verifier(version)
verifier := download.NewSha256Verifier(sha256sum)
if err := download.NewDownloader(verifier, fetcher).Get(uri, downloadPath); err != nil {
return "", errors.Wrap(err, "failed to download and verify file")
}
Expand All @@ -64,33 +64,32 @@ func downloadAndMove(version, uri string, fos []index.FileOperation, downloadPat
// to not get the plugin dir in a bad state if it fails during the process.
func Install(p environment.Paths, plugin index.Plugin, forceDownloadFile string) error {
glog.V(2).Infof("Looking for installed versions")
_, ok, err := findInstalledPluginVersion(p.InstallPath(), p.BinPath(), plugin.Name)
if err != nil {
return err
}
if ok {
_, err := receipt.Load(p.PluginInstallReceiptPath(plugin.Name))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

if err == nil {
return ErrIsAlreadyInstalled
} else if !os.IsNotExist(err) {
return errors.Wrap(err, "failed to look up plugin receipt")
}

glog.V(1).Infof("Finding download target for plugin %s", plugin.Name)
version, uri, fos, bin, err := getDownloadTarget(plugin)
version, sha256, uri, fos, bin, err := getDownloadTarget(plugin)
if err != nil {
return err
}

// The actual install should be the last action so that a failure during receipt
// saving does not result in an installed plugin without receipt.
glog.V(3).Infof("Install plugin %s", plugin.Name)
if err := install(plugin.Name, version, uri, bin, p, fos, forceDownloadFile); err != nil {
if err := install(plugin.Name, version, sha256, uri, bin, p, fos, forceDownloadFile); err != nil {
return errors.Wrap(err, "install failed")
}
glog.V(3).Infof("Storing install receipt for plugin %s", plugin.Name)
err = receipt.Store(plugin, p.PluginInstallReceiptPath(plugin.Name))
return errors.Wrap(err, "installation receipt could not be stored, uninstall may fail")
}

func install(plugin, version, uri, bin string, p environment.Paths, fos []index.FileOperation, forceDownloadFile string) error {
dst, err := downloadAndMove(version, uri, fos, filepath.Join(p.DownloadPath(), plugin), p.PluginInstallPath(plugin), forceDownloadFile)
func install(plugin, version, sha256sum, uri, bin string, p environment.Paths, fos []index.FileOperation, forceDownloadFile string) error {
dst, err := downloadAndMove(version, sha256sum, uri, fos, filepath.Join(p.DownloadPath(), plugin), p.PluginInstallPath(plugin), forceDownloadFile)
if err != nil {
return errors.Wrap(err, "failed to download and move during installation")
}
Expand All @@ -113,18 +112,22 @@ func install(plugin, version, uri, bin string, p environment.Paths, fos []index.
// Uninstall will uninstall a plugin.
func Uninstall(p environment.Paths, name string) error {
if name == krewPluginName {
return errors.Errorf("removing krew is not allowed through krew. Please run:\n\t rm -r %s", p.BasePath())
glog.Errorf("Removing krew through krew is not supported.")
if !isWindows() { // assume POSIX-like
glog.Errorf("If you’d like to uninstall krew altogether, run:\n\trm -rf -- %q", p.BasePath())
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
}
return errors.New("self-uninstall not allowed")
}
glog.V(3).Infof("Finding installed version to delete")
version, installed, err := findInstalledPluginVersion(p.InstallPath(), p.BinPath(), name)
if err != nil {
return errors.Wrap(err, "can't uninstall plugin")
}
if !installed {
return ErrIsNotInstalled

if _, err := receipt.Load(p.PluginInstallReceiptPath(name)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, a simpler Lstat would do.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if os.IsNotExist(err) {
return ErrIsNotInstalled
}
return errors.Wrapf(err, "failed to look up install receipt for plugin %q", name)
}

glog.V(1).Infof("Deleting plugin version %s", version)
glog.V(1).Infof("Deleting plugin %s", name)

symlinkPath := filepath.Join(p.BinPath(), pluginNameToBin(name, isWindows()))
glog.V(3).Infof("Unlink %q", symlinkPath)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

glog.V(3).Infof("Deleting plugin receipt %q", PluginInstallReceiptPath)
err = os.Remove(PluginInstallReceiptPath)
return errors.Wrapf(err, "could not remove plugin receipt %q", PluginInstallReceiptPath)
pluginReceiptPath := p.PluginInstallReceiptPath(name)
glog.V(3).Infof("Deleting plugin receipt %q", pluginReceiptPath)
err := os.Remove(pluginReceiptPath)
return errors.Wrapf(err, "could not remove plugin receipt %q", pluginReceiptPath)
}

func createOrUpdateLink(binDir string, binary string, plugin string) error {
Expand Down
35 changes: 24 additions & 11 deletions pkg/installation/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (

"sigs.k8s.io/krew/pkg/environment"
"sigs.k8s.io/krew/pkg/index"
"sigs.k8s.io/krew/pkg/installation/semver"
"sigs.k8s.io/krew/pkg/receipt"

"github.com/golang/glog"
Expand All @@ -29,22 +30,34 @@ import (
// Upgrade will reinstall and delete the old plugin. The operation tries
// to not get the plugin dir in a bad state if it fails during the process.
func Upgrade(p environment.Paths, plugin index.Plugin) error {
oldVersion, ok, err := findInstalledPluginVersion(p.InstallPath(), p.BinPath(), plugin.Name)
installReceipt, err := receipt.Load(p.PluginInstallReceiptPath(plugin.Name))
if err != nil {
return errors.Wrap(err, "could not detect installed plugin oldVersion")
}
if !ok {
return errors.Errorf("can't upgrade plugin %q, it is not installed", plugin.Name)
return errors.Wrapf(err, "failed to load install receipt for plugin %q", plugin.Name)
}

// Check allowed installation
newVersion, uri, fos, binName, err := getDownloadTarget(plugin)
if oldVersion == newVersion {
return ErrIsAlreadyUpgraded
curVersion := installReceipt.Spec.Version
curv, err := semver.Parse(curVersion)
if err != nil {
return errors.Wrapf(err, "failed to parse installed version (%q) plugin %q as semantic version", curVersion, plugin.Name)
}

// Find available installation candidate
newVersion, sha256sum, uri, fos, binName, err := getDownloadTarget(plugin)
if err != nil {
return errors.Wrap(err, "failed to get the current download target")
}
newv, err := semver.Parse(newVersion)
if err != nil {
return errors.Wrapf(err, "failed to parse installation candidate version spec (%q) for plugin %q", newVersion, plugin.Name)
}
glog.V(2).Infof("Comparing versions: current=%s target=%s", curv, newv)

// See if it's a newer version
if !semver.Less(curv, newv) {
glog.V(3).Infof("Plugin does not need upgrade (%s ≥ %s)", curv, newv)
return ErrIsAlreadyUpgraded
}
glog.V(1).Infof("Plugin needs upgrade (%s < %s)", curv, newv)

glog.V(2).Infof("Upgrading install receipt for plugin %s", plugin.Name)
if err = receipt.Store(plugin, p.PluginInstallReceiptPath(plugin.Name)); err != nil {
Expand All @@ -53,13 +66,13 @@ func Upgrade(p environment.Paths, plugin index.Plugin) error {

// Re-Install
glog.V(1).Infof("Installing new version %s", newVersion)
if err := install(plugin.Name, newVersion, uri, binName, p, fos, ""); err != nil {
if err := install(plugin.Name, newVersion, sha256sum, uri, binName, p, fos, ""); err != nil {
return errors.Wrap(err, "failed to install new version")
}

// Clean old installations
glog.V(4).Infof("Starting old version cleanup")
return removePluginVersionFromFS(p, plugin, newVersion, oldVersion)
return removePluginVersionFromFS(p, plugin, newVersion, curVersion)
}

// removePluginVersionFromFS will remove a plugin directly if it not krew.
Expand Down
50 changes: 12 additions & 38 deletions pkg/installation/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
package installation

import (
"os"
"path/filepath"
"strings"

"github.com/golang/glog"
"github.com/pkg/errors"
Expand All @@ -28,31 +26,6 @@ import (
"sigs.k8s.io/krew/pkg/receipt"
)

func findInstalledPluginVersion(installPath, binDir, pluginName string) (name string, installed bool, err error) {
if !index.IsSafePluginName(pluginName) {
return "", false, errors.Errorf("the plugin name %q is not allowed", pluginName)
}
glog.V(3).Infof("Searching for installed versions of %s in %q", pluginName, binDir)
link, err := os.Readlink(filepath.Join(binDir, pluginNameToBin(pluginName, isWindows())))
if os.IsNotExist(err) {
return "", false, nil
} else if err != nil {
return "", false, errors.Wrap(err, "could not read plugin link")
}

if !filepath.IsAbs(link) {
if link, err = filepath.Abs(filepath.Join(binDir, link)); err != nil {
return "", true, errors.Wrapf(err, "failed to get the absolute path for the link of %q", link)
}
}

name, err = pluginVersionFromPath(installPath, link)
if err != nil {
return "", true, errors.Wrap(err, "cloud not parse plugin version")
}
return name, true, nil
}

func pluginVersionFromPath(installPath, pluginPath string) (string, error) {
// plugin path: {install_path}/{plugin_name}/{version}/...
elems, ok := pathutil.IsSubPath(installPath, pluginPath)
Expand All @@ -62,22 +35,23 @@ func pluginVersionFromPath(installPath, pluginPath string) (string, error) {
return elems[1], nil
}

func getPluginVersion(p index.Platform) (version, uri string) {
return strings.ToLower(p.Sha256), p.URI
}

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
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// code smell. More specifically we return all-or-nothing, so ideally this
// should be converted into a struct, like InstallOperation{} contains all
// the data needed to install a plugin.
p, ok, err := index.Spec.GetMatchingPlatform()
if err != nil {
return "", "", nil, p.Bin, errors.Wrap(err, "failed to get matching platforms")
return "", "", "", nil, p.Bin, errors.Wrap(err, "failed to get matching platforms")
}
if !ok {
return "", "", nil, p.Bin, errors.New("no matching platform found")
return "", "", "", nil, p.Bin, errors.New("no matching platform found")
}
version, uri = getPluginVersion(p)
glog.V(4).Infof("Matching plugin version is %s", version)

return version, uri, p.Files, p.Bin, nil
version = index.Spec.Version
uri = p.URI
sha256sum = p.Sha256
ahmetb marked this conversation as resolved.
Show resolved Hide resolved
glog.V(4).Infof("found a matching platform, version=%s checksum=%s", version, sha256sum)
return version, sha256sum, uri, p.Files, p.Bin, nil
}

// ListInstalledPlugins returns a list of all install plugins in a
Expand Down
Loading