Make files in spec.platforms[] optional (#261)

* Allow `files` specification to be unspecified in plugin manifests

If unspecified, default to `{From: "*", To: "."}`. When `files` is given,
but empty, an error is produced instead.

Also validate that files specification has both fields set.

* Review comments

- Improve readability of error messages.

* Move defaults for file operations into `install` method

docs/DEVELOPER_GUIDE.md

@@ -104,7 +104,8 @@ spec:
     uri: https://github.com/example/foo/releases/v1.0.zip
     # sha256sum of the file downloaded above:
     sha256: "208fde0b9f42ef71f79864b1ce594a70832c47dc5426e10ca73bf02e54d499d0"
-    files:                     # copy the used files out of the zip archive
+    # copy the used files out of the zip archive, defaults to `[{from: "*", to: "."}]`
+    files:
     - from: "/foo-*/unix/*.sh" # path to the files extracted from archive
       to: "."               # '.' refers to the root of plugin install directory
     bin: "./kubectl-foo"  # path to the plugin executable after copying files above
@@ -169,20 +170,12 @@ be installed. You can use the `files` field in the plugin manifest to specify
 which files should be copied into the plugin directory after extracting the
 plugin archive.
 
-The `file:` list specifies the copy operations (like `mv(1) <from> <to>`) to
-the files `from` the archive `to` the installation destination.
-
-**Example:** Copy all files in the `bin/` directory of the extracted archive to
-the root of your plugin:
-
-```yaml
-    files:
-    - from: bin/*.exe
-      to: .
-```
+The `files:` list specifies the copy operations (like `mv(1) <from> <to>`) to
+the files `from` the archive `to` the installation destination. If the `files`
+list is unspecified, it defaults to `[{from: "*", to: "."}]`. Mind that a
+wildcard may install more files than desired.
 
-Given the file operation above, if the extracted plugin archive looked like
-this:
+**Example:** Consider a plugin whose extracted archive looks like
 
 ```text
 .
@@ -193,12 +186,36 @@ this:
     └── kubectl-foo-windows.exe
 ```
 
-The resulting installation directory would up just with:
-
-```text
-.
-└── krew-foo-windows.exe
-```
+- To copy all files in the `bin/` directory of the extracted archive to
+  the root of your plugin, leave out the `files:` field, which is equivalent to
+
+  ```yaml
+      files: [{from: "*", to: "."}]
+  ```
+
+  This copies out binaries for both platforms to the installation directory
+  onto the user’s machine, despite only one of them will be used:
+
+  ```text
+  .
+  └── krew-foo-windows.exe
+  └── krew-foo-linux
+  ```
+
+- For a more fine-grained control, specify each copy operation:
+
+  ```yaml
+      files:
+      - from: bin/*.exe
+        to: .
+  ```
+
+  This will result in the following files in the installation directory:
+
+  ```text
+  .
+  └── krew-foo-windows.exe
+  ```
 
 #### Specifying plugin executable
 

pkg/index/indexscanner/testdata/testindex/dontscan.yaml

@@ -20,13 +20,15 @@ spec:
   platforms:
   - files:
     - from: "*"
+      to: "."
     uri: https://example.com
     sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     selector:
       matchExpressions:
       - {key: os, operator: In, values: [macos, linux]}
   - files:
     - from: "*"
+      to: "."
     uri: https://example.com
     sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     selector:

pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml

@@ -21,6 +21,7 @@ spec:
   platforms:
   - files:
     - from: "*"
+      to: "."
     uri: https://example.com
     sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     bin: kubectl-bar
@@ -29,6 +30,7 @@ spec:
         os: windows
   - files:
     - from: "*"
+      to: "."
     uri: https://example.com
     sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     bin: kubectl-bar

pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml

@@ -21,6 +21,7 @@ spec:
   platforms:
   - files:
     - from: "*"
+      to: "."
     uri: https://example.com
     sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     bin: kubectl-foo
@@ -29,6 +30,7 @@ spec:
       - {key: os, operator: In, values: [macos, linux]}
   - files:
     - from: "*"
+      to: "."
     uri: https://example.com
     sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     bin: kubectl-foo

pkg/index/validation/validate.go

@@ -99,26 +99,43 @@ func ValidatePlugin(name string, p index.Plugin) error {
 // validatePlatform checks Platform for structural validity.
 func validatePlatform(p index.Platform) error {
 	if p.URI == "" {
-		return errors.New("URI has to be set")
+		return errors.New("`uri` has to be set")
 	}
 	if p.Sha256 == "" {
-		return errors.New("sha256 sum has to be set")
+		return errors.New("`sha256` sum has to be set")
 	}
 	if !isValidSHA256(p.Sha256) {
-		return errors.Errorf("sha256 value %s is not valid, must match pattern %s", p.Sha256, sha256Pattern)
+		return errors.Errorf("`sha256` value %s is not valid, must match pattern %s", p.Sha256, sha256Pattern)
 	}
 	if p.Bin == "" {
-		return errors.New("bin has to be set")
+		return errors.New("`bin` has to be set")
 	}
-	if len(p.Files) == 0 {
-		return errors.New("can't have a plugin without specifying file operations")
+	if err := validateFiles(p.Files); err != nil {
+		return errors.Wrap(err, "`files` is invalid")
 	}
 	if err := validateSelector(p.Selector); err != nil {
 		return errors.Wrap(err, "invalid platform selector")
 	}
 	return nil
 }
 
+func validateFiles(fops []index.FileOperation) error {
+	if fops == nil {
+		return nil
+	}
+	if len(fops) == 0 {
+		return errors.New("`files` has to be unspecified or non-empty")
+	}
+	for _, op := range fops {
+		if op.From == "" {
+			return errors.New("`from` field has to be set")
+		} else if op.To == "" {
+			return errors.New("`to` field has to be set")
+		}
+	}
+	return nil
+}
+
 // validateSelector checks if the platform selector uses supported keys and is not empty or nil.
 func validateSelector(sel *metav1.LabelSelector) error {
 	if sel == nil {
@@ -143,10 +160,10 @@ func validateSelector(sel *metav1.LabelSelector) error {
 	}
 
 	if sel.MatchLabels != nil && len(sel.MatchLabels) == 0 {
-		return errors.New("matchLabels specified but empty")
+		return errors.New("`matchLabels` specified but empty")
 	}
 	if sel.MatchExpressions != nil && len(sel.MatchExpressions) == 0 {
-		return errors.New("matchExpressions specified but empty")
+		return errors.New("`matchExpressions` specified but empty")
 	}
 
 	return nil

pkg/index/validation/validate_test.go

@@ -146,10 +146,10 @@ func TestValidatePlugin(t *testing.T) {
 			wantErr:    true,
 		},
 		{
-			name:       "no file operations",
+			name:       "empty file operations",
 			pluginName: "foo",
 			plugin: testutil.NewPlugin().WithName("foo").WithPlatforms(
-				testutil.NewPlatform().WithFiles(nil).V()).V(),
+				testutil.NewPlatform().WithFiles([]index.FileOperation{}).V()).V(),
 			wantErr: true,
 		},
 		{
@@ -190,8 +190,8 @@ func TestValidatePlatform(t *testing.T) {
 			wantErr:  true,
 		},
 		{
-			name:     "no file operations",
-			platform: testutil.NewPlatform().WithFiles(nil).V(),
+			name:     "empty file operations",
+			platform: testutil.NewPlatform().WithFiles([]index.FileOperation{}).V(),
 			wantErr:  true,
 		},
 		{
@@ -282,3 +282,44 @@ func Test_validateSelector(t *testing.T) {
 		})
 	}
 }
+
+func Test_validateFiles(t *testing.T) {
+	tests := []struct {
+		name    string
+		files   []index.FileOperation
+		wantErr bool
+	}{
+		{
+			name:    "success",
+			files:   []index.FileOperation{{From: "here", To: "there"}},
+			wantErr: false,
+		},
+		{
+			name:    "unspecified file operations",
+			files:   nil,
+			wantErr: false,
+		},
+		{
+			name:    "empty file operations",
+			files:   []index.FileOperation{},
+			wantErr: true,
+		},
+		{
+			name:    "empty `to` field in file operations",
+			files:   []index.FileOperation{{From: "present", To: ""}},
+			wantErr: true,
+		},
+		{
+			name:    "empty `from` field in file operations",
+			files:   []index.FileOperation{{From: "", To: "present"}},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := validateFiles(tt.files); (err != nil) != tt.wantErr {
+				t.Errorf("validateFiles() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

pkg/installation/install.go

@@ -107,6 +107,7 @@ func install(op installOperation, opts InstallOpts) error {
 		return errors.Wrap(err, "failed to download and extract")
 	}
 
+	applyDefaults(&op.platform)
 	if err := moveToInstallDir(op.downloadStagingDir, op.installDir, op.platform.Files); err != nil {
 		return errors.Wrap(err, "failed while moving files to the installation directory")
 	}
@@ -127,6 +128,13 @@ func install(op installOperation, opts InstallOpts) error {
 	return errors.Wrap(err, "failed to link installed plugin")
 }
 
+func applyDefaults(platform *index.Platform) {
+	if platform.Files == nil {
+		platform.Files = []index.FileOperation{{From: "*", To: "."}}
+		glog.V(4).Infof("file operation not specified, assuming %v", platform.Files)
+	}
+}
+
 // downloadAndExtract downloads the specified archive uri (or uses the provided overrideFile, if a non-empty value)
 // while validating its checksum with the provided sha256sum, and extracts its contents to extractDir that must be.
 // created.

pkg/installation/install_test.go

@@ -25,6 +25,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"sigs.k8s.io/krew/pkg/environment"
 	"sigs.k8s.io/krew/pkg/index"
 	"sigs.k8s.io/krew/pkg/testutil"
@@ -257,3 +259,41 @@ func Test_downloadAndExtract_fileOverride(t *testing.T) {
 		t.Fatal("no files found in the extract output directory")
 	}
 }
+
+func Test_applyDefaults(t *testing.T) {
+	tests := []struct {
+		name     string
+		platform index.Platform
+		expected index.Platform
+	}{
+		{
+			name:     "with files given",
+			platform: testutil.NewPlatform().WithFiles([]index.FileOperation{{From: "here", To: "there"}}).V(),
+			expected: testutil.NewPlatform().WithFiles([]index.FileOperation{{From: "here", To: "there"}}).V(),
+		},
+		{
+			name:     "with empty files",
+			platform: testutil.NewPlatform().WithFiles([]index.FileOperation{}).V(),
+			expected: testutil.NewPlatform().WithFiles([]index.FileOperation{}).V(),
+		},
+		{
+			name:     "with unspecified files",
+			platform: testutil.NewPlatform().WithFiles(nil).V(),
+			expected: testutil.NewPlatform().WithFiles([]index.FileOperation{{From: "*", To: "."}}).V(),
+		},
+		{
+			name:     "no defaults for other fields",
+			platform: testutil.NewPlatform().WithBin("").WithOS("").WithSelector(nil).WithSHA256("").WithURI("").V(),
+			expected: testutil.NewPlatform().WithBin("").WithOS("").WithSelector(nil).WithSHA256("").WithURI("").V(),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			applyDefaults(&test.platform)
+			if diff := cmp.Diff(test.platform, test.expected); diff != "" {
+				t.Error(diff)
+			}
+		})
+	}
+}