diff --git a/docs/cmd/kn_trigger_create.md b/docs/cmd/kn_trigger_create.md index 62990fd47c..f087b1818c 100644 --- a/docs/cmd/kn_trigger_create.md +++ b/docs/cmd/kn_trigger_create.md @@ -22,7 +22,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 []string Key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo + --filter strings Key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo -h, --help help for create -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 4980eef54d..8cfc77d2ae 100644 --- a/docs/cmd/kn_trigger_update.md +++ b/docs/cmd/kn_trigger_update.md @@ -29,7 +29,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 []string Key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo + --filter strings Key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo -h, --help help for update -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 c546fd67b4..10933ce5ae 100644 --- a/pkg/eventing/v1alpha1/client.go +++ b/pkg/eventing/v1alpha1/client.go @@ -170,7 +170,7 @@ func (b *TriggerBuilder) Broker(broker string) *TriggerBuilder { return b } -func (b *TriggerBuilder) AddFilter(key, value string) *TriggerBuilder { +func (b *TriggerBuilder) Filters(filters map[string]string) *TriggerBuilder { filter := b.trigger.Spec.Filter if filter == nil { filter = &v1alpha1.TriggerFilter{} @@ -181,20 +181,9 @@ func (b *TriggerBuilder) AddFilter(key, value string) *TriggerBuilder { attributes = &v1alpha1.TriggerFilterAttributes{} filter.Attributes = attributes } - (*attributes)[key] = value - return b -} - -func (b *TriggerBuilder) RemoveFilter(key string) *TriggerBuilder { - filter := b.trigger.Spec.Filter - if filter == nil { - return b - } - attributes := filter.Attributes - if attributes == nil { - return b + for k, v := range filters { + (*attributes)[k] = v } - delete(*attributes, key) return b } diff --git a/pkg/eventing/v1alpha1/client_test.go b/pkg/eventing/v1alpha1/client_test.go index 84f7c996a6..ce2ef5adec 100644 --- a/pkg/eventing/v1alpha1/client_test.go +++ b/pkg/eventing/v1alpha1/client_test.go @@ -128,12 +128,12 @@ func TestListTrigger(t *testing.T) { func TestTriggerBuilder(t *testing.T) { a := NewTriggerBuilder("testtrigger") - a.AddFilter("type", "foo").AddFilter("source", "bar") + a.Filters(map[string]string{"type": "foo"}) - t.Run("update filter", func(t *testing.T) { + t.Run("update filters", func(t *testing.T) { b := NewTriggerBuilderFromExisting(a.Build()) assert.DeepEqual(t, b.Build(), a.Build()) - b.AddFilter("type", "new").RemoveFilter("source") + b.Filters(map[string]string{"type": "new"}) expected := &v1alpha1.TriggerFilter{ Attributes: &v1alpha1.TriggerFilterAttributes{ "type": "new", @@ -142,22 +142,10 @@ func TestTriggerBuilder(t *testing.T) { assert.DeepEqual(t, expected, b.Build().Spec.Filter) }) - t.Run("update filter with only deletions", func(t *testing.T) { + t.Run("update filters with both new entry and updated entry", func(t *testing.T) { b := NewTriggerBuilderFromExisting(a.Build()) assert.DeepEqual(t, b.Build(), a.Build()) - b.RemoveFilter("source") - expected := &v1alpha1.TriggerFilter{ - Attributes: &v1alpha1.TriggerFilterAttributes{ - "type": "foo", - }, - } - assert.DeepEqual(t, expected, b.Build().Spec.Filter) - }) - - t.Run("update filter with only updates", func(t *testing.T) { - b := NewTriggerBuilderFromExisting(a.Build()) - assert.DeepEqual(t, b.Build(), a.Build()) - b.AddFilter("type", "new") + b.Filters(map[string]string{"type": "new", "source": "bar"}) expected := &v1alpha1.TriggerFilter{ Attributes: &v1alpha1.TriggerFilterAttributes{ "type": "new", @@ -172,6 +160,6 @@ func newTrigger(name string) *v1alpha1.Trigger { return NewTriggerBuilder(name). Namespace(testNamespace). Broker("default"). - AddFilter("type", "foo"). + Filters(map[string]string{"type": "foo"}). Build() } diff --git a/pkg/kn/commands/trigger/create.go b/pkg/kn/commands/trigger/create.go index 5d82b12240..906633fe02 100644 --- a/pkg/kn/commands/trigger/create.go +++ b/pkg/kn/commands/trigger/create.go @@ -20,10 +20,9 @@ import ( "github.com/spf13/cobra" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + client_v1alpha1 "knative.dev/client/pkg/eventing/v1alpha1" "knative.dev/client/pkg/kn/commands" "knative.dev/client/pkg/kn/commands/flags" - "knative.dev/eventing/pkg/apis/eventing/v1alpha1" duckv1 "knative.dev/pkg/apis/duck/v1" ) @@ -73,19 +72,18 @@ func NewTriggerCreateCommand(p *commands.KnParams) *cobra.Command { "cannot create trigger '%s' "+ "because %s", name, err) } - if filters == nil { - return fmt.Errorf( - "cannot create trigger '%s' "+ - "because filters are required", name) - } - trigger := constructTrigger(name, namespace, triggerUpdateFlags.Broker, filters) - trigger.Spec.Subscriber = &duckv1.Destination{ - Ref: objectRef.Ref, - URI: objectRef.URI, - } - - err = eventingClient.CreateTrigger(trigger) + triggerBuilder := client_v1alpha1. + NewTriggerBuilder(name). + Namespace(namespace). + Broker(triggerUpdateFlags.Broker). + Filters(filters). + Subscriber(&duckv1.Destination{ + Ref: objectRef.Ref, + URI: objectRef.URI, + }) + + err = eventingClient.CreateTrigger(triggerBuilder.Build()) if err != nil { return fmt.Errorf( "cannot create trigger '%s' in namespace '%s' "+ @@ -99,27 +97,6 @@ func NewTriggerCreateCommand(p *commands.KnParams) *cobra.Command { triggerUpdateFlags.Add(cmd) sinkFlags.Add(cmd) cmd.MarkFlagRequired("sink") - cmd.MarkFlagRequired("filter") return cmd } - -// constructTrigger is to create an instance of v1alpha1.Trigger -func constructTrigger(name string, namespace string, broker string, filters map[string]string) *v1alpha1.Trigger { - trigger := v1alpha1.Trigger{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - }, - Spec: v1alpha1.TriggerSpec{ - Broker: broker, - }, - } - - triggerFilterAttributes := v1alpha1.TriggerFilterAttributes(filters) - trigger.Spec.Filter = &v1alpha1.TriggerFilter{ - Attributes: &triggerFilterAttributes, - } - - return &trigger -} diff --git a/pkg/kn/commands/trigger/create_test.go b/pkg/kn/commands/trigger/create_test.go index e6d1ebace9..f01019fc31 100644 --- a/pkg/kn/commands/trigger/create_test.go +++ b/pkg/kn/commands/trigger/create_test.go @@ -67,13 +67,6 @@ func TestNoSinkError(t *testing.T) { assert.ErrorContains(t, err, "required flag(s)", "sink", "not set") } -func TestNoFilterError(t *testing.T) { - eventingClient := eventing_client.NewMockKnEventingClient(t) - _, err := executeTriggerCommand(eventingClient, nil, "create", triggerName, "--broker", "mybroker", - "--sink", "svc:mysvc") - assert.ErrorContains(t, err, "required flag(s)", "filter", "not set") -} - func TestTriggerCreateMultipleFilter(t *testing.T) { eventingClient := eventing_client.NewMockKnEventingClient(t) dynamicClient := dynamic_fake.CreateFakeKnDynamicClient("default", &serving_v1alpha1.Service{ @@ -91,3 +84,20 @@ func TestTriggerCreateMultipleFilter(t *testing.T) { eventingRecorder.Validate() } + +func TestTriggerCreateWithoutFilter(t *testing.T) { + eventingClient := eventing_client.NewMockKnEventingClient(t) + dynamicClient := dynamic_fake.CreateFakeKnDynamicClient("default", &serving_v1alpha1.Service{ + TypeMeta: metav1.TypeMeta{Kind: "Service", APIVersion: "serving.knative.dev/v1alpha1"}, + ObjectMeta: metav1.ObjectMeta{Name: "mysvc", Namespace: "default"}, + }) + + eventingRecorder := eventingClient.Recorder() + eventingRecorder.CreateTrigger(createTrigger("default", triggerName, nil, "mybroker", "mysvc"), nil) + + out, err := executeTriggerCommand(eventingClient, dynamicClient, "create", triggerName, "--broker", "mybroker", "--sink", "svc:mysvc") + assert.NilError(t, err, "Trigger should be created") + util.ContainsAll(out, "Trigger", triggerName, "created", "namespace", "default") + + eventingRecorder.Validate() +} diff --git a/pkg/kn/commands/trigger/trigger_test.go b/pkg/kn/commands/trigger/trigger_test.go index 9c09d00db4..05e58cc70f 100644 --- a/pkg/kn/commands/trigger/trigger_test.go +++ b/pkg/kn/commands/trigger/trigger_test.go @@ -78,20 +78,16 @@ func executeTriggerCommand(triggerClient eventc_v1alpha1.KnEventingClient, dynam func createTrigger(namespace string, name string, filters map[string]string, broker string, svcname string) *v1alpha1.Trigger { triggerBuilder := eventc_v1alpha1.NewTriggerBuilder(name). Namespace(namespace). - Broker(broker) - - for k, v := range filters { - triggerBuilder.AddFilter(k, v) - } - - triggerBuilder.Subscriber(&duckv1.Destination{ - Ref: &corev1.ObjectReference{ - Name: svcname, - Kind: "Service", - Namespace: "default", - APIVersion: "serving.knative.dev/v1alpha1", - }, - }) + Broker(broker). + Filters(filters). + Subscriber(&duckv1.Destination{ + Ref: &corev1.ObjectReference{ + Name: svcname, + Kind: "Service", + Namespace: "default", + APIVersion: "serving.knative.dev/v1alpha1", + }, + }) return triggerBuilder.Build() } diff --git a/pkg/kn/commands/trigger/update.go b/pkg/kn/commands/trigger/update.go index be63a63d37..16b95c6d29 100644 --- a/pkg/kn/commands/trigger/update.go +++ b/pkg/kn/commands/trigger/update.go @@ -23,6 +23,8 @@ import ( client_v1alpha1 "knative.dev/client/pkg/eventing/v1alpha1" "knative.dev/client/pkg/kn/commands" "knative.dev/client/pkg/kn/commands/flags" + "knative.dev/client/pkg/util" + "knative.dev/eventing/pkg/apis/eventing/v1alpha1" duckv1 "knative.dev/pkg/apis/duck/v1" ) @@ -82,12 +84,8 @@ func NewTriggerUpdateCommand(p *commands.KnParams) *cobra.Command { return fmt.Errorf( "cannot update trigger '%s' because %s", name, err) } - for k, v := range updated { - b.AddFilter(k, v) - } - for _, k := range removed { - b.RemoveFilter(k) - } + existing := extractFilters(trigger) + b.Filters(existing.Merge(updated).Remove(removed)) } if cmd.Flags().Changed("sink") { destination, err := sinkFlags.ResolveSink(dynamicClient, namespace) @@ -112,3 +110,13 @@ func NewTriggerUpdateCommand(p *commands.KnParams) *cobra.Command { return cmd } + +func extractFilters(trigger *v1alpha1.Trigger) util.StringMap { + attributes := make(util.StringMap) + if trigger.Spec.Filter != nil && trigger.Spec.Filter.Attributes != nil { + for k, v := range *trigger.Spec.Filter.Attributes { + attributes[k] = v + } + } + return attributes +} diff --git a/pkg/kn/commands/trigger/update_flags.go b/pkg/kn/commands/trigger/update_flags.go index 9cabb07029..061f283674 100644 --- a/pkg/kn/commands/trigger/update_flags.go +++ b/pkg/kn/commands/trigger/update_flags.go @@ -21,32 +21,13 @@ import ( "knative.dev/client/pkg/util" ) -type filterArray []string - -func (filters *filterArray) String() string { - str := "" - for _, filter := range *filters { - str = str + filter + " " - } - return str -} - -func (filters *filterArray) Set(value string) error { - *filters = append(*filters, value) - return nil -} - -func (filters *filterArray) Type() string { - return "[]string" -} - // TriggerUpdateFlags are flags for create and update a trigger type TriggerUpdateFlags struct { Broker string - Filters filterArray + Filters []string } -// GetFilter to return a map type of filters +// GetFilters to return a map type of filters func (f *TriggerUpdateFlags) GetFilters() (map[string]string, error) { filters, err := util.MapFromArray(f.Filters, "=") if err != nil { @@ -55,7 +36,7 @@ func (f *TriggerUpdateFlags) GetFilters() (map[string]string, error) { return filters, nil } -// GetFilter to return a map type of filters +// GetUpdateFilters to return a map type of filters func (f *TriggerUpdateFlags) GetUpdateFilters() (map[string]string, []string, error) { filters, err := util.MapFromArrayAllowingSingles(f.Filters, "=") if err != nil { @@ -68,5 +49,5 @@ 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().Var(&f.Filters, "filter", "Key-value pair for exact CloudEvent attribute matching against incoming events, e.g type=dev.knative.foo") + 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/pkg/kn/commands/trigger/update_flags_test.go b/pkg/kn/commands/trigger/update_flags_test.go index 3a9b3c0c63..703a7fcdfc 100644 --- a/pkg/kn/commands/trigger/update_flags_test.go +++ b/pkg/kn/commands/trigger/update_flags_test.go @@ -24,7 +24,7 @@ import ( func TestGetFilters(t *testing.T) { t.Run("get multiple filters", func(t *testing.T) { createFlag := TriggerUpdateFlags{ - Filters: filterArray{"type=abc.edf.ghi", "attr=value"}, + Filters: []string{"type=abc.edf.ghi", "attr=value"}, } created, err := createFlag.GetFilters() wanted := map[string]string{ @@ -37,26 +37,26 @@ func TestGetFilters(t *testing.T) { t.Run("get filters with errors", func(t *testing.T) { createFlag := TriggerUpdateFlags{ - Filters: filterArray{"type"}, + Filters: []string{"type"}, } _, err := createFlag.GetFilters() assert.ErrorContains(t, err, "Invalid --filter") createFlag = TriggerUpdateFlags{ - Filters: filterArray{"type="}, + Filters: []string{"type="}, } filters, err := createFlag.GetFilters() wanted := map[string]string{"type": ""} assert.DeepEqual(t, wanted, filters) createFlag = TriggerUpdateFlags{ - Filters: filterArray{"=value"}, + Filters: []string{"=value"}, } _, err = createFlag.GetFilters() assert.ErrorContains(t, err, "Invalid --filter") createFlag = TriggerUpdateFlags{ - Filters: filterArray{"="}, + Filters: []string{"="}, } _, err = createFlag.GetFilters() assert.ErrorContains(t, err, "Invalid --filter") @@ -64,7 +64,7 @@ func TestGetFilters(t *testing.T) { t.Run("get duplicate filters", func(t *testing.T) { createFlag := TriggerUpdateFlags{ - Filters: filterArray{"type=foo", "type=bar"}, + Filters: []string{"type=foo", "type=bar"}, } _, err := createFlag.GetFilters() assert.ErrorContains(t, err, "duplicate") @@ -74,7 +74,7 @@ func TestGetFilters(t *testing.T) { func TestGetUpdateFilters(t *testing.T) { t.Run("get updated filters", func(t *testing.T) { createFlag := TriggerUpdateFlags{ - Filters: filterArray{"type=abc.edf.ghi", "attr=value"}, + Filters: []string{"type=abc.edf.ghi", "attr=value"}, } updated, removed, err := createFlag.GetUpdateFilters() wanted := map[string]string{ @@ -88,7 +88,7 @@ func TestGetUpdateFilters(t *testing.T) { t.Run("get deleted filters", func(t *testing.T) { createFlag := TriggerUpdateFlags{ - Filters: filterArray{"type-", "attr-"}, + Filters: []string{"type-", "attr-"}, } updated, removed, err := createFlag.GetUpdateFilters() wanted := []string{"type", "attr"} @@ -101,7 +101,7 @@ func TestGetUpdateFilters(t *testing.T) { t.Run("get updated & deleted filters", func(t *testing.T) { createFlag := TriggerUpdateFlags{ - Filters: filterArray{"type=foo", "attr-", "source=bar", "env-"}, + Filters: []string{"type=foo", "attr-", "source=bar", "env-"}, } updated, removed, err := createFlag.GetUpdateFilters() wantedRemoved := []string{"attr", "env"} @@ -118,7 +118,7 @@ func TestGetUpdateFilters(t *testing.T) { t.Run("update duplicate filters", func(t *testing.T) { createFlag := TriggerUpdateFlags{ - Filters: filterArray{"type=foo", "type=bar"}, + Filters: []string{"type=foo", "type=bar"}, } _, _, err := createFlag.GetUpdateFilters() assert.ErrorContains(t, err, "duplicate") diff --git a/pkg/util/parsing_helper.go b/pkg/util/parsing_helper.go index 8bd77f5ade..eaf070bc95 100644 --- a/pkg/util/parsing_helper.go +++ b/pkg/util/parsing_helper.go @@ -50,6 +50,16 @@ func MapFromArray(arr []string, delimiter string) (map[string]string, error) { return mapFromArray(arr, delimiter, false) } +func Add(original *map[string]string, toAdd map[string]string, toRemove []string) map[string]string { + for k, v := range toAdd { + (*original)[k] = v + } + for _, k := range toRemove { + delete(*original, k) + } + return *original +} + func ParseMinusSuffix(m map[string]string) []string { stringToRemove := []string{} for key := range m { @@ -61,6 +71,25 @@ func ParseMinusSuffix(m map[string]string) []string { return stringToRemove } +// StringMap is a map which key and value are strings +type StringMap map[string]string + +// Merge to merge a map to a StringMap +func (m StringMap) Merge(toMerge map[string]string) StringMap { + for k, v := range toMerge { + m[k] = v + } + return m +} + +// Remove to remove from StringMap +func (m StringMap) Remove(toRemove []string) StringMap { + for _, k := range toRemove { + delete(m, k) + } + return m +} + // mapFromArray takes an array of strings where each item is a (key, value) pair // separated by a delimiter and returns a map where keys are mapped to their respective values. // If allowSingles is true, values without a delimiter will be added as keys pointing to empty strings diff --git a/pkg/util/parsing_helper_test.go b/pkg/util/parsing_helper_test.go index b04757d6b7..e7d135178d 100644 --- a/pkg/util/parsing_helper_test.go +++ b/pkg/util/parsing_helper_test.go @@ -92,3 +92,13 @@ func TestParseMinusSuffix(t *testing.T) { assert.DeepEqual(t, expectedMap, inputMap) assert.DeepEqual(t, expectedStringToRemove, stringToRemove) } + +func TestStringMap(t *testing.T) { + inputMap := StringMap{"a1": "b1", "a2": "b2"} + mergedMap := map[string]string{"a1": "b1-new", "a3": "b3"} + removedKeys := []string{"a2", "a4"} + + inputMap.Merge(mergedMap).Remove(removedKeys) + expectedMap := StringMap{"a1": "b1-new", "a3": "b3"} + assert.DeepEqual(t, expectedMap, inputMap) +}