Update eventtype v1beta3 type to reflect spec changes #7708

Cali0707 · 2024-02-20T16:36:51Z

Part of #7265

Proposed Changes

Update the eventtype CRD to have the new v1beta3 schema
Update the eventtype v1beta3 type and validation to reflect the new schema
Fix unit tests

Signed-off-by: Calum Murray <[email protected]>

Cali0707 · 2024-02-20T16:37:03Z

/cc @dsimansk @matzew @pierDipi

codecov · 2024-02-20T16:43:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.22%. Comparing base (580f3c7) to head (2c3a610).
Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7708      +/-   ##
==========================================
+ Coverage   69.04%   69.22%   +0.17%     
==========================================
  Files         338      339       +1     
  Lines       19350    19494     +144     
==========================================
+ Hits        13360    13494     +134     
- Misses       5328     5337       +9     
- Partials      662      663       +1

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

matzew · 2024-02-21T07:48:09Z

pkg/apis/eventing/v1beta3/eventtype_types.go

+type EventAttributeDefinition struct {
+	// Required determines whether this attribute must be set on corresponding CloudEvents.
+	Required bool `json:"required"`
+	// Value is a string representing the allowable values for the EventType attribute.
+	// It may be a single value such as "/apis/v1/namespaces/default/pingsource/ps", or it could be a template
+	// for the allowed values, such as "/apis/v1/namespaces/{namespace}/pingsource/{sourceName}.
+	// To specify a section of the string value which may change between different CloudEvents
+	// you can use curly brackets {} and optionally a variable name between them.
+	Value string `json:"value,omitempty"`


I'd argue that the original proposed name is useful, to identify what the attr def is is about, is useful. see:

spec: attributes: - name: type value: com.shop.domain.order.new required: true - name: specversion value: "1.0" - name: id required: true - name: source value: "/customer-system/node/some-id-value" - name: time required: true - name: myextension required: false ...

That makes sense @matzew , I was basing this off of your earlier WIP PR, where I think this would end up mapping to a CRD like:

spec: attributes: type: value: com.shop.domain.order.new required: true specversion: value: "1.0" ...

If you think the proposed version with name makes more sense, I'm happy to update this to reflect that idea instead - I just had guessed that the PR was a "newer" version of the idea than the issue :)

Signed-off-by: Calum Murray <[email protected]>

Cali0707 · 2024-02-27T16:05:56Z

/cc @matzew @pierDipi @dsimansk

Signed-off-by: Calum Murray <[email protected]>

dsimansk · 2024-02-28T09:48:12Z

config/core/resources/eventtype.yaml

@@ -22,6 +22,132 @@ metadata:
 spec:
  group: eventing.knative.dev
  versions:
+  - name: v1beta3
+    served: false


The new version is not served as a precaution for now?

Yeah, I figured we could serve it when we update the reconciler to reconcile the new type

dsimansk · 2024-02-28T09:54:04Z

pkg/apis/eventing/v1beta3/eventtype_types.go

+	Name string `json:"name"`
+	// Required determines whether this attribute must be set on corresponding CloudEvents.
+	Required bool `json:"required"`
+	// Value is a string representing the allowable values for the EventType attribute.


Allowed values are derived from CloudEvent Spec? Should we have a mechanism to specify validation ruleset or validation strictness perhaps?

/cc @pierDipi @matzew

@dsimansk I was thinking maybe something like #7729 could work? This way various eventtype producers could provide rules/guidance over what potential values their eventtype attribute would have.

dsimansk

@Cali0707 thanks for taking over this part! Looks good to me, I've added 2 question in the review, but none is a blocker to be resolved for the merge.

Adding hold for other reviews.
/hold

Cali0707 · 2024-03-11T15:11:02Z

/cc @matzew @pierDipi

Any thoughts on this PR? Can we unhold, or are there more changes needed?

matzew · 2024-03-14T16:49:35Z

pkg/apis/eventing/v1beta3/eventtype_defaults_test.go

-					Schema: testSchema,
+					Attributes: []EventAttributeDefinition{
+						EventAttributeDefinition{
+							Value:    "test-type",


would name not also really make sense here?

should we require name? for the attr def?

Signed-off-by: Calum Murray <[email protected]>

Cali0707 · 2024-03-18T16:38:32Z

/cc @matzew @dsimansk

pierDipi · 2024-03-25T12:08:22Z

config/core/resources/eventtype.yaml

+                items:
+                  type: object
+                  required:
+                    - name


isn't value also required ?

Not currently, no. Currently you can just add a name and whether or not that attribute is required without specifying what value it may have. I think of this as a way of saying "the event may have attribute xyz". But, I could understand making this field required too. WDYT we should do @pierDipi ?

yes, that is exactly what I was thinking too. IMO this is useful for "extensions",

the following is IMO valid:

- name: myextension required: false

while some attribute names are requied to have a value (e.g. all the CE "required" attrs)

pierDipi · 2024-03-25T12:09:27Z

config/core/resources/eventtype.yaml

+      jsonPath: ".spec.attributes.type.value"
+    - name: Source
+      type: string
+      jsonPath: ".spec.attributeds.source.value"


I don't see these 2 attributes

Oh yeah good catch! I wrote these when we were using a map to hold all the attributes, rather than an array

Signed-off-by: Calum Murray <[email protected]>

Cali0707 · 2024-03-25T12:53:04Z

/cc @pierDipi

matzew · 2024-03-25T15:29:29Z

/test reconciler-tests

pierDipi · 2024-03-25T18:00:53Z

pkg/apis/eventing/v1beta3/eventtype_validation.go

+	if _, ok := attributes["specversion"]; !ok {
+		missingFields = append(missingFields, "specversion")
+	}
+	if _, ok := attributes["id"]; !ok {
+		missingFields = append(missingFields, "id")
+	}


I'm wondering that to make the usage easier we could have id and specversion be optional even if it's technically required

but I think starting with required is fine as making it optional would be a non-breaking change while the opposite is not true

That makes sense to me, especially if id is going to just be a UUID, and specversion will probably just be 1.0

pierDipi

/lgtm
/approve

knative-prow · 2024-03-25T18:03:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, 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~~ [Cali0707,dsimansk,pierDipi]

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

pierDipi · 2024-03-27T15:23:49Z

/unhold

pierDipi · 2024-03-27T16:54:23Z

/test reconciler-tests

Cali0707 · 2024-03-27T21:57:40Z

/test reconciler-tests

knative-prow · 2024-03-27T22:09:35Z

@Cali0707: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
reconciler-tests_eventing_main	`2c3a610`	link	unknown	`/test reconciler-tests`

Your PR dashboard.

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. I understand the commands that are listed here.

Cali0707 · 2024-03-28T13:24:06Z

/test reconciler-tests

Cali0707 added 3 commits February 20, 2024 11:01

feat: update eventtype CRD to have new schema

72b7513

Signed-off-by: Calum Murray <[email protected]>

feat: update eventtype v1beta3 types to reflect CRD changes

9bc4262

Signed-off-by: Calum Murray <[email protected]>

fix: unit tests pass with new schema

1057fd1

Signed-off-by: Calum Murray <[email protected]>

knative-prow bot requested review from aliok and Leo6Leo February 20, 2024 16:36

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 20, 2024

knative-prow bot requested review from dsimansk, matzew and pierDipi February 20, 2024 16:37

matzew reviewed Feb 21, 2024

View reviewed changes

fix: eventtype attributes are an array now

c63aa4a

Signed-off-by: Calum Murray <[email protected]>

knative-prow bot requested a review from matzew February 27, 2024 16:05

Cali0707 added 2 commits February 27, 2024 11:14

Merge branch 'main' into et-v1beta3-types

bef8f6b

fix: lineage tests pass again with new eventtype spec

6d0313d

Signed-off-by: Calum Murray <[email protected]>

dsimansk reviewed Feb 28, 2024

View reviewed changes

dsimansk approved these changes Feb 28, 2024

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2024

knative-prow bot assigned dsimansk Feb 28, 2024

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

matzew reviewed Mar 14, 2024

View reviewed changes

cleanup: tests use attribute names always

ab034b5

Signed-off-by: Calum Murray <[email protected]>

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2024

knative-prow bot requested a review from dsimansk March 18, 2024 16:38

knative-prow bot requested a review from matzew March 18, 2024 16:38

Cali0707 mentioned this pull request Mar 19, 2024

Event Lineage: Map Sources to Graph Edge & Node #7683

Closed

pierDipi reviewed Mar 25, 2024

View reviewed changes

fix: jsonpath for the type and source attributes is correct

2c3a610

Signed-off-by: Calum Murray <[email protected]>

knative-prow bot requested a review from pierDipi March 25, 2024 12:53

pierDipi reviewed Mar 25, 2024

View reviewed changes

pierDipi approved these changes Mar 25, 2024

View reviewed changes

knative-prow bot assigned pierDipi Mar 25, 2024

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

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024

knative-prow bot merged commit e581d36 into knative:main Mar 27, 2024
33 of 39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update eventtype v1beta3 type to reflect spec changes #7708

Update eventtype v1beta3 type to reflect spec changes #7708

Cali0707 commented Feb 20, 2024

Cali0707 commented Feb 20, 2024

codecov bot commented Feb 20, 2024 •

edited

Loading

matzew Feb 21, 2024

Cali0707 Feb 21, 2024

Cali0707 commented Feb 27, 2024

dsimansk Feb 28, 2024

Cali0707 Feb 28, 2024

dsimansk Feb 28, 2024

Cali0707 Mar 18, 2024

dsimansk left a comment

Cali0707 commented Mar 11, 2024

matzew Mar 14, 2024

Cali0707 commented Mar 18, 2024

pierDipi Mar 25, 2024

Cali0707 Mar 25, 2024

matzew Mar 25, 2024

pierDipi Mar 25, 2024

Cali0707 Mar 25, 2024

Cali0707 commented Mar 25, 2024

matzew commented Mar 25, 2024

pierDipi Mar 25, 2024 •

edited

Loading

pierDipi Mar 25, 2024

Cali0707 Mar 25, 2024

pierDipi left a comment

knative-prow bot commented Mar 25, 2024

pierDipi commented Mar 27, 2024

pierDipi commented Mar 27, 2024

Cali0707 commented Mar 27, 2024

knative-prow bot commented Mar 27, 2024

Cali0707 commented Mar 28, 2024

Update eventtype v1beta3 type to reflect spec changes #7708

Update eventtype v1beta3 type to reflect spec changes #7708

Conversation

Cali0707 commented Feb 20, 2024

Proposed Changes

Cali0707 commented Feb 20, 2024

codecov bot commented Feb 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cali0707 commented Feb 27, 2024

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 left a comment

Choose a reason for hiding this comment

Cali0707 commented Mar 11, 2024

Choose a reason for hiding this comment

Cali0707 commented Mar 18, 2024

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

Cali0707 commented Mar 25, 2024

matzew commented Mar 25, 2024

pierDipi Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Mar 25, 2024

pierDipi commented Mar 27, 2024

pierDipi commented Mar 27, 2024

Cali0707 commented Mar 27, 2024

knative-prow bot commented Mar 27, 2024

Cali0707 commented Mar 28, 2024

codecov bot commented Feb 20, 2024 •

edited

Loading

pierDipi Mar 25, 2024 •

edited

Loading