Repository: knative/client
Pull request #749 (2020-03-19 19:18:43)

fix(trigger): Backport trigger filter fixes
## Description

Backport trigger filter fixes to `0.13.x` release.

## Changes

<!-- Please add list of more detailed changes. These changes should be reflected also in the commit messages -->

* Cherry-pick 58ee9c4788bd9655cede37d710fa45762a9ef2fe
* Cherry-pick 1b0e7124d7706171bde34d2ffe0958a9954eaefe
* Cherry-pick 5e73d8dad37b955a06075c2a3b990eaa595f94f0
* Cherry-pick a0a762b87eb9a7ad4b19b5b1b37869c511faf3a8

**Note:** I've updated changelog to point to next `0.13.1` release, but I'm not quiet sure about PR ref. Should the URL point to original PR or this one?



knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/749#" title="Author self-approved">dsimansk</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **evankanderson**
You can assign the PR to them by writing `/assign @evankanderson` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/client/blob/release-0.13/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["evankanderson"]} -->
dsimansk: I think "Windows plugin issue" is a good candidate for a backport too, https://github.com/knative/client/pull/738.
googlebot: All (the pull request submitter and all commit authors) CLAs are signed, **but** one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project.  Please have them confirm that by leaving a comment that contains only `@googlebot I consent.` in this pull request.

*Note to project maintainer:* There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent.  In those cases, you can manually confirm consent of the commit author(s), and set the `cla` label to `yes` (if enabled on your project).

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fknative%2Fclient%2Fpull%2F749) for more info**.

<!-- need_author_consent -->
dsimansk: Added cherry-pick 5e73d8dad37b955a06075c2a3b990eaa595f94f0

@MIBc could you please add comment `@googlebot I consent.` to make CLA bot happy, see the above.
MIBc: @googlebot I consent.
googlebot: CLAs look good, thanks!

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fknative%2Fclient%2Fpull%2F749) for more info**.

<!-- ok -->
dsimansk: Added cherry-pick a0a762b87eb9a7ad4b19b5b1b37869c511faf3a8
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/749#" title="Author self-approved">dsimansk</a>*, *<a href="https://github.com/knative/client/pull/749#pullrequestreview-380147735" title="LGTM">rhuss</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/client/blob/release-0.13/OWNERS)~~ [rhuss]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index 4872db20..5588b673 100644
--- a/CHANGELOG.adoc
+++ b/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%"]
diff --git a/docs/cmd/kn_trigger_create.md b/docs/cmd/kn_trigger_create.md
index 62510d41..d480786b 100644
--- a/docs/cmd/kn_trigger_create.md
+++ b/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
 ```
 
diff --git a/docs/plugins/README.md b/docs/plugins/README.md
index b885efe0..4f0d1ab4 100644
--- a/docs/plugins/README.md
+++ b/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
diff --git a/pkg/eventing/v1alpha1/client.go b/pkg/eventing/v1alpha1/client.go
index 49bee43e..16037fe7 100644
--- a/pkg/eventing/v1alpha1/client.go
+++ b/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
 }
diff --git a/pkg/kn/commands/plugin/handler.go b/pkg/kn/commands/plugin/handler.go
index 5fc81043..7ee7c4ee 100644
--- a/pkg/kn/commands/plugin/handler.go
+++ b/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)
 }
 
diff --git a/pkg/kn/commands/trigger/create.go b/pkg/kn/commands/trigger/create.go
index ecf0523f..d7ab59aa 100644
--- a/pkg/kn/commands/trigger/create.go
+++ b/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) {
diff --git a/pkg/kn/commands/trigger/describe.go b/pkg/kn/commands/trigger/describe.go
index 6b56ebd8..f8083115 100644
--- a/pkg/kn/commands/trigger/describe.go
+++ b/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)
+		}
 	}
 }
diff --git a/pkg/kn/commands/types.go b/pkg/kn/commands/types.go
index b7b72352..d7a84019 100644
--- a/pkg/kn/commands/types.go
+++ b/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 patch:
diff --git a/pkg/eventing/v1alpha1/client_test.go b/pkg/eventing/v1alpha1/client_test.go
index 351e6fc2..419824f9 100644
--- a/pkg/eventing/v1alpha1/client_test.go
+++ 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)
diff --git a/pkg/kn/commands/plugin/handler_test.go b/pkg/kn/commands/plugin/handler_test.go
index 65d5b424..5757a7eb 100644
--- a/pkg/kn/commands/plugin/handler_test.go
+++ b/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) {
diff --git a/pkg/kn/commands/plugin/plugin_test_helper.go b/pkg/kn/commands/plugin/plugin_test_helper.go
index 7af57a74..c080d460 100644
--- a/pkg/kn/commands/plugin/plugin_test_helper.go
+++ b/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
diff --git a/test/e2e/trigger_test.go b/test/e2e/trigger_test.go
index 55c16867..ee4b36ba 100644
--- a/test/e2e/trigger_test.go
+++ b/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))
 }