From 0ee10abb6caf979c5e2efe909a0841885ed65405 Mon Sep 17 00:00:00 2001
From: Ahmet Alp Balkan <ahmetb@google.com>
Date: Wed, 4 Dec 2019 19:48:28 -0800
Subject: [PATCH] skip/fail upgrade if no longer matches a platform

It turns out when upgrade process looks to a plugin manifest that exists
but does no longer have a matching Platform spec, we were returning nil error,
which was surfacing as "upgraded" (see #423).

This addresses the problem by returning an error in this case. However, the
'upgrade' command fails with this error if a plugin list is given to the cmd.

If 'krew upgrade' is called without args (i.e. upgrade all plugins), ensuring
this doesn't fail the cmd, but prints warnings (just like current 'Skipping'
behavior this cmd has).

I think this 'skipping with a warning' behavior is justified because it can
make upgrade command entirely malfunctional when a plugin manifest is updated
breaking the upgrades for that plugin.

(There are still some cases that make 'upgrade' command fatally fail, such as
an installed plugin gone from manifest, which we should address separately.)

Adding integration tests to capture both cases.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
---
 cmd/krew/cmd/upgrade.go          | 15 ++++++++++++++-
 integration_test/upgrade_test.go | 26 ++++++++++++++++++++++++++
 internal/installation/upgrade.go |  3 ++-
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/cmd/krew/cmd/upgrade.go b/cmd/krew/cmd/upgrade.go
index 8c7bfc01..09119546 100644
--- a/cmd/krew/cmd/upgrade.go
+++ b/cmd/krew/cmd/upgrade.go
@@ -41,9 +41,11 @@ To only upgrade single plugins provide them as arguments:
 kubectl krew upgrade foo bar"`,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			var ignoreUpgraded bool
+			var skipErrors bool
+
 			var pluginNames []string
-			// Upgrade all plugins.
 			if len(args) == 0 {
+				// Upgrade all plugins.
 				installed, err := installation.ListInstalledPlugins(paths.InstallReceiptsPath())
 				if err != nil {
 					return errors.Wrap(err, "failed to find all installed versions")
@@ -52,10 +54,13 @@ kubectl krew upgrade foo bar"`,
 					pluginNames = append(pluginNames, name)
 				}
 				ignoreUpgraded = true
+				skipErrors = true
 			} else {
+				// Upgrade certain plugins
 				pluginNames = args
 			}
 
+			var nErrors int
 			for _, name := range pluginNames {
 				plugin, err := indexscanner.LoadPluginFileFromFS(paths.IndexPluginsPath(), name)
 				if err != nil {
@@ -69,11 +74,19 @@ kubectl krew upgrade foo bar"`,
 					continue
 				}
 				if err != nil {
+					nErrors++
+					if skipErrors {
+						fmt.Fprintf(os.Stderr, "WARNING: failed to upgrade plugin %q, skipping (error: %v)\n", plugin.Name, err)
+						continue
+					}
 					return errors.Wrapf(err, "failed to upgrade plugin %q", plugin.Name)
 				}
 				fmt.Fprintf(os.Stderr, "Upgraded plugin: %s\n", plugin.Name)
 				internal.PrintSecurityNotice()
 			}
+			if nErrors > 0 {
+				fmt.Fprintf(os.Stderr, "WARNING: Some plugins failed to upgrade, check logs above.\n")
+			}
 			return nil
 		},
 		PreRunE: func(cmd *cobra.Command, args []string) error {
diff --git a/integration_test/upgrade_test.go b/integration_test/upgrade_test.go
index 508f9f25..e0c71938 100644
--- a/integration_test/upgrade_test.go
+++ b/integration_test/upgrade_test.go
@@ -17,6 +17,7 @@ package integrationtest
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"sigs.k8s.io/krew/pkg/constants"
@@ -49,6 +50,31 @@ func TestKrewUpgrade(t *testing.T) {
 	}
 }
 
+func TestKrewUpgradeWhenPlatformNoLongerMatches(t *testing.T) {
+	skipShort(t)
+
+	test, cleanup := NewTest(t)
+	defer cleanup()
+
+	test.WithIndex().
+		Krew("install", validPlugin).
+		RunOrFail()
+
+	test.WithEnv("KREW_OS", "somethingelse")
+
+	// if upgrading 'all' plugins, must succeed
+	out := string(test.Krew("upgrade", "--no-update-index").RunOrFailOutput())
+	if !strings.Contains(out, "WARNING: Some plugins failed to upgrade") {
+		t.Fatalf("upgrade all plugins output doesn't contain warnings about failed plugins:\n%s", out)
+	}
+
+	// if upgrading a specific plugin, it must fail, because no longer matching to a platform
+	err := test.Krew("upgrade", validPlugin, "--no-update-index").Run()
+	if err == nil {
+		t.Fatal("expected failure when upgraded a specific plugin that no longer has a matching platform")
+	}
+}
+
 func resolvePluginSymlink(test *ITest, plugin string) string {
 	test.t.Helper()
 	linkToPlugin, err := test.LookupExecutable("kubectl-" + plugin)
diff --git a/internal/installation/upgrade.go b/internal/installation/upgrade.go
index 36c47ad0..cfa853be 100644
--- a/internal/installation/upgrade.go
+++ b/internal/installation/upgrade.go
@@ -48,7 +48,8 @@ func Upgrade(p environment.Paths, plugin index.Plugin) error {
 		return errors.Wrap(err, "failed trying to find a matching platform in plugin spec")
 	}
 	if !ok {
-		return errors.Wrapf(err, "plugin %q does not offer installation for this platform", plugin.Name)
+		os, arch := osArch()
+		return errors.Errorf("plugin %q does not offer installation for this platform (%s/%s)", plugin.Name, os, arch)
 	}
 
 	newVersion := plugin.Spec.Version