adding WithRetry option

[luthermonson]

With PR 86 recently merged you now let the package user get the RetryAfter from the a field on the client and you can do your own backing off/retry... I always felt the library should include it so here is WithRetry option to let package users define how many retries to attempt before failing. Defaults to 0 for no retry at all and let's you set an integer for how many retries to do. WithRetry(3) will let the client retry three times after waiting the RetryAfter backoff time and then error out with the normal RateLimitError as expected after the third attempt if it's still being rate limited.

Changes

• added WithRetry(int) option
• moved to http status codes for some files
• refactored usagecharge tests for consistency
• added go 1.14 and upgraded to bionic for travisci build
• updated "invalid invalid protocol" test for 1.14

[codecov]

Codecov Report

Merging #87 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #87   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files          31       32    +1     
  Lines        1256     1281   +25     
=======================================
+ Hits         1254     1279   +25     
  Misses          1        1           
  Partials        1        1           

Impacted Files	Coverage Δ
goshopify.go	100.00% <100.00%> (ø)
options.go	100.00% <100.00%> (ø)

---

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 1dad68b...2536d39. Read the comment docs.

[luthermonson]

@rickywiens @gordcurrie could i get some movement on this? i want to add WithDebug next and want to make sure we have some blessings on the refactor to the options.go file before I add anything else too it.

[oliver006]

General comment: this change will make the API retry only when a rate limit error is being returned but the wording "WithRetries" is so generic that I'd expect retries in other cases too.
E.g. internet connectivity (line 303 failing?) or server side errors (500s)

[luthermonson]

@oliver006 we could change it to WithRateLimitRetry() or I can make it retry on any non-200. Which would you prefer?

[oliver006]

Which would you prefer? - I think i'd expect the client to retry on a rate-limit error and on any 500 server errors but I'd defer to the owners of this repo (@bold-commerce) to hear what their opinion is.

[ysurtayev-bold]

@luthermonson please respond to @oliver006 comments. All of them make sense to me.
I also will be adding response about the naming / cases when it it should retry.
Otherwise looks good. Thank you!

[ysurtayev-bold]

Which would you prefer? - I think i'd expect the client to retry on a rate-limit error and on any 500 server errors but I'd defer to the owners of this repo (@bold-commerce) to hear what their opinion is.

From my point of view it make sense to retry if there is a chance of not getting the same error again. For example connection issues or rate limiter response. I do not think it should retry on any 500 error. So if you can extend implementation to cover more scenarios when to retry, we can keep the current name for the parameter. If you prefer to limit it to rate-limiter responses, it will be better to rename the parameter as suggested by @oliver006 .

[luthermonson]

@ysurtayev-bold @oliver006 as per this https://www.shopify.dev/concepts/about-apis/response-codes i added a check for 503, there's a switch now where we could add more as needed but now it's retrying on 429 and 503. tests updated too. thanks

[luthermonson]

if anyone has ideas why the tests pre 1.13 are failing let me know. no time to look at it tonight. thx

[luthermonson]

@oliver006 @ysurtayev-bold hopefully everything addressed, please give it a last look

[oliver006]

@luthermonson - I'm a bit busy with work this week but will go over this again as soon as I can.

[luthermonson]

@gordcurrie you might want to get this in, the UserCharge tests are failing for everyone now and this PR refactors them to avoid the reflect.DeepEqual so they pass. i understand if we still want changes to the idea but the tests are blocking other PRs

[gordcurrie]

Approach makes sense to me. I'll merge once the conversation around tests is resolved.

[gordcurrie]

Looks great to me thanks for the contribution and thanks to all involved in reviewing and making suggestions.

[oliver006]

🎉