Move platform detection to pkg/installation

- moved Platform matching methods to pkg/installation, where:
  - they make more sense
  - they can be tested without worrying about cyclic imports due to pkg/testutil.
- rewrote some tests in platform_test.go to be shorter.
- added a test for "no platforms specified" case in TestValidatePlugin.

- added a check to verify-code-patterns that uses a heuristic to detect inline
  initializations of index.(Plugin|Platform) structs.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

cmd/krew/cmd/info.go

@@ -27,6 +27,7 @@ import (
 
 	"sigs.k8s.io/krew/pkg/index"
 	"sigs.k8s.io/krew/pkg/info"
+	"sigs.k8s.io/krew/pkg/installation"
 )
 
 // infoCmd represents the info command
@@ -56,7 +57,7 @@ Example:
 
 func printPluginInfo(out io.Writer, plugin index.Plugin) {
 	fmt.Fprintf(out, "NAME: %s\n", plugin.Name)
-	if platform, ok, err := plugin.Spec.GetMatchingPlatform(); err == nil && ok {
+	if platform, ok, err := installation.GetMatchingPlatform(plugin.Spec.Platforms); err == nil && ok {
 		if platform.URI != "" {
 			fmt.Fprintf(out, "URI: %s\n", platform.URI)
 			fmt.Fprintf(out, "SHA256: %s\n", platform.Sha256)

cmd/krew/cmd/search.go

@@ -80,7 +80,7 @@ Examples:
 			var status string
 			if _, ok := installed[name]; ok {
 				status = "yes"
-			} else if _, ok, err := plugin.Spec.GetMatchingPlatform(); err != nil {
+			} else if _, ok, err := installation.GetMatchingPlatform(plugin.Spec.Platforms); err != nil {
 				return errors.Wrapf(err, "failed to get the matching platform for plugin %s", name)
 			} else if ok {
 				status = "no"

cmd/validate-krew-manifest/main_test.go

@@ -166,13 +166,8 @@ func Test_findAnyMatchingPlatform(t *testing.T) {
 }
 
 func Test_isOverlappingPlatformSelectors_noOverlap(t *testing.T) {
-	p1 := index.Platform{
-		Selector: &metav1.LabelSelector{MatchExpressions: []v1.LabelSelectorRequirement{{
-			Key:      "os",
-			Operator: v1.LabelSelectorOpIn,
-			Values:   []string{"darwin", "linux"}}}}}
-	p2 := index.Platform{
-		Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "windows"}}}
+	p1 := testutil.NewPlatform().WithOSes("darwin", "linux").V()
+	p2 := testutil.NewPlatform().WithOSes("windows").V()
 
 	err := isOverlappingPlatformSelectors([]index.Platform{p1, p2})
 	if err != nil {
@@ -181,14 +176,8 @@ func Test_isOverlappingPlatformSelectors_noOverlap(t *testing.T) {
 }
 
 func Test_isOverlappingPlatformSelectors_overlap(t *testing.T) {
-	p1 := index.Platform{
-		Selector: &metav1.LabelSelector{MatchExpressions: []v1.LabelSelectorRequirement{{
-			Key:      "os",
-			Operator: v1.LabelSelectorOpIn,
-			Values:   []string{"darwin", "linux"}}}}}
-	p2 := index.Platform{
-		Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"os": "darwin"}}}
-
+	p1 := testutil.NewPlatform().WithOS("darwin").V()
+	p2 := testutil.NewPlatform().WithOSes("darwin", "linux").V()
 	err := isOverlappingPlatformSelectors([]index.Platform{p1, p2})
 	if err == nil {
 		t.Fatal("expected overlap")

hack/verify-code-patterns.sh

@@ -43,3 +43,17 @@ if [[ -n "$out" ]]; then
   echo >&2 "$out"
   exit 1
 fi
+
+# Do not initialize index.{Plugin,Platform} structs in test code.
+#
+# TODO(ahmetb): ideally this plugin should cleanly scan all (index\.)(Plugin|Platform) occurrences except slice literals
+# (which are prefixed with []). However I could not get negative lookbehind pattern to work with grep to match to ones
+# that do not have a [] prefix.
+out="$(grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn '\s+(index\.)(Plugin|Platform){' || true)"
+if [[ -n "$out" ]]; then
+  echo >&2 "Do not use index.Platform or index.Plugin structs directly in tests,"
+  echo >&2 "use testutil.NewPlugin() or testutil.NewPlatform() instead:"
+  echo >&2 "-----"
+  echo >&2 "$out"
+  exit 1
+fi

pkg/index/validation/validate_test.go

@@ -150,6 +150,12 @@ func TestValidatePlugin(t *testing.T) {
 			plugin:     testutil.NewPlugin().WithName("foo").WithVersion("v01.02.3-a").V(),
 			wantErr:    true,
 		},
+		{
+			name:       "no platform specified",
+			pluginName: "foo",
+			plugin:     testutil.NewPlugin().WithName("foo").WithPlatforms().V(),
+			wantErr:    true,
+		},
 		{
 			name:       "no file operations",
 			pluginName: "foo",

pkg/index/platform.go → pkg/installation/platform.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package index
+package installation
 
 import (
 	"os"
@@ -22,34 +22,37 @@ import (
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+
+	"sigs.k8s.io/krew/pkg/index"
 )
 
 // GetMatchingPlatform finds the platform spec in the specified plugin that
 // matches the OS/arch of the current machine (can be overridden via KREW_OS
 // and/or KREW_ARCH).
-func (s *PluginSpec) GetMatchingPlatform() (Platform, bool, error) {
+func GetMatchingPlatform(platforms []index.Platform) (index.Platform, bool, error) {
 	os, arch := osArch()
-	glog.V(4).Infof("Using os=%s arch=%s", os, arch)
-	return s.matchPlatformToSystemEnvs(os, arch)
+	return matchPlatform(platforms, os, arch)
 }
 
-func (s *PluginSpec) matchPlatformToSystemEnvs(os, arch string) (Platform, bool, error) {
+// matchPlatform returns the first matching platform to given os/arch.
+func matchPlatform(platforms []index.Platform, os, arch string) (index.Platform, bool, error) {
 	envLabels := labels.Set{
 		"os":   os,
 		"arch": arch,
 	}
 	glog.V(2).Infof("Matching platform for labels(%v)", envLabels)
-	for i, platform := range s.Platforms {
+
+	for i, platform := range platforms {
 		sel, err := metav1.LabelSelectorAsSelector(platform.Selector)
 		if err != nil {
-			return Platform{}, false, errors.Wrap(err, "failed to compile label selector")
+			return index.Platform{}, false, errors.Wrap(err, "failed to compile label selector")
 		}
 		if sel.Matches(envLabels) {
 			glog.V(2).Infof("Found matching platform with index (%d)", i)
 			return platform, true, nil
 		}
 	}
-	return Platform{}, false, nil
+	return index.Platform{}, false, nil
 }
 
 // osArch returns the OS/arch combination to be used on the current system. It

pkg/installation/platform_test.go

@@ -0,0 +1,79 @@
+// Copyright 2019 The Kubernetes Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package installation
+
+import (
+	"os"
+	"runtime"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+
+	"sigs.k8s.io/krew/pkg/index"
+	"sigs.k8s.io/krew/pkg/testutil"
+)
+
+func Test_osArch(t *testing.T) {
+	inOS, inArch := runtime.GOOS, runtime.GOARCH
+	outOS, outArch := osArch()
+	if inOS != outOS {
+		t.Fatalf("returned OS=%q; expected=%q", outOS, inOS)
+	}
+	if inArch != outArch {
+		t.Fatalf("returned Arch=%q; expected=%q", outArch, inArch)
+	}
+}
+func Test_osArch_override(t *testing.T) {
+	customOS, customArch := "dragons", "metav1"
+	os.Setenv("KREW_OS", customOS)
+	os.Setenv("KREW_ARCH", customArch)
+	defer func() {
+		os.Unsetenv("KREW_ARCH")
+		os.Unsetenv("KREW_OS")
+	}()
+
+	outOS, outArch := osArch()
+	if customOS != outOS {
+		t.Fatalf("returned OS=%q; expected=%q", outOS, customOS)
+	}
+	if customArch != outArch {
+		t.Fatalf("returned Arch=%q; expected=%q", outArch, customArch)
+	}
+}
+
+func Test_matchPlatform(t *testing.T) {
+	const targetOS, targetArch = "foo", "amd64"
+	matchingPlatform := testutil.NewPlatform().WithOSArch("foo", "amd64").V()
+	nonMatchingPlatform := testutil.NewPlatform().WithOSArch("bar", "amd64").V()
+
+	p, ok, err := matchPlatform([]index.Platform{matchingPlatform, nonMatchingPlatform}, targetOS, targetArch)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !ok {
+		t.Fatal("failed to find a match")
+	}
+	if diff := cmp.Diff(p, matchingPlatform); diff != "" {
+		t.Fatalf("got a different object from the matching platform:\n%s", diff)
+	}
+
+	_, ok, err = matchPlatform([]index.Platform{nonMatchingPlatform}, targetOS, targetArch)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if ok {
+		t.Fatal("got a matching platform, but was not expecting")
+	}
+}

pkg/installation/util.go

@@ -40,7 +40,7 @@ func getDownloadTarget(index index.Plugin) (version, sha256sum, uri string, fos
 	// 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()
+	p, ok, err := GetMatchingPlatform(index.Spec.Platforms)
 	if err != nil {
 		return "", "", "", nil, p.Bin, errors.Wrap(err, "failed to get matching platforms")
 	}

pkg/testutil/plugin.go

@@ -79,7 +79,15 @@ func NewPlatform() *R {
 }
 
 func (p *R) WithOS(os string) *R {
-	p.v.Selector.MatchLabels = map[string]string{"os": os}
+	p.v.Selector = &metav1.LabelSelector{MatchLabels: map[string]string{"os": os}}
+	return p
+}
+
+func (p *R) WithOSes(os ...string) *R {
+	p.v.Selector = &metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{{
+		Key:      "os",
+		Operator: metav1.LabelSelectorOpIn,
+		Values:   os}}}
 	return p
 }