skip/fail upgrade if no longer matches a platform (#427)

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 <[email protected]>

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 {

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)

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