chore(trigger): Cleanup Trigger create API (#561)

* further updates

* chore: Fix calls to apiserver create

* fix namespace issue

* fix

docs/cmd/kn_source_apiserver_create.md

@@ -26,9 +26,8 @@ kn source apiserver create NAME --resource RESOURCE --service-account ACCOUNTNAM
                                  "Ref" sends only the reference to the resource,
                                  "Resource" send the full resource. (default "Ref")
   -n, --namespace string         Specify the namespace to operate in.
-      --resource strings         Comma seperate Kind:APIVersion:isController list, e.g. Event:v1:true.
-                                 "APIVersion" and "isControler" can be omitted.
-                                 "APIVersion" is "v1" by default, "isController" is "false" by default.
+      --resource strings         Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true.
+                                 "isController" can be omitted and is "false" by default.
       --service-account string   Name of the service account to use to run this source
   -s, --sink string              Addressable sink for events
 ```

docs/cmd/kn_source_apiserver_update.md

@@ -26,9 +26,8 @@ kn source apiserver update NAME --resource RESOURCE --service-account ACCOUNTNAM
                                  "Ref" sends only the reference to the resource,
                                  "Resource" send the full resource. (default "Ref")
   -n, --namespace string         Specify the namespace to operate in.
-      --resource strings         Comma seperate Kind:APIVersion:isController list, e.g. Event:v1:true.
-                                 "APIVersion" and "isControler" can be omitted.
-                                 "APIVersion" is "v1" by default, "isController" is "false" by default.
+      --resource strings         Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true.
+                                 "isController" can be omitted and is "false" by default.
       --service-account string   Name of the service account to use to run this source
   -s, --sink string              Addressable sink for events
 ```

pkg/eventing/v1alpha1/client.go

@@ -18,12 +18,13 @@ import (
 	apis_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	kn_errors "knative.dev/client/pkg/errors"
-	"knative.dev/client/pkg/util"
 	"knative.dev/eventing/pkg/apis/eventing/v1alpha1"
 	"knative.dev/eventing/pkg/client/clientset/versioned/scheme"
 	client_v1alpha1 "knative.dev/eventing/pkg/client/clientset/versioned/typed/eventing/v1alpha1"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
+
+	kn_errors "knative.dev/client/pkg/errors"
+	"knative.dev/client/pkg/util"
 )
 
 const (
@@ -36,7 +37,7 @@ type KnEventingClient interface {
 	// Namespace in which this client is operating for
 	Namespace() string
 	// CreateTrigger is used to create an instance of trigger
-	CreateTrigger(trigger *v1alpha1.Trigger) (*v1alpha1.Trigger, error)
+	CreateTrigger(trigger *v1alpha1.Trigger) error
 	// DeleteTrigger is used to delete an instance of trigger
 	DeleteTrigger(name string) error
 	// GetTrigger is used to get an instance of trigger
@@ -64,12 +65,12 @@ func NewKnEventingClient(client client_v1alpha1.EventingV1alpha1Interface, names
 }
 
 //CreateTrigger is used to create an instance of trigger
-func (c *knEventingClient) CreateTrigger(trigger *v1alpha1.Trigger) (*v1alpha1.Trigger, error) {
+func (c *knEventingClient) CreateTrigger(trigger *v1alpha1.Trigger) error {
 	trigger, err := c.client.Triggers(c.namespace).Create(trigger)
 	if err != nil {
-		return nil, kn_errors.GetError(err)
+		return kn_errors.GetError(err)
 	}
-	return trigger, nil
+	return nil
 }
 
 //DeleteTrigger is used to delete an instance of trigger
@@ -147,50 +148,53 @@ func NewTriggerBuilder(name string) *TriggerBuilder {
 }
 
 // NewTriggerBuilderFromExisting for building the object from existing Trigger object
-func NewTriggerBuilderFromExisting(tr *v1alpha1.Trigger) *TriggerBuilder {
-	return &TriggerBuilder{trigger: tr.DeepCopy()}
+func NewTriggerBuilderFromExisting(trigger *v1alpha1.Trigger) *TriggerBuilder {
+	return &TriggerBuilder{trigger: trigger.DeepCopy()}
 }
 
-// Broker to set the broker of trigger object
-func (b *TriggerBuilder) Broker(broker string) *TriggerBuilder {
-	if broker != "" {
-		b.trigger.Spec.Broker = broker
-	}
+// Namespace for this trigger
+func (b *TriggerBuilder) Namespace(ns string) *TriggerBuilder {
+	b.trigger.Namespace = ns
 	return b
 }
 
-// Filters to set the filters of trigger object
-func (b *TriggerBuilder) Filters(filters map[string]string) *TriggerBuilder {
-	if filters != nil {
-		triggerFilterAttributes := v1alpha1.TriggerFilterAttributes(filters)
-		b.trigger.Spec.Filter = &v1alpha1.TriggerFilter{
-			Attributes: &triggerFilterAttributes,
-		}
-	}
+// Subscriber for the trigger to send to (it's a Sink actually)
+func (b *TriggerBuilder) Subscriber(subscriber *duckv1.Destination) *TriggerBuilder {
+	b.trigger.Spec.Subscriber = subscriber
 	return b
 }
 
-// UpdateFilters to update the filters of trigger object
-func (b *TriggerBuilder) UpdateFilters(toUpdate map[string]string, toRemove []string) *TriggerBuilder {
-	if b.trigger.Spec.Filter == nil {
-		b.Filters(toUpdate)
-		return b
-	}
+// Broker to set the broker of trigger object
+func (b *TriggerBuilder) Broker(broker string) *TriggerBuilder {
+	b.trigger.Spec.Broker = broker
+	return b
+}
 
-	existing := map[string]string(*b.trigger.Spec.Filter.Attributes)
-	for key, value := range toUpdate {
-		existing[key] = value
+func (b *TriggerBuilder) AddFilter(key, value string) *TriggerBuilder {
+	filter := b.trigger.Spec.Filter
+	if filter == nil {
+		filter = &v1alpha1.TriggerFilter{}
+		b.trigger.Spec.Filter = filter
 	}
-	for _, key := range toRemove {
-		delete(existing, key)
+	attributes := filter.Attributes
+	if attributes == nil {
+		attributes = &v1alpha1.TriggerFilterAttributes{}
+		filter.Attributes = attributes
 	}
-	b.Filters(existing)
+	(*attributes)[key] = value
 	return b
 }
 
-// Sink to set the subscriber of trigger object
-func (b *TriggerBuilder) Sink(sink *duckv1.Destination) *TriggerBuilder {
-	b.trigger.Spec.Subscriber = sink
+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
+	}
+	delete(*attributes, key)
 	return b
 }
 

pkg/eventing/v1alpha1/client_mock.go

@@ -60,13 +60,13 @@ func (c *MockKnEventingClient) Namespace() string {
 
 // CreateTrigger records a call for CreateCronJobSource with the expected error
 func (sr *EventingRecorder) CreateTrigger(trigger interface{}, err error) {
-	sr.r.Add("CreateTrigger", []interface{}{trigger}, []interface{}{trigger, err})
+	sr.r.Add("CreateTrigger", []interface{}{trigger}, []interface{}{err})
 }
 
 // CreateTrigger performs a previously recorded action
-func (c *MockKnEventingClient) CreateTrigger(trigger *v1alpha1.Trigger) (*v1alpha1.Trigger, error) {
+func (c *MockKnEventingClient) CreateTrigger(trigger *v1alpha1.Trigger) error {
 	call := c.recorder.r.VerifyCall("CreateTrigger", trigger)
-	return call.Result[0].(*v1alpha1.Trigger), mock.ErrorOrNil(call.Result[1])
+	return mock.ErrorOrNil(call.Result[0])
 }
 
 // GetTrigger records a call for GetTrigger with the expected object or error. Either trigger or err should be nil

pkg/eventing/v1alpha1/client_test.go

@@ -73,14 +73,12 @@ func TestCreateTrigger(t *testing.T) {
 		})
 
 	t.Run("create trigger without error", func(t *testing.T) {
-		ins, err := client.CreateTrigger(objNew)
+		err := client.CreateTrigger(objNew)
 		assert.NilError(t, err)
-		assert.Equal(t, ins.Name, name)
-		assert.Equal(t, ins.Namespace, testNamespace)
 	})
 
 	t.Run("create trigger with an error returns an error object", func(t *testing.T) {
-		_, err := client.CreateTrigger(newTrigger("unknown"))
+		err := client.CreateTrigger(newTrigger("unknown"))
 		assert.ErrorContains(t, err, "unknown")
 	})
 }
@@ -130,12 +128,12 @@ func TestListTrigger(t *testing.T) {
 
 func TestTriggerBuilder(t *testing.T) {
 	a := NewTriggerBuilder("testtrigger")
-	a.Filters(map[string]string{"type": "foo", "source": "bar"})
+	a.AddFilter("type", "foo").AddFilter("source", "bar")
 
 	t.Run("update filter", func(t *testing.T) {
 		b := NewTriggerBuilderFromExisting(a.Build())
 		assert.DeepEqual(t, b.Build(), a.Build())
-		b.UpdateFilters(map[string]string{"type": "new"}, []string{"source"})
+		b.AddFilter("type", "new").RemoveFilter("source")
 		expected := &v1alpha1.TriggerFilter{
 			Attributes: &v1alpha1.TriggerFilterAttributes{
 				"type": "new",
@@ -147,7 +145,7 @@ func TestTriggerBuilder(t *testing.T) {
 	t.Run("update filter with only deletions", func(t *testing.T) {
 		b := NewTriggerBuilderFromExisting(a.Build())
 		assert.DeepEqual(t, b.Build(), a.Build())
-		b.UpdateFilters(nil, []string{"source"})
+		b.RemoveFilter("source")
 		expected := &v1alpha1.TriggerFilter{
 			Attributes: &v1alpha1.TriggerFilterAttributes{
 				"type": "foo",
@@ -159,7 +157,7 @@ func TestTriggerBuilder(t *testing.T) {
 	t.Run("update filter with only updates", func(t *testing.T) {
 		b := NewTriggerBuilderFromExisting(a.Build())
 		assert.DeepEqual(t, b.Build(), a.Build())
-		b.UpdateFilters(map[string]string{"type": "new"}, nil)
+		b.AddFilter("type", "new")
 		expected := &v1alpha1.TriggerFilter{
 			Attributes: &v1alpha1.TriggerFilterAttributes{
 				"type":   "new",
@@ -168,14 +166,12 @@ func TestTriggerBuilder(t *testing.T) {
 		}
 		assert.DeepEqual(t, expected, b.Build().Spec.Filter)
 	})
-
 }
 
 func newTrigger(name string) *v1alpha1.Trigger {
-	b := NewTriggerBuilder(name)
-	b.Filters(map[string]string{"type": "foo"})
-	b.Broker("default")
-	b.trigger.Name = name
-	b.trigger.Namespace = testNamespace
-	return b.Build()
+	return NewTriggerBuilder(name).
+		Namespace(testNamespace).
+		Broker("default").
+		AddFilter("type", "foo").
+		Build()
 }

pkg/kn/commands/source/apiserver/create.go

@@ -61,22 +61,27 @@ func NewAPIServerCreateCommand(p *commands.KnParams) *cobra.Command {
 			if err != nil {
 				return fmt.Errorf(
 					"cannot create ApiServerSource '%s' in namespace '%s' "+
-						"because %s", name, namespace, err)
+						"because: %s", name, namespace, err)
 			}
 
 			// create
+			resources, err := updateFlags.GetAPIServerResourceArray()
+			if err != nil {
+				return err
+			}
+
 			err = apiSourceClient.CreateAPIServerSource(
 				v1alpha1.NewAPIServerSourceBuilder(name).
-					Resources(updateFlags.GetAPIServerResourceArray()).
 					ServiceAccount(updateFlags.ServiceAccountName).
 					Mode(updateFlags.Mode).
+					Resources(*resources).
 					Sink(objectRef).
 					Build())
 
 			if err != nil {
 				return fmt.Errorf(
 					"cannot create ApiServerSource '%s' in namespace '%s' "+
-						"because %s", name, namespace, err)
+						"because: %s", name, namespace, err)
 			}
 
 			if err == nil {

pkg/kn/commands/source/apiserver/create_flag_test.go

@@ -28,8 +28,8 @@ func TestGetAPIServerResourceArray(t *testing.T) {
 			Mode:               "Ref",
 			Resources:          []string{"Service:serving.knative.dev/v1alpha1:true"},
 		}
-		created := createFlag.GetAPIServerResourceArray()
-		wanted := []sources_v1alpha1.ApiServerResource{{
+		created, _ := createFlag.GetAPIServerResourceArray()
+		wanted := &[]sources_v1alpha1.ApiServerResource{{
 			APIVersion: "serving.knative.dev/v1alpha1",
 			Kind:       "Service",
 			Controller: true,
@@ -42,8 +42,8 @@ func TestGetAPIServerResourceArray(t *testing.T) {
 			Mode:               "Ref",
 			Resources:          []string{"Service:serving.knative.dev/v1alpha1"},
 		}
-		created = createFlag.GetAPIServerResourceArray()
-		wanted = []sources_v1alpha1.ApiServerResource{{
+		created, _ = createFlag.GetAPIServerResourceArray()
+		wanted = &[]sources_v1alpha1.ApiServerResource{{
 			APIVersion: "serving.knative.dev/v1alpha1",
 			Kind:       "Service",
 			Controller: false,
@@ -54,10 +54,10 @@ func TestGetAPIServerResourceArray(t *testing.T) {
 		createFlag = APIServerSourceUpdateFlags{
 			ServiceAccountName: "test-sa",
 			Mode:               "Ref",
-			Resources:          []string{"Service"},
+			Resources:          []string{"Service:v1"},
 		}
-		created = createFlag.GetAPIServerResourceArray()
-		wanted = []sources_v1alpha1.ApiServerResource{{
+		created, _ = createFlag.GetAPIServerResourceArray()
+		wanted = &[]sources_v1alpha1.ApiServerResource{{
 			APIVersion: "v1",
 			Kind:       "Service",
 			Controller: false,
@@ -71,8 +71,8 @@ func TestGetAPIServerResourceArray(t *testing.T) {
 			Mode:               "Resource",
 			Resources:          []string{"Event:v1:true", "Pod:v2:false"},
 		}
-		created := createFlag.GetAPIServerResourceArray()
-		wanted := []sources_v1alpha1.ApiServerResource{{
+		created, _ := createFlag.GetAPIServerResourceArray()
+		wanted := &[]sources_v1alpha1.ApiServerResource{{
 			APIVersion: "v1",
 			Kind:       "Event",
 			Controller: true,
@@ -87,11 +87,11 @@ func TestGetAPIServerResourceArray(t *testing.T) {
 		createFlag = APIServerSourceUpdateFlags{
 			ServiceAccountName: "test-sa",
 			Mode:               "Resource",
-			Resources:          []string{"Event", "Pod"},
+			Resources:          []string{"Event:v1", "Pod:v1"},
 		}
-		created = createFlag.GetAPIServerResourceArray()
+		created, _ = createFlag.GetAPIServerResourceArray()
 
-		wanted = []sources_v1alpha1.ApiServerResource{{
+		wanted = &[]sources_v1alpha1.ApiServerResource{{
 			APIVersion: "v1",
 			Kind:       "Event",
 			Controller: false,

pkg/kn/commands/source/apiserver/create_test.go

@@ -52,7 +52,7 @@ func TestSinkNotFoundError(t *testing.T) {
 	servingClient := knserving_client.NewMockKnServiceClient(t)
 	apiServerClient := knsources_v1alpha1.NewMockKnAPIServerSourceClient(t)
 
-	errorMsg := "cannot create ApiServerSource 'testsource' in namespace 'default' because no Service svc found"
+	errorMsg := "cannot create ApiServerSource 'testsource' in namespace 'default' because: no Service svc found"
 	servingRecorder := servingClient.Recorder()
 	servingRecorder.GetService("testsvc", nil, errors.New("no Service svc found"))