pkg/cincinnati: scope HTTP request to a context #83
Conversation
openshift-ci-robot
added
the
size/S
| } | ||
| } | ||
|
|
||
| updates, err := checkForUpdate(clusterID, upstream, channel, currentVersion) |
There was a problem hiding this comment.
I would assume this is the only place that needs the ctx, why do any body up the chain care about ctx for checking updates?
There was a problem hiding this comment.
I can cut the chain at any arbitrary point that you prefer, but please do note that the topmost Until layer already takes a stop channel, but doesn't propagate that down. I'd rather suggest to leave the TODO here and later fix the caller to unify the cancellation chain.
There was a problem hiding this comment.
FYI I landed a fix in k8s master to propagate the context through Until: kubernetes/kubernetes#72928
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/test e2e-aws |
|
This was on hold due to the freeze being in place at the time, it should be fine now. /hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/retest |
1 similar comment
|
/retest |
openshift-ci-robot
added
the
needs-rebase
lucab
force-pushed
the
ups/cincinnati-ctx
branch
from
3aeed76 to
9fcd4dd
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lucab If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci-robot
removed
the
needs-rebase
This adds a context to remote cincinnati calls. The topmost context is left floating as the caller is not context-aware at the moment.
lucab
force-pushed
the
ups/cincinnati-ctx
branch
from
9fcd4dd to
278bfb4
Compare
|
Rebased, bump for a review. As stated, I can cut the context wherever you prefer right now, but I personally think that longer term it makes sense to wire all contexts to the top-level one. |
|
Gentle ping for a review. |
|
@lucab: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Carried in #410. /close |
|
@wking: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
5 participants
This adds a context to remote cincinnati calls. The topmost context is
left floating as the caller is not context-aware at the moment.