fix(trigger): Backport trigger filter fixes (knative#749)

* fix(trigger): Make --filter flag truly optional (knative#745)

* fix(trigger): Make --filter flag truly optional

* fix(trigger): Update trigger docs

* chore: Update changelog

* fix(trigger): Fix filter delete for trigger update (knative#746)

* chore: Update changelog

* Fix plugin execution for Windows (knative#738)

Fixes knative#737

* Use exec.Command instend of syscall.Exec for windows.
* Fix a bug in plugin handler test when running it on windows.
* Fix typo.

* fix: Fix default config path on Windows (knative#752)

Co-authored-by: Lv Jiawei <[email protected]>

CHANGELOG.adoc

@@ -12,6 +12,28 @@
 | https://github.com/knative/client/pull/[#]
 ////
 
+## v0.13.1 (unreleased)
+
+[cols="1,10,3", options="header", width="100%"]
+|===
+| | Description | PR
+
+| 🐛
+| Fix filter delete for trigger update command
+| https://github.com/knative/client/pull/746[#746]
+
+| 🐛
+| Fix trigger create --filter flag to be optional
+| https://github.com/knative/client/pull/745[#745]
+
+| 🐛
+| Fix plugin execution for Windows.
+| https://github.com/knative/client/pull/738[#738]
+
+| 🐛
+| Fix default config path on Windows
+| https://github.com/knative/client/pull/751[#751]
+
 ## v0.13.0 (2020-03-11)
 
 [cols="1,10,3", options="header", width="100%"]

docs/cmd/kn_trigger_create.md

@@ -7,14 +7,17 @@ Create a trigger
 Create a trigger
 
 ```
-kn trigger create NAME --broker BROKER --filter KEY=VALUE --sink SINK [flags]
+kn trigger create NAME --broker BROKER --sink SINK [flags]
 ```
 
 ### Examples
 
 ```
 
-  # Create a trigger 'mytrigger' to declare a subscription to events with attribute 'type=dev.knative.foo' from default broker. The subscriber is service 'mysvc'
+  # Create a trigger 'mytrigger' to declare a subscription to events from default broker. The subscriber is service 'mysvc'
+  kn trigger create mytrigger --broker default --sink svc:mysvc
+
+  # Create a trigger to filter events with attribute 'type=dev.knative.foo'
   kn trigger create mytrigger --broker default --filter type=dev.knative.foo --sink svc:mysvc
 ```
 

docs/plugins/README.md

@@ -5,7 +5,7 @@ Plugins follow a similar architecture to
 with some small differences. One key difference is that `kn` plugins can either
 live in your `PATH` or in a chosen and specified directory.
 [Kn plugins](https://github.com/knative/client/tree/master/docs/cmd/kn_plugin.md)
-shows how to install and create new plugins as well as gives some examples and
+show how to install and create new plugins as well as gives some examples and
 best practices.
 
 To see what plugins are installed on your machine, you can use the

pkg/eventing/v1alpha1/client.go

@@ -180,18 +180,18 @@ func (b *TriggerBuilder) InjectBroker(inject bool) *TriggerBuilder {
 }
 
 func (b *TriggerBuilder) Filters(filters map[string]string) *TriggerBuilder {
+	if len(filters) == 0 {
+		b.trigger.Spec.Filter = &v1alpha1.TriggerFilter{}
+		return b
+	}
 	filter := b.trigger.Spec.Filter
 	if filter == nil {
 		filter = &v1alpha1.TriggerFilter{}
 		b.trigger.Spec.Filter = filter
 	}
-	attributes := filter.Attributes
-	if attributes == nil {
-		attributes = &v1alpha1.TriggerFilterAttributes{}
-		filter.Attributes = attributes
-	}
+	filter.Attributes = &v1alpha1.TriggerFilterAttributes{}
 	for k, v := range filters {
-		(*attributes)[k] = v
+		(*filter.Attributes)[k] = v
 	}
 	return b
 }

pkg/eventing/v1alpha1/client_test.go

@@ -155,6 +155,17 @@ func TestTriggerBuilder(t *testing.T) {
 		assert.DeepEqual(t, expected, b.Build().Spec.Filter)
 	})
 
+	t.Run("update filters to remove filters", func(t *testing.T) {
+		b := NewTriggerBuilderFromExisting(a.Build())
+		assert.DeepEqual(t, b.Build(), a.Build())
+		b.Filters(nil)
+		expected := &v1alpha1.TriggerFilter{}
+		assert.DeepEqual(t, expected, b.Build().Spec.Filter)
+
+		b.Filters((make(map[string]string)))
+		assert.DeepEqual(t, expected, b.Build().Spec.Filter)
+	})
+
 	t.Run("add and remove inject annotation", func(t *testing.T) {
 		b := NewTriggerBuilder("broker-trigger")
 		b.InjectBroker(true)

pkg/kn/commands/plugin/handler.go

@@ -20,6 +20,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"syscall"
 
@@ -93,6 +94,18 @@ func (h *DefaultPluginHandler) Lookup(name string) (string, bool) {
 
 // Execute implements PluginHandler
 func (h *DefaultPluginHandler) Execute(executablePath string, cmdArgs, environment []string) error {
+	if runtime.GOOS == "windows" {
+		cmd := exec.Command(executablePath, cmdArgs...)
+		cmd.Stdout = os.Stdout
+		cmd.Stderr = os.Stderr
+		cmd.Stdin = os.Stdin
+		cmd.Env = environment
+		err := cmd.Run()
+		if err == nil {
+			os.Exit(0)
+		}
+		return err
+	}
 	return syscall.Exec(executablePath, cmdArgs, environment)
 }
 

pkg/kn/commands/plugin/handler_test.go

@@ -19,6 +19,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 
 	"gotest.tools/assert"
@@ -45,6 +46,9 @@ func TestPluginHandler(t *testing.T) {
 
 	beforeEach := func(t *testing.T) {
 		pluginName = "fake"
+		if runtime.GOOS == "windows" {
+			pluginName += ".bat"
+		}
 		pluginPath = CreateTestPluginInPath(t, "kn-"+pluginName, KnTestPluginScript, FileModeExecutable, tmpPathDir)
 		assert.Assert(t, pluginPath != "")
 
@@ -134,6 +138,14 @@ func TestPluginHandler(t *testing.T) {
 			err = pluginHandler.Execute(bogusPath, []string{bogusPath}, os.Environ())
 			assert.Assert(t, err != nil, fmt.Sprintf("bogus plugin in path %s unexpectedly executed OK", bogusPath))
 		})
+		t.Run("executing fake plugin successfully", func(t *testing.T) {
+			setup(t)
+			defer cleanup(t)
+			beforeEach(t)
+
+			err = pluginHandler.Execute(pluginPath, []string{}, os.Environ())
+			assert.Assert(t, err == nil, fmt.Sprintf("fail to execute fake plugin in path %s ", pluginPath))
+		})
 	})
 
 	t.Run("HandlePluginCommand", func(t *testing.T) {

pkg/kn/commands/plugin/plugin_test_helper.go

@@ -18,7 +18,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"runtime"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -58,13 +57,9 @@ func CreateTestPlugin(t *testing.T, name, script string, fileMode os.FileMode) s
 // CreateTestPluginInPath with name, path, script, and fileMode and return the tmp random path
 func CreateTestPluginInPath(t *testing.T, name, script string, fileMode os.FileMode, path string) string {
 	fullPath := filepath.Join(path, name)
-	if runtime.GOOS == "windows" {
-		fullPath += ".bat"
-	}
 	err := ioutil.WriteFile(fullPath, []byte(script), fileMode)
 	assert.NilError(t, err)
-
-	return filepath.Join(path, name)
+	return fullPath
 }
 
 // DeleteTestPlugin with path

pkg/kn/commands/trigger/create.go

@@ -33,10 +33,13 @@ func NewTriggerCreateCommand(p *commands.KnParams) *cobra.Command {
 	var sinkFlags flags.SinkFlags
 
 	cmd := &cobra.Command{
-		Use:   "create NAME --broker BROKER --filter KEY=VALUE --sink SINK",
+		Use:   "create NAME --broker BROKER --sink SINK",
 		Short: "Create a trigger",
 		Example: `
-  # Create a trigger 'mytrigger' to declare a subscription to events with attribute 'type=dev.knative.foo' from default broker. The subscriber is service 'mysvc'
+  # Create a trigger 'mytrigger' to declare a subscription to events from default broker. The subscriber is service 'mysvc'
+  kn trigger create mytrigger --broker default --sink svc:mysvc
+
+  # Create a trigger to filter events with attribute 'type=dev.knative.foo'
   kn trigger create mytrigger --broker default --filter type=dev.knative.foo --sink svc:mysvc`,
 
 		RunE: func(cmd *cobra.Command, args []string) (err error) {

pkg/kn/commands/trigger/describe.go

@@ -112,8 +112,10 @@ func writeSink(dw printers.PrefixWriter, sink *duckv1.Destination) {
 func writeTrigger(dw printers.PrefixWriter, trigger *v1alpha1.Trigger, printDetails bool) {
 	commands.WriteMetadata(dw, &trigger.ObjectMeta, printDetails)
 	dw.WriteAttribute("Broker", trigger.Spec.Broker)
-	subWriter := dw.WriteAttribute("Filter", "")
-	for key, value := range *trigger.Spec.Filter.Attributes {
-		subWriter.WriteAttribute(key, value)
+	if trigger.Spec.Filter != nil && trigger.Spec.Filter.Attributes != nil {
+		subWriter := dw.WriteAttribute("Filter", "")
+		for key, value := range *trigger.Spec.Filter.Attributes {
+			subWriter.WriteAttribute(key, value)
+		}
 	}
 }

pkg/kn/commands/types.go

@@ -19,6 +19,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
 
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/rest"
@@ -41,8 +42,8 @@ var CfgFile string
 
 // Cfg is Kn's configuration values
 var Cfg Config = Config{
-	DefaultConfigDir: "~/.config/kn",
-	DefaultPluginDir: "~/.config/kn/plugins",
+	DefaultConfigDir: newDefaultConfigPath(""),
+	DefaultPluginDir: newDefaultConfigPath("plugins"),
 	PluginsDir:       "",
 	LookupPlugins:    newBoolP(false),
 }
@@ -195,3 +196,11 @@ func newBoolP(b bool) *bool {
 	aBool := b
 	return &aBool
 }
+
+// Returns default config path based on target OS
+func newDefaultConfigPath(subDir string) string {
+	if runtime.GOOS == "windows" {
+		return filepath.Join(os.Getenv("APPDATA"), "kn", subDir)
+	}
+	return filepath.Join("~", ".config", "kn", subDir)
+}

test/e2e/trigger_test.go

@@ -46,7 +46,7 @@ func TestBrokerTrigger(t *testing.T) {
 	test.serviceCreate(t, r, "sinksvc1")
 
 	t.Log("create triggers and list them")
-	test.triggerCreate(t, r, "trigger1", "sinksvc0", []string{"a=b"})
+	test.triggerCreate(t, r, "trigger1", "sinksvc0", nil)
 	test.triggerCreate(t, r, "trigger2", "sinksvc1", []string{"type=knative.dev.bar", "source=ping"})
 	test.verifyTriggerList(t, r, "trigger1", "trigger2")
 	test.triggerDelete(t, r, "trigger1")
@@ -57,6 +57,15 @@ func TestBrokerTrigger(t *testing.T) {
 	test.triggerDelete(t, r, "deltrigger")
 	test.verifyTriggerNotfound(t, r, "deltrigger")
 
+	t.Log("create a trigger with filters and remove them one by one")
+	test.triggerCreate(t, r, "filtertrigger", "sinksvc0", []string{"foo=bar", "source=ping"})
+	test.verifyTriggerDescribe(t, r, "filtertrigger", "default", "sinksvc0", []string{"foo", "bar", "source", "ping"})
+	test.triggerUpdate(t, r, "filtertrigger", "foo-", "sinksvc0")
+	test.verifyTriggerDescribe(t, r, "filtertrigger", "default", "sinksvc0", []string{"source", "ping"})
+	test.triggerUpdate(t, r, "filtertrigger", "source-", "sinksvc0")
+	test.verifyTriggerDescribe(t, r, "filtertrigger", "default", "sinksvc0", nil)
+	test.triggerDelete(t, r, "filtertrigger")
+
 	t.Log("create a trigger, describe and update it")
 	test.triggerCreate(t, r, "updtrigger", "sinksvc0", []string{"a=b"})
 	test.verifyTriggerDescribe(t, r, "updtrigger", "default", "sinksvc0", []string{"a", "b"})
@@ -93,8 +102,10 @@ func (test *e2eTest) lableNamespaceForDefaultBroker(t *testing.T) error {
 
 func (test *e2eTest) triggerCreate(t *testing.T, r *KnRunResultCollector, name string, sinksvc string, filters []string) {
 	args := []string{"trigger", "create", name, "--broker", "default", "--sink", "svc:" + sinksvc}
-	for _, v := range filters {
-		args = append(args, "--filter", v)
+	if len(filters) > 0 {
+		for _, v := range filters {
+			args = append(args, "--filter", v)
+		}
 	}
 	out := test.kn.Run(args...)
 	r.AssertNoError(out)
@@ -128,7 +139,11 @@ func (test *e2eTest) verifyTriggerList(t *testing.T, r *KnRunResultCollector, tr
 func (test *e2eTest) verifyTriggerDescribe(t *testing.T, r *KnRunResultCollector, name string, broker string, sink string, filters []string) {
 	out := test.kn.Run("trigger", "describe", name)
 	r.AssertNoError(out)
-	assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, filters...))
+	if len(filters) > 0 {
+		assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, filters...))
+	} else {
+		assert.Check(t, util.ContainsNone(out.Stdout, "Filter"))
+	}
 	assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, name, broker, sink))
 }