[ADDED] transport option in client settings for rest client #263
Conversation
Signed-off-by: Mohammed Zaki <[email protected]>
|
|
There was a problem hiding this comment.
@zakisk - This only sets the transport for the REST client which is only used by a few of our API features - IE Integration URLs for CECs, Deploy Events, etc
The bulk of our API is in graphql and there is a clientGQL.go file that sets up that client. That file specifically make use a couple of wrapped transport to provide timeout, retry, backoff, etc.
For us to accept this MR we'd need a solution that works for both OR we'd need to somehow scope this only to the REST side if thats what you need the custom transport for.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #263 +/- ##
==========================================
- Coverage 77.77% 77.71% -0.06%
==========================================
Files 50 50
Lines 3275 3280 +5
==========================================
+ Hits 2547 2549 +2
- Misses 531 534 +3
Partials 197 197 ☔ View full report in Codecov by Sentry. |
|
@rocktavious yeah, I'll think a solution for both clients. Thank you |
|
@rocktavious Hello, I would like to inquire if it's possible to add another transport in ClinetSettings for GraphQL client and will use the default transport from retryablehttp if user doesn't pass one? Does this solution appear to be viable and effective to you? |
|
@zakisk - I guess whats the root cause we are trying to fix. If the root problem is that the REST client doesn't automatically support retry and backoff, would it be better to setup the retryablehttp transport for the REST client like we did with the GraphQL client? Maybe we make the default transport in the ClientSettings the retryablehttp one and feed it into both REST and GraphQL. Then if the end user wants to override that its on them to provide a transport that supports retry, backoff, etc but it still goes to both client types? Personally i would prefer to minimize as many variable code paths as possible so know the root cause we are trying to fix would be helpful. |
|
Closing due to staleness |
Issues
This PR fixes #197
Changelog