Enable trigger filter to be optional by daisy-ycguo · Pull Request #603 · knative/client

daisy-ycguo · 2020-01-09T06:52:38Z

Proposed Changes

Make trigger filter to be optional
Handle filter flag in a same way as other string array flags
use Builder to create a trigger in the creation command

rhuss

Looks good, with some minor cosmetic suggestions.

Thanks !

rhuss · 2020-01-09T10:20:48Z


-			trigger := constructTrigger(name, namespace, triggerUpdateFlags.Broker, filters)
-			trigger.Spec.Subscriber = &duckv1.Destination{
+			trigger := client_v1alpha1.NewTriggerBuilder(name)


better call it triggerBuilder

rhuss · 2020-01-09T10:23:20Z

-			trigger := constructTrigger(name, namespace, triggerUpdateFlags.Broker, filters)
-			trigger.Spec.Subscriber = &duckv1.Destination{
+			trigger := client_v1alpha1.NewTriggerBuilder(name)
+			trigger.Namespace(namespace)


you can chain the calls directly as in

triggerBuilder := NewTriggerBuilder(name).Namespace(...).Broker(....).Subscriber(....)

rhuss · 2020-01-09T10:24:47Z

+			trigger := client_v1alpha1.NewTriggerBuilder(name)
+			trigger.Namespace(namespace)
+			trigger.Broker(triggerUpdateFlags.Broker)
+			for k, v := range filters {


We could conside to add a Filters(map[string]string) builder method so you could easily do the full chaining (and providers a nicer API for the builder)

rhuss

Thanks, but I have still some nit-picks around the builder API (see below)

rhuss · 2020-01-10T09:05:01Z

 }

-func (b *TriggerBuilder) AddFilter(key, value string) *TriggerBuilder {
+func (b *TriggerBuilder) AddSingleFilter(key, value string) *TriggerBuilder {


I'd suggest to keep AddFilter and jut add a Filters method for setting the full field (like for the other methods)

In the latest version, I keep only Filters to add multiple filters.

rhuss · 2020-01-10T09:06:38Z

 	return b
 }

+func (b *TriggerBuilder) RemoveMultipleFilters(keys []string) *TriggerBuilder {


Do wee need this ? (and also RemoveFilter). At the end its a builder with a very short lifetime. If you don't want to have something added, just don't add it. IMO, we should only have methods which 'build up' something.

The remove filter will remove existing filters setting by --filter name-.
If I need to remove some existing filters, what should I do ? Get the existing map of filters, then remove some keys outside builder, and then set back to builder ?

I keep only RemoveFilters in the latest version to remove those filters from existing attributes setting by --filter name-

rhuss · 2020-01-10T10:10:15Z

-				for _, k := range removed {
-					b.RemoveFilter(k)
-				}
+				b.Filters(updated).RemoveFilters(removed)


Sorry, my last nit-pick (promised :). What do you think about:

b.Filters(maputil.RemoveKeysFromMap(updated, removed))

and a maputil.RemoveFrom() (but maybe in an already existing util package):

func RemoveKeysFromMap(original map[string]string, toRemove []string) map[string]string { .... }

That way this method could be re-used for other use cases and we don't have to add a Remove method to a builder.

wdyt ?

never mind. Thank you for the suggestion.
The problem of your suggestion is that I need to get the original value of filters map to remove. I think we want to hide the detail of trigger Spec by Builder. The value of filters map is Spec.Filter.Attributes, and Spec.Filter may be nil at the beginning. If I don't add a method GetFilters() to Builder, I have to manage the trigger Spec structure to get the filter. So my code may like below:

var attributes *map[string]string attributes = nil if b.Build().Spec.Filter != nil { attributes = b.Build().Spec.Filter.Attributes } b.Filters(maputil.AddAndRemoveFromMap(attributes, updated,removed))

and a method to the util:

func AddAndRemoveFromMap(original *map[string]string, toAdd map[string]string, toRemove []string) map[string]string { ...... }

If I add a GetFilters() to Builder, it may be much simpler:

b.Filters(maputil.AddAndRemoveFromMap(b.GetFilters(), updated,removed))

but it violates the rule that builder is to build something.

Do I make sense ? What's your option?

My suggestion:

// Extract filters directly from original trigger (not builder). Return nil if none. existing := extractFilters(trigger) // Use chained util method, so that they can bey reused in different contexts: // * type StringMap map[string]string // * func StringMap(map [string]string) StringMap : Just cast or copy to the new type (then call it "NewStringMap") // * func (m SringMap) Merge(toMerge map[string]string) StringMap // Keys from the toMerge map override those from the wrapped map // * func (m map[string]string) RemovKeys(toRemove string[]) for removing the keys of a map. If this no b.Filters(maputil.StringMap(existing).Merge(updated).Remove(removed))

This would have the benefit:

Builder is still immutable and write-only

Reusable string map manipulation functions which can be easily used in different contexts

Easy to use API (maybe a bit harder to implement but obey one of the principles of good API design: "It should be as easy to use as possible, even when its might harder to implement that parts that are hidden from the user".

Instead of introducing a "StringMap" you could equally cast to the type "StringMap" via existing.(StringMap).Merge(updated).Remove(toRemove)

Does this make sense ?

Thanks. I made a small update to your suggestion.

I used builder as parameter in extractFilters, not trigger. I don't like to introduce a dependency to knative.dev/eventing/pkg/apis/eventing/v1alpha1 in the update command. If I use trigger as parameter, I have to add this dependency. Here is the definition of extractFilters

func extractFilters(builder *client_v1alpha1.TriggerBuilder) util.StringMap

Is that OK with you?

I used builder as parameter in extractFilters, not trigger. I don't like to introduce a dependency to knative.dev/eventing/pkg/apis/eventing/v1alpha1 in the update command. If I use trigger as parameter, I have to add this dependency. Here is the definition of extractFilters

Don't we already have the trigger available in this method ? with extractFilters() I mean just a function within update.go itself (just holding the loop that you've mentioned). I don't think we should put that into the builder.

I refer to this line:

client/pkg/kn/commands/trigger/update.go

Line 69 in 5c2ac96

trigger, err := eventingClient.GetTrigger(name)

Ah, you mean a goimport. Good point, but we have the implicit dependency anyway (e.g. via the constructor arg of the builder or the return value). Alternatively you could add the function to client_v1alpha1 package, like client_v1alpha1.ExtractFilters(trigger)

It's better than diving into a builder. E.g. in generally when you call Build() on a Builder this is considered often to be a final action, so you can't call Build() a second time on the same builder (because it performs some more persisten action). So I would prefer to avoid that in favor of the utility in the client package.

Yes, we already have the implicit dependency. I added a line to goimport in update.go, and used trigger as input parameter in the latest version. Please review. Thank you.

rhuss · 2020-01-13T19:16:41Z

+
+func extractFilters(trigger *v1alpha1.Trigger) util.StringMap {
+	if trigger.Spec.Filter == nil {
+		return nil


You should return an empty map here, otherwise you can a panic when you call existing.Merge and existing is nil. Its also inconsistent to return different things when Filter == nil or Filter.Attributes == nil

Changed it in the latest version. Thank you.

maximilien

minor

maximilien · 2020-01-13T22:41:01Z

 	})

-	t.Run("update filter with only deletions", func(t *testing.T) {
+	t.Run("update filter with add and update", func(t *testing.T) {


this description reads odd... any way to make this clearer?

fixed. Thank you.

knative-metrics-robot · 2020-01-14T02:00:07Z

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/eventing/v1alpha1/client.go	80.3%	81.5%	1.2
pkg/kn/commands/trigger/create.go	82.5%	81.2%	-1.2
pkg/kn/commands/trigger/update.go	83.3%	86.7%	3.3
pkg/util/parsing_helper.go	97.0%	86.4%	-10.6

rhuss

Thanks a lot ! Looks good now, and I think we have now also a nice API for builder with allowing to remove parts.

Good work!

/lgtm

knative-prow-robot · 2020-01-14T07:32:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, 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~~ [rhuss]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 9, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 9, 2020

knative-prow-robot requested review from navidshaikh and rhuss January 9, 2020 06:52

daisy-ycguo force-pushed the optional branch from ced382f to c12b738 Compare January 9, 2020 06:54

rhuss suggested changes Jan 9, 2020

View reviewed changes

daisy-ycguo force-pushed the optional branch from c12b738 to 332eaa2 Compare January 10, 2020 08:53

rhuss suggested changes Jan 10, 2020

View reviewed changes

Enable trigger filter to be optional

e9f3b54

daisy-ycguo force-pushed the optional branch from 332eaa2 to e9f3b54 Compare January 10, 2020 09:26

rhuss reviewed Jan 10, 2020

View reviewed changes

daisy-ycguo force-pushed the optional branch 2 times, most recently from 5c2ac96 to e2f8966 Compare January 13, 2020 14:43

rhuss reviewed Jan 13, 2020

View reviewed changes

maximilien suggested changes Jan 13, 2020

View reviewed changes

remove RemoveFilters from builder

c675b85

daisy-ycguo force-pushed the optional branch from e2f8966 to c675b85 Compare January 14, 2020 01:57

rhuss approved these changes Jan 14, 2020

View reviewed changes

knative-prow-robot assigned rhuss Jan 14, 2020

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

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

knative-prow-robot merged commit 71c9238 into knative:master Jan 14, 2020

rhuss mentioned this pull request Jan 15, 2020

Improve 'source apiserver update --resource' #590

Merged

dsimansk added a commit to dsimansk/client that referenced this pull request Apr 7, 2025

Refactor CI setup (knative#603)

380c8f1

Conversation

daisy-ycguo commented Jan 9, 2020

Proposed Changes

Uh oh!

rhuss 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!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daisy-ycguo 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!

Choose a reason for hiding this comment

Uh oh!

daisy-ycguo 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 13, 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 13, 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!

daisy-ycguo Jan 13, 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!

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!

knative-metrics-robot commented Jan 14, 2020

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jan 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

daisy-ycguo Jan 10, 2020 •

edited

Loading

daisy-ycguo Jan 10, 2020 •

edited

Loading

daisy-ycguo Jan 13, 2020 •

edited

Loading

daisy-ycguo Jan 13, 2020 •

edited

Loading

daisy-ycguo Jan 13, 2020 •

edited

Loading