Improve 'source apiserver update --resource' by daisy-ycguo · Pull Request #590 · knative/client

daisy-ycguo · 2019-12-23T10:02:50Z

Fixes #565

Proposed Changes

Improve source apiserver update --resource to follow the common update semantics
Support remove resource when suffixed with -

maximilien

Thanks for the contributions @daisy-ycguo
See my comments.
/ok-to-test.

maximilien · 2020-01-07T21:23:18Z

                                 "Resource" send the full resource. (default "Ref")
  -n, --namespace string         Specify the namespace to operate in.
-      --resource strings         Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true.
+      --resource stringArray     Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true.


The example shows a / but the format does not

Deployment:apps/v1:true is just the format of Kind:APIVersion:isController. APIVersion usually looks like:

serving.knative.dev/v1 or

simply v1, only means kubernetes API V1.

I could change it to v1 if it looks confused.

I would keep apps/v1 as v1 without an apigroup is more the exception than the rule. Giving a full example makes more sense.

maximilien · 2020-01-07T21:23:48Z

                                 "Resource" send the full resource. (default "Ref")
  -n, --namespace string         Specify the namespace to operate in.
-      --resource strings         Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true.
+      --resource stringArray     Specification for which events to listen, in the format Kind:APIVersion:isController, e.g. Deployment:apps/v1:true.


same as previous comment. Not / in example but not the format

see my previous comments

maximilien · 2020-01-07T21:56:25Z

+		Controller: isController,
+	}
+	b.apiServerSource.Spec.Resources = append(resources, resourceRef)
+	return b


Is this return of b done to stack the calls to add resource? e.g., AddResource(...).AddResource(...) otherwise it seems weird ad Go code, as matter of style, does not have a return of the instance in a method...

I follow the same style in other eventing clients, e.g. cronjob_client and eventing_client.

I agree with you that the return is unmeaning, unless @rhuss could give an explanation which we didn't think about. If we all agree to remove the return, I will suggest to create a separate issue to track and we can change them all together.

This is classical pattern used in many languages which allows for chained calls for elegant, DSL-like APIs. Do you remember the review comment where there was a check for a nil-receiver in a method (in some dependency) ? That is exactly for that use case of chained calls, where an intermediate call could return nil. So its also used in Go quite a bit (and btw is very popular in Java as well).

Some references illustrating that chaining is quite popular in Go, too (and they all return the object itself):

https://github.com/mauricioklein/go-chainable

https://vikash1976.wordpress.com/2017/02/03/method-chaining-in-go-lang-through-interface/

https://riptutorial.com/go/example/13465/chaining-methods

https://stackoverflow.com/questions/27297702/go-method-chaining-and-error-handling

....

BTW, you can still ignore the returned value if you like.

maximilien · 2020-01-07T21:57:03Z

+			return b, nil
+		}
+	}
+	return b, fmt.Errorf("cannot find resource %s:%s:%t to remove", version, kind, isController)


In this case because of the error you would not able able to stack calls.

If the resource could be found, the function will return at the line 193 without error.
If the resource could not be found after traversing the whole structure, the function will return at line 196 with an error.
Sorry I don't get your point. Could you help to explain more of your comment ?

@maximilien is right, chaining works on methods which return a single return value, so returning two values makes no sense in this pattern.

But as we discussed in the other PR having mutating methods on a builder is an anti-pattern (as then the builder turns more into a regular domain objects). Maybe we can solve this use case differently, too ?

If not, we could agree that removing a non-existing resource is just a no-op and gets ignored, so that you can return directly the builder again.

maximilien · 2020-01-07T21:58:01Z

+		Controller: isController,
+	}
+	b.apiServerSource.Spec.Resources = append(resources, resourceRef)
+	return b


Nit... Also b while short is not very descriptive... perhaps builder is better.

b is short for builder. Again, I will suggest to create a separate issue to change all client codes, including cronjob_client, eventing_client and apiserver_client.

maximilien · 2020-01-07T22:00:45Z

