Skip to content
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

Added RetryConditions to Request #436

Merged
merged 2 commits into from Sep 16, 2021

Conversation

rohitkg98
Copy link
Contributor

@rohitkg98 rohitkg98 commented Jul 14, 2021

  • Retry conditions can now also be added to Request

Closes #433
Closes #315
Closes #324

@rohitkg98
Copy link
Contributor Author

@jeevatkm any updates on this?

request.go Outdated
@@ -747,7 +758,7 @@ func (r *Request) Execute(method, url string) (*Response, error) {
Retries(r.client.RetryCount),
WaitTime(r.client.RetryWaitTime),
MaxWaitTime(r.client.RetryMaxWaitTime),
RetryConditions(r.client.RetryConditions),
RetryConditions(append(r.client.RetryConditions, r.retryConditions...)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohitkg98 If we are adding a Retry condition feature at the request level; is it a request retry condition that should take place first instead of the client?

@moorereason your thoughts!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohitkg98 Basically, Request retry conditions have to execute first and then client instance level. So we need to flip the append arguments.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking for this feature as well. The most specific (request specific retry condition), should apply first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to check request specific conditions before client ones

@codecov
Copy link

codecov bot commented Sep 14, 2021

Codecov Report

Merging #436 (9a6409d) into master (70336cb) will increase coverage by 0.54%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #436      +/-   ##
==========================================
+ Coverage   96.06%   96.60%   +0.54%     
==========================================
  Files          10       10              
  Lines        1041     1325     +284     
==========================================
+ Hits         1000     1280     +280     
- Misses         23       27       +4     
  Partials       18       18              
Impacted Files Coverage Δ
client.go 97.59% <ø> (+0.73%) ⬆️
request.go 97.81% <100.00%> (-2.19%) ⬇️
resty.go 100.00% <0.00%> (ø)
retry.go 100.00% <0.00%> (ø)
trace.go 100.00% <0.00%> (ø)
response.go 100.00% <0.00%> (ø)
transport.go 100.00% <0.00%> (ø)
middleware.go 92.52% <0.00%> (+1.02%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70336cb...9a6409d. Read the comment docs.

@chb0github
Copy link

Stoked for a merge

@chb0github
Copy link

??

Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rohitkg98 Thanks for the PR and contribution.

@jeevatkm jeevatkm merged commit 2cae8cd into go-resty:master Sep 16, 2021
@jeevatkm jeevatkm added this to the v2.7.0 Milestone milestone Sep 16, 2021
@chb0github
Copy link

When will this be released? I updated my go.mod dependency to 2.7.0 and it isn't resolving

jeevatkm added a commit that referenced this pull request Oct 25, 2021
hurracrane pushed a commit to Shopify/resty that referenced this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants