feat: Add experimental filters to trigger describe cmd #1794

dsimansk · 2023-03-22T12:34:09Z

Description

Changes

🎁 feat: Add cesql filter field to trigger describe cmd

Preview:

➜  client git:(pr/desc-cesql-filter) ✗ kn trigger describe broker-display
Name:         broker-display
Namespace:    default
Labels:       eventing.knative.dev/broker=mybroker
Annotations:  eventing.knative.dev/creator=kubernetes-admin, eventing.knative.dev/lastModifier=ku ...
Age:          12h
Broker:       mybroker
Filter:
  type:       dev.knative.demo.event.processed

Filters (experimental):
  CESQL:                 LOWER(type) = 'my-event-type'

Sink:
  Name:      event-display
  Resource:  Service (serving.knative.dev/v1)

Conditions:
  OK TYPE    AGE REASON

Reference

Fixes #1785

Release Note

Add experimental filters to `trigger describe` cmd

dsimansk · 2023-03-22T12:34:20Z

/cc @rhuss @pierDipi @matzew

knative-prow

@dsimansk: 0 warnings.

In response to this:

Description

Changes

🎁 feat: Add cesql filter field to trigger describe cmd

Preview:

➜  client git:(pr/desc-cesql-filter) ✗ kn trigger describe broker-display
Name:         broker-display
Namespace:    default
Labels:       eventing.knative.dev/broker=mybroker
Annotations:  eventing.knative.dev/creator=kubernetes-admin, eventing.knative.dev/lastModifier=ku ...
Age:          12h
Broker:       mybroker
Filter:
 type:       dev.knative.demo.event.processed

Filters (experimental):
 CESQL:                 LOWER(type) = 'my-event-type'

Sink:
 Name:      event-display
 Resource:  Service (serving.knative.dev/v1)

Conditions:
 OK TYPE    AGE REASON

Reference

Fixes #1785

Release Note

Add CESQL filter field to `trigger describe` cmd

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-03-22T12:41:54Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06 🎉

Comparison is base (7586217) 79.87% compared to head (0e2e461) 79.94%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1794      +/-   ##
==========================================
+ Coverage   79.87%   79.94%   +0.06%     
==========================================
  Files         174      174              
  Lines       13593    13639      +46     
==========================================
+ Hits        10858    10904      +46     
  Misses       1992     1992              
  Partials      743      743

