[WIP] Handle errors

[dpordomingo]

depends on #49 [implement MultiToken]
fix #3 [handle RateLimit errors]
fix #18 [handle Abuse errors]

TODO

• Rebase over Multi token support #49 [implement MultiToken]
• Integration tests testing: Retry + RateLimit (Could be added in a new PR, or also in this PR)

Changes grom ghsync

• the retry policy is implemented by github.RetryTransport, not by github.RateLimitTransport

Also needed:

• adf3a6b Enhance retryTransport:
  • retry also 500 errors
  • ensure it does not return errors but the ones from Transport.RoundTrip
  • log last error and the number of retries
• ee869d8 Restore the Response&Request when read from Transport
  • Since the Request.Body and Response.Body are read from any transport.RoundTrip,
    it is needed to restore them before reusing it again;
    otherwise, their Body won't be readable from other nested Transport.

How does it work?

Trying to fetch an org with a GH token under an AbuseRateLimmit block, whose quota also expired:

1. github.Downloader tries to get the data, but GH answers with n 403, AbuseRateLimmit,
2. the error is intercepted by github.RateLimitTransport, and the lock is enabled for the time specified by the headers from the 403, AbuseRateLimmit; the error is bubbled up
3. the error is captured by github.RetryTransport, and retried
4. github.RateLimitTransport is locked, so it sleeps till the lock expires.
5. once the lock expired, the request is processed. In a regular situation, an AbuseRateLimmit will disappear, but in this example, it appeared a RateLimmit for that same query which is intercepted again by github.RateLimitTransport, and the lock is enabled again for the time specified by the headers from the 200, StatusOk; the error is bubbled up
6. the error is captured by github.RetryTransport, and retried
7. github.RateLimitTransport is locked, so it sleeps till the lock expires.
8. once the lock expired, the request is processed with no new limits.

---

• I have updated the CHANGELOG file according to the conventions in keepachangelog.com
• This PR contains changes that do not require a mention in the CHANGELOG file

[se7entyse7en]

build on top of #49 [implement MultiToken]

Do you need this? they should be independent as you work at transport level. I mean if this is ready, I won't wait for #49.

Integration tests testing: Retry + RateLimit

AFAIS We already have unittests, let's open a different issue for integration tests, also maybe @kyrcha could take care of it.

I found (and fixed) some bugs caused when the Request/Response are read from a Transport, which cause that they're no longer available for other Transport in the chain.

AFAIR there was already a fix for this in ghsync.

[kyrcha]

I have reproduced (I think) all of the possible HTTP codes:

• 200
• 400
• 401
• 403
• 502
• 200 but with rate limit exceeded

for this issue #40 so tests will be added for such matters.

[dpordomingo]

@se7entyse7en Yes, the rebase over #49 is needed; i.e. it's also causing the current conflict.
More details in my review for that PR
TL;DR you moved the Transport chain definition elsewhere, so I need to adapt this.

@se7entyse7en No, the issue about the ruined body was not added in ghsync because there was no Request.Body needed at all (because the query was built directly from the Request.URL.)
More details in 00086cf

[dpordomingo]

.

[dpordomingo]

I'll create a new PR from an upstream branch

[kyrcha]

I will wait for the new PR for the rest

[kyrcha]

First or better Pop?

[kyrcha]

According to previous reviews by @lwsanty the metadata-retrieval variables should be in the middle. I am guessing it is better if we build a linter for that matter.

[dpordomingo]

Thanks. Will do.