Repository: knative/client (go)

Issue:
Add E2E tests for trigger
This should involve the usage of a broker and a source (ideally a cronjob source) and should check if the filtering works.

Pull request:
Add E2E tests for trigger
Fixes #568

## Proposed Changes

* Add e2e tests for trigger
* Add retry mechanism in trigger update when meeting with conflicting error.

Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index 00912708..fd138675 100644
--- a/CHANGELOG.adoc
+++ b/CHANGELOG.adoc
@@ -38,7 +38,16 @@
 
 | 🧽
 | Add `no-wait` instead of `--async` and deprecate it
-https://github.com/knative/client/pull/639[#639]
+| https://github.com/knative/client/pull/639[#639]
+
+| 🧽
+| Refactor service `create_test.go` and `update_test.go` to
+| remove unecessary `sync` parameter in setup call
+| https://github.com/knative/client/pull/656[#656]
+
+| 🐛
+| Fix `--image` flag to only allow single occurence
+| https://github.com/knative/client/pull/647[#647]
 
 ## v0.12.0 (2020-01-29)
 
diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go
index 25753a85..8731eec0 100644
--- a/pkg/kn/commands/service/configuration_edit_flags.go
+++ b/pkg/kn/commands/service/configuration_edit_flags.go
@@ -30,7 +30,7 @@ import (
 
 type ConfigurationEditFlags struct {
 	// Direct field manipulation
-	Image   string
+	Image   uniqueStringArg
 	Env     []string
 	EnvFrom []string
 	Mount   []string
@@ -67,6 +67,25 @@ type ResourceFlags struct {
 	Memory string
 }
 
+// -- uniqueStringArg Value
+// Custom implementation of flag.Value interface to prevent multiple value assignment.
+// Useful to enforce unique use of flags, e.g. --image.
+type uniqueStringArg string
+
+func (s *uniqueStringArg) Set(val string) error {
+	if len(*s) > 0 {
+		return errors.New("can be provided only once")
+	}
+	*s = uniqueStringArg(val)
+	return nil
+}
+
+func (s *uniqueStringArg) Type() string {
+	return "string"
+}
+
+func (s *uniqueStringArg) String() string { return string(*s) }
+
 // markFlagMakesRevision indicates that a flag will create a new revision if you
 // set it.
 func (p *ConfigurationEditFlags) markFlagMakesRevision(f string) {
@@ -75,7 +94,7 @@ func (p *ConfigurationEditFlags) markFlagMakesRevision(f string) {
 
 // addSharedFlags adds the flags common between create & update.
 func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {
-	command.Flags().StringVar(&p.Image, "image", "", "Image to run.")
+	command.Flags().VarP(&p.Image, "image", "", "Image to run.")
 	p.markFlagMakesRevision("image")
 	command.Flags().StringArrayVarP(&p.Env, "env", "e", []string{},
 		"Environment variable to set. NAME=value; you may provide this flag "+
@@ -251,7 +270,7 @@ func (p *ConfigurationEditFlags) Apply(
 	}
 	imageSet := false
 	if cmd.Flags().Changed("image") {
-		err = servinglib.UpdateImage(template, p.Image)
+		err = servinglib.UpdateImage(template, p.Image.String())
 		if err != nil {
 			return err
 		}
diff --git a/pkg/kn/commands/trigger/trigger.go b/pkg/kn/commands/trigger/trigger.go
index d6dfd9de..7338077d 100644
--- a/pkg/kn/commands/trigger/trigger.go
+++ b/pkg/kn/commands/trigger/trigger.go
@@ -20,6 +20,11 @@ import (
 	"knative.dev/client/pkg/kn/commands"
 )
 
+const (
+	// How often to retry in case of an optimistic lock error when replacing a trigger (--force)
+	MaxUpdateRetries = 3
+)
+
 // NewTriggerCommand to create trigger command group
 func NewTriggerCommand(p *commands.KnParams) *cobra.Command {
 	triggerCmd := &cobra.Command{
diff --git a/pkg/kn/commands/trigger/update.go b/pkg/kn/commands/trigger/update.go
index 1a7c752c..fa5a036e 100644
--- a/pkg/kn/commands/trigger/update.go
+++ b/pkg/kn/commands/trigger/update.go
@@ -19,6 +19,7 @@ import (
 	"fmt"
 
 	"github.com/spf13/cobra"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 
 	"knative.dev/eventing/pkg/apis/eventing/v1alpha1"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
@@ -68,43 +69,52 @@ func NewTriggerUpdateCommand(p *commands.KnParams) *cobra.Command {
 				return err
 			}
 
-			trigger, err := eventingClient.GetTrigger(name)
-			if err != nil {
-				return err
-			}
+			var retries = 0
+			for {
+				trigger, err := eventingClient.GetTrigger(name)
+				if err != nil {
+					return err
+				}
 
-			b := client_v1alpha1.NewTriggerBuilderFromExisting(trigger)
+				b := client_v1alpha1.NewTriggerBuilderFromExisting(trigger)
 
-			if cmd.Flags().Changed("broker") {
-				return fmt.Errorf(
-					"cannot update trigger '%s' because broker is immutable", name)
-			}
-			if cmd.Flags().Changed("filter") {
-				updated, removed, err := triggerUpdateFlags.GetUpdateFilters()
-				if err != nil {
+				if cmd.Flags().Changed("broker") {
 					return fmt.Errorf(
-						"cannot update trigger '%s' because %s", name, err)
+						"cannot update trigger '%s' because broker is immutable", name)
 				}
-				existing := extractFilters(trigger)
-				b.Filters(existing.Merge(updated).Remove(removed))
-			}
-			if cmd.Flags().Changed("sink") {
-				destination, err := sinkFlags.ResolveSink(dynamicClient, namespace)
+				if cmd.Flags().Changed("filter") {
+					updated, removed, err := triggerUpdateFlags.GetUpdateFilters()
+					if err != nil {
+						return fmt.Errorf(
+							"cannot update trigger '%s' because %s", name, err)
+					}
+					existing := extractFilters(trigger)
+					b.Filters(existing.Merge(updated).Remove(removed))
+				}
+				if cmd.Flags().Changed("sink") {
+					destination, err := sinkFlags.ResolveSink(dynamicClient, namespace)
+					if err != nil {
+						return err
+					}
+					b.Subscriber(&duckv1.Destination{
+						Ref: destination.Ref,
+						URI: destination.URI,
+					})
+				}
+				err = eventingClient.UpdateTrigger(b.Build())
 				if err != nil {
+					if apierrors.IsConflict(err) && retries < MaxUpdateRetries {
+						retries++
+						continue
+					}
 					return err
 				}
-				b.Subscriber(&duckv1.Destination{
-					Ref: destination.Ref,
-					URI: destination.URI,
-				})
-			}
-			err = eventingClient.UpdateTrigger(b.Build())
-			if err == nil {
 				fmt.Fprintf(cmd.OutOrStdout(), "Trigger '%s' updated in namespace '%s'.\n", name, namespace)
+				return nil
 			}
-			return err
 		},
 	}
+
 	commands.AddNamespaceFlags(cmd.Flags(), false)
 	triggerUpdateFlags.Add(cmd)
 	sinkFlags.Add(cmd)


Test patch:
diff --git a/pkg/kn/commands/service/create_test.go b/pkg/kn/commands/service/create_test.go
index c765bf9a..6eb522ac 100644
--- a/pkg/kn/commands/service/create_test.go
+++ b/pkg/kn/commands/service/create_test.go
@@ -29,6 +29,7 @@ import (
 
 	"knative.dev/client/pkg/kn/commands"
 	servinglib "knative.dev/client/pkg/serving"
+	"knative.dev/client/pkg/util"
 	"knative.dev/client/pkg/wait"
 
 	corev1 "k8s.io/api/core/v1"
@@ -38,13 +39,14 @@ import (
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
-func fakeServiceCreate(args []string, withExistingService bool, sync bool) (
+func fakeServiceCreate(args []string, withExistingService bool) (
 	action clienttesting.Action,
 	service *servingv1.Service,
 	output string,
 	err error) {
 	knParams := &commands.KnParams{}
 	nrGetCalled := 0
+	sync := !noWait(args)
 	cmd, fakeServing, buf := commands.CreateTestKnCommand(NewServiceCommand(knParams), knParams)
 	fakeServing.AddReactor("get", "services",
 		func(a clienttesting.Action) (bool, runtime.Object, error) {
@@ -117,7 +119,7 @@ func getServiceEvents(name string) []watch.Event {
 
 func TestServiceCreateImage(t *testing.T) {
 	action, created, output, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--no-wait"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--no-wait"}, false)
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("create", "services") {
@@ -134,9 +136,16 @@ func TestServiceCreateImage(t *testing.T) {
 	}
 }
 
+func TestServiceCreateWithMultipleImages(t *testing.T) {
+	_, _, _, err := fakeServiceCreate([]string{
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"}, false)
+
+	assert.Assert(t, util.ContainsAll(err.Error(), "\"--image\"", "\"gcr.io/bar/foo:baz\"", "flag", "once"))
+}
+
 func TestServiceCreateImageSync(t *testing.T) {
 	action, created, output, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz"}, false, true)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz"}, false)
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("create", "services") {
@@ -160,7 +169,7 @@ func TestServiceCreateImageSync(t *testing.T) {
 
 func TestServiceCreateCommand(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--cmd", "/app/start", "--no-wait"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--cmd", "/app/start", "--no-wait"}, false)
 	assert.NilError(t, err)
 	assert.Assert(t, action.Matches("create", "services"))
 
@@ -171,7 +180,9 @@ func TestServiceCreateCommand(t *testing.T) {
 
 func TestServiceCreateArg(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--arg", "myArg1", "--arg", "--myArg2", "--arg", "--myArg3=3", "--no-wait"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
+		"--arg", "myArg1", "--arg", "--myArg2", "--arg", "--myArg3=3",
+		"--no-wait"}, false)
 	assert.NilError(t, err)
 	assert.Assert(t, action.Matches("create", "services"))
 
@@ -184,7 +195,8 @@ func TestServiceCreateArg(t *testing.T) {
 
 func TestServiceCreateEnv(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "-e", "A=DOGS", "--env", "B=WOLVES", "--env=EMPTY", "--no-wait"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
+		"-e", "A=DOGS", "--env", "B=WOLVES", "--env=EMPTY", "--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -220,7 +232,9 @@ func TestServiceCreateEnv(t *testing.T) {
 
 func TestServiceCreateWithRequests(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--requests-cpu", "250m", "--requests-memory", "64Mi", "--no-wait"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
+		"--requests-cpu", "250m", "--requests-memory", "64Mi",
+		"--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -246,7 +260,9 @@ func TestServiceCreateWithRequests(t *testing.T) {
 
 func TestServiceCreateWithLimits(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--limits-cpu", "1000m", "--limits-memory", "1024Mi", "--no-wait"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
+		"--limits-cpu", "1000m", "--limits-memory", "1024Mi",
+		"--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -272,7 +288,9 @@ func TestServiceCreateWithLimits(t *testing.T) {
 
 func TestServiceCreateRequestsLimitsCPU(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--requests-cpu", "250m", "--limits-cpu", "1000m", "--no-wait"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
+		"--requests-cpu", "250m", "--limits-cpu", "1000m",
+		"--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -309,7 +327,10 @@ func TestServiceCreateRequestsLimitsCPU(t *testing.T) {
 
 func TestServiceCreateRequestsLimitsMemory(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--requests-memory", "64Mi", "--limits-memory", "1024Mi", "--no-wait"}, false, false)
+		"service", "create", "foo",
+		"--image", "gcr.io/foo/bar:baz",
+		"--requests-memory", "64Mi",
+		"--limits-memory", "1024Mi", "--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -347,7 +368,9 @@ func TestServiceCreateRequestsLimitsMemory(t *testing.T) {
 func TestServiceCreateMaxMinScale(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
 		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
-		"--min-scale", "1", "--max-scale", "5", "--concurrency-target", "10", "--concurrency-limit", "100", "--no-wait"}, false, false)
+		"--min-scale", "1", "--max-scale", "5",
+		"--concurrency-target", "10", "--concurrency-limit", "100",
+		"--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -384,7 +407,8 @@ func TestServiceCreateRequestsLimitsCPUMemory(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
 		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--requests-cpu", "250m", "--limits-cpu", "1000m",
-		"--requests-memory", "64Mi", "--limits-memory", "1024Mi", "--no-wait"}, false, false)
+		"--requests-memory", "64Mi", "--limits-memory", "1024Mi",
+		"--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -431,7 +455,7 @@ func parseQuantity(t *testing.T, quantityString string) resource.Quantity {
 
 func TestServiceCreateImageExistsAndNoForce(t *testing.T) {
 	_, _, output, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:v2", "--no-wait"}, true, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:v2", "--no-wait"}, true)
 	if err == nil {
 		t.Fatal(err)
 	}
@@ -446,7 +470,7 @@ func TestServiceCreateImageExistsAndNoForce(t *testing.T) {
 
 func TestServiceCreateImageForce(t *testing.T) {
 	action, created, output, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--force", "--image", "gcr.io/foo/bar:v2", "--no-wait"}, true, false)
+		"service", "create", "foo", "--force", "--image", "gcr.io/foo/bar:v2", "--no-wait"}, true)
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("update", "services") {
@@ -464,12 +488,14 @@ func TestServiceCreateImageForce(t *testing.T) {
 
 func TestServiceCreateEnvForce(t *testing.T) {
 	_, _, _, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--image", "gcr.io/foo/bar:v1", "-e", "A=DOGS", "--env", "B=WOLVES", "--no-wait"}, false, false)
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:v1",
+		"-e", "A=DOGS", "--env", "B=WOLVES", "--no-wait"}, false)
 	if err != nil {
 		t.Fatal(err)
 	}
 	action, created, output, err := fakeServiceCreate([]string{
-		"service", "create", "foo", "--force", "--image", "gcr.io/foo/bar:v2", "-e", "A=CATS", "--env", "B=LIONS", "--no-wait"}, false, false)
+		"service", "create", "foo", "--force", "--image", "gcr.io/foo/bar:v2",
+		"-e", "A=CATS", "--env", "B=LIONS", "--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
@@ -503,7 +529,7 @@ func TestServiceCreateWithServiceAccountName(t *testing.T) {
 	action, created, _, err := fakeServiceCreate([]string{
 		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz",
 		"--service-account", "foo-bar-account",
-		"--no-wait"}, false, false)
+		"--no-wait"}, false)
 
 	if err != nil {
 		t.Fatal(err)
diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go
index d80be908..a04fd0e9 100644
--- a/pkg/kn/commands/service/update_test.go
+++ b/pkg/kn/commands/service/update_test.go
@@ -42,13 +42,14 @@ var exampleImageByDigest = "gcr.io/foo/bar@sha256:deadbeefdeadbeef"
 var exampleRevisionName = "foo-asdf"
 var exampleRevisionName2 = "foo-xyzzy"
 
-func fakeServiceUpdate(original *servingv1.Service, args []string, sync bool) (
+func fakeServiceUpdate(original *servingv1.Service, args []string) (
 	action clienttesting.Action,
 	updated *servingv1.Service,
 	output string,
 	err error) {
 	var reconciled servingv1.Service
 	knParams := &commands.KnParams{}
+	sync := !noWait(args)
 	cmd, fakeServing, buf := commands.CreateTestKnCommand(NewServiceCommand(knParams), knParams)
 	fakeServing.AddReactor("update", "*",
 		func(a clienttesting.Action) (bool, runtime.Object, error) {
@@ -88,6 +89,7 @@ func fakeServiceUpdate(original *servingv1.Service, args []string, sync bool) (
 			rev.Status.ImageDigest = exampleImageByDigest
 			return true, rev, nil
 		})
+
 	if sync {
 		fakeServing.AddWatchReactor("services",
 			func(a clienttesting.Action) (bool, watch.Interface, error) {
@@ -118,9 +120,7 @@ func fakeServiceUpdate(original *servingv1.Service, args []string, sync bool) (
 func TestServcieUpdateNoFlags(t *testing.T) {
 	orig := newEmptyService()
 
-	action, _, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo"}, false)
-
+	action, _, _, err := fakeServiceUpdate(orig, []string{"service", "update", "foo"})
 	if action != nil {
 		t.Errorf("Unexpected action if no flag(s) set")
 	}
@@ -145,7 +145,7 @@ func TestServiceUpdateImageSync(t *testing.T) {
 	}
 
 	action, updated, output, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar"}, true)
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar"})
 
 	assert.NilError(t, err)
 	assert.Assert(t, action.Matches("update", "services"))
@@ -167,7 +167,7 @@ func TestServiceUpdateImage(t *testing.T) {
 	}
 
 	action, updated, output, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--no-wait"}, false)
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -191,6 +191,14 @@ func TestServiceUpdateImage(t *testing.T) {
 	}
 }
 
+func TestServiceUpdateWithMultipleImages(t *testing.T) {
+	orig := newEmptyService()
+	_, _, _, err := fakeServiceUpdate(orig, []string{
+		"service", "create", "foo", "--image", "gcr.io/foo/bar:baz", "--image", "gcr.io/bar/foo:baz", "--no-wait"})
+
+	assert.Assert(t, util.ContainsAll(err.Error(), "\"--image\"", "\"gcr.io/bar/foo:baz\"", "flag", "once"))
+}
+
 func TestServiceUpdateCommand(t *testing.T) {
 	orig := newEmptyService()
 
@@ -199,7 +207,7 @@ func TestServiceUpdateCommand(t *testing.T) {
 	err := servinglib.UpdateContainerCommand(origTemplate, "./start")
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--cmd", "/app/start", "--no-wait"}, false)
+		"service", "update", "foo", "--cmd", "/app/start", "--no-wait"})
 	assert.NilError(t, err)
 	assert.Assert(t, action.Matches("update", "services"))
 
@@ -216,7 +224,7 @@ func TestServiceUpdateArg(t *testing.T) {
 	assert.NilError(t, err)
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--arg", "myArg1", "--arg", "--myArg2", "--arg", "--myArg3=3", "--no-wait"}, false)
+		"service", "update", "foo", "--arg", "myArg1", "--arg", "--myArg2", "--arg", "--myArg3=3", "--no-wait"})
 	assert.NilError(t, err)
 	assert.Assert(t, action.Matches("update", "services"))
 
@@ -232,7 +240,7 @@ func TestServiceUpdateRevisionNameExplicit(t *testing.T) {
 
 	// Test user provides prefix
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--revision-name", "foo-dogs", "--namespace", "bar", "--no-wait"}, false)
+		"service", "update", "foo", "--revision-name", "foo-dogs", "--namespace", "bar", "--no-wait"})
 	assert.NilError(t, err)
 	if !action.Matches("update", "services") {
 		t.Fatalf("Bad action %v", action)
@@ -250,7 +258,7 @@ func TestServiceUpdateRevisionNameGenerated(t *testing.T) {
 
 	// Test prefix added by command
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--no-wait"}, false)
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--no-wait"})
 	assert.NilError(t, err)
 	if !action.Matches("update", "services") {
 		t.Fatalf("Bad action %v", action)
@@ -268,7 +276,7 @@ func TestServiceUpdateRevisionNameCleared(t *testing.T) {
 	template.Name = "foo-asdf"
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--revision-name=", "--no-wait"}, false)
+		"service", "update", "foo", "--image", "gcr.io/foo/quux:xyzzy", "--namespace", "bar", "--revision-name=", "--no-wait"})
 
 	assert.NilError(t, err)
 	if !action.Matches("update", "services") {
@@ -287,7 +295,7 @@ func TestServiceUpdateRevisionNameNoMutationNoChange(t *testing.T) {
 
 	// Test prefix added by command
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "--namespace", "bar", "--no-wait"}, false)
+		"service", "update", "foo", "--namespace", "bar", "--no-wait"})
 	assert.NilError(t, err)
 	if !action.Matches("update", "services") {
 		t.Fatalf("Bad action %v", action)
@@ -302,7 +310,7 @@ func TestServiceUpdateMaxMinScale(t *testing.T) {
 
 	action, updated, _, err := fakeServiceUpdate(original, []string{
 		"service", "update", "foo",
-		"--min-scale", "1", "--max-scale", "5", "--concurrency-target", "10", "--concurrency-limit", "100", "--no-wait"}, false)
+		"--min-scale", "1", "--max-scale", "5", "--concurrency-target", "10", "--concurrency-limit", "100", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -348,7 +356,7 @@ func TestServiceUpdateEnv(t *testing.T) {
 	servinglib.UpdateImage(template, "gcr.io/foo/bar:baz")
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "-e", "TARGET=Awesome", "--env", "EXISTING-", "--env=OTHEREXISTING-=whatever", "--no-wait"}, false)
+		"service", "update", "foo", "-e", "TARGET=Awesome", "--env", "EXISTING-", "--env=OTHEREXISTING-=whatever", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -377,7 +385,7 @@ func TestServiceUpdatePinsToDigestWhenAsked(t *testing.T) {
 	}
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "-e", "TARGET=Awesome", "--lock-to-digest", "--no-wait"}, false)
+		"service", "update", "foo", "-e", "TARGET=Awesome", "--lock-to-digest", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -400,7 +408,7 @@ func TestServiceUpdatePinsToDigestWhenPreviouslyDidSo(t *testing.T) {
 	}
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "-e", "TARGET=Awesome", "--no-wait"}, false)
+		"service", "update", "foo", "-e", "TARGET=Awesome", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -420,7 +428,7 @@ func TestServiceUpdateDoesntPinToDigestWhenUnAsked(t *testing.T) {
 	err := servinglib.UpdateImage(&template, "gcr.io/foo/bar:baz")
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "-e", "TARGET=Awesome", "--no-lock-to-digest", "--no-wait"}, false)
+		"service", "update", "foo", "-e", "TARGET=Awesome", "--no-lock-to-digest", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -446,7 +454,7 @@ func TestServiceUpdateDoesntPinToDigestWhenPreviouslyDidnt(t *testing.T) {
 	}
 
 	action, updated, _, err := fakeServiceUpdate(orig, []string{
-		"service", "update", "foo", "-e", "TARGET=Awesome", "--no-wait"}, false)
+		"service", "update", "foo", "-e", "TARGET=Awesome", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -465,7 +473,7 @@ func TestServiceUpdateRequestsLimitsCPU(t *testing.T) {
 	service := createMockServiceWithResources(t, "250", "64Mi", "1000m", "1024Mi")
 
 	action, updated, _, err := fakeServiceUpdate(service, []string{
-		"service", "update", "foo", "--requests-cpu", "500m", "--limits-cpu", "1000m", "--no-wait"}, false)
+		"service", "update", "foo", "--requests-cpu", "500m", "--limits-cpu", "1000m", "--no-wait"})
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("update", "services") {
@@ -503,7 +511,7 @@ func TestServiceUpdateRequestsLimitsMemory(t *testing.T) {
 	service := createMockServiceWithResources(t, "100m", "64Mi", "1000m", "1024Mi")
 
 	action, updated, _, err := fakeServiceUpdate(service, []string{
-		"service", "update", "foo", "--requests-memory", "128Mi", "--limits-memory", "2048Mi", "--no-wait"}, false)
+		"service", "update", "foo", "--requests-memory", "128Mi", "--limits-memory", "2048Mi", "--no-wait"})
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("update", "services") {
@@ -543,7 +551,7 @@ func TestServiceUpdateRequestsLimitsCPU_and_Memory(t *testing.T) {
 	action, updated, _, err := fakeServiceUpdate(service, []string{
 		"service", "update", "foo",
 		"--requests-cpu", "500m", "--limits-cpu", "2000m",
-		"--requests-memory", "128Mi", "--limits-memory", "2048Mi", "--no-wait"}, false)
+		"--requests-memory", "128Mi", "--limits-memory", "2048Mi", "--no-wait"})
 	if err != nil {
 		t.Fatal(err)
 	} else if !action.Matches("update", "services") {
@@ -587,7 +595,7 @@ func TestServiceUpdateLabelWhenEmpty(t *testing.T) {
 	origContainer.Image = "gcr.io/foo/bar:latest"
 
 	action, updated, _, err := fakeServiceUpdate(original, []string{
-		"service", "update", "foo", "-l", "a=mouse", "--label", "b=cookie", "-l=single", "--no-wait"}, false)
+		"service", "update", "foo", "-l", "a=mouse", "--label", "b=cookie", "-l=single", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -620,7 +628,7 @@ func TestServiceUpdateLabelExisting(t *testing.T) {
 	originalTemplate.ObjectMeta.Labels = map[string]string{"already": "here", "tobe": "removed"}
 
 	action, updated, _, err := fakeServiceUpdate(original, []string{
-		"service", "update", "foo", "-l", "already=gone", "--label=tobe-", "--label", "b=", "--no-wait"}, false)
+		"service", "update", "foo", "-l", "already=gone", "--label=tobe-", "--label", "b=", "--no-wait"})
 
 	if err != nil {
 		t.Fatal(err)
@@ -678,3 +686,12 @@ func createMockServiceWithResources(t *testing.T, requestCPU, requestMemory, lim
 
 	return service
 }
+
+func noWait(args []string) bool {
+	for _, arg := range args {
+		if arg == "--no-wait" {
+			return true
+		}
+	}
+	return false
+}
diff --git a/test/e2e/trigger_test.go b/test/e2e/trigger_test.go
new file mode 100644
index 00000000..ff4e5495
--- /dev/null
+++ b/test/e2e/trigger_test.go
@@ -0,0 +1,126 @@
+// Copyright 2020 The Knative 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 im
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// +build e2e
+// +build !serving
+
+package e2e
+
+import (
+	"strings"
+	"testing"
+	"time"
+
+	"gotest.tools/assert"
+	"k8s.io/apimachinery/pkg/util/wait"
+	"knative.dev/client/pkg/util"
+)
+
+func TestBrokerTrigger(t *testing.T) {
+	t.Parallel()
+	test := NewE2eTest(t)
+	test.Setup(t)
+	defer test.Teardown(t)
+
+	err := test.lableNamespaceForDefaultBroker(t)
+	assert.NilError(t, err)
+	test.serviceCreate(t, "sinksvc0")
+	test.serviceCreate(t, "sinksvc1")
+
+	t.Run("create triggers and list them", func(t *testing.T) {
+		test.triggerCreate(t, "trigger1", []string{"a=b"}, "sinksvc0")
+		test.triggerCreate(t, "trigger2", []string{"type=knative.dev.bar", "source=cronjob"}, "sinksvc1")
+		test.verifyTriggerList(t, []string{"trigger1", "trigger2"})
+	})
+
+	t.Run("create a trigger and delete it", func(t *testing.T) {
+		test.triggerCreate(t, "deltrigger", []string{"a=b"}, "sinksvc0")
+		test.triggerDelete(t, "deltrigger")
+		test.verifyTriggerNotfound(t, "deltrigger")
+	})
+
+	t.Run("create a trigger, describe and update it", func(t *testing.T) {
+		test.triggerCreate(t, "updtrigger", []string{"a=b"}, "sinksvc0")
+		test.verifyTriggerDescribe(t, "updtrigger", []string{"a", "b"}, "default", "sinksvc0")
+		test.triggerUpdate(t, "updtrigger", "type=knative.dev.bar", "sinksvc1")
+		test.verifyTriggerDescribe(t, "updtrigger", []string{"a", "b", "type", "knative.dev.bar"}, "default", "sinksvc1")
+	})
+
+	t.Run("create trigger with error return", func(t *testing.T) {
+		test.triggerCreateMissingSink(t, "errtrigger", "notfound")
+	})
+}
+
+func (test *e2eTest) lableNamespaceForDefaultBroker(t *testing.T) error {
+	kubectl := kubectl{t, Logger{}}
+
+	_, err := kubectl.RunWithOpts([]string{"label", "namespace", test.kn.namespace, "knative-eventing-injection=enabled"}, runOpts{})
+	if err != nil {
+		t.Fatalf("Error executing 'kubectl label namespace %s knative-eventing-injection=enabled'. Error: %s", test.kn.namespace, err.Error())
+	}
+
+	return wait.PollImmediate(10*time.Second, 5*time.Minute, func() (bool, error) {
+		out, err := kubectl.RunWithOpts([]string{"get", "broker", "-n", test.kn.namespace, "-o=jsonpath='{.items[0].status.conditions[?(@.type==\"Ready\")].status}'"}, runOpts{AllowError: true})
+		if err != nil {
+			return false, nil
+		} else {
+			return strings.Contains(out, "True"), nil
+		}
+	})
+}
+
+func (test *e2eTest) triggerCreate(t *testing.T, name string, filters []string, sinksvc string) {
+	args := []string{"trigger", "create", name, "--broker", "default", "--sink", "svc:" + sinksvc}
+	for _, v := range filters {
+		args = append(args, "--filter", v)
+	}
+	out, err := test.kn.RunWithOpts(args, runOpts{NoNamespace: false})
+	assert.NilError(t, err)
+	assert.Check(t, util.ContainsAllIgnoreCase(out, "Trigger", name, "created", "namespace", test.kn.namespace))
+}
+
+func (test *e2eTest) triggerCreateMissingSink(t *testing.T, name string, sinksvc string) {
+	_, err := test.kn.RunWithOpts([]string{"trigger", "create", name, "--broker", "default", "--sink", "svc:" + sinksvc}, runOpts{NoNamespace: false, AllowError: true})
+	assert.ErrorContains(t, err, "services.serving.knative.dev", "not found")
+}
+
+func (test *e2eTest) triggerDelete(t *testing.T, name string) {
+	out, err := test.kn.RunWithOpts([]string{"trigger", "delete", name}, runOpts{NoNamespace: false})
+	assert.NilError(t, err)
+	assert.Check(t, util.ContainsAllIgnoreCase(out, "Trigger", name, "deleted", "namespace", test.kn.namespace))
+}
+
+func (test *e2eTest) triggerUpdate(t *testing.T, name string, filter string, sinksvc string) {
+	out, err := test.kn.RunWithOpts([]string{"trigger", "update", name, "--filter", filter, "--sink", "svc:" + sinksvc}, runOpts{NoNamespace: false})
+	assert.NilError(t, err)
+	assert.Check(t, util.ContainsAllIgnoreCase(out, "Trigger", name, "updated", "namespace", test.kn.namespace))
+}
+
+func (test *e2eTest) verifyTriggerList(t *testing.T, triggers []string) {
+	out, err := test.kn.RunWithOpts([]string{"trigger", "list"}, runOpts{NoNamespace: false})
+	assert.NilError(t, err)
+	assert.Check(t, util.ContainsAllIgnoreCase(out, triggers...))
+}
+
+func (test *e2eTest) verifyTriggerDescribe(t *testing.T, name string, filters []string, broker string, sink string) {
+	out, err := test.kn.RunWithOpts([]string{"trigger", "describe", name}, runOpts{NoNamespace: false})
+	assert.NilError(t, err)
+	assert.Check(t, util.ContainsAllIgnoreCase(out, filters...))
+	assert.Check(t, util.ContainsAllIgnoreCase(out, name, broker, sink))
+}
+
+func (test *e2eTest) verifyTriggerNotfound(t *testing.T, name string) {
+	_, err := test.kn.RunWithOpts([]string{"trigger", "describe", name}, runOpts{NoNamespace: false, AllowError: true})
+	assert.ErrorContains(t, err, name, "not found")
+}