-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: move the rollout controller to event driven model #1029
base: main
Are you sure you want to change the base?
Conversation
f5fb9bf
to
a9ea18c
Compare
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.
Hi Ryan! I've added some comments, PTAL 🙏
ed4ed9d
to
7fba3b1
Compare
The bug was the workGenerator controller will mark the binding as overridden failed but rollout controller doesn't consider that binding has "FAILED". This leads to the roll out stuck because the binding can be ready thus it prevent the maxNumber to go over. There needs to be a follow up on the workGenerator/applier to differentiate (best effort) user error/not retriable error from retriable. |
07e30c7
to
5ed16b6
Compare
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 ;) Has some minor comments; will add them later -> merging the PR now upon test completion to unblock progress
Description of your changes
Fixes #
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer