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

Create eventtypes on reply events to triggers and subscriptions #4077

Merged
merged 13 commits into from
Aug 26, 2024
Merged

Create eventtypes on reply events to triggers and subscriptions #4077

merged 13 commits into from
Aug 26, 2024

Conversation

Cali0707
Copy link
Member

@Cali0707 Cali0707 commented Aug 20, 2024 •
edited by pierDipi
Loading

Fixes #4076
Fixes #4075

Proposed Changes

  • Refactor the contract to have the feature flags available at the resource level, not on the ingress level.
  • Handle autocreate on replies in the dispatcher
  • Add an e2e test

Release Note

When eventtype autocreate is enabled, eventtypes are now created referencing subscriptions/triggers when there is a reply to the subscription/trigger

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2024
@knative-prow knative-prow bot requested a review from aliok August 20, 2024 14:37
@knative-prow knative-prow bot added area/data-plane size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 20, 2024
@knative-prow knative-prow bot requested a review from pierDipi August 20, 2024 14:37
@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test labels Aug 20, 2024
@Cali0707
Copy link
Member Author

/cc @matzew

@knative-prow knative-prow bot requested a review from matzew August 20, 2024 14:39
pierDipi
pierDipi reviewed Aug 20, 2024
View reviewed changes
proto/contract.proto
Comment on lines +297 to +299
message FeatureFlags {
bool enableEventTypeAutocreate = 1;
}
Copy link
Member

@pierDipi pierDipi Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@creydr wondering if we should pass the full list here as a Map ? and perhaps call the field in Resource CoreFeatureFlags to distinguish between core config-features and Kafka ones config-kafka-features (in case we need them in the future)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that would be good in the future, I just wasn't 100% sure about the approach so I decided to keep it minimal when starting on this PR. Happy to add the full set here/in a follow up PR if it makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Having the full set could also help us on some OIDC stuff to get rid of the config-features CM mount point and watchers.

Copy link
Contributor

Choose a reason for hiding this comment

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

only one question from my side: is there a reason, why we would place it in the Resource message type, instead of at the top level on the Contract (similar to the trust bundles), as they should not really change between resources

Copy link
Member Author

Choose a reason for hiding this comment

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

This was because for EventType autocreate we sometimes want to disable it for a resource even when it is enabled. Specifically, if a channel is owned by a MTChannelBasedBroker, we should not create eventtypes on the channel as they will already be created by the broker

Copy link
Member

Choose a reason for hiding this comment

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

I like that additional flexibility on the resource level

@Cali0707
fix merge conflicts
7560453 
Signed-off-by: Calum Murray <[email protected]>
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 41.89189% with 43 lines in your changes missing coverage. Please review.

Project coverage is 48.42%. Comparing base (ba49d0f) to head (7192915).
Report is 2 commits behind head on main.

Files Patch % Lines
control-plane/pkg/contract/contract.pb.go 17.39% 37 Missing and 1 partial ⚠️
control-plane/pkg/reconciler/channel/channel.go 50.00% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4077      +/-   ##
==========================================
- Coverage   48.49%   48.42%   -0.07%     
==========================================
  Files         244      244              
  Lines       14712    14764      +52     
==========================================
+ Hits         7135     7150      +15     
- Misses       6866     6901      +35     
- Partials      711      713       +2

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

@pierDipi
Copy link
Member

/retest

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2024
@Cali0707 Cali0707 force-pushed the et-autocreate-reply branch from a38f944 to e07ee21 Compare August 20, 2024 20:03
@Cali0707
fix merge conflicts
48f2554 
Signed-off-by: Calum Murray <[email protected]>
@Cali0707 Cali0707 requested review from pierDipi and creydr August 21, 2024 13:54
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2024
@pierDipi
Copy link
Member

/retest

pierDipi
pierDipi approved these changes Aug 26, 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 Aug 26, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Aug 26, 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

@knative-prow knative-prow bot merged commit 4adf16f into knative-extensions:main Aug 26, 2024
35 of 37 checks passed
@mgencur mgencur mentioned this pull request Sep 10, 2024
Open
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

@aliok aliok Awaiting requested review from aliok

@matzew matzew Awaiting requested review from matzew

@creydr creydr Awaiting requested review from creydr

Assignees

@pierDipi pierDipi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane area/data-plane area/test 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
4 participants
@Cali0707 @pierDipi @creydr @knative-prow-robot