From b8124648479a205518f4ebec9392b465d24fe368 Mon Sep 17 00:00:00 2001 From: Daisy Guo Date: Mon, 13 Jan 2020 17:04:55 +0800 Subject: [PATCH] add more tests and update based on comments --- docs/cmd/kn_source_apiserver_create.md | 6 +- docs/cmd/kn_source_apiserver_update.md | 4 +- .../sources/v1alpha1/apiserver_client.go | 40 ---- pkg/kn/commands/source/apiserver/create.go | 6 +- pkg/kn/commands/source/apiserver/flags.go | 103 ++++++---- .../commands/source/apiserver/flags_test.go | 180 ++++++++++++------ pkg/kn/commands/source/apiserver/update.go | 12 +- pkg/util/parsing_helper.go | 4 +- pkg/util/parsing_helper_test.go | 12 +- 9 files changed, 213 insertions(+), 154 deletions(-) diff --git a/docs/cmd/kn_source_apiserver_create.md b/docs/cmd/kn_source_apiserver_create.md index 28df5a8f7c..4483d5f244 100644 --- a/docs/cmd/kn_source_apiserver_create.md +++ b/docs/cmd/kn_source_apiserver_create.md @@ -15,7 +15,7 @@ kn source apiserver create NAME --resource RESOURCE --service-account ACCOUNTNAM ``` # Create an ApiServerSource 'k8sevents' which consumes Kubernetes events and sends message to service 'mysvc' as a cloudevent - kn source apiserver create k8sevents --resource Event --service-account myaccountname --sink svc:mysvc + kn source apiserver create k8sevents --resource Event:v1 --service-account myaccountname --sink svc:mysvc ``` ### Options @@ -26,8 +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 stringArray 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. + --resource stringArray Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. "Event:v1:true". + "isController" can be omitted and is "false" by default, e.g. "Event:v1". --service-account string Name of the service account to use to run this source -s, --sink string Addressable sink for events ``` diff --git a/docs/cmd/kn_source_apiserver_update.md b/docs/cmd/kn_source_apiserver_update.md index 9face71071..f6c561685c 100644 --- a/docs/cmd/kn_source_apiserver_update.md +++ b/docs/cmd/kn_source_apiserver_update.md @@ -26,8 +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 stringArray 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. + --resource stringArray Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. "Event:v1:true". + "isController" can be omitted and is "false" by default, e.g. "Event:v1". --service-account string Name of the service account to use to run this source -s, --sink string Addressable sink for events ``` diff --git a/pkg/eventing/sources/v1alpha1/apiserver_client.go b/pkg/eventing/sources/v1alpha1/apiserver_client.go index 409300d584..3a29ffd20a 100644 --- a/pkg/eventing/sources/v1alpha1/apiserver_client.go +++ b/pkg/eventing/sources/v1alpha1/apiserver_client.go @@ -15,9 +15,6 @@ package v1alpha1 import ( - "fmt" - "reflect" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kn_errors "knative.dev/client/pkg/errors" "knative.dev/eventing/pkg/apis/sources/v1alpha1" @@ -159,43 +156,6 @@ func (b *APIServerSourceBuilder) Resources(resources []v1alpha1.ApiServerResourc return b } -// AddResource which should be streamed -func (b *APIServerSourceBuilder) AddResource(version string, kind string, isController bool) *APIServerSourceBuilder { - resources := b.apiServerSource.Spec.Resources - if resources == nil { - resources = []v1alpha1.ApiServerResource{} - b.apiServerSource.Spec.Resources = resources - } - resourceRef := v1alpha1.ApiServerResource{ - APIVersion: version, - Kind: kind, - Controller: isController, - } - b.apiServerSource.Spec.Resources = append(resources, resourceRef) - return b -} - -// RemoveResource which should be streamed -func (b *APIServerSourceBuilder) RemoveResource(version string, kind string, isController bool) (*APIServerSourceBuilder, error) { - resources := b.apiServerSource.Spec.Resources - if resources == nil { - resources = []v1alpha1.ApiServerResource{} - b.apiServerSource.Spec.Resources = resources - } - resourceRef := v1alpha1.ApiServerResource{ - APIVersion: version, - Kind: kind, - Controller: isController, - } - for i, k := range resources { - if reflect.DeepEqual(k, resourceRef) { - resources = append(resources[:i], resources[i+1:]...) - return b, nil - } - } - return b, fmt.Errorf("cannot find resource %s:%s:%t to remove", version, kind, isController) -} - // ServiceAccount with which this source should operate func (b *APIServerSourceBuilder) ServiceAccount(sa string) *APIServerSourceBuilder { b.apiServerSource.Spec.ServiceAccountName = sa diff --git a/pkg/kn/commands/source/apiserver/create.go b/pkg/kn/commands/source/apiserver/create.go index dd1f432561..00c004f98a 100644 --- a/pkg/kn/commands/source/apiserver/create.go +++ b/pkg/kn/commands/source/apiserver/create.go @@ -35,7 +35,7 @@ func NewAPIServerCreateCommand(p *commands.KnParams) *cobra.Command { Short: "Create an ApiServer source.", Example: ` # Create an ApiServerSource 'k8sevents' which consumes Kubernetes events and sends message to service 'mysvc' as a cloudevent - kn source apiserver create k8sevents --resource Event --service-account myaccountname --sink svc:mysvc`, + kn source apiserver create k8sevents --resource Event:v1 --service-account myaccountname --sink svc:mysvc`, RunE: func(cmd *cobra.Command, args []string) (err error) { if len(args) != 1 { @@ -71,9 +71,7 @@ func NewAPIServerCreateCommand(p *commands.KnParams) *cobra.Command { if err != nil { return err } - for _, k := range resources { - b.AddResource(k.ApiVersion, k.Kind, k.IsController) - } + b.Resources(resources) err = apiSourceClient.CreateAPIServerSource(b.Build()) diff --git a/pkg/kn/commands/source/apiserver/flags.go b/pkg/kn/commands/source/apiserver/flags.go index ac516b4d9b..8475f9e14c 100644 --- a/pkg/kn/commands/source/apiserver/flags.go +++ b/pkg/kn/commands/source/apiserver/flags.go @@ -16,6 +16,7 @@ package apiserver import ( "fmt" + "reflect" "sort" "strconv" "strings" @@ -41,15 +42,9 @@ type APIServerSourceUpdateFlags struct { Resources []string } -type resourceSpec struct { - Kind string - ApiVersion string - IsController bool -} - -// getAPIServerResourceArray is to return an array of ApiServerResource from a string. A sample is Event:v1:true,Pod:v2:false -func (f *APIServerSourceUpdateFlags) getAPIServerResourceArray() ([]resourceSpec, error) { - var resourceList []resourceSpec +// getAPIServerResourceArray is to construct an array of resources. +func (f *APIServerSourceUpdateFlags) getAPIServerResourceArray() ([]v1alpha1.ApiServerResource, error) { + var resourceList []v1alpha1.ApiServerResource for _, r := range f.Resources { resourceSpec, err := getValidResource(r) if err != nil { @@ -60,46 +55,88 @@ func (f *APIServerSourceUpdateFlags) getAPIServerResourceArray() ([]resourceSpec return resourceList, nil } -// getAPIServerResourceArray is to return an array of ApiServerResource from a string. A sample is Event:v1:true -func (f *APIServerSourceUpdateFlags) getUpdateAPIServerResourceArray() ([]resourceSpec, []resourceSpec, error) { - var added []resourceSpec - var removed []resourceSpec +// updateExistingAPIServerResourceArray is to update an array of resources. +func (f *APIServerSourceUpdateFlags) updateExistingAPIServerResourceArray(existing []v1alpha1.ApiServerResource) ([]v1alpha1.ApiServerResource, error) { + var found bool - addedArray, removedArray := util.AddListAndRemovalListFromArray(f.Resources) - for _, r := range addedArray { - resourceSpec, err := getValidResource(r) - if err != nil { - return nil, nil, err + added, removed, err := f.getUpdateAPIServerResourceArray() + if err != nil { + return nil, err + } + + if existing == nil { + existing = []v1alpha1.ApiServerResource{} + } + + existing = append(existing, added...) + + for _, item := range removed { + found = false + for i, ref := range existing { + if reflect.DeepEqual(item, ref) { + existing = append(existing[:i], existing[i+1:]...) + found = true + break + } + } + if !found { + return nil, fmt.Errorf("cannot find resource %s:%s:%t to remove", item.Kind, item.APIVersion, item.Controller) } - added = append(added, *resourceSpec) } - for _, r := range removedArray { + return existing, nil +} + +// getUpdateAPIServerResourceArray is to construct an array of resources for update action. +func (f *APIServerSourceUpdateFlags) getUpdateAPIServerResourceArray() ([]v1alpha1.ApiServerResource, []v1alpha1.ApiServerResource, error) { + addedArray, removedArray := util.AddedAndRemovalListsFromArray(f.Resources) + added, err := constructApiServerResourceArray(addedArray) + if err != nil { + return nil, nil, err + } + removed, err := constructApiServerResourceArray(removedArray) + if err != nil { + return nil, nil, err + } + return added, removed, nil +} + +func constructApiServerResourceArray(s []string) ([]v1alpha1.ApiServerResource, error) { + array := make([]v1alpha1.ApiServerResource, 0) + for _, r := range s { resourceSpec, err := getValidResource(r) if err != nil { - return nil, nil, err + return array, err } - removed = append(removed, *resourceSpec) + array = append(array, *resourceSpec) } - return added, removed, nil + return array, nil } -func getValidResource(resource string) (*resourceSpec, error) { - var isController = false //false as default +//getValidResource is to parse resource spec from a string +func getValidResource(resource string) (*v1alpha1.ApiServerResource, error) { + var isController bool var err error - parts := strings.Split(resource, apiVersionSplitChar) + parts := strings.SplitN(resource, apiVersionSplitChar, 3) + if len(parts[0]) == 0 { + return nil, fmt.Errorf("cannot find 'Kind' part in resource specification %s (expected: ", resource) + } kind := parts[0] - if len(parts) < 2 { - return nil, fmt.Errorf("no APIVersion given for resource %s", resource) + + if len(parts) < 2 || len(parts[1]) == 0 { + return nil, fmt.Errorf("cannot find 'APIVersion' part in resource specification %s (expected: ", resource) } version := parts[1] - if len(parts) >= 3 { + + if len(parts) >= 3 && len(parts[2]) > 0 { isController, err = strconv.ParseBool(parts[2]) if err != nil { - return nil, fmt.Errorf("cannot parse controller flage in resource specification %s", resource) + return nil, fmt.Errorf("controller flag is not a boolean in resource specification %s (expected: )", resource) } + } else { + isController = false } - return &resourceSpec{Kind: kind, ApiVersion: version, IsController: isController}, nil + return &v1alpha1.ApiServerResource{Kind: kind, APIVersion: version, Controller: isController}, nil } //Add is to set parameters @@ -117,8 +154,8 @@ func (f *APIServerSourceUpdateFlags) Add(cmd *cobra.Command) { cmd.Flags().StringArrayVar(&f.Resources, "resource", []string{}, - `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.`) + `Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. "Event:v1:true". +"isController" can be omitted and is "false" by default, e.g. "Event:v1".`) } // APIServerSourceListHandlers handles printing human readable table for `kn source apiserver list` command's output diff --git a/pkg/kn/commands/source/apiserver/flags_test.go b/pkg/kn/commands/source/apiserver/flags_test.go index 8d790f57e7..bcd34a1940 100644 --- a/pkg/kn/commands/source/apiserver/flags_test.go +++ b/pkg/kn/commands/source/apiserver/flags_test.go @@ -18,6 +18,7 @@ import ( "testing" "gotest.tools/assert" + "knative.dev/eventing/pkg/apis/sources/v1alpha1" ) func TestGetAPIServerResourceArray(t *testing.T) { @@ -28,38 +29,25 @@ func TestGetAPIServerResourceArray(t *testing.T) { Resources: []string{"Service:serving.knative.dev/v1alpha1:true"}, } created, _ := createFlag.getAPIServerResourceArray() - wanted := []resourceSpec{{ - Kind: "Service", - ApiVersion: "serving.knative.dev/v1alpha1", - IsController: true, + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Service", + APIVersion: "serving.knative.dev/v1alpha1", + Controller: true, }} assert.DeepEqual(t, wanted, created) + }) - // default isController - createFlag = APIServerSourceUpdateFlags{ + t.Run("get single apiserver resource when isController is default", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ ServiceAccountName: "test-sa", Mode: "Ref", Resources: []string{"Service:serving.knative.dev/v1alpha1"}, } - created, _ = createFlag.getAPIServerResourceArray() - wanted = []resourceSpec{{ - Kind: "Service", - ApiVersion: "serving.knative.dev/v1alpha1", - IsController: false, - }} - assert.DeepEqual(t, wanted, created) - - // default api version and isController - createFlag = APIServerSourceUpdateFlags{ - ServiceAccountName: "test-sa", - Mode: "Ref", - Resources: []string{"Service:v1"}, - } - created, _ = createFlag.getAPIServerResourceArray() - wanted = []resourceSpec{{ - Kind: "Service", - ApiVersion: "v1", - IsController: false, + created, _ := createFlag.getAPIServerResourceArray() + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Service", + APIVersion: "serving.knative.dev/v1alpha1", + Controller: false, }} assert.DeepEqual(t, wanted, created) }) @@ -71,36 +59,70 @@ func TestGetAPIServerResourceArray(t *testing.T) { Resources: []string{"Event:v1:true", "Pod:v2:false"}, } created, _ := createFlag.getAPIServerResourceArray() - wanted := []resourceSpec{{ - Kind: "Event", - ApiVersion: "v1", - IsController: true, + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: true, }, { - Kind: "Pod", - ApiVersion: "v2", - IsController: false, + Kind: "Pod", + APIVersion: "v2", + Controller: false, }} assert.DeepEqual(t, wanted, created) + }) - // default api version and isController - createFlag = APIServerSourceUpdateFlags{ + t.Run("get multiple apiserver resources when isController is default", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ ServiceAccountName: "test-sa", Mode: "Resource", Resources: []string{"Event:v1", "Pod:v1"}, } - created, _ = createFlag.getAPIServerResourceArray() + created, _ := createFlag.getAPIServerResourceArray() - wanted = []resourceSpec{{ - Kind: "Event", - ApiVersion: "v1", - IsController: false, + wanted := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: false, }, { - Kind: "Pod", - ApiVersion: "v1", - IsController: false, + Kind: "Pod", + APIVersion: "v1", + Controller: false, }} assert.DeepEqual(t, wanted, created) }) + + t.Run("get apiserver resource when isController has error", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Ref", + Resources: []string{"Event:v1:xxx"}, + } + _, err := createFlag.getAPIServerResourceArray() + errorMsg := "controller flag is not a boolean in resource specification Event:v1:xxx (expected: )" + assert.Error(t, err, errorMsg) + }) + + t.Run("get apiserver resources when kind has error", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Ref", + Resources: []string{":v2:true"}, + } + _, err := createFlag.getAPIServerResourceArray() + errorMsg := "cannot find 'Kind' part in resource specification :v2:true (expected: " + assert.Error(t, err, errorMsg) + }) + + t.Run("get apiserver resources when APIVersion has error", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Ref", + Resources: []string{"kind"}, + } + _, err := createFlag.getAPIServerResourceArray() + errorMsg := "cannot find 'APIVersion' part in resource specification kind (expected: " + assert.Error(t, err, errorMsg) + }) } func TestGetUpdateAPIServerResourceArray(t *testing.T) { @@ -111,15 +133,15 @@ func TestGetUpdateAPIServerResourceArray(t *testing.T) { Resources: []string{"Event:v1:true", "Pod:v2:false-"}, } added, removed, _ := createFlag.getUpdateAPIServerResourceArray() - addwanted := []resourceSpec{{ - Kind: "Event", - ApiVersion: "v1", - IsController: true, + addwanted := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: true, }} - removewanted := []resourceSpec{{ - Kind: "Pod", - ApiVersion: "v2", - IsController: false, + removewanted := []v1alpha1.ApiServerResource{{ + Kind: "Pod", + APIVersion: "v2", + Controller: false, }} assert.DeepEqual(t, added, addwanted) assert.DeepEqual(t, removed, removewanted) @@ -132,17 +154,59 @@ func TestGetUpdateAPIServerResourceArray(t *testing.T) { } added, removed, _ = createFlag.getUpdateAPIServerResourceArray() - removewanted = []resourceSpec{{ - Kind: "Event", - ApiVersion: "v1", - IsController: false, + removewanted = []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: false, }} - addwanted = []resourceSpec{{ - Kind: "Pod", - ApiVersion: "v1", - IsController: false, + addwanted = []v1alpha1.ApiServerResource{{ + Kind: "Pod", + APIVersion: "v1", + Controller: false, }} assert.DeepEqual(t, added, addwanted) assert.DeepEqual(t, removed, removewanted) }) } + +func TestUpdateExistingAPIServerResourceArray(t *testing.T) { + existing := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: false, + }, { + Kind: "Pod", + APIVersion: "v1", + Controller: false, + }} + + t.Run("update existing apiserver resources", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Resource", + Resources: []string{"Deployment:v1:true", "Pod:v1:false-"}, + } + updated, _ := createFlag.updateExistingAPIServerResourceArray(existing) + updatedWanted := []v1alpha1.ApiServerResource{{ + Kind: "Event", + APIVersion: "v1", + Controller: false, + }, { + Kind: "Deployment", + APIVersion: "v1", + Controller: true, + }} + assert.DeepEqual(t, updated, updatedWanted) + }) + + t.Run("update existing apiserver resources with error", func(t *testing.T) { + createFlag := APIServerSourceUpdateFlags{ + ServiceAccountName: "test-sa", + Mode: "Resource", + Resources: []string{"Deployment:v1:true", "Pod:v2:false-"}, + } + _, err := createFlag.updateExistingAPIServerResourceArray(existing) + errorMsg := "cannot find resource Pod:v2:false to remove" + assert.Error(t, err, errorMsg) + }) +} diff --git a/pkg/kn/commands/source/apiserver/update.go b/pkg/kn/commands/source/apiserver/update.go index e48cf992d5..b8b279016e 100644 --- a/pkg/kn/commands/source/apiserver/update.go +++ b/pkg/kn/commands/source/apiserver/update.go @@ -74,19 +74,11 @@ func NewAPIServerUpdateCommand(p *commands.KnParams) *cobra.Command { } if cmd.Flags().Changed("resource") { - added, removed, err := apiServerUpdateFlags.getUpdateAPIServerResourceArray() + updateExisting, err := apiServerUpdateFlags.updateExistingAPIServerResourceArray(source.Spec.Resources) if err != nil { return err } - for _, k := range removed { - _, err = b.RemoveResource(k.ApiVersion, k.Kind, k.IsController) - if err != nil { - return err - } - } - for _, k := range added { - b.AddResource(k.ApiVersion, k.Kind, k.IsController) - } + b.Resources(updateExisting) } if cmd.Flags().Changed("sink") { diff --git a/pkg/util/parsing_helper.go b/pkg/util/parsing_helper.go index 27bdbf4e0d..0837a4f87e 100644 --- a/pkg/util/parsing_helper.go +++ b/pkg/util/parsing_helper.go @@ -90,8 +90,8 @@ func (m StringMap) Remove(toRemove []string) StringMap { return m } -// AddListAndRemovalListFromArray returns a list of add entries and a list of removal entries -func AddListAndRemovalListFromArray(m []string) ([]string, []string) { +// AddedAndRemovalListsFromArray returns a list of added entries and a list of removal entries +func AddedAndRemovalListsFromArray(m []string) ([]string, []string) { stringToRemove := []string{} stringToAdd := []string{} for _, key := range m { diff --git a/pkg/util/parsing_helper_test.go b/pkg/util/parsing_helper_test.go index 88352db146..e5914b505d 100644 --- a/pkg/util/parsing_helper_test.go +++ b/pkg/util/parsing_helper_test.go @@ -103,8 +103,16 @@ func TestStringMap(t *testing.T) { assert.DeepEqual(t, expectedMap, inputMap) } -func TestAddListAndRemovalListFromArray(t *testing.T) { - addList, removeList := AddListAndRemovalListFromArray([]string{"addvalue1", "remove1-", "addvalue2", "remove2-"}) +func TestAddedAndRemovalListFromArray(t *testing.T) { + addList, removeList := AddedAndRemovalListsFromArray([]string{"addvalue1", "remove1-", "addvalue2", "remove2-"}) assert.DeepEqual(t, []string{"addvalue1", "addvalue2"}, addList) assert.DeepEqual(t, []string{"remove1", "remove2"}, removeList) + + addList, removeList = AddedAndRemovalListsFromArray([]string{"remove1-"}) + assert.DeepEqual(t, []string{}, addList) + assert.DeepEqual(t, []string{"remove1"}, removeList) + + addList, removeList = AddedAndRemovalListsFromArray([]string{"addvalue1"}) + assert.DeepEqual(t, []string{"addvalue1"}, addList) + assert.DeepEqual(t, []string{}, removeList) }