+	return added, removed, nil
 }

 func getValidResource(resource string) (*resourceSpec, error) {


you should add comment for all code. I would rather see for public methods and functions but since you added above then should be consistent...

Added a comment.

maximilien · 2020-01-07T22:01:54Z

 "Resource" send the full resource.`)
-	cmd.Flags().StringSliceVar(&f.Resources,
+	cmd.Flags().StringArrayVar(&f.Resources,
 		"resource",


Why does the name resource not match the variable f.Resources? Should this not be "resources"?

resource is a flag. Resources is an array of resource.
User uses --resource abc --resource def to set multiple resources, which will be saved in f.Resources.

maximilien · 2020-01-07T22:02:34Z

+			Mode:               "Ref",
+			Resources:          []string{"Service:serving.knative.dev/v1alpha1:true"},
+		}
+		created, _ := createFlag.getAPIServerResourceArray()


why are you ignoring errors? Just assert that they are nil

will add some tests to test errors. Thank you for pointing out.

maximilien · 2020-01-07T22:03:50Z

+)
+
+func TestGetAPIServerResourceArray(t *testing.T) {
+	t.Run("get single apiserver resource", func(t *testing.T) {


I think since there are many combination you are testing you should offer a small comment summary of the table and what combination you are testing. This would even help you if you need to add or remove tests. Or seperate them into their own t.Run calls

simple comments are added before test cases. e.g. line 38 and line 52.
But I think maybe separating them into a single t.Run is more readable.

maximilien · 2020-01-07T22:06:51Z

 	assert.DeepEqual(t, expectedStringToRemove, stringToRemove)
 }
+
+func TestAddListAndRemovalListFromArray(t *testing.T) {


I would add tests for corner cases. For instance,

add one item and remove none,

add no items, remove one,

add one item and remove one,

add none and remove none.

maximilien · 2020-01-14T21:57:42Z

Is this ready? Also, looks like you have some conflicts to resolve. Thanks.

rhuss · 2020-01-15T09:09:44Z

+				}
+				for _, k := range added {
+					b.AddResource(k.ApiVersion, k.Kind, k.IsController)
+				}


I would follow the same scheme here as we had for trigger filters in #603 , I think it covers the same use case.

ok. will do.

Resource is not a string map, but an array. I cannot leverage stringmap here.
I have to think about how to handle.

Ah, right. Unfortunately, there are no generics in golang, that would make things much easier. Its also not easy to simulate with interfaces ;(

rhuss

thanks ! lgtm but I would remove the 'remove' method from the builder and model it the same as in #603

daisy-ycguo · 2020-01-15T10:52:41Z

+}
+
+// updateExistingAPIServerResourceArray is to update an array of resources.
+func (f *APIServerSourceUpdateFlags) updateExistingAPIServerResourceArray(existing []v1alpha1.ApiServerResource) ([]v1alpha1.ApiServerResource, error) {


I would like to move this method to parsing_helper.go to provide a general method UpdateArray to update existing array.

func UpdateArray(existing []interface{}, added []interface{}, removed []interface{}) ([]interface{}, error)

My question is which type (or key word) I should use to describe a general array here ?

I tried to use []interface{} but I get an error:

cannot use existing (type []"knative.dev/eventing/pkg/apis/sources/v1alpha1".ApiServerResource) as type []interface {} in argument to util.UpdateArray

That's not easily possible, see https://golang.org/doc/faq#convert_slice_of_interface

What you could do is to introduce a simple utility method which converts an []ApiServerResource to a []interface{} and feed that into the generic method like in

fromISlice(UpdateArray(toISlice(existing), toISlice(added), toISlice(removed)))

and do the conversion between ApiServerSource and interfaces slices before and after calling the function. But I don't think its worth the hassle.

oh, ok. Then I keep as it is.
Please review the latest version. I have removed AddResource and RemoveResource from Builder.

daisy-ycguo · 2020-01-15T15:13:18Z

Is this ready? Also, looks like you have some conflicts to resolve. Thanks.

Yes, it's ready for a review.

rhuss

Some minor comments, otherwise looks good to me.

rhuss · 2020-01-16T07:32:45Z

+		}
+		added = append(added, *resourceSpec)
+	}
+	for _, r := range removedArray {


you could put the loop into a seperate function and call it twice, for adds and removals. That would reduce duplicated code a bit.

rhuss · 2020-01-16T07:34:54Z

-	parts := strings.Split(resource, apiVersionSplitChar)
+	parts := strings.SplitN(resource, apiVersionSplitChar, 3)
+	if len(parts[0]) == 0 {
+		return nil, fmt.Errorf("cannot find KIND in resource specification %s", resource)


Suggested change

return nil, fmt.Errorf("cannot find KIND in resource specification %s", resource)

return nil, fmt.Errorf("cannot find 'Kind' part in resource specification %s (expected: <Kind:ApiVersion[:controllerFlag]>", resource)

rhuss · 2020-01-16T07:36:14Z

+	if len(parts) >= 2 && len(parts[1]) > 0 {
+		version = parts[1]
+	} else {
+		version = "v1"


Do we really want a default to "v1" ? I would prefer to be always explicit with the ApiVersion and make it mandatory (as you have to do it for any k8s resource, too, when deploying to the server)

I just want to make a simple format for ApiServerSource, e.g. Event or Pod or Deployment can be described as --resource Event --resource Pod --resource Deployment which look more artistic, other than --resource Event:v1 --resource Pod:v1 --resource Deployment:v1

So if no ApiVersion provides, I use v1 instead. If user want to use default version but set a controller flag, it could be describe as Event::true.
If you don't agree with this kind of contraction rule, I could delete this feature.

My point is that this is a bit surprising for Kubernetes users, as the apiVersion is a mandatory field in a resource descriptors. Also, imagine now that the Event or Pod spec evolves to a v2 version. Would you then also update the default to get to this newest version ? Will a user still using v1 and, more important, is she aware of that not the latest event version is used ? Maybe even v1 is getting removed later on then, making the default useless.

I understand that you want to make it as easy and concise as possible. However, its one of key principle of Kubernetes (and hence Knative, too) that to identify a resource you alway need apiVersion + kind.

So I really would be explicit here to (a) be on par with the Kubernetes spec and (b) to avoid future surprises (and (c) to avoid 'hacks' like '::' to get to the third option).

btw, Deployment is in apps/v1, not v1. There are only a handful of resources that are really in v1.

oh. Got your point. Let me remove the default value.

Done. The latest version is submitted and ready for review. @rhuss

rhuss · 2020-01-16T07:37:50Z

+	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)


Suggested change

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: <Kind:ApiVersion[:controllerFlag]>)", resource)

maximilien · 2020-01-16T23:12:16Z

/test pull-knative-client-integration-tests

daisy-ycguo · 2020-01-21T12:49:24Z

@maximilien @rhuss Are we ready to merge this ? I'm on vacation. I could still address the comments if you have. Thank you.

…ate semantics

knative-metrics-robot · 2020-01-27T12:30:37Z

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/source/apiserver/create.go	83.9%	84.8%	1.0
pkg/kn/commands/source/apiserver/flags.go	66.2%	75.2%	9.0
pkg/util/parsing_helper.go	86.4%	88.2%	1.9

rhuss

thanks, lgtm !

/lgtm

knative-prow-robot · 2020-01-28T14:48:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, maximilien, rhuss

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [maximilien,rhuss]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Update kn-plugin-source-kafka to v0.19.0 * Enable Kafka for E2E tests * Update vendor dir

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Dec 23, 2019

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 23, 2019

knative-prow-robot requested review from evankanderson and maximilien December 23, 2019 10:03

daisy-ycguo force-pushed the updateapiserver branch from 0254441 to 847a337 Compare December 23, 2019 10:06

navidshaikh suggested changes Dec 31, 2019

View reviewed changes

Comment thread pkg/kn/commands/source/apiserver/flags.go Outdated

daisy-ycguo force-pushed the updateapiserver branch from 847a337 to 8564205 Compare January 7, 2020 05:49

maximilien approved these changes Jan 7, 2020

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2020

daisy-ycguo force-pushed the updateapiserver branch 2 times, most recently from 467edd9 to fc74cbb Compare January 15, 2020 09:00

rhuss reviewed Jan 15, 2020

View reviewed changes

rhuss suggested changes Jan 15, 2020

View reviewed changes

daisy-ycguo force-pushed the updateapiserver branch from fc74cbb to 6edd8b7 Compare January 15, 2020 10:16

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2020

daisy-ycguo commented Jan 15, 2020

View reviewed changes

rhuss suggested changes Jan 16, 2020

View reviewed changes

daisy-ycguo force-pushed the updateapiserver branch 2 times, most recently from cf02e79 to b812464 Compare January 16, 2020 15:17

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 16, 2020

Daisy Guo added 3 commits January 27, 2020 20:18

improve 'source apiserver update --resource' to follow the common upd…

ecfeb90

…ate semantics

add more tests and update based on comments

6ad0e98

rebase

1276fbf

daisy-ycguo force-pushed the updateapiserver branch from b812464 to 1276fbf Compare January 27, 2020 12:27

rhuss approved these changes Jan 28, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jan 28, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2020

knative-prow-robot merged commit 00da006 into knative:master Jan 28, 2020

dsimansk added a commit to dsimansk/client that referenced this pull request Jan 27, 2021

[release-v0.19.1] Update kn-plugin-source-kafka to v0.19.0 (knative#590)

0c3f545

* Update kn-plugin-source-kafka to v0.19.0 * Enable Kafka for E2E tests * Update vendor dir

	return nil, fmt.Errorf("cannot find KIND in resource specification %s", resource)
	return nil, fmt.Errorf("cannot find 'Kind' part in resource specification %s (expected: <Kind:ApiVersion[:controllerFlag]>", resource)

	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: <Kind:ApiVersion[:controllerFlag]>)", resource)

Conversation

daisy-ycguo commented Dec 23, 2019

Proposed Changes

Uh oh!

Uh oh!

maximilien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daisy-ycguo Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhuss Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daisy-ycguo Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhuss Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daisy-ycguo Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximilien commented Jan 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

daisy-ycguo Jan 9, 2020 •

edited

Loading

rhuss Jan 10, 2020 •

edited

Loading

daisy-ycguo Jan 9, 2020 •

edited

Loading

rhuss Jan 15, 2020 •

edited

Loading

daisy-ycguo Jan 9, 2020 •

edited

Loading

daisy-ycguo Jan 15, 2020 •

edited

Loading

rhuss Jan 15, 2020 •

edited

Loading

daisy-ycguo Jan 15, 2020 •

edited

Loading

daisy-ycguo Jan 16, 2020 •

edited

Loading