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.

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

Create eventtypes on reply events to triggers and subscriptions #4077

Merged
merged 13 commits into from
Aug 26, 2024

Conversation

Cali0707
Copy link
Member

@Cali0707 Cali0707 commented Aug 20, 2024

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

Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
@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
Comment on lines +297 to +299
message FeatureFlags {
bool enableEventTypeAutocreate = 1;
}
Copy link
Member

@pierDipi pierDipi Aug 20, 2024

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

Signed-off-by: Calum Murray <cmurray@redhat.com>
@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
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Copy link

codecov bot commented Aug 20, 2024

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
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
@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

…ance

Signed-off-by: Calum Murray <cmurray@redhat.com>
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
4 participants