Add attribute filter transform function #7909

Cali0707 · 2024-05-08T19:23:51Z

Fixes #7685

Proposed Changes

Add a transform function for an attributes filter
Use the transform function depending on the filter on the trigger in the constructor

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow · 2024-05-08T19:23:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707

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~~ [Cali0707]

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

Cali0707 · 2024-05-08T19:24:25Z

/cc @Leo6Leo @pierDipi @creydr

This is just WIP until I add unit tests (probably tomorrow), so the actual changes are ready for a review!

codecov · 2024-05-08T19:30:07Z

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 69.49%. Comparing base (7e1c082) to head (c0a7e97).
Report is 46 commits behind head on main.

Files	Patch %	Lines
pkg/graph/constructor.go	84.61%	1 Missing and 1 partial ⚠️
pkg/graph/transforms.go	95.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7909      +/-   ##
==========================================
+ Coverage   69.22%   69.49%   +0.27%     
==========================================
  Files         339      344       +5     
  Lines       19494    15943    -3551     
==========================================
- Hits        13494    11079    -2415     
+ Misses       5337     4187    -1150     
- Partials      663      677      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-05-08T21:10:20Z

rbac.go:40: an error on the server ("Internal Server Error: \"/apis/rbac.authorization.k8s.io/v1/namespaces/test-ngogqfqt/rolebindings/subscriber-walrgdtq\": the server is currently unable to handle the request") has prevented the request from succeeding

Looks like GKE issues

/retest-required

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-05-10T19:58:09Z

/cc @Leo6Leo @creydr @pierDipi

This is ready for review now

Signed-off-by: Calum Murray <cmurray@redhat.com>

Leo6Leo

Left few comments, otherwise it looks good! @Cali0707

pkg/graph/constructor.go

pkg/graph/transforms_test.go

pkg/graph/transforms.go

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-05-14T18:28:22Z

/cc @Leo6Leo

Cali0707 · 2024-05-14T19:24:36Z

/test upgrade-tests

Leo6Leo · 2024-05-15T11:47:35Z

/lgtm

pierDipi · 2024-05-15T12:10:52Z

pkg/graph/transforms.go

+	return "attributes-filter"
+}
+
+func buildRegexForAttribute(attribute string) (*regexp.Regexp, error) {


Can we add a comment on what this is doing ?

pierDipi · 2024-05-15T12:11:53Z

pkg/graph/transforms.go

+
+var _ Transform = &AttributesFilterTransform{}
+
+func (aft *AttributesFilterTransform) Apply(et *eventingv1beta3.EventType, tfc TransformFunctionContext) (*eventingv1beta3.EventType, TransformFunctionContext) {


Comments would be helpful to describe what is the function doing

pierDipi · 2024-05-15T12:18:11Z

pkg/graph/transforms.go

+	var chunk strings.Builder
+	for i := 0; i < len(attribute); {
+		if attribute[i] == '{' {
+			chunks = append(chunks, chunk.String(), "[a-zA-Z]+")


I don't think regex [a-zA-Z]+ matches every possible CE value

https://github.com/cloudevents/spec/blob/main/cloudevents/spec.md#type-system

Also is the { or } not allowed? because in that case, we're using a possible char as delimiter

pierDipi · 2024-05-15T12:18:19Z

pkg/graph/transforms.go

+	for i := 0; i < len(attribute); {
+		if attribute[i] == '{' {
+			chunks = append(chunks, chunk.String(), "[a-zA-Z]+")
+			chunk.Reset()
+
+			offset := strings.Index(attribute[i:], "}")
+			if offset == -1 {
+				return nil, fmt.Errorf("no closing bracket for variable")
+			}
+
+			i += offset + 1
+			continue
+		}
+
+		chunk.WriteByte(attribute[i])
+		i++
+	}
+
+	chunks = append(chunks, chunk.String(), "$")


are we validating that attributes with variables are well formatted?

I think this would produce wrong results when the value looks like this one {abc}}

Cali0707 · 2024-05-15T13:24:50Z

Whoops @pierDipi looks like this somehow got merged before I could address your feedback. I'll open a follow up PR later today to try and make the changes you asked for

Cali0707 added 3 commits May 2, 2024 15:57

wip

078152c

Signed-off-by: Calum Murray <cmurray@redhat.com>

feat: added transform function for attribute filter

98d7798

Signed-off-by: Calum Murray <cmurray@redhat.com>

feat: use attributes filter in constructor

c61c068

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2024

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 8, 2024

knative-prow bot requested review from Leo6Leo and lionelvillard May 8, 2024 19:24

knative-prow bot requested review from creydr and pierDipi May 8, 2024 19:24

fix: handle missing attribute properly

f83cf4c

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 added 2 commits May 10, 2024 15:56

fix: update index for finding variable closing bracket is correct now

8422b21

Signed-off-by: Calum Murray <cmurray@redhat.com>

test: added unit tests for attributes filter transform function

57c3287

Signed-off-by: Calum Murray <cmurray@redhat.com>

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 May 10, 2024

Cali0707 changed the title ~~[WIP]: Add attribute filter transform function~~ Add attribute filter transform function May 10, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2024

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2024

fix merge conflicts

286adbc

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2024

Leo6Leo reviewed May 14, 2024

View reviewed changes

pkg/graph/constructor.go Show resolved Hide resolved

pkg/graph/transforms_test.go Show resolved Hide resolved

pkg/graph/transforms.go Outdated Show resolved Hide resolved

fix: attributes filter transform handles variables at end properly

c0a7e97

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot requested a review from Leo6Leo May 14, 2024 18:28

knative-prow bot assigned Leo6Leo May 15, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 15, 2024

pierDipi reviewed May 15, 2024

View reviewed changes

knative-prow bot merged commit 3189fc2 into knative:main May 15, 2024
35 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add attribute filter transform function #7909

Add attribute filter transform function #7909

Cali0707 commented May 8, 2024

knative-prow bot commented May 8, 2024

Cali0707 commented May 8, 2024

codecov bot commented May 8, 2024 •

edited

Cali0707 commented May 8, 2024

Cali0707 commented May 10, 2024

Leo6Leo left a comment

Cali0707 commented May 14, 2024

Cali0707 commented May 14, 2024

Leo6Leo commented May 15, 2024

pierDipi May 15, 2024

pierDipi May 15, 2024

pierDipi May 15, 2024

pierDipi May 15, 2024 •

edited

pierDipi May 15, 2024

Cali0707 commented May 15, 2024


		var _ Transform = &AttributesFilterTransform{}

		func (aft AttributesFilterTransform) Apply(et eventingv1beta3.EventType, tfc TransformFunctionContext) (*eventingv1beta3.EventType, TransformFunctionContext) {

Add attribute filter transform function #7909

Add attribute filter transform function #7909

Conversation

Cali0707 commented May 8, 2024

Proposed Changes

knative-prow bot commented May 8, 2024

Cali0707 commented May 8, 2024

codecov bot commented May 8, 2024 • edited

Codecov Report

Cali0707 commented May 8, 2024

Cali0707 commented May 10, 2024

Leo6Leo left a comment

Choose a reason for hiding this comment

Cali0707 commented May 14, 2024

Cali0707 commented May 14, 2024

Leo6Leo commented May 15, 2024

pierDipi May 15, 2024

Choose a reason for hiding this comment

pierDipi May 15, 2024

Choose a reason for hiding this comment

pierDipi May 15, 2024

Choose a reason for hiding this comment

pierDipi May 15, 2024 • edited

Choose a reason for hiding this comment

pierDipi May 15, 2024

Choose a reason for hiding this comment

Cali0707 commented May 15, 2024

codecov bot commented May 8, 2024 •

edited

pierDipi May 15, 2024 •

edited