-
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
Use triggerv2 controller in shared main #3558
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
Skipping CI for Draft Pull Request. |
/test all |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3558 +/- ##
============================================
+ Coverage 52.43% 54.15% +1.72%
- Complexity 874 875 +1
============================================
Files 342 346 +4
Lines 21415 18193 -3222
Branches 284 288 +4
============================================
- Hits 11229 9853 -1376
+ Misses 9276 7415 -1861
- Partials 910 925 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Calum Murray <cmurray@redhat.com>
/test all |
1 similar comment
/test all |
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
/test all |
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
/test all |
Signed-off-by: Calum Murray <cmurray@redhat.com>
/test all |
Signed-off-by: Calum Murray <cmurray@redhat.com>
/test all |
Signed-off-by: Calum Murray <cmurray@redhat.com>
/test upgrade-tests |
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
One nit, we can fix it later
} | ||
|
||
func (k *kafkaDeploymentDeleter) deleteDeployment(ctx context.Context, deploymentName string) error { | ||
err := k.waiteStatefulSetExists(ctx, deploymentName) |
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.
typo waite....
unless it's a word I don't know about
/test reconciler-tests-namespaced-broker |
@pierDipi can I unhold this PR? |
It looks like the hold was originally because this PR was on hold because of the 1.13 branch cut, I'm unholding now since we are a month away from the branch cut for 1.14 /unhold |
/test reconciler-tests-namespaced-broker |
/retest-required |
/test reconciler-tests-namespaced-broker |
/retest-required |
/test reconciler-tests-namespaced-broker |
/test reconciler-tests |
/retest-required |
Signed-off-by: Calum Murray <cmurray@redhat.com>
/cc @pierDipi Would you mind taking a look at my most recent commit? I changed the namespaced_controller for the triggers to use a filtered global resync, and to resync the triggers instead of the brokers. This seems to have fixed the namespaced broker tests, but I'm not sure if there was a reason in the past for en-queuing the namespaced brokers instead of triggers when doing a global resync of the triggers |
/retest-required |
/test reconciler-tests |
Signed-off-by: Calum Murray <cmurray@redhat.com>
/retest-required |
/test reconciler-tests-namespaced-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.
/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 |
de17c00
into
knative-extensions:main
if err != nil { | ||
return fmt.Errorf("failed to resolve auth context: %w", err) | ||
} | ||
|
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.
Not sure why we dropped this here
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.
Oh yeah, I think this might have snuck in while trying to fix merge conflicts.
As we move towards scaling triggers with KEDA, we first need to migrate triggers to use consumergroups.
Proposed Changes
contract-resources
configmap, and for the broker-dispatcher to be deployed as a statefulsetRelease Note
Docs