Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ETs reference subscriptions/triggers on reply #7733

Merged
merged 9 commits into from
Apr 3, 2024
Merged

ETs reference subscriptions/triggers on reply #7733

merged 9 commits into from
Apr 3, 2024

Conversation

Cali0707
Copy link
Member

Fixes #7698
Fixes #7699

Proposed Changes

While I was working on #7699 I realized that the fix for #7699 would lead to a simpler solution to #7698 than I had in #7710. This PR now has the fix for both issues

  • Add the et autocreate on reply logic to the kncloudevents package
  • Update the filter handler and fanout request handlers to set the correct kncloudevent dispatch options if et autocreate is enabled
  • Update the SubscriberSpec to include the info needed to do EventType autocreate on replies

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note

EventTypes are now autocreated on Triggers and Subscriptions when there is a reply event sent to them

Docs

@Cali0707
feat: support et autocreate on replies in kncloudevents dispatcher
d3e83fc 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
feat: et autocreate on replies references triggers
73bffd6 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
feat: et autocreate reference subscriptions on replies
cf10315 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
test: added rekt tests for eventtype autocreate on replies
53b8b49 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2024
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Feb 28, 2024
@Cali0707
fix: unit tests pass
b5f7281 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 1, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 1, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 68.92%. Comparing base (bbb9051) to head (91c27e1).
Report is 16 commits behind head on main.

❗ Current head 91c27e1 differs from pull request most recent head ae29366. Consider uploading reports for the commit ae29366 to get more accurate results

Files Patch % Lines
pkg/kncloudevents/event_dispatcher.go 0.00% 25 Missing and 1 partial ⚠️
cmd/broker/filter/main.go 0.00% 15 Missing ⚠️
pkg/channel/fanout/fanout_event_handler.go 16.66% 13 Missing and 2 partials ⚠️
pkg/broker/filter/filter_handler.go 0.00% 11 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7733      +/-   ##
==========================================
- Coverage   69.15%   68.92%   -0.23%     
==========================================
  Files         339      339              
  Lines       19431    19502      +71     
==========================================
+ Hits        13437    13442       +5     
- Misses       5332     5394      +62     
- Partials      662      666       +4

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

pierDipi
pierDipi reviewed Mar 13, 2024
View reviewed changes
pkg/apis/duck/v1/subscribable_types.go Outdated
Comment on lines 41 to 43
// Namespace is used to identify the namespace of the original subscription object.
// +optional
Namespace *string `json:"namespace,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing this in more details, I think that we can add the namespace when the new cross ns feature is introduced so that we can place the namespace usage behind feature flag and test it extensively as it's a critical security area.

For now, I would use the channel/subscription namespace instead of this custom field

pierDipi
pierDipi reviewed Mar 13, 2024
View reviewed changes
test/experimental/features/eventtype_autocreate/features.go Outdated

f.Alpha("broker").
Must("deliver events to subscriber", assert.OnStore(sink).MatchEvent(cetest.HasId(event.ID())).AtLeast(1)).
Must("create event type referencing the trigger", eventtype.WaitForEventType(eventtype.AssertReferencePresent(trigger.AsKReference(triggerName))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe matching also the type might provide an additional coverage

@Cali0707
address review comments
65a995d 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 18, 2024
@Cali0707
Copy link
Member Author

/cc @pierDipi

@knative-prow knative-prow bot requested a review from pierDipi March 18, 2024 17:17
@Cali0707
fix: imc dispatcher reconciler no longer panics on error
91c27e1 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 18, 2024
@pierDipi pierDipi mentioned this pull request Mar 28, 2024
Closed
5 tasks
@Cali0707
use GroupVersion variables instead of hardcoded strings
ae29366 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

Cali0707 commented Apr 2, 2024

/cc @pierDipi

@knative-prow knative-prow bot requested a review from pierDipi April 2, 2024 18:08
@Cali0707
Copy link
Member Author

Cali0707 commented Apr 2, 2024

/retest-required

pierDipi
pierDipi approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Apr 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, 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:

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

@pierDipi
Copy link
Member

pierDipi commented Apr 3, 2024

/test reconciler-tests

@knative-prow knative-prow bot merged commit 366afed into knative:main Apr 3, 2024
34 of 38 checks passed
This was referenced Apr 22, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi approved these changes

@creydr creydr Awaiting requested review from creydr

@Leo6Leo Leo6Leo Awaiting requested review from Leo6Leo

@evankanderson evankanderson Awaiting requested review from evankanderson

Assignees

@pierDipi pierDipi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
2 participants
@Cali0707 @pierDipi