-
Notifications
You must be signed in to change notification settings - Fork 327
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
chore(*) migrate from closed channel to context for app lifetime #2804
Conversation
This is required because newer versions of the controller-runtime will rely on context instead of a closed channel to indicate that the app should shutdown Signed-off-by: Charly Molter <charly.molter@konghq.com>
Related to #2764 |
Codecov Report
@@ Coverage Diff @@
## master #2804 +/- ##
==========================================
+ Coverage 52.04% 52.08% +0.03%
==========================================
Files 882 882
Lines 51320 51321 +1
==========================================
+ Hits 26708 26728 +20
+ Misses 22490 22469 -21
- Partials 2122 2124 +2
Continue to review full report at Codecov.
|
@@ -84,11 +84,11 @@ func (r *PodReconciler) Reconcile(req kube_ctrl.Request) (kube_ctrl.Result, erro | |||
if pod.Namespace != r.SystemNamespace { | |||
return kube_ctrl.Result{}, errors.Errorf("Ingress can only be deployed in system namespace %q", r.SystemNamespace) | |||
} | |||
services, err := r.findMatchingServices(pod) | |||
services, err := r.findMatchingServices(ctx, pod) |
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.
Rather than passing the context down everywhere, what about making it a public member of PodReconciler
, and if it isn't already set in Reconcile
, initialize it to context.Background()
there.
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.
In the upgrade to the next version of controller-runtime this method becomes Reconcile(ctx, req)
so knowing this I'd rather keep it this way.
@@ -178,7 +179,8 @@ var _ = Describe("Dataplane Lifecycle", func() { | |||
err := callbacks.OnStreamRequest(streamId, &req) | |||
Expect(err).ToNot(HaveOccurred()) | |||
|
|||
close(shutdown) | |||
cancel() | |||
cancel() |
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.
Twice?
Signed-off-by: Charly Molter <charly.molter@konghq.com>
Signed-off-by: Charly Molter <charly.molter@konghq.com>
This is required because newer versions of the controller-runtime will rely on context instead of a closed channel to indicate that the app should shutdown Signed-off-by: Charly Molter <charly.molter@konghq.com> (cherry picked from commit 09949cf) # Conflicts: # .golangci.yml
This is required because newer versions of the controller-runtime will rely on context instead of a closed channel to indicate that the app should shutdown Signed-off-by: Charly Molter <charly.molter@konghq.com>
…ahq#2804) This is required because newer versions of the controller-runtime will rely on context instead of a closed channel to indicate that the app should shutdown Signed-off-by: Charly Molter <charly.molter@konghq.com>
…ahq#2804) This is required because newer versions of the controller-runtime will rely on context instead of a closed channel to indicate that the app should shutdown Signed-off-by: Charly Molter <charly.molter@konghq.com>
This is required because newer versions of the controller-runtime
will rely on context instead of a closed channel to indicate that
the app should shutdown
Signed-off-by: Charly Molter charly.molter@konghq.com
backport-to-stable
label if the code is backwards compatible. Otherwise, list breaking changes.