Add context for all API interactions #86
Conversation
|
@armando-rodriguez-cko Would you mind having a look at this when you get 10mins please? |
|
Thank you @jamieaitken!, we will take a look. |
|
Hey @armando-rodriguez-cko 👋 Was wondering if you have had time to look over the PR? |
|
@jamieaitken we are reviewing, sorry for the delay. |
|
Hi @jamieaitken, we are still reviewing. |
|
This is a great way to support instrumentation on the underlying HTTP client as well, using eg https://github.com/DataDog/dd-trace-go/blob/main/contrib/net/http/roundtripper.go#L120 for datadog |
|
Hi @jamieaitken, we are still reviewing. We don't forget it |
armando-rodriguez-cko
force-pushed
the
add-context
branch
from
2b366bf to
7c7414c
Compare
|
Hey @armando-rodriguez-cko 👋 Wondering if you got anywhere with this? |
|
We are finishing the review, we are sorry. We will try to finish it in two weeks. |
|
Hey @armando-rodriguez-cko 👋 Do we still intend to merge this? If not, I can close the PR |
I apologise for the big delay, I want to merge it, but I don't have enough time now, but I will make an effort to do that, I am sorry again. |
|
Hey @armando-rodriguez-cko 👋 Are we any closer to getting this merged? If it's easier, I can limit this PR to only add Context for NAS interactions? |
armando-rodriguez-cko
merged commit d97d60f
into
checkout:beta-context
|
Hi @jamieaitken , I apologise for the delay, I am merging your branch in a beta branch to test it better |
Hey folks 👋 I've added a context counterpart for all public API functions so that requests will be cancelled if the client cancels.
My reasoning for creating a counterpart instead of changing the existing functions was to avoid breaking changes and as for the approach, I took inspiration from how the standard library dealt with http.NewRequest and http.NewRequestWithContext