From 0544cd4effe6c01c26c0ad2ec0f1611f3ac20113 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 10 Mar 2020 11:57:22 +0100 Subject: [PATCH 1/5] feat(trigger): annotate trigger to inject default broker --- docs/cmd/kn_trigger_create.md | 1 + docs/cmd/kn_trigger_update.md | 1 + pkg/eventing/v1alpha1/client.go | 6 ++- pkg/eventing/v1alpha1/client_test.go | 2 +- pkg/kn/commands/trigger/create.go | 2 +- pkg/kn/commands/trigger/trigger_test.go | 2 +- pkg/kn/commands/trigger/update_flags.go | 6 ++- test/e2e/trigger_inject_broker_test.go | 59 +++++++++++++++++++++++++ 8 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 test/e2e/trigger_inject_broker_test.go diff --git a/docs/cmd/kn_trigger_create.md b/docs/cmd/kn_trigger_create.md index f0211d7c4f..62510d41c1 100644 --- a/docs/cmd/kn_trigger_create.md +++ b/docs/cmd/kn_trigger_create.md @@ -24,6 +24,7 @@ kn trigger create NAME --broker BROKER --filter KEY=VALUE --sink SINK [flags] --broker string Name of the Broker which the trigger associates with. (default "default") --filter strings Key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo -h, --help help for create + --inject-broker Create new broker with name default through common annotation -n, --namespace string Specify the namespace to operate in. -s, --sink string Addressable sink for events ``` diff --git a/docs/cmd/kn_trigger_update.md b/docs/cmd/kn_trigger_update.md index 3ac46d6ba6..4dab1adb5d 100644 --- a/docs/cmd/kn_trigger_update.md +++ b/docs/cmd/kn_trigger_update.md @@ -31,6 +31,7 @@ kn trigger update NAME --filter KEY=VALUE --sink SINK [flags] --broker string Name of the Broker which the trigger associates with. (default "default") --filter strings Key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo -h, --help help for update + --inject-broker Create new broker with name default through common annotation -n, --namespace string Specify the namespace to operate in. -s, --sink string Addressable sink for events ``` diff --git a/pkg/eventing/v1alpha1/client.go b/pkg/eventing/v1alpha1/client.go index de62a9c5d4..fc2efc4dc4 100644 --- a/pkg/eventing/v1alpha1/client.go +++ b/pkg/eventing/v1alpha1/client.go @@ -161,8 +161,12 @@ func (b *TriggerBuilder) Subscriber(subscriber *duckv1.Destination) *TriggerBuil } // Broker to set the broker of trigger object -func (b *TriggerBuilder) Broker(broker string) *TriggerBuilder { +func (b *TriggerBuilder) Broker(broker string, inject bool) *TriggerBuilder { b.trigger.Spec.Broker = broker + if inject { + meta_v1.SetMetaDataAnnotation(&b.trigger.ObjectMeta, "knative-eventing-injection", "enabled") + } + return b } diff --git a/pkg/eventing/v1alpha1/client_test.go b/pkg/eventing/v1alpha1/client_test.go index ce2ef5adec..614c786862 100644 --- a/pkg/eventing/v1alpha1/client_test.go +++ b/pkg/eventing/v1alpha1/client_test.go @@ -159,7 +159,7 @@ func TestTriggerBuilder(t *testing.T) { func newTrigger(name string) *v1alpha1.Trigger { return NewTriggerBuilder(name). Namespace(testNamespace). - Broker("default"). + Broker("default", false). Filters(map[string]string{"type": "foo"}). Build() } diff --git a/pkg/kn/commands/trigger/create.go b/pkg/kn/commands/trigger/create.go index 065d3b911d..769a68c4d8 100644 --- a/pkg/kn/commands/trigger/create.go +++ b/pkg/kn/commands/trigger/create.go @@ -77,7 +77,7 @@ func NewTriggerCreateCommand(p *commands.KnParams) *cobra.Command { triggerBuilder := client_v1alpha1. NewTriggerBuilder(name). Namespace(namespace). - Broker(triggerUpdateFlags.Broker). + Broker(triggerUpdateFlags.Broker, triggerUpdateFlags.InjectBroker). Filters(filters). Subscriber(&duckv1.Destination{ Ref: objectRef.Ref, diff --git a/pkg/kn/commands/trigger/trigger_test.go b/pkg/kn/commands/trigger/trigger_test.go index 89e5dff47e..a87d71f13c 100644 --- a/pkg/kn/commands/trigger/trigger_test.go +++ b/pkg/kn/commands/trigger/trigger_test.go @@ -78,7 +78,7 @@ func executeTriggerCommand(triggerClient eventc_v1alpha1.KnEventingClient, dynam func createTrigger(namespace string, name string, filters map[string]string, broker string, svcname string) *v1alpha1.Trigger { return eventc_v1alpha1.NewTriggerBuilder(name). Namespace(namespace). - Broker(broker). + Broker(broker, false). Filters(filters). Subscriber(createServiceSink(svcname)). Build() diff --git a/pkg/kn/commands/trigger/update_flags.go b/pkg/kn/commands/trigger/update_flags.go index 061f283674..0825c487aa 100644 --- a/pkg/kn/commands/trigger/update_flags.go +++ b/pkg/kn/commands/trigger/update_flags.go @@ -23,8 +23,9 @@ import ( // TriggerUpdateFlags are flags for create and update a trigger type TriggerUpdateFlags struct { - Broker string - Filters []string + Broker string + InjectBroker bool + Filters []string } // GetFilters to return a map type of filters @@ -49,5 +50,6 @@ func (f *TriggerUpdateFlags) GetUpdateFilters() (map[string]string, []string, er //Add is to set parameters func (f *TriggerUpdateFlags) Add(cmd *cobra.Command) { cmd.Flags().StringVar(&f.Broker, "broker", "default", "Name of the Broker which the trigger associates with.") + cmd.Flags().BoolVar(&f.InjectBroker, "inject-broker", false, "Create new broker with name default through common annotation") cmd.Flags().StringSliceVar(&f.Filters, "filter", nil, "Key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo") } diff --git a/test/e2e/trigger_inject_broker_test.go b/test/e2e/trigger_inject_broker_test.go new file mode 100644 index 0000000000..b99b4c8d8c --- /dev/null +++ b/test/e2e/trigger_inject_broker_test.go @@ -0,0 +1,59 @@ +// 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 ( + "testing" + + "gotest.tools/assert" + "knative.dev/client/pkg/util" +) + +func TestInjectBrokerTrigger(t *testing.T) { + t.Parallel() + test, err := NewE2eTest() + assert.NilError(t, err) + defer func() { + assert.NilError(t, test.Teardown()) + }() + + r := NewKnRunResultCollector(t) + defer r.DumpIfFailed() + + assert.NilError(t, err) + + test.serviceCreate(t, r, "sinksvc0") + test.serviceCreate(t, r, "sinksvc1") + + t.Log("create triggers and list them") + test.triggerCreateWithInject(t, r, "trigger1", "sinksvc0", []string{"a=b"}) + test.triggerCreateWithInject(t, r, "trigger2", "sinksvc1", []string{"type=knative.dev.bar", "source=ping"}) + test.verifyTriggerList(t, r, "trigger1", "trigger2") + test.triggerDelete(t, r, "trigger1") + test.triggerDelete(t, r, "trigger2") +} + +func (test *e2eTest) triggerCreateWithInject(t *testing.T, r *KnRunResultCollector, name string, sinksvc string, filters []string) { + args := []string{"trigger", "create", name, "--broker", "default", "--inject-broker", "--sink", "svc:" + sinksvc} + for _, v := range filters { + args = append(args, "--filter", v) + } + out := test.kn.Run(args...) + r.AssertNoError(out) + assert.Check(t, util.ContainsAllIgnoreCase(out.Stdout, "Trigger", name, "created", "namespace", test.kn.namespace)) +} From cb701850b707e21c83fb665d069cd3bde88c49bf Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 10 Mar 2020 14:09:08 +0100 Subject: [PATCH 2/5] chore: update changelog --- CHANGELOG.adoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 27191a42e9..c038631a45 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -85,6 +85,10 @@ | 🎁 | add kn service export command for exporting a service | https://github.com/knative/client/pull/669[#669] + +| 🎁 +| Add flag --inject-broker to trigger create command +| https://github.com/knative/client/pull/726[#726] |=== ## v0.12.0 (2020-01-29) From 6027bca8756c3aec6eb2e7c46f54815094f8b57c Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 10 Mar 2020 17:31:03 +0100 Subject: [PATCH 3/5] fix: reflect review feedback --- pkg/eventing/v1alpha1/client.go | 12 ++++++++---- pkg/eventing/v1alpha1/client_test.go | 14 +++++++++++++- pkg/kn/commands/trigger/create.go | 6 +++++- pkg/kn/commands/trigger/create_test.go | 18 ++++++++++++++++++ pkg/kn/commands/trigger/trigger_test.go | 7 ++++++- 5 files changed, 50 insertions(+), 7 deletions(-) diff --git a/pkg/eventing/v1alpha1/client.go b/pkg/eventing/v1alpha1/client.go index fc2efc4dc4..4961be8035 100644 --- a/pkg/eventing/v1alpha1/client.go +++ b/pkg/eventing/v1alpha1/client.go @@ -161,11 +161,15 @@ func (b *TriggerBuilder) Subscriber(subscriber *duckv1.Destination) *TriggerBuil } // Broker to set the broker of trigger object -func (b *TriggerBuilder) Broker(broker string, inject bool) *TriggerBuilder { +func (b *TriggerBuilder) Broker(broker string) *TriggerBuilder { b.trigger.Spec.Broker = broker - if inject { - meta_v1.SetMetaDataAnnotation(&b.trigger.ObjectMeta, "knative-eventing-injection", "enabled") - } + + return b +} + +// InjectBroker to add annotation to setup default broker +func (b *TriggerBuilder) InjectBroker() *TriggerBuilder { + meta_v1.SetMetaDataAnnotation(&b.trigger.ObjectMeta, v1alpha1.InjectionAnnotation, "enabled") return b } diff --git a/pkg/eventing/v1alpha1/client_test.go b/pkg/eventing/v1alpha1/client_test.go index 614c786862..c60ac47801 100644 --- a/pkg/eventing/v1alpha1/client_test.go +++ b/pkg/eventing/v1alpha1/client_test.go @@ -154,12 +154,24 @@ func TestTriggerBuilder(t *testing.T) { } assert.DeepEqual(t, expected, b.Build().Spec.Filter) }) + + t.Run("add inject annotation", func(t *testing.T) { + b := NewTriggerBuilder("broker-trigger") + b.InjectBroker() + expected := &metav1.ObjectMeta{ + Annotations: map[string]string{ + v1alpha1.InjectionAnnotation: "enabled", + }, + } + assert.DeepEqual(t, expected.Annotations, b.Build().ObjectMeta.Annotations) + + }) } func newTrigger(name string) *v1alpha1.Trigger { return NewTriggerBuilder(name). Namespace(testNamespace). - Broker("default", false). + Broker("default"). Filters(map[string]string{"type": "foo"}). Build() } diff --git a/pkg/kn/commands/trigger/create.go b/pkg/kn/commands/trigger/create.go index 769a68c4d8..5db3560b59 100644 --- a/pkg/kn/commands/trigger/create.go +++ b/pkg/kn/commands/trigger/create.go @@ -77,12 +77,16 @@ func NewTriggerCreateCommand(p *commands.KnParams) *cobra.Command { triggerBuilder := client_v1alpha1. NewTriggerBuilder(name). Namespace(namespace). - Broker(triggerUpdateFlags.Broker, triggerUpdateFlags.InjectBroker). + Broker(triggerUpdateFlags.Broker). Filters(filters). Subscriber(&duckv1.Destination{ Ref: objectRef.Ref, URI: objectRef.URI, }) + // add inject annotation only condition are satisfied + if triggerUpdateFlags.Broker == "default" && triggerUpdateFlags.InjectBroker { + triggerBuilder.InjectBroker() + } err = eventingClient.CreateTrigger(triggerBuilder.Build()) if err != nil { diff --git a/pkg/kn/commands/trigger/create_test.go b/pkg/kn/commands/trigger/create_test.go index 102c66f352..dff4e130c7 100644 --- a/pkg/kn/commands/trigger/create_test.go +++ b/pkg/kn/commands/trigger/create_test.go @@ -49,6 +49,24 @@ func TestTriggerCreate(t *testing.T) { eventingRecorder.Validate() } +func TestTriggerWithInjectCreate(t *testing.T) { + eventingClient := clienteventingv1alpha1.NewMockKnEventingClient(t) + dynamicClient := dynamicfake.CreateFakeKnDynamicClient("default", &servingv1.Service{ + TypeMeta: metav1.TypeMeta{Kind: "Service", APIVersion: "serving.knative.dev/v1"}, + ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "default"}, + }) + + eventingRecorder := eventingClient.Recorder() + eventingRecorder.CreateTrigger(createTriggerWithInject("default", triggerName, map[string]string{"type": "dev.knative.foo"}, "default", "mysvc"), nil) + + out, err := executeTriggerCommand(eventingClient, dynamicClient, "create", triggerName, "--broker", "default", "--inject-broker", + "--filter", "type=dev.knative.foo", "--sink", "svc:mysvc") + assert.NilError(t, err, "Trigger should be created") + util.ContainsAll(out, "Trigger", triggerName, "created", "namespace", "default") + + eventingRecorder.Validate() +} + func TestSinkNotFoundError(t *testing.T) { eventingClient := clienteventingv1alpha1.NewMockKnEventingClient(t) dynamicClient := dynamicfake.CreateFakeKnDynamicClient("default") diff --git a/pkg/kn/commands/trigger/trigger_test.go b/pkg/kn/commands/trigger/trigger_test.go index a87d71f13c..d6d5c1af8d 100644 --- a/pkg/kn/commands/trigger/trigger_test.go +++ b/pkg/kn/commands/trigger/trigger_test.go @@ -78,12 +78,17 @@ func executeTriggerCommand(triggerClient eventc_v1alpha1.KnEventingClient, dynam func createTrigger(namespace string, name string, filters map[string]string, broker string, svcname string) *v1alpha1.Trigger { return eventc_v1alpha1.NewTriggerBuilder(name). Namespace(namespace). - Broker(broker, false). + Broker(broker). Filters(filters). Subscriber(createServiceSink(svcname)). Build() } +func createTriggerWithInject(namespace string, name string, filters map[string]string, broker string, svcname string) *v1alpha1.Trigger { + t := createTrigger(namespace, name, filters, broker, svcname) + return eventc_v1alpha1.NewTriggerBuilderFromExisting(t).InjectBroker().Build() +} + func createTriggerWithStatus(namespace string, name string, filters map[string]string, broker string, svcname string) *v1alpha1.Trigger { wanted := createTrigger(namespace, name, filters, broker, svcname) wanted.Status = v1alpha1.TriggerStatus{ From d5335679b681b44d65564ba0d491b14ca2574ed4 Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 10 Mar 2020 17:33:09 +0100 Subject: [PATCH 4/5] fix: code comment wording --- pkg/kn/commands/trigger/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kn/commands/trigger/create.go b/pkg/kn/commands/trigger/create.go index 5db3560b59..db2c45e20e 100644 --- a/pkg/kn/commands/trigger/create.go +++ b/pkg/kn/commands/trigger/create.go @@ -83,7 +83,7 @@ func NewTriggerCreateCommand(p *commands.KnParams) *cobra.Command { Ref: objectRef.Ref, URI: objectRef.URI, }) - // add inject annotation only condition are satisfied + // add inject annotation only if flag is present and broker name is `default` if triggerUpdateFlags.Broker == "default" && triggerUpdateFlags.InjectBroker { triggerBuilder.InjectBroker() } From 5baf7a3714a7eee81884e509304762b694dc237b Mon Sep 17 00:00:00 2001 From: David Simansky Date: Tue, 10 Mar 2020 18:58:23 +0100 Subject: [PATCH 5/5] fix: reflect review feedback --- pkg/eventing/v1alpha1/client.go | 11 ++++++++--- pkg/eventing/v1alpha1/client_test.go | 9 +++++++-- pkg/kn/commands/trigger/create.go | 11 ++++++++--- pkg/kn/commands/trigger/create_test.go | 12 ++++++++++++ pkg/kn/commands/trigger/trigger_test.go | 2 +- test/e2e/trigger_inject_broker_test.go | 6 ++++++ 6 files changed, 42 insertions(+), 9 deletions(-) diff --git a/pkg/eventing/v1alpha1/client.go b/pkg/eventing/v1alpha1/client.go index 4961be8035..49bee43ecb 100644 --- a/pkg/eventing/v1alpha1/client.go +++ b/pkg/eventing/v1alpha1/client.go @@ -168,9 +168,14 @@ func (b *TriggerBuilder) Broker(broker string) *TriggerBuilder { } // InjectBroker to add annotation to setup default broker -func (b *TriggerBuilder) InjectBroker() *TriggerBuilder { - meta_v1.SetMetaDataAnnotation(&b.trigger.ObjectMeta, v1alpha1.InjectionAnnotation, "enabled") - +func (b *TriggerBuilder) InjectBroker(inject bool) *TriggerBuilder { + if inject { + meta_v1.SetMetaDataAnnotation(&b.trigger.ObjectMeta, v1alpha1.InjectionAnnotation, "enabled") + } else { + if meta_v1.HasAnnotation(b.trigger.ObjectMeta, v1alpha1.InjectionAnnotation) { + delete(b.trigger.ObjectMeta.Annotations, v1alpha1.InjectionAnnotation) + } + } return b } diff --git a/pkg/eventing/v1alpha1/client_test.go b/pkg/eventing/v1alpha1/client_test.go index c60ac47801..351e6fc2d2 100644 --- a/pkg/eventing/v1alpha1/client_test.go +++ b/pkg/eventing/v1alpha1/client_test.go @@ -155,9 +155,9 @@ func TestTriggerBuilder(t *testing.T) { assert.DeepEqual(t, expected, b.Build().Spec.Filter) }) - t.Run("add inject annotation", func(t *testing.T) { + t.Run("add and remove inject annotation", func(t *testing.T) { b := NewTriggerBuilder("broker-trigger") - b.InjectBroker() + b.InjectBroker(true) expected := &metav1.ObjectMeta{ Annotations: map[string]string{ v1alpha1.InjectionAnnotation: "enabled", @@ -165,7 +165,12 @@ func TestTriggerBuilder(t *testing.T) { } assert.DeepEqual(t, expected.Annotations, b.Build().ObjectMeta.Annotations) + b = NewTriggerBuilderFromExisting(b.Build()) + b.InjectBroker(false) + assert.DeepEqual(t, make(map[string]string), b.Build().ObjectMeta.Annotations) + }) + } func newTrigger(name string) *v1alpha1.Trigger { diff --git a/pkg/kn/commands/trigger/create.go b/pkg/kn/commands/trigger/create.go index db2c45e20e..ecf0523f60 100644 --- a/pkg/kn/commands/trigger/create.go +++ b/pkg/kn/commands/trigger/create.go @@ -83,9 +83,14 @@ func NewTriggerCreateCommand(p *commands.KnParams) *cobra.Command { Ref: objectRef.Ref, URI: objectRef.URI, }) - // add inject annotation only if flag is present and broker name is `default` - if triggerUpdateFlags.Broker == "default" && triggerUpdateFlags.InjectBroker { - triggerBuilder.InjectBroker() + // add inject annotation only if flag broker name is `default` + if triggerUpdateFlags.InjectBroker { + if triggerUpdateFlags.Broker == "default" { + triggerBuilder.InjectBroker(true) + } else { + return fmt.Errorf("cannot create trigger '%s' in namespace '%s' "+ + "because broker name must be 'default' if '--inject-broker' flag is used", name, namespace) + } } err = eventingClient.CreateTrigger(triggerBuilder.Build()) diff --git a/pkg/kn/commands/trigger/create_test.go b/pkg/kn/commands/trigger/create_test.go index dff4e130c7..bc7150066a 100644 --- a/pkg/kn/commands/trigger/create_test.go +++ b/pkg/kn/commands/trigger/create_test.go @@ -67,6 +67,18 @@ func TestTriggerWithInjectCreate(t *testing.T) { eventingRecorder.Validate() } +func TestTriggetWithInjecError(t *testing.T) { + eventingClient := clienteventingv1alpha1.NewMockKnEventingClient(t) + dynamicClient := dynamicfake.CreateFakeKnDynamicClient("default", &servingv1.Service{ + TypeMeta: metav1.TypeMeta{Kind: "Service", APIVersion: "serving.knative.dev/v1"}, + ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "default"}, + }) + + _, err := executeTriggerCommand(eventingClient, dynamicClient, "create", triggerName, "--broker", "mybroker", "--inject-broker", + "--filter", "type=dev.knative.foo", "--sink", "svc:mysvc") + assert.ErrorContains(t, err, "broker", "name", "'default'", "--inject-broker", "flag") +} + func TestSinkNotFoundError(t *testing.T) { eventingClient := clienteventingv1alpha1.NewMockKnEventingClient(t) dynamicClient := dynamicfake.CreateFakeKnDynamicClient("default") diff --git a/pkg/kn/commands/trigger/trigger_test.go b/pkg/kn/commands/trigger/trigger_test.go index d6d5c1af8d..7cda60ad0a 100644 --- a/pkg/kn/commands/trigger/trigger_test.go +++ b/pkg/kn/commands/trigger/trigger_test.go @@ -86,7 +86,7 @@ func createTrigger(namespace string, name string, filters map[string]string, bro func createTriggerWithInject(namespace string, name string, filters map[string]string, broker string, svcname string) *v1alpha1.Trigger { t := createTrigger(namespace, name, filters, broker, svcname) - return eventc_v1alpha1.NewTriggerBuilderFromExisting(t).InjectBroker().Build() + return eventc_v1alpha1.NewTriggerBuilderFromExisting(t).InjectBroker(true).Build() } func createTriggerWithStatus(namespace string, name string, filters map[string]string, broker string, svcname string) *v1alpha1.Trigger { diff --git a/test/e2e/trigger_inject_broker_test.go b/test/e2e/trigger_inject_broker_test.go index b99b4c8d8c..234d96618b 100644 --- a/test/e2e/trigger_inject_broker_test.go +++ b/test/e2e/trigger_inject_broker_test.go @@ -46,6 +46,12 @@ func TestInjectBrokerTrigger(t *testing.T) { test.verifyTriggerList(t, r, "trigger1", "trigger2") test.triggerDelete(t, r, "trigger1") test.triggerDelete(t, r, "trigger2") + + t.Log("create trigger with error") + out := test.kn.Run("trigger", "create", "errorTrigger", "--broker", "mybroker", "--inject-broker", + "--sink", "svc:sinksvc0", "--filter", "a=b") + r.AssertError(out) + assert.Check(t, util.ContainsAllIgnoreCase(out.Stderr, "broker", "name", "'default'", "--inject-broker", "flag")) } func (test *e2eTest) triggerCreateWithInject(t *testing.T, r *KnRunResultCollector, name string, sinksvc string, filters []string) {