Impacted Files	Coverage Δ
pkg/kn/commands/trigger/describe.go	`79.66% <100.00%> (+12.99%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/kn/commands/trigger/describe.go

pierDipi

It looks good to me overall, I agree with your comment that more unit tests would be necessary

pierDipi · 2023-03-23T11:52:26Z

pkg/kn/commands/trigger/describe.go

+// writeNestedFilters goes through SubscriptionsAPIFilter and writes its content accordingly
+func writeNestedFilters(dw printers.PrefixWriter, filter v1beta1.SubscriptionsAPIFilter) {
+	v := reflect.ValueOf(filter)
+	for i := 0; i < v.NumField(); i++ {
+		field := v.Type().Field(i)
+		fieldValue := v.Field(i)
+
+		// Write if it's non-zero string, fields: CESQL
+		if fieldValue.Kind() == reflect.String && !fieldValue.IsZero() {
+			dw.WriteAttribute(field.Name, fieldValue.String())
+		}
+		// Write map[string]string key:value pairs of field: Exact, Prefix, Suffix
+		if fieldValue.Kind() == reflect.Map && fieldValue.Len() > 0 {
+			for k, v := range fieldValue.Interface().(map[string]string) {
+				dw.WriteAttribute(k, v)
+			}
+		}
+
+		// iterate through []SubscriptionsAPIFilter of fields: All, Any
+		if fieldValue.Kind() == reflect.Slice {
+			for j := 0; j < fieldValue.Len(); j++ {
+				element := fieldValue.Index(j)
+				// Write filter field name only and create next indentation
+				dw = dw.WriteAttribute(field.Name, "")
+				// Call write recursively for struct SubscriptionsAPIFilter
+				if element.Kind() == reflect.Struct {
+					writeNestedFilters(dw, element.Interface().(v1beta1.SubscriptionsAPIFilter))
+				}
+			}
+		}
+
+		// Call write recursively for struct SubscriptionsAPIFilter of field: Not
+		if fieldValue.Kind() == reflect.Struct {
+			writeNestedFilters(dw, fieldValue.Interface().(v1beta1.SubscriptionsAPIFilter))
+		}
+	}
 }


you have the best context here whether reflection is the best way to go or not

My intuition was that reflection will make it a bit shorter and cleaner than hardcoded fields. Plus hopefully it will be more future proof any of the fields is added/changed.
At the end it got a bit out of hand and it's not much shorter etc., but the benefit of having it a bit more dynamic hopefully stayed.

That would be also my question, whether it wouldn't be easier to use the fields directly and make it easier to comprehend. When new fields come up, one probably needs to check whether they fit in the layout anyway. Not sure how it looks like, sorry, I'm a bit disconnected. But let me ask GPT-4 ...

@rhuss here's the struct to show: https://github.com/knative/eventing/blob/main/pkg/apis/eventing/v1/trigger_types.go#L124

Can you tell me what the new method does and can give me a sample output what it would print ?

func writeNestedFilters(dw printers.PrefixWriter, filter v1beta1.SubscriptionsAPIFilter) { if filter.CESQL != "" { dw.WriteAttribute("CESQL", filter.CESQL) } if len(filter.Exact) > 0 { for k, v := range filter.Exact { dw.WriteAttribute(k, v) } } if len(filter.Prefix) > 0 { for k, v := range filter.Prefix { dw.WriteAttribute(k, v) } } if len(filter.Suffix) > 0 { for k, v := range filter.Suffix { dw.WriteAttribute(k, v) } } if len(filter.All) > 0 { dw = dw.WriteAttribute("All", "") for _, nestedFilter := range filter.All { writeNestedFilters(dw, nestedFilter) } } if len(filter.Any) > 0 { dw = dw.WriteAttribute("Any", "") for _, nestedFilter := range filter.Any { writeNestedFilters(dw, nestedFilter) } } if filter.Not != nil { dw = dw.WriteAttribute("Not", "") writeNestedFilters(dw, *filter.Not) } }

This version doesn't entirely cut it, but I think I would prefer the simpler version that is more hardcoded as the reflect-based version, while flexible, is probably a bit over the top as I think one would need to adjust the output anyway if the filter gets more complex.

Sure, I'll refactor it. :)

I've pushed revamped version with exact fields + unit test. 0e2e461

dsimansk · 2023-04-06T12:27:57Z

@rhuss a gentle ping, the PR is ready! :)

dsimansk · 2023-04-19T08:11:45Z

@pierDipi would you mind getting back to the PR?

pierDipi

This looks good to me!

/lgtm
/approve

knative-prow · 2023-04-19T08:21:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, pierDipi

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dsimansk,pierDipi]

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

feat: Add cesql filter field to trigger describe cmd

bb110d4

knative-prow bot requested review from dprotaso, itsmurugappan and psschwei March 22, 2023 12:34

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2023

knative-prow bot reviewed Mar 22, 2023

View reviewed changes

knative-prow bot requested review from matzew, pierDipi and rhuss March 22, 2023 12:34

knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 22, 2023

dsimansk commented Mar 22, 2023

View reviewed changes

pkg/kn/commands/trigger/describe.go Show resolved Hide resolved

Refactor trigger describe based on defined fields in filter

cc0999e

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 22, 2023

Fix comment typo

b22cbc0

dsimansk force-pushed the pr/desc-cesql-filter branch from bb94097 to b22cbc0 Compare March 22, 2023 15:35

dsimansk commented Mar 22, 2023

View reviewed changes

pkg/kn/commands/trigger/describe.go Show resolved Hide resolved

dprotaso removed their request for review March 22, 2023 19:05

pierDipi reviewed Mar 23, 2023

View reviewed changes

Refactor without reflection

0e2e461

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 23, 2023

dsimansk changed the title ~~feat: Add cesql filter field to trigger describe cmd~~ feat: Add experimental filters to trigger describe cmd Mar 24, 2023

pierDipi approved these changes Apr 19, 2023

View reviewed changes

knative-prow bot assigned pierDipi Apr 19, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2023

knative-prow bot merged commit e251354 into knative:main Apr 19, 2023

dsimansk mentioned this pull request Jul 27, 2023

CLI: add support for experimental new filters API in trigger #1833

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add experimental filters to trigger describe cmd #1794

feat: Add experimental filters to trigger describe cmd #1794

dsimansk commented Mar 22, 2023 •

edited

Loading

dsimansk commented Mar 22, 2023

knative-prow bot left a comment

codecov bot commented Mar 22, 2023 •

edited

Loading

pierDipi left a comment •

edited

Loading

pierDipi Mar 23, 2023

dsimansk Mar 23, 2023 •

edited

Loading

rhuss Mar 23, 2023

dsimansk Mar 23, 2023

rhuss Mar 23, 2023

rhuss Mar 23, 2023

rhuss Mar 23, 2023

rhuss Mar 23, 2023

dsimansk Mar 23, 2023

dsimansk Mar 24, 2023

dsimansk commented Apr 6, 2023

dsimansk commented Apr 19, 2023

pierDipi left a comment

knative-prow bot commented Apr 19, 2023

feat: Add experimental filters to trigger describe cmd #1794

feat: Add experimental filters to trigger describe cmd #1794

Conversation

dsimansk commented Mar 22, 2023 • edited Loading

Description

Changes

Reference

dsimansk commented Mar 22, 2023

knative-prow bot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Mar 22, 2023 • edited Loading

Codecov Report

pierDipi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Apr 6, 2023

dsimansk commented Apr 19, 2023

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Apr 19, 2023

dsimansk commented Mar 22, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

pierDipi left a comment •

edited

Loading

dsimansk Mar 23, 2023 •

edited

Loading