-
Notifications
You must be signed in to change notification settings - Fork 117
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
Create eventtypes on reply events to triggers and subscriptions #4077
Conversation
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>
/cc @matzew |
message FeatureFlags { | ||
bool enableEventTypeAutocreate = 1; | ||
} |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Codecov ReportAttention: Patch coverage is
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. |
/retest |
Signed-off-by: Calum Murray <cmurray@redhat.com>
a38f944
to
e07ee21
Compare
Signed-off-by: Calum Murray <cmurray@redhat.com>
/retest |
…ance Signed-off-by: Calum Murray <cmurray@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[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 |
4adf16f
into
knative-extensions:main
Fixes #4076
Fixes #4075
Proposed Changes
Release Note