-
Notifications
You must be signed in to change notification settings - Fork 674
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
Send DAG status updates via StatusUpdateHandler #2613
Conversation
I pushed |
86d8355
to
7244bae
Compare
Codecov Report
@@ Coverage Diff @@
## master #2613 +/- ##
==========================================
- Coverage 76.79% 76.71% -0.08%
==========================================
Files 71 71
Lines 5502 5505 +3
==========================================
- Hits 4225 4223 -2
- Misses 1190 1197 +7
+ Partials 87 85 -2
|
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
cmd/contour/serve.go
Outdated
// Now we have the statusUpdateWriter, we can create the StatusWriter, which will take the | ||
// status updates from the DAG, and send them to the status update handler. | ||
eventHandler.StatusClient = &k8s.StatusWriter{ | ||
Updater: suw, |
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.
Nit: it would be a bit cleaner if we just called sh.Writer()
where necessary, rather than having a temporary suw
.
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.
1939d6f
to
36f62e8
Compare
Updates the DAG StatusClient to use the StatusUpdateWriter pattern. Fixes projectcontour#2560 Fixes projectcontour#2580 Fixes projectcontour#2522 We should be able to remove the StatusClient eventually, but that API also needs refactoring for adding Conditions (currently under projectcontour#2495), so for now I've just updated it to send updates via the StatusUpdateWriter rather than make API calls directly. Signed-off-by: Nick Young <ynick@vmware.com>
36f62e8
to
203fe82
Compare
@youngnick I consistently hit #2522 when running contour integration tests in my kind cluster. I tried the docker image with this fix multiple times and can confirm it fixes #2522 and all the integration tests passes now. |
Updates the DAG StatusClient to use the StatusUpdateWriter pattern.
Fixes #2560
Fixes #2580 - I think it fixes it, but I haven't been able to reproduce, even with the excellent script provided by @primeroz.
Fixes #2522 - I've confirmed this one with testing.
We should be able to remove the StatusClient eventually, but that API also needs refactoring for adding Conditions (currently under #2495), so for now I've just updated it to send updates via the StatusUpdateWriter rather than make API calls directly.
Signed-off-by: Nick Young ynick@vmware.com