Added retry of functions

[alankstewart]

Retry has been added to most GitHubServices methods. Also fixed bug in paginated search and list functions where the page parameter was not being handled correctly which resulted in erroneous search results.

Simplified exceptions, reducing artifact source to one exception, ArtifactSourceException, which is a concrete class now . The rest were overkill

[kipz]

Would be nice if we could surface the actual github response code and text somehow.

[alankstewart]

It will be when merged

[alankstewart]

With regard to a single exception, i.e. ArtifactSourceException, I don't think rugs will be affected. The three previous ones, ArtifactSourceAccessException, ArtifactSourceUpdateException, and ArtifactSourceCreationException all extendedArtifactSourceException(albeit an abstract class previously). But these subclasses were not be used any differently to any of the others.

[kipz]

Should we have a CHANGELOG entry here?

[alankstewart]

Yes, will add change log entries when it is released as 1.0.0-m.14.

[ddgenome]

Seems like squash on merge would be a good idea.

[alankstewart]

I assumed squashing was always the way to go

[ddgenome]

Yes, sorry I was not clear. Retry on all internal server errors, i.e., all error codes >= 500.

On Thu, Jul 27, 2017 at 7:36 AM Alan Stewart notifications@github.com wrote:

@alankstewart commented on this pull request.

In src/main/scala/com/atomist/source/git/github/GitHubServices.scala https://github.com/atomist/artifact-source/pull/41#discussion_r129829103 :

• case Some(data) => method match {
• case Post => Http(url).postData(data)
• case Put => Http(url).put(data)
• case Patch => Http(url).postData(data).method(method.toString)
• case _ => Http(url).postData(data)
• }
• case None => Http(url).method(method.toString)
• }).headers(headers)
• .params(params)
• .exec((code: Int, headers: Map[String, IndexedSeq[String]], is: InputStream) => code match {
• case 200 | 201 => fromJsonT
• case success if 202 until 206 contains success =>
• logger.debug(s"${headers("Status").head}")
• ().asInstanceOf[T]
• case 401 | 403 | 422 => throw DoNotRetryException(s"${headers("Status").head}")
• case serverError if serverError >= 500 => throw DoNotRetryException(s"${headers("Status").head}")

Hmm. Ok. But other 5xx not?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/atomist/artifact-source/pull/41#discussion_r129829103, or mute the thread https://github.com/notifications/unsubscribe-auth/AADiGaJ_qpCBI3SMn01sMkpX3QWQSKS7ks5sSIQ6gaJpZM4OkzRq .

--

David Dooling

[alankstewart]

Added all server errors to retry