-
Notifications
You must be signed in to change notification settings - Fork 590
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
List applying policies in Parallel #8083
List applying policies in Parallel #8083
Conversation
Signed-off-by: rahulii <r.sawra@gmail.com>
Skipping CI for Draft Pull Request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8083 +/- ##
==========================================
+ Coverage 67.84% 67.87% +0.03%
==========================================
Files 367 367
Lines 17349 17474 +125
==========================================
+ Hits 11770 11861 +91
- Misses 4844 4873 +29
- Partials 735 740 +5 ☔ View full report in Codecov by Sentry. |
Signed-off-by: rahulii <r.sawra@gmail.com>
/ok-to-test |
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.
Thank you for the PR! @rahulii , great job! Could you also take a look at the formatting issue that is reported by reviewdog?
Signed-off-by: rahulii <r.sawra@gmail.com>
Recently reviewdog started reporting failures on the whole project (not just modified files in the PR). We're trying to go through and fix those issues, so unless the reviewdog failure is on one of the files you changed in this PR don't worry about it - we are working to fix it all |
thanks for the update @Cali0707 👍🏼 |
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
Thanks for the great work! It looks good to me right now.
/cc @Cali0707
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.
Looks good, just a few things I think we need to fix up and then this should be good to merge - nice work @rahulii !
Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
var globalResync func() | ||
featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"), func(name string, value interface{}) { | ||
if globalResync != nil { | ||
globalResync() | ||
} | ||
}) | ||
featureStore.WatchConfigs(cmw) |
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.
Nice! We just need to use this store in the parallelreconciler.NewImpl(ctx, r, ...options)
now (line 58). See for example how that is done here:
eventing/pkg/reconciler/inmemorychannel/controller/controller.go
Lines 105 to 109 in bb2e0a3
impl := inmemorychannelreconciler.NewImpl(ctx, r, func(impl *controller.Impl) controller.Options { | |
return controller.Options{ | |
ConfigStore: featureStore, | |
} | |
}) |
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.
sure, thanks for reminding @Cali0707 👍🏼
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.
done, PTAL!
Signed-off-by: rahulii <r.sawra@gmail.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
Thanks for all your work here @rahulii !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, rahulii 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 |
Fixes #7979
Proposed Changes
🎁 List EventPolicies in the Parallel .status.policies, which apply for it.
🎁 Add the EventPoliciesReady condition to indicate, if the referenced policies are Ready
Pre-review Checklist
Release Note
Docs