Repository navigation
service_client: Accept per-call Context - #2274
pierreprinetti wants to merge 1 commit into
Conversation
32d8fff to
c9e81f3
Compare
|
Build failed.
|
|
Build failed.
|
5733344 to
2ec28d7
Compare
|
Build failed.
|
9d175d0 to
927c51c
Compare
Adds the ability to pass per-call context.Context.
Adds a WithContext method to the ServiceClient, which will return a new
ServiceClient.
The context added to ServiceClient will be used in addition to the
ProviderClient's Context to make HTTP calls.
Example use:
```Go
ctx := context.Background()
allPages, err := servers.List(client.WithContext(ctx), servers.ListOpts{}).AllPages()
// the original `client` is untouched
```
927c51c to
13da401
Compare
|
Build failed.
|
|
The test failures look related; I'm investigating those next week. I am not merging anything anyway until there's some buy-in on the general idea of adding a context at the service level. |
|
Hi, @pierreprinetti is this still on your radar? I'm still interested in this feature. |
Yes it is! We definitely want to land support for call-scoped context, however we will probably rather opt for a non-breaking solution like new |
|
I now believe that this PR is not the way to go. The ProviderClient's context provides cancellation for the lifespan of authentication. What we need is the ability to cancel a single call independently. There is no need to merge the two contexts. The next proposal should focus on adding XxxWithContext functions that just pass their ctx to the http.Request. |
3 participants
Adds the ability to pass per-call context.Context.
Adds a WithContext method to the ServiceClient, which will return a new
ServiceClient.
The context added to ServiceClient will be used in addition to the
ProviderClient's Context to make HTTP calls.
Example use:
Most of the complexity of this patch is a consequence of the choice to merge the ServiceClient context with the ProviderClient's, instead of just overwriting it. This is another choice that is up for debate.
Alternative B:
However I see one issue with this alternative: not all verbs currently accept an "options". See for example
servers.Get(client *gophercloud.ServiceClient, id string). Such a change would not then be backwards-compatible.Alternative C:
Duplicate every user-facing function with a
WithContextversionAlternative D:
Wait until the next Major release to change every single user-facing function to accept a Context as the first argument.
Any more ideas?
For #1987