From b49faeb5eca8257899e089f1200799fc68e0a335 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Thu, 20 Apr 2017 11:35:51 +0200 Subject: [PATCH 1/9] Add bot ability to get issue comments --- .../dotty/tools/bot/PullRequestService.scala | 24 +++++++++++++------ bot/src/dotty/tools/bot/model/Github.scala | 2 ++ bot/test/PRServiceTests.scala | 15 ++++++++++++ 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/bot/src/dotty/tools/bot/PullRequestService.scala b/bot/src/dotty/tools/bot/PullRequestService.scala index 5ae0b37d0411..4b521c84a6eb 100644 --- a/bot/src/dotty/tools/bot/PullRequestService.scala +++ b/bot/src/dotty/tools/bot/PullRequestService.scala @@ -63,14 +63,17 @@ trait PullRequestService { def statusUrl(sha: String): String = s"https://api.github.com/repos/lampepfl/dotty/statuses/$sha" + def issueCommentsUrl(issueNbr: Int): String = + s"https://api.github.com/repos/lampepfl/dotty/issues/$issueNbr/comments" + def toUri(url: String): Task[Uri] = Uri.fromString(url).fold(Task.fail, Task.now) - def getRequest(endpoint: Uri): Request = - Request(uri = endpoint, method = Method.GET).putHeaders(authHeader) + def getRequest(endpoint: Uri): Task[Request] = + Request(uri = endpoint, method = Method.GET).putHeaders(authHeader).pure[Task] - def postRequest(endpoint: Uri): Request = - Request(uri = endpoint, method = Method.POST).putHeaders(authHeader) + def postRequest(endpoint: Uri): Task[Request] = + Request(uri = endpoint, method = Method.POST).putHeaders(authHeader).pure[Task] def shutdownClient(client: Client): Unit = client.shutdownNow() @@ -89,7 +92,7 @@ trait PullRequestService { def checkUser(user: String): Task[Commit => CommitStatus] = { val claStatus = for { endpoint <- toUri(claUrl(user)) - claReq <- getRequest(endpoint).pure[Task] + claReq <- getRequest(endpoint) claRes <- httpClient.expect(claReq)(jsonOf[CLASignature]) } yield { (commit: Commit) => if (claRes.signed) Valid(user, commit) @@ -133,7 +136,7 @@ trait PullRequestService { for { endpoint <- toUri(statusUrl(cm.commit.sha)) - req <- postRequest(endpoint).withBody(stat.asJson).pure[Task] + req <- postRequest(endpoint).withBody(stat.asJson) res <- httpClient.expect(req)(jsonOf[StatusResponse]) } yield res } @@ -158,7 +161,7 @@ trait PullRequestService { def makeRequest(url: String): Task[List[Commit]] = for { endpoint <- toUri(url) - req <- getRequest(endpoint).pure[Task] + req <- getRequest(endpoint) res <- httpClient.fetch(req){ res => val link = CaseInsensitiveString("Link") val next = findNext(res.headers.get(link)).map(makeRequest).getOrElse(Task.now(Nil)) @@ -170,6 +173,13 @@ trait PullRequestService { makeRequest(commitsUrl(issueNbr)) } + def getComments(issueNbr: Int, httpClient: Client): Task[List[Comment]] = + for { + endpoint <- toUri(issueCommentsUrl(issueNbr)) + req <- getRequest(endpoint) + res <- httpClient.expect(req)(jsonOf[List[Comment]]) + } yield res + def checkPullRequest(issue: Issue): Task[Response] = { val httpClient = PooledHttp1Client() diff --git a/bot/src/dotty/tools/bot/model/Github.scala b/bot/src/dotty/tools/bot/model/Github.scala index fafa2b86a37a..e3814deb47a1 100644 --- a/bot/src/dotty/tools/bot/model/Github.scala +++ b/bot/src/dotty/tools/bot/model/Github.scala @@ -40,4 +40,6 @@ object Github { id: Long, state: String ) + + case class Comment(user: Author) } diff --git a/bot/test/PRServiceTests.scala b/bot/test/PRServiceTests.scala index 08155d59dfd9..2858b42c73fe 100644 --- a/bot/test/PRServiceTests.scala +++ b/bot/test/PRServiceTests.scala @@ -10,12 +10,20 @@ import io.circe.parser.decode import model.Github._ import org.http4s.client.blaze._ +import org.http4s.client.Client import scalaz.concurrent.Task class PRServiceTests extends PullRequestService { val user = sys.env("USER") val token = sys.env("TOKEN") + private def withClient[A](f: Client => Task[A]): A = { + val httpClient = PooledHttp1Client() + val ret = f(httpClient).run + httpClient.shutdownNow() + ret + } + def getResource(r: String): String = Option(getClass.getResourceAsStream(r)).map(scala.io.Source.fromInputStream) .map(_.mkString) @@ -60,6 +68,13 @@ class PRServiceTests extends PullRequestService { ) } + @Test def canGetComments = { + val comments: List[Comment] = withClient(c => getComments(2136, c)) + + assert(comments.find(_.user.login == Some("odersky")).isDefined, + "Could not find Martin's comment on PR 2136") + } + @Test def canCheckCLA = { val httpClient = PooledHttp1Client() val validUserCommit = Commit("sha-here", Author(Some("felixmulder")), Author(Some("felixmulder")), CommitInfo("")) From e6ca06ac63283004380ca3d4910f634f950d5490 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Thu, 20 Apr 2017 14:53:21 +0200 Subject: [PATCH 2/9] Change to approving only latest commit, stub methods --- .../dotty/tools/bot/PullRequestService.scala | 137 +++++++++++++----- bot/src/dotty/tools/bot/model/Github.scala | 2 + 2 files changed, 103 insertions(+), 36 deletions(-) diff --git a/bot/src/dotty/tools/bot/PullRequestService.scala b/bot/src/dotty/tools/bot/PullRequestService.scala index 4b521c84a6eb..4fa7d1d21881 100644 --- a/bot/src/dotty/tools/bot/PullRequestService.scala +++ b/bot/src/dotty/tools/bot/PullRequestService.scala @@ -75,17 +75,18 @@ trait PullRequestService { def postRequest(endpoint: Uri): Task[Request] = Request(uri = endpoint, method = Method.POST).putHeaders(authHeader).pure[Task] - def shutdownClient(client: Client): Unit = - client.shutdownNow() + def shutdownClient(client: Client): Task[Unit] = + client.shutdownNow().pure[Task] sealed trait CommitStatus { def commit: Commit def isValid: Boolean } - final case class Valid(user: String, commit: Commit) extends CommitStatus { def isValid = true } + final case class Valid(user: Option[String], commit: Commit) extends CommitStatus { def isValid = true } final case class Invalid(user: String, commit: Commit) extends CommitStatus { def isValid = false } final case class CLAServiceDown(user: String, commit: Commit) extends CommitStatus { def isValid = false } final case class MissingUser(commit: Commit) extends CommitStatus { def isValid = false } + final case class InvalidPrevious(users: List[String], commit: Commit) extends CommitStatus { def isValid = false } /** Partitions invalid and valid commits */ def checkCLA(xs: List[Commit], httpClient: Client): Task[List[CommitStatus]] = { @@ -95,7 +96,7 @@ trait PullRequestService { claReq <- getRequest(endpoint) claRes <- httpClient.expect(claReq)(jsonOf[CLASignature]) } yield { (commit: Commit) => - if (claRes.signed) Valid(user, commit) + if (claRes.signed) Valid(Some(user), commit) else Invalid(user, commit) } @@ -117,33 +118,36 @@ trait PullRequestService { }.map(_.flatten) } - def sendStatuses(xs: List[CommitStatus], httpClient: Client): Task[List[StatusResponse]] = { - def setStatus(cm: CommitStatus): Task[StatusResponse] = { - val desc = - if (cm.isValid) "User signed CLA" - else "User needs to sign cla: https://www.lightbend.com/contribute/cla/scala" - - val stat = cm match { - case Valid(user, commit) => - Status("success", claUrl(user), desc) - case Invalid(user, commit) => - Status("failure", claUrl(user), desc) - case MissingUser(commit) => - Status("failure", "", "Missing valid github user for this PR") - case CLAServiceDown(user, commit) => - Status("pending", claUrl(user), "CLA Service is down") - } - - for { - endpoint <- toUri(statusUrl(cm.commit.sha)) - req <- postRequest(endpoint).withBody(stat.asJson) - res <- httpClient.expect(req)(jsonOf[StatusResponse]) - } yield res + def setStatus(cm: CommitStatus, httpClient: Client): Task[StatusResponse] = { + val desc = + if (cm.isValid) "User signed CLA" + else "User needs to sign cla: https://www.lightbend.com/contribute/cla/scala" + + val stat = cm match { + case Valid(Some(user), commit) => + Status("success", claUrl(user), desc) + case Valid(None, commit) => + Status("success", "", "All contributors signed CLA") + case Invalid(user, commit) => + Status("failure", claUrl(user), desc) + case MissingUser(commit) => + Status("failure", "", "Missing valid github user for this PR") + case CLAServiceDown(user, commit) => + Status("pending", claUrl(user), "CLA Service is down") + case InvalidPrevious(users, latestCommit) => + Status("failure", "", users.map("@" + _).mkString(", ") + " have not signed the CLA") } - Task.gatherUnordered(xs.map(setStatus)) + for { + endpoint <- toUri(statusUrl(cm.commit.sha)) + req <- postRequest(endpoint).withBody(stat.asJson) + res <- httpClient.expect(req)(jsonOf[StatusResponse]) + } yield res } + def sendStatuses(xs: List[CommitStatus], httpClient: Client): Task[List[StatusResponse]] = + Task.gatherUnordered(xs.map(setStatus(_, httpClient))) + private[this] val ExtractLink = """<([^>]+)>; rel="([^"]+)"""".r def findNext(header: Option[Header]): Option[String] = header.flatMap { header => val value = header.value @@ -157,6 +161,7 @@ trait PullRequestService { .headOption } + /** Ordered from earliest to latest */ def getCommits(issueNbr: Int, httpClient: Client): Task[List[Commit]] = { def makeRequest(url: String): Task[List[Commit]] = for { @@ -180,20 +185,80 @@ trait PullRequestService { res <- httpClient.expect(req)(jsonOf[List[Comment]]) } yield res - def checkPullRequest(issue: Issue): Task[Response] = { - val httpClient = PooledHttp1Client() + private def usersFromInvalid(xs: List[CommitStatus]) = + xs.collect { case Invalid(user, _) => user } + + def sendInitialComment(invalidUsers: List[String]): Task[Unit] = ??? + + def checkFreshPR(issue: Issue): Task[Response] = { + + val httpClient = PooledHttp1Client() for { - // First get all the commits from the PR commits <- getCommits(issue.number, httpClient) - - // Then check the CLA of each commit for both author and committer statuses <- checkCLA(commits, httpClient) - // Send statuses to Github and exit - _ <- sendStatuses(statuses, httpClient) - _ <- shutdownClient(httpClient).pure[Task] - resp <- Ok("All statuses checked") + (validStatuses, invalidStatuses) = statuses.partition(_.isValid) + invalidUsers = usersFromInvalid(invalidStatuses) + + // Mark the invalid statuses: + _ <- sendStatuses(invalidStatuses, httpClient) + + // Set status of last to indicate previous failures or all good: + _ <- { + if (invalidStatuses.nonEmpty) + setStatus(InvalidPrevious(invalidUsers, commits.last), httpClient) + else + setStatus(statuses.last, httpClient) + } + + // Send positive comment: + _ <- sendInitialComment(invalidUsers) + _ <- shutdownClient(httpClient) + resp <- Ok("Fresh PR checked") + } yield resp + + } + + // TODO: Could this be done with `issueNbr` instead? + def getStatuses(commits: List[Commit], client: Client): Task[List[StatusResponse]] = + ??? + + private def extractCommit(status: StatusResponse): Task[Commit] = + ??? + + def recheckCLA(statuses: List[StatusResponse], client: Client): Task[List[CommitStatus]] = + for { + commits <- Task.gatherUnordered(statuses.map(extractCommit)) + statuses <- checkCLA(commits, client) + } yield statuses + + def checkSynchronize(issue: Issue): Task[Response] = { + val httpClient = PooledHttp1Client() + + for { + commits <- getCommits(issue.number, httpClient) + statuses <- getStatuses(commits, httpClient) + stillInvalid <- recheckCLA(statuses, httpClient) + + // Set final commit status based on `stillInvalid`: + _ <- { + if (stillInvalid.nonEmpty) + setStatus(InvalidPrevious(usersFromInvalid(stillInvalid), commits.last), httpClient) + else { + val lastCommit = commits.last + setStatus(Valid(lastCommit.author.login, lastCommit), httpClient) + } + } + _ <- shutdownClient(httpClient) + resp <- Ok("Updated PR checked") } yield resp } + + def checkPullRequest(issue: Issue): Task[Response] = + issue.action match { + case "opened" => checkFreshPR(issue) + case "synchronize" => checkSynchronize(issue) + case action => BadRequest(s"Unhandled action: $action") + } } diff --git a/bot/src/dotty/tools/bot/model/Github.scala b/bot/src/dotty/tools/bot/model/Github.scala index e3814deb47a1..8a825c6ff31f 100644 --- a/bot/src/dotty/tools/bot/model/Github.scala +++ b/bot/src/dotty/tools/bot/model/Github.scala @@ -9,6 +9,7 @@ object Github { ) case class Issue( + action: String, // "opened", "reopened", "closed", "synchronize" number: Int, pull_request: Option[PullRequest] ) @@ -35,6 +36,7 @@ object Github { context: String = "CLA" ) + // TODO: Can we get a `Commit` from `StatusResponse`? case class StatusResponse( url: String, id: Long, From 6a633efd80c5615aaae4d3d091fd143b56a0fc11 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 24 Apr 2017 11:52:29 +0200 Subject: [PATCH 3/9] Implement rechecking of CLA --- .../dotty/tools/bot/PullRequestService.scala | 57 ++++++++++++------- bot/src/dotty/tools/bot/model/Github.scala | 4 +- bot/test/PRServiceTests.scala | 27 +++++++++ 3 files changed, 66 insertions(+), 22 deletions(-) diff --git a/bot/src/dotty/tools/bot/PullRequestService.scala b/bot/src/dotty/tools/bot/PullRequestService.scala index 4fa7d1d21881..491adb7f211e 100644 --- a/bot/src/dotty/tools/bot/PullRequestService.scala +++ b/bot/src/dotty/tools/bot/PullRequestService.scala @@ -54,17 +54,19 @@ trait PullRequestService { currentVersion: String ) + private[this] val githubUrl = "https://api.github.com" + def claUrl(userName: String): String = s"https://www.lightbend.com/contribute/cla/scala/check/$userName" def commitsUrl(prNumber: Int): String = - s"https://api.github.com/repos/lampepfl/dotty/pulls/$prNumber/commits?per_page=100" + s"$githubUrl/repos/lampepfl/dotty/pulls/$prNumber/commits?per_page=100" def statusUrl(sha: String): String = - s"https://api.github.com/repos/lampepfl/dotty/statuses/$sha" + s"$githubUrl/repos/lampepfl/dotty/statuses/$sha" def issueCommentsUrl(issueNbr: Int): String = - s"https://api.github.com/repos/lampepfl/dotty/issues/$issueNbr/comments" + s"$githubUrl/repos/lampepfl/dotty/issues/$issueNbr/comments" def toUri(url: String): Task[Uri] = Uri.fromString(url).fold(Task.fail, Task.now) @@ -201,7 +203,7 @@ trait PullRequestService { (validStatuses, invalidStatuses) = statuses.partition(_.isValid) invalidUsers = usersFromInvalid(invalidStatuses) - // Mark the invalid statuses: + // Mark the invalid commits: _ <- sendStatuses(invalidStatuses, httpClient) // Set status of last to indicate previous failures or all good: @@ -220,35 +222,48 @@ trait PullRequestService { } - // TODO: Could this be done with `issueNbr` instead? + def getStatus(commit: Commit, client: Client): Task[StatusResponse] = + for { + endpoint <- toUri(statusUrl(commit.sha)) + req <- getRequest(endpoint) + res <- client.expect(req)(jsonOf[List[StatusResponse]]) + } yield res.head + def getStatuses(commits: List[Commit], client: Client): Task[List[StatusResponse]] = - ??? + Task.gatherUnordered(commits.map(getStatus(_, client))) - private def extractCommit(status: StatusResponse): Task[Commit] = - ??? + private def extractCommitSha(status: StatusResponse): Task[String] = + Task.delay(status.sha) + + def recheckCLA(statuses: List[StatusResponse], commits: List[Commit], client: Client): Task[List[CommitStatus]] = { + /** Return the matching commits from the SHAs */ + def prunedCommits(shas: List[String]): Task[List[Commit]] = + Task.delay(commits.filter(cm => shas.contains(cm.sha))) - def recheckCLA(statuses: List[StatusResponse], client: Client): Task[List[CommitStatus]] = for { - commits <- Task.gatherUnordered(statuses.map(extractCommit)) - statuses <- checkCLA(commits, client) + commitShas <- Task.gatherUnordered(statuses.map(extractCommitSha)) + commits <- prunedCommits(commitShas) + statuses <- checkCLA(commits, client) } yield statuses + } def checkSynchronize(issue: Issue): Task[Response] = { val httpClient = PooledHttp1Client() for { - commits <- getCommits(issue.number, httpClient) - statuses <- getStatuses(commits, httpClient) - stillInvalid <- recheckCLA(statuses, httpClient) + commits <- getCommits(issue.number, httpClient) + statuses <- checkCLA(commits, httpClient) + + (_, invalid) = statuses.partition(_.isValid) + + _ <- sendStatuses(invalid, httpClient) - // Set final commit status based on `stillInvalid`: + // Set final commit status based on `invalid`: _ <- { - if (stillInvalid.nonEmpty) - setStatus(InvalidPrevious(usersFromInvalid(stillInvalid), commits.last), httpClient) - else { - val lastCommit = commits.last - setStatus(Valid(lastCommit.author.login, lastCommit), httpClient) - } + if (invalid.nonEmpty) + setStatus(InvalidPrevious(usersFromInvalid(invalid), commits.last), httpClient) + else + setStatus(statuses.last, httpClient) } _ <- shutdownClient(httpClient) resp <- Ok("Updated PR checked") diff --git a/bot/src/dotty/tools/bot/model/Github.scala b/bot/src/dotty/tools/bot/model/Github.scala index 8a825c6ff31f..9ff834d04483 100644 --- a/bot/src/dotty/tools/bot/model/Github.scala +++ b/bot/src/dotty/tools/bot/model/Github.scala @@ -41,7 +41,9 @@ object Github { url: String, id: Long, state: String - ) + ) { + def sha: String = url.split('/').last + } case class Comment(user: Author) } diff --git a/bot/test/PRServiceTests.scala b/bot/test/PRServiceTests.scala index 2858b42c73fe..b6682f756e1e 100644 --- a/bot/test/PRServiceTests.scala +++ b/bot/test/PRServiceTests.scala @@ -103,4 +103,31 @@ class PRServiceTests extends PullRequestService { httpClient.shutdownNow() } + + @Test def canGetStatus = { + val sha = "fa64b4b613fe5e78a5b4185b4aeda89e2f1446ff" + val commit = Commit(sha, Author(None), Author(None), CommitInfo("")) + val status = withClient(getStatus(commit, _)) + + assert(status.sha == sha, "someting wong") + } + + @Test def canRecheckCLA = { + val shas = + "1d62587cb3f41dafd796b0c92ec1c22d95b879f9" :: + "ad60a386f488a16612c093576bf7bf4d9f0073bf" :: + Nil + + val commits = shas.map { sha => + Commit(sha, Author(Some("felixmulder")), Author(Some("felixmulder")), CommitInfo("")) + } + + val statuses = shas.map { sha => + StatusResponse("https://api.github.com/repos/lampepfl/dotty/statuses/" + sha, 0, "failure") + } + + val rechecked = withClient(recheckCLA(statuses, commits, _)) + + assert(rechecked.forall(cs => cs.isValid), s"Should have set all statuses to valid, but got: $rechecked") + } } From 426390764d88fabdd2044600febfb8988dd2b7a5 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 24 Apr 2017 11:52:37 +0200 Subject: [PATCH 4/9] Use `withClient` helper in all tests --- bot/test/PRServiceTests.scala | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/bot/test/PRServiceTests.scala b/bot/test/PRServiceTests.scala index b6682f756e1e..89acfe9e5900 100644 --- a/bot/test/PRServiceTests.scala +++ b/bot/test/PRServiceTests.scala @@ -40,10 +40,8 @@ class PRServiceTests extends PullRequestService { } @Test def canGetAllCommitsFromPR = { - val httpClient = PooledHttp1Client() val issueNbr = 1941 // has 2 commits: https://github.com/lampepfl/dotty/pull/1941/commits - - val List(c1, c2) = getCommits(issueNbr, httpClient).run + val List(c1, c2) = withClient(getCommits(issueNbr, _)) assertEquals( "Represent untyped operators as Ident instead of Name", @@ -57,10 +55,8 @@ class PRServiceTests extends PullRequestService { } @Test def canGetMoreThan100Commits = { - val httpClient = PooledHttp1Client() val issueNbr = 1840 // has >100 commits: https://github.com/lampepfl/dotty/pull/1840/commits - - val numberOfCommits = getCommits(issueNbr, httpClient).run.length + val numberOfCommits = withClient(getCommits(issueNbr, _)).length assert( numberOfCommits > 100, @@ -69,27 +65,24 @@ class PRServiceTests extends PullRequestService { } @Test def canGetComments = { - val comments: List[Comment] = withClient(c => getComments(2136, c)) + val comments: List[Comment] = withClient(getComments(2136, _)) assert(comments.find(_.user.login == Some("odersky")).isDefined, "Could not find Martin's comment on PR 2136") } @Test def canCheckCLA = { - val httpClient = PooledHttp1Client() val validUserCommit = Commit("sha-here", Author(Some("felixmulder")), Author(Some("felixmulder")), CommitInfo("")) - val statuses: List[CommitStatus] = checkCLA(validUserCommit :: Nil, httpClient).run + val statuses: List[CommitStatus] = withClient(checkCLA(validUserCommit :: Nil, _)) assert(statuses.length == 1, s"wrong number of valid statuses: got ${statuses.length}, expected 1") - httpClient.shutdownNow() } @Test def canSetStatus = { - val httpClient = PooledHttp1Client() val sha = "fa64b4b613fe5e78a5b4185b4aeda89e2f1446ff" val status = Invalid("smarter", Commit(sha, Author(Some("smarter")), Author(Some("smarter")), CommitInfo(""))) - val statuses: List[StatusResponse] = sendStatuses(status :: Nil, httpClient).run + val statuses: List[StatusResponse] = withClient(sendStatuses(status :: Nil, _)) assert( statuses.length == 1, @@ -100,8 +93,6 @@ class PRServiceTests extends PullRequestService { statuses.head.state == "failure", s"status set had wrong state, expected 'failure', got: ${statuses.head.state}" ) - - httpClient.shutdownNow() } @Test def canGetStatus = { From 8494fc5460940edd31fdd651d8e1aeaa332b9044 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Mon, 24 Apr 2017 15:51:14 +0200 Subject: [PATCH 5/9] Implement posting initial comment --- .../dotty/tools/bot/PullRequestService.scala | 98 +++++++++++++++++-- bot/src/dotty/tools/bot/model/Github.scala | 30 +++--- bot/test/PRServiceTests.scala | 13 +++ 3 files changed, 120 insertions(+), 21 deletions(-) diff --git a/bot/src/dotty/tools/bot/PullRequestService.scala b/bot/src/dotty/tools/bot/PullRequestService.scala index 491adb7f211e..b184aba857cc 100644 --- a/bot/src/dotty/tools/bot/PullRequestService.scala +++ b/bot/src/dotty/tools/bot/PullRequestService.scala @@ -3,7 +3,7 @@ package dotty.tools.bot import org.http4s.{ Status => _, _ } import org.http4s.client.blaze._ import org.http4s.client.Client -import org.http4s.headers.Authorization +import org.http4s.headers.{ Accept, Authorization } import cats.syntax.applicative._ import scalaz.concurrent.Task @@ -47,6 +47,10 @@ trait PullRequestService { new Authorization(creds) } + private[this] lazy val previewAcceptHeader = + Accept.parse("application/vnd.github.black-cat-preview+json") + .getOrElse(throw new Exception("Couldn't initialize accept header")) + private final case class CLASignature( user: String, signed: Boolean, @@ -68,14 +72,20 @@ trait PullRequestService { def issueCommentsUrl(issueNbr: Int): String = s"$githubUrl/repos/lampepfl/dotty/issues/$issueNbr/comments" + def reviewUrl(issueNbr: Int): String = + s"$githubUrl/repos/lampepfl/dotty/pulls/$issueNbr/reviews" + def toUri(url: String): Task[Uri] = Uri.fromString(url).fold(Task.fail, Task.now) def getRequest(endpoint: Uri): Task[Request] = - Request(uri = endpoint, method = Method.GET).putHeaders(authHeader).pure[Task] + Request(uri = endpoint, method = Method.GET).putHeaders(authHeader) + .pure[Task] def postRequest(endpoint: Uri): Task[Request] = - Request(uri = endpoint, method = Method.POST).putHeaders(authHeader).pure[Task] + Request(uri = endpoint, method = Method.POST) + .putHeaders(authHeader, previewAcceptHeader) + .pure[Task] def shutdownClient(client: Client): Task[Unit] = client.shutdownNow().pure[Task] @@ -191,11 +201,87 @@ trait PullRequestService { private def usersFromInvalid(xs: List[CommitStatus]) = xs.collect { case Invalid(user, _) => user } - def sendInitialComment(invalidUsers: List[String]): Task[Unit] = ??? + def hasBadCommitMessages(commits: List[Commit]): Boolean = + commits.exists { cm => + val firstLine = cm.commit.message.takeWhile(_ != '\n').trim.toLowerCase + val firstWord = firstLine.takeWhile(x => x != ':' && x != ' ') + val containsColon = firstLine.contains(':') - def checkFreshPR(issue: Issue): Task[Response] = { + val wrongTense = firstWord match { + case "added" | "fixed" | "removed" | "moved" | "changed" | + "finalized" | "re-added" + => true + + case "adds" | "fixes" | "removes" | "moves" | "changes" | + "finalizes" | "re-adds" + => true + + case _ + => false + } + + wrongTense || firstLine.last == '.' || firstLine.length > 100 + } + def sendInitialComment(issueNbr: Int, invalidUsers: List[String], commits: List[Commit], client: Client): Task[ReviewResponse] = { + + val cla = if (invalidUsers.nonEmpty) { + s"""|## CLA ## + |In order for us to be able to accept your contribution, all users + |must sign the Scala CLA. + | + |Users: + |${ invalidUsers.map("@" + _).mkString("- ", "\n- ", "") } + | + |Could you folks please sign the CLA? :pray: + | + |Please do this here: https://www.lightbend.com/contribute/cla/scala + |""".stripMargin + } else "All contributors have signed the CLA, thank you! :heart:" + + val commitRules = if (hasBadCommitMessages(commits)) { + """|## Commit Messages ## + |We want to keep history, but for that to actually be useful we have + |some rules on how to format our commit messages ([relevant xkcd](https://xkcd.com/1296/)). + | + |Please stick to these guidelines for commit messages: + | + |> 1. Separate subject from body with a blank line + |> 1. When fixing an issue, start your commit message with `Fix #: ` + |> 1. Limit the subject line to 80 characters + |> 1. Capitalize the subject line + |> 1. Do not end the subject line with a period + |> 1. Use the imperative mood in the subject line ("Added" instead of "Add") + |> 1. Wrap the body at 80 characters + |> 1. Use the body to explain what and why vs. how + |> + |> adapted from https://chris.beams.io/posts/git-commit""".stripMargin + } else "" + + val body = + s"""|Hello, and thank you for opening this PR! :tada: + | + |If you haven't already, please request a review from one of our + |collaborators (have no fear, we don't bite)! + | + |$cla + | + |$commitRules + | + |Have an awesome day! :sunny:""".stripMargin + + val review = Review.comment(body) + + for { + endpoint <- toUri(reviewUrl(issueNbr)) + req <- postRequest(endpoint).withBody(review.asJson) + res <- client.expect(req)(jsonOf[ReviewResponse]) + } yield res + } + + def checkFreshPR(issue: Issue): Task[Response] = { val httpClient = PooledHttp1Client() + for { commits <- getCommits(issue.number, httpClient) statuses <- checkCLA(commits, httpClient) @@ -215,7 +301,7 @@ trait PullRequestService { } // Send positive comment: - _ <- sendInitialComment(invalidUsers) + _ <- sendInitialComment(issue.number, invalidUsers, commits, httpClient) _ <- shutdownClient(httpClient) resp <- Ok("Fresh PR checked") } yield resp diff --git a/bot/src/dotty/tools/bot/model/Github.scala b/bot/src/dotty/tools/bot/model/Github.scala index 9ff834d04483..80bca3ad1042 100644 --- a/bot/src/dotty/tools/bot/model/Github.scala +++ b/bot/src/dotty/tools/bot/model/Github.scala @@ -2,21 +2,15 @@ package dotty.tools.bot package model object Github { - case class PullRequest( - url: String, - id: Long, - commits_url: String - ) - case class Issue( action: String, // "opened", "reopened", "closed", "synchronize" number: Int, pull_request: Option[PullRequest] ) - case class CommitInfo( - message: String - ) + case class PullRequest(url: String, id: Long, commits_url: String) + + case class CommitInfo(message: String) case class Commit( sha: String, @@ -36,14 +30,20 @@ object Github { context: String = "CLA" ) - // TODO: Can we get a `Commit` from `StatusResponse`? - case class StatusResponse( - url: String, - id: Long, - state: String - ) { + case class StatusResponse(url: String, id: Long, state: String) { def sha: String = url.split('/').last } case class Comment(user: Author) + + /** A PR review */ + case class Review (body: String, event: String) + + object Review { + def approve(body: String) = Review(body, "APPROVE") + def requestChanges(body: String) = Review(body, "REQUEST_CHANGES") + def comment(body: String) = Review(body, "COMMENT") + } + + case class ReviewResponse(body: String, state: String, id: Long) } diff --git a/bot/test/PRServiceTests.scala b/bot/test/PRServiceTests.scala index 89acfe9e5900..860438697d89 100644 --- a/bot/test/PRServiceTests.scala +++ b/bot/test/PRServiceTests.scala @@ -121,4 +121,17 @@ class PRServiceTests extends PullRequestService { assert(rechecked.forall(cs => cs.isValid), s"Should have set all statuses to valid, but got: $rechecked") } + + @Test def canPostReview = { + val invalidUsers = "felixmulder" :: "smarter" :: Nil + val commit = Commit("", Author(Some("smarter")), Author(Some("smarter")), CommitInfo("Added stuff")) + val res = withClient(sendInitialComment(2281, invalidUsers, commit :: Nil, _)) + + assert( + res.body.contains("We want to keep history") && + res.body.contains("Could you folks please sign the CLA?") && + res.body.contains("Have an awesome day!"), + s"Body of review was not as expected:\n${res.body}" + ) + } } From 01da59f12a0a962967ffd42d24430cb6144f8722 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Tue, 25 Apr 2017 09:56:30 +0200 Subject: [PATCH 6/9] Factor out HTTP client functionality --- bot/src/dotty/tools/bot/Main.scala | 7 +- .../dotty/tools/bot/PullRequestService.scala | 96 ++++--------------- .../dotty/tools/bot/util/HttpClientAux.scala | 35 +++++++ .../tools/bot/util/TaskIsApplicative.scala | 13 +++ bot/test/PRServiceTests.scala | 26 +---- 5 files changed, 77 insertions(+), 100 deletions(-) create mode 100644 bot/src/dotty/tools/bot/util/HttpClientAux.scala create mode 100644 bot/src/dotty/tools/bot/util/TaskIsApplicative.scala diff --git a/bot/src/dotty/tools/bot/Main.scala b/bot/src/dotty/tools/bot/Main.scala index 00b4a2735f94..8edd1dcc9cab 100644 --- a/bot/src/dotty/tools/bot/Main.scala +++ b/bot/src/dotty/tools/bot/Main.scala @@ -7,9 +7,10 @@ import scalaz.concurrent.Task object Main extends ServerApp with PullRequestService { - val user = sys.env("USER") - val token = sys.env("TOKEN") - val port = sys.env("PORT").toInt + val githubUser = sys.env("GITHUB_USER") + val githubToken = sys.env("GITHUB_TOKEN") + val droneToken = sys.env("DRONE_TOKEN") + val port = sys.env("PORT").toInt /** Services mounted to the server */ final val services = prService diff --git a/bot/src/dotty/tools/bot/PullRequestService.scala b/bot/src/dotty/tools/bot/PullRequestService.scala index b184aba857cc..3d3861a757d9 100644 --- a/bot/src/dotty/tools/bot/PullRequestService.scala +++ b/bot/src/dotty/tools/bot/PullRequestService.scala @@ -1,4 +1,5 @@ -package dotty.tools.bot +package dotty.tools +package bot import org.http4s.{ Status => _, _ } import org.http4s.client.blaze._ @@ -18,23 +19,19 @@ import org.http4s.dsl._ import org.http4s.util._ import model.Github._ - -object TaskIsApplicative { - implicit val taskIsApplicative = new cats.Applicative[Task] { - def pure[A](x: A): Task[A] = Task.now(x) - def ap[A, B](ff: Task[A => B])(fa: Task[A]): Task[B] = - for(f <- ff; a <- fa) yield f(a) - } -} -import TaskIsApplicative._ +import bot.util.TaskIsApplicative._ +import bot.util.HttpClientAux._ trait PullRequestService { /** Username for authorized admin */ - def user: String + def githubUser: String /** OAuth token needed for user to create statuses */ - def token: String + def githubToken: String + + /** OAuth token for drone, needed to cancel builds */ + def droneToken: String /** Pull Request HTTP service */ val prService = HttpService { @@ -42,15 +39,6 @@ trait PullRequestService { request.as(jsonOf[Issue]).flatMap(checkPullRequest) } - private[this] lazy val authHeader = { - val creds = BasicCredentials(user, token) - new Authorization(creds) - } - - private[this] lazy val previewAcceptHeader = - Accept.parse("application/vnd.github.black-cat-preview+json") - .getOrElse(throw new Exception("Couldn't initialize accept header")) - private final case class CLASignature( user: String, signed: Boolean, @@ -75,18 +63,6 @@ trait PullRequestService { def reviewUrl(issueNbr: Int): String = s"$githubUrl/repos/lampepfl/dotty/pulls/$issueNbr/reviews" - def toUri(url: String): Task[Uri] = - Uri.fromString(url).fold(Task.fail, Task.now) - - def getRequest(endpoint: Uri): Task[Request] = - Request(uri = endpoint, method = Method.GET).putHeaders(authHeader) - .pure[Task] - - def postRequest(endpoint: Uri): Task[Request] = - Request(uri = endpoint, method = Method.POST) - .putHeaders(authHeader, previewAcceptHeader) - .pure[Task] - def shutdownClient(client: Client): Task[Unit] = client.shutdownNow().pure[Task] @@ -104,9 +80,7 @@ trait PullRequestService { def checkCLA(xs: List[Commit], httpClient: Client): Task[List[CommitStatus]] = { def checkUser(user: String): Task[Commit => CommitStatus] = { val claStatus = for { - endpoint <- toUri(claUrl(user)) - claReq <- getRequest(endpoint) - claRes <- httpClient.expect(claReq)(jsonOf[CLASignature]) + claRes <- httpClient.expect(get(claUrl(user)))(jsonOf[CLASignature]) } yield { (commit: Commit) => if (claRes.signed) Valid(Some(user), commit) else Invalid(user, commit) @@ -151,9 +125,8 @@ trait PullRequestService { } for { - endpoint <- toUri(statusUrl(cm.commit.sha)) - req <- postRequest(endpoint).withBody(stat.asJson) - res <- httpClient.expect(req)(jsonOf[StatusResponse]) + req <- post(statusUrl(cm.commit.sha)).withAuth(githubUser, githubToken) + res <- httpClient.expect(req.withBody(stat.asJson))(jsonOf[StatusResponse]) } yield res } @@ -177,9 +150,7 @@ trait PullRequestService { def getCommits(issueNbr: Int, httpClient: Client): Task[List[Commit]] = { def makeRequest(url: String): Task[List[Commit]] = for { - endpoint <- toUri(url) - req <- getRequest(endpoint) - res <- httpClient.fetch(req){ res => + res <- httpClient.fetch(get(url)) { res => val link = CaseInsensitiveString("Link") val next = findNext(res.headers.get(link)).map(makeRequest).getOrElse(Task.now(Nil)) @@ -191,12 +162,7 @@ trait PullRequestService { } def getComments(issueNbr: Int, httpClient: Client): Task[List[Comment]] = - for { - endpoint <- toUri(issueCommentsUrl(issueNbr)) - req <- getRequest(endpoint) - res <- httpClient.expect(req)(jsonOf[List[Comment]]) - } yield res - + httpClient.expect(get(issueCommentsUrl(issueNbr)))(jsonOf[List[Comment]]) private def usersFromInvalid(xs: List[CommitStatus]) = xs.collect { case Invalid(user, _) => user } @@ -273,9 +239,8 @@ trait PullRequestService { val review = Review.comment(body) for { - endpoint <- toUri(reviewUrl(issueNbr)) - req <- postRequest(endpoint).withBody(review.asJson) - res <- client.expect(req)(jsonOf[ReviewResponse]) + req <- post(reviewUrl(issueNbr)).withAuth(githubUser, githubToken) + res <- client.expect(req.withBody(review.asJson))(jsonOf[ReviewResponse]) } yield res } @@ -308,30 +273,12 @@ trait PullRequestService { } - def getStatus(commit: Commit, client: Client): Task[StatusResponse] = - for { - endpoint <- toUri(statusUrl(commit.sha)) - req <- getRequest(endpoint) - res <- client.expect(req)(jsonOf[List[StatusResponse]]) - } yield res.head - - def getStatuses(commits: List[Commit], client: Client): Task[List[StatusResponse]] = - Task.gatherUnordered(commits.map(getStatus(_, client))) + def getStatus(commit: Commit, client: Client): Task[List[StatusResponse]] = + client.expect(get(statusUrl(commit.sha)))(jsonOf[List[StatusResponse]]) private def extractCommitSha(status: StatusResponse): Task[String] = Task.delay(status.sha) - def recheckCLA(statuses: List[StatusResponse], commits: List[Commit], client: Client): Task[List[CommitStatus]] = { - /** Return the matching commits from the SHAs */ - def prunedCommits(shas: List[String]): Task[List[Commit]] = - Task.delay(commits.filter(cm => shas.contains(cm.sha))) - - for { - commitShas <- Task.gatherUnordered(statuses.map(extractCommitSha)) - commits <- prunedCommits(commitShas) - statuses <- checkCLA(commits, client) - } yield statuses - } def checkSynchronize(issue: Issue): Task[Response] = { val httpClient = PooledHttp1Client() @@ -339,10 +286,9 @@ trait PullRequestService { for { commits <- getCommits(issue.number, httpClient) statuses <- checkCLA(commits, httpClient) - - (_, invalid) = statuses.partition(_.isValid) - - _ <- sendStatuses(invalid, httpClient) + invalid = statuses.filterNot(_.isValid) + _ <- sendStatuses(invalid, httpClient) + _ <- cancelBuilds(commits.dropRight(1), httpClient) // Set final commit status based on `invalid`: _ <- { diff --git a/bot/src/dotty/tools/bot/util/HttpClientAux.scala b/bot/src/dotty/tools/bot/util/HttpClientAux.scala new file mode 100644 index 000000000000..d5fbe363dc14 --- /dev/null +++ b/bot/src/dotty/tools/bot/util/HttpClientAux.scala @@ -0,0 +1,35 @@ +package dotty.tools.bot.util + +import org.http4s._ +import scalaz.concurrent.Task +import org.http4s.headers.{ Accept, Authorization } + +object HttpClientAux { + def uriFromString(url: String): Task[Uri] = + Uri.fromString(url).fold(Task.fail, Task.now) + + implicit class RicherTask(val task: Task[Request]) extends AnyVal { + def withOauth2(token: String): Task[Request] = + task.map(_.putHeaders(new Authorization(new OAuth2BearerToken(token)))) + + def withAuth(user: String, pass: String): Task[Request] = + task.map(_.putHeaders(new Authorization(BasicCredentials(user, pass)))) + } + + private[this] lazy val previewAcceptHeader = + Accept.parse("application/vnd.github.black-cat-preview+json") + .getOrElse(throw new Exception("Couldn't initialize accept header")) + + @inline private def request(method: Method, endpoint: Uri): Request = + Request(uri = endpoint, method = method) + .putHeaders(previewAcceptHeader) + + def get(endpoint: String): Task[Request] = + uriFromString(endpoint).map(request(Method.GET, _)) + + def post(endpoint: String): Task[Request] = + uriFromString(endpoint).map(request(Method.POST, _)) + + def delete(endpoint: String): Task[Request] = + uriFromString(endpoint).map(request(Method.DELETE, _)) +} diff --git a/bot/src/dotty/tools/bot/util/TaskIsApplicative.scala b/bot/src/dotty/tools/bot/util/TaskIsApplicative.scala new file mode 100644 index 000000000000..c97b1ac5f456 --- /dev/null +++ b/bot/src/dotty/tools/bot/util/TaskIsApplicative.scala @@ -0,0 +1,13 @@ +package dotty.tools.bot +package util + +import cats.syntax.applicative._ +import scalaz.concurrent.Task + +object TaskIsApplicative { + implicit val taskIsApplicative = new cats.Applicative[Task] { + def pure[A](x: A): Task[A] = Task.now(x) + def ap[A, B](ff: Task[A => B])(fa: Task[A]): Task[B] = + for(f <- ff; a <- fa) yield f(a) + } +} diff --git a/bot/test/PRServiceTests.scala b/bot/test/PRServiceTests.scala index 860438697d89..4d827d6b86a8 100644 --- a/bot/test/PRServiceTests.scala +++ b/bot/test/PRServiceTests.scala @@ -14,8 +14,9 @@ import org.http4s.client.Client import scalaz.concurrent.Task class PRServiceTests extends PullRequestService { - val user = sys.env("USER") - val token = sys.env("TOKEN") + val githubUser = sys.env("GITHUB_USER") + val githubToken = sys.env("GITHUB_TOKEN") + val droneToken = sys.env("DRONE_TOKEN") private def withClient[A](f: Client => Task[A]): A = { val httpClient = PooledHttp1Client() @@ -98,30 +99,11 @@ class PRServiceTests extends PullRequestService { @Test def canGetStatus = { val sha = "fa64b4b613fe5e78a5b4185b4aeda89e2f1446ff" val commit = Commit(sha, Author(None), Author(None), CommitInfo("")) - val status = withClient(getStatus(commit, _)) + val status = withClient(getStatus(commit, _)).head assert(status.sha == sha, "someting wong") } - @Test def canRecheckCLA = { - val shas = - "1d62587cb3f41dafd796b0c92ec1c22d95b879f9" :: - "ad60a386f488a16612c093576bf7bf4d9f0073bf" :: - Nil - - val commits = shas.map { sha => - Commit(sha, Author(Some("felixmulder")), Author(Some("felixmulder")), CommitInfo("")) - } - - val statuses = shas.map { sha => - StatusResponse("https://api.github.com/repos/lampepfl/dotty/statuses/" + sha, 0, "failure") - } - - val rechecked = withClient(recheckCLA(statuses, commits, _)) - - assert(rechecked.forall(cs => cs.isValid), s"Should have set all statuses to valid, but got: $rechecked") - } - @Test def canPostReview = { val invalidUsers = "felixmulder" :: "smarter" :: Nil val commit = Commit("", Author(Some("smarter")), Author(Some("smarter")), CommitInfo("Added stuff")) From 51f8bc1af0014e957c1dc815d6c25cdbb5898521 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Tue, 25 Apr 2017 11:07:27 +0200 Subject: [PATCH 7/9] Add ability for bot to stop outdated jobs --- .../dotty/tools/bot/PullRequestService.scala | 16 +++++- bot/src/dotty/tools/bot/model/Drone.scala | 49 +++++++++++++++++++ bot/src/dotty/tools/bot/model/Github.scala | 8 ++- bot/test/PRServiceTests.scala | 8 +++ 4 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 bot/src/dotty/tools/bot/model/Drone.scala diff --git a/bot/src/dotty/tools/bot/PullRequestService.scala b/bot/src/dotty/tools/bot/PullRequestService.scala index 3d3861a757d9..fb13b1c45bef 100644 --- a/bot/src/dotty/tools/bot/PullRequestService.scala +++ b/bot/src/dotty/tools/bot/PullRequestService.scala @@ -19,6 +19,7 @@ import org.http4s.dsl._ import org.http4s.util._ import model.Github._ +import model.Drone import bot.util.TaskIsApplicative._ import bot.util.HttpClientAux._ @@ -279,6 +280,19 @@ trait PullRequestService { private def extractCommitSha(status: StatusResponse): Task[String] = Task.delay(status.sha) + def cancelBuilds(commits: List[Commit])(implicit client: Client): Task[Boolean] = + Task.gatherUnordered { + val droneContext = "continuous-integration/drone/pr" + commits.map { commit => + for { + statuses <- getStatus(commit, client) + cancellable = statuses.filter(status => status.state == "pending" && status.context == droneContext) + runningJobs = cancellable.map(_.target_url.split('/').last.toInt) + cancelled <- Task.gatherUnordered(runningJobs.map(Drone.stopBuild(_, droneToken))) + } yield cancelled.foldLeft(true)(_ == _) + } + } + .map(xs => xs.foldLeft(true)(_ == _)) def checkSynchronize(issue: Issue): Task[Response] = { val httpClient = PooledHttp1Client() @@ -288,7 +302,7 @@ trait PullRequestService { statuses <- checkCLA(commits, httpClient) invalid = statuses.filterNot(_.isValid) _ <- sendStatuses(invalid, httpClient) - _ <- cancelBuilds(commits.dropRight(1), httpClient) + _ <- cancelBuilds(commits.dropRight(1))(httpClient) // Set final commit status based on `invalid`: _ <- { diff --git a/bot/src/dotty/tools/bot/model/Drone.scala b/bot/src/dotty/tools/bot/model/Drone.scala new file mode 100644 index 000000000000..710f18554333 --- /dev/null +++ b/bot/src/dotty/tools/bot/model/Drone.scala @@ -0,0 +1,49 @@ +package dotty.tools +package bot +package model + +import io.circe._ +import io.circe.generic.auto._ +import io.circe.syntax._ + +import org.http4s._ +import org.http4s.circe._ +import org.http4s.client.Client + +import scalaz.concurrent.Task +import bot.util.HttpClientAux + +object Drone { + import HttpClientAux._ + + case class Build( + number: Int, + event: String, + status: String, + commit: String, + author: String + ) + + private[this] val baseUrl = "http://dotty-ci.epfl.ch/api" + + private def job(id: Int) = + s"$baseUrl/repos/lampepfl/dotty/builds/$id" + + private def job(id: Int, subId: Int) = + s"$baseUrl/repos/lampepfl/dotty/builds/$id/$subId" + + def stopBuild(id: Int, token: String)(implicit client: Client): Task[Boolean] = { + def resToBoolean(res: Response): Task[Boolean] = Task.now { + res.status.code >= 200 && res.status.code < 400 + } + + val responses = List(1, 2, 3, 4).map { subId => + client.fetch(delete(job(id, subId)).withOauth2(token))(resToBoolean) + } + + Task.gatherUnordered(responses).map(xs => xs.exists(_ == true)) + } + + def startBuild(id: Int, token: String)(implicit client: Client): Task[Build] = + client.expect(post(job(id)).withOauth2(token))(jsonOf[Build]) +} diff --git a/bot/src/dotty/tools/bot/model/Github.scala b/bot/src/dotty/tools/bot/model/Github.scala index 80bca3ad1042..f3465579360b 100644 --- a/bot/src/dotty/tools/bot/model/Github.scala +++ b/bot/src/dotty/tools/bot/model/Github.scala @@ -30,7 +30,13 @@ object Github { context: String = "CLA" ) - case class StatusResponse(url: String, id: Long, state: String) { + case class StatusResponse( + url: String, + id: Long, + state: String, + context: String, + target_url: String + ) { def sha: String = url.split('/').last } diff --git a/bot/test/PRServiceTests.scala b/bot/test/PRServiceTests.scala index 4d827d6b86a8..f1b22ad68754 100644 --- a/bot/test/PRServiceTests.scala +++ b/bot/test/PRServiceTests.scala @@ -9,6 +9,7 @@ import io.circe.syntax._ import io.circe.parser.decode import model.Github._ +import model.Drone import org.http4s.client.blaze._ import org.http4s.client.Client import scalaz.concurrent.Task @@ -116,4 +117,11 @@ class PRServiceTests extends PullRequestService { s"Body of review was not as expected:\n${res.body}" ) } + + @Test def canStartAndStopBuild = { + val build = withClient(implicit client => Drone.startBuild(1921, droneToken)) + assert(build.status == "pending" || build.status == "building") + val killed = withClient(implicit client => Drone.stopBuild(1921, droneToken)) + assert(killed, "Couldn't kill build") + } } From aad721f1e6371f26e5c7527c71cf41b4bf412578 Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Tue, 25 Apr 2017 15:27:48 +0200 Subject: [PATCH 8/9] Add ability for bot to restart things when mentioned --- .../dotty/tools/bot/PullRequestService.scala | 134 +++++++++++- bot/src/dotty/tools/bot/model/Github.scala | 12 +- bot/test/PRServiceTests.scala | 37 +++- .../test-mention-no-understandy.json | 203 ++++++++++++++++++ bot/test/resources/test-mention.json | 203 ++++++++++++++++++ 5 files changed, 572 insertions(+), 17 deletions(-) create mode 100644 bot/test/resources/test-mention-no-understandy.json create mode 100644 bot/test/resources/test-mention.json diff --git a/bot/src/dotty/tools/bot/PullRequestService.scala b/bot/src/dotty/tools/bot/PullRequestService.scala index fb13b1c45bef..c95254e98f46 100644 --- a/bot/src/dotty/tools/bot/PullRequestService.scala +++ b/bot/src/dotty/tools/bot/PullRequestService.scala @@ -37,9 +37,29 @@ trait PullRequestService { /** Pull Request HTTP service */ val prService = HttpService { case request @ POST -> Root => - request.as(jsonOf[Issue]).flatMap(checkPullRequest) + val githubEvent = + request.headers + .get(CaseInsensitiveString("X-GitHub-Event")) + .map(_.value).getOrElse("") + + githubEvent match { + case "pull_request" => + request.as(jsonOf[Issue]).flatMap(checkPullRequest) + + case "issue_comment" => + request.as(jsonOf[IssueComment]).flatMap(respondToComment) + + case "" => + BadRequest("Missing header: X-Github-Event") + + case event => + BadRequest("Unsupported event: $event") + + } } + private[this] val droneContext = "continuous-integration/drone/pr" + private final case class CLASignature( user: String, signed: Boolean, @@ -148,7 +168,7 @@ trait PullRequestService { } /** Ordered from earliest to latest */ - def getCommits(issueNbr: Int, httpClient: Client): Task[List[Commit]] = { + def getCommits(issueNbr: Int)(implicit httpClient: Client): Task[List[Commit]] = { def makeRequest(url: String): Task[List[Commit]] = for { res <- httpClient.fetch(get(url)) { res => @@ -246,10 +266,10 @@ trait PullRequestService { } def checkFreshPR(issue: Issue): Task[Response] = { - val httpClient = PooledHttp1Client() + implicit val httpClient = PooledHttp1Client() for { - commits <- getCommits(issue.number, httpClient) + commits <- getCommits(issue.number) statuses <- checkCLA(commits, httpClient) (validStatuses, invalidStatuses) = statuses.partition(_.isValid) @@ -280,9 +300,31 @@ trait PullRequestService { private def extractCommitSha(status: StatusResponse): Task[String] = Task.delay(status.sha) + def startBuild(commit: Commit)(implicit client: Client): Task[Drone.Build] = { + def pendingStatus(targetUrl: String): Status = + Status("pending", targetUrl, "build restarted by bot", droneContext) + + def filterStatuses(xs: List[StatusResponse]): Task[Int] = + xs.filter { status => + (status.state == "failure" || status.state == "success") && + status.context == droneContext + } + .map(status => Task.now(status.target_url.split('/').last.toInt)) + .headOption + .getOrElse(Task.fail(new NoSuchElementException("Couldn't find drone build for PR"))) + + for { + statuses <- getStatus(commit, client) + failed <- filterStatuses(statuses) + build <- Drone.startBuild(failed, droneToken) + setStatusReq <- post(statusUrl(commit.sha)).withAuth(githubUser, githubToken) + newStatus = pendingStatus(s"http://dotty-ci.epfl.ch/lampepfl/dotty/$failed").asJson + _ <- client.expect(setStatusReq.withBody(newStatus))(jsonOf[StatusResponse]) + } yield build + } + def cancelBuilds(commits: List[Commit])(implicit client: Client): Task[Boolean] = Task.gatherUnordered { - val droneContext = "continuous-integration/drone/pr" commits.map { commit => for { statuses <- getStatus(commit, client) @@ -295,10 +337,10 @@ trait PullRequestService { .map(xs => xs.foldLeft(true)(_ == _)) def checkSynchronize(issue: Issue): Task[Response] = { - val httpClient = PooledHttp1Client() + implicit val httpClient = PooledHttp1Client() for { - commits <- getCommits(issue.number, httpClient) + commits <- getCommits(issue.number) statuses <- checkCLA(commits, httpClient) invalid = statuses.filterNot(_.isValid) _ <- sendStatuses(invalid, httpClient) @@ -318,8 +360,80 @@ trait PullRequestService { def checkPullRequest(issue: Issue): Task[Response] = issue.action match { - case "opened" => checkFreshPR(issue) - case "synchronize" => checkSynchronize(issue) - case action => BadRequest(s"Unhandled action: $action") + case Some("opened") => checkFreshPR(issue) + case Some("synchronize") => checkSynchronize(issue) + case Some(action) => BadRequest(s"Unhandled action: $action") + case None => BadRequest("Cannot check pull request, missing action field") + } + + def restartCI(issue: Issue): Task[Response] = { + implicit val client = PooledHttp1Client() + + def restartedComment: Comment = { + import scala.util.Random + val answers = Array( + "Okidokey, boss! :clap:", + "You got it, homie! :pray:", + "No problem, big shot! :punch:", + "Sure thing, I got your back! :heart:", + "No WAY! :-1: ...wait, don't fire me please! There, I did it! :tada:" + ) + + Comment(Author(None), answers(Random.nextInt(answers.length))) } + + for { + commits <- getCommits(issue.number) + latest = commits.last + _ <- cancelBuilds(latest :: Nil) + _ <- startBuild(latest) + req <- post(issueCommentsUrl(issue.number)).withAuth(githubUser, githubToken) + _ <- client.fetch(req.withBody(restartedComment.asJson))(Task.now) + res <- Ok("Replied to request for CI restart") + } yield res + } + + def cannotUnderstand(line: String, issueComment: IssueComment): Task[Response] = { + implicit val client = PooledHttp1Client() + val comment = Comment(Author(None), { + s"""Hey, sorry - I could not understand what you meant by: + | + |> $line + | + |I'm just a dumb bot after all :cry: + | + |I mostly understand when your mention contains these words: + | + |- (re)check (the) cla + |- recheck + |- restart drone + | + |Maybe if you want to make me smarter, you could open a PR? :heart_eyes: + |""".stripMargin + }) + for { + req <- post(issueCommentsUrl(issueComment.issue.number)).withAuth(githubUser, githubToken) + _ <- client.fetch(req.withBody(comment.asJson))(Task.now) + res <- Ok("Delivered could not understand comment") + } yield res + } + + def extractMention(body: String): Option[String] = + body.lines.find(_.startsWith("@dotty-bot:")) + + /** TODO: The implementation here could be quite elegant if we used a trie instead */ + def interpretMention(line: String, issueComment: IssueComment): Task[Response] = { + val loweredLine = line.toLowerCase + if (loweredLine.contains("check cla") || loweredLine.contains("check the cla")) + checkSynchronize(issueComment.issue) + else if (loweredLine.contains("recheck") || loweredLine.contains("restart drone")) + restartCI(issueComment.issue) + else + cannotUnderstand(line, issueComment) + } + + def respondToComment(issueComment: IssueComment): Task[Response] = + extractMention(issueComment.comment.body) + .map(interpretMention(_, issueComment)) + .getOrElse(Ok("Nothing to do here, move along!")) } diff --git a/bot/src/dotty/tools/bot/model/Github.scala b/bot/src/dotty/tools/bot/model/Github.scala index f3465579360b..b28e57f669bc 100644 --- a/bot/src/dotty/tools/bot/model/Github.scala +++ b/bot/src/dotty/tools/bot/model/Github.scala @@ -3,12 +3,12 @@ package model object Github { case class Issue( - action: String, // "opened", "reopened", "closed", "synchronize" + action: Option[String], // "opened", "reopened", "closed", "synchronize" number: Int, pull_request: Option[PullRequest] ) - case class PullRequest(url: String, id: Long, commits_url: String) + case class PullRequest(url: String, id: Option[Long], commits_url: Option[String]) case class CommitInfo(message: String) @@ -40,10 +40,8 @@ object Github { def sha: String = url.split('/').last } - case class Comment(user: Author) - /** A PR review */ - case class Review (body: String, event: String) + case class Review(body: String, event: String) object Review { def approve(body: String) = Review(body, "APPROVE") @@ -52,4 +50,8 @@ object Github { } case class ReviewResponse(body: String, state: String, id: Long) + + case class IssueComment(action: String, issue: Issue, comment: Comment) + + case class Comment(user: Author, body: String) } diff --git a/bot/test/PRServiceTests.scala b/bot/test/PRServiceTests.scala index f1b22ad68754..f80def044a2d 100644 --- a/bot/test/PRServiceTests.scala +++ b/bot/test/PRServiceTests.scala @@ -41,9 +41,22 @@ class PRServiceTests extends PullRequestService { assert(issue.pull_request.isDefined, "missing pull request") } + @Test def canUnmarshalIssueComment = { + val json = getResource("/test-mention.json") + val issueComment: IssueComment = decode[IssueComment](json) match { + case Right(is: IssueComment) => is + case Left(ex) => throw ex + } + + assert( + issueComment.comment.body == "@dotty-bot: could you recheck this please?", + s"incorrect body: ${issueComment.comment.body}" + ) + } + @Test def canGetAllCommitsFromPR = { val issueNbr = 1941 // has 2 commits: https://github.com/lampepfl/dotty/pull/1941/commits - val List(c1, c2) = withClient(getCommits(issueNbr, _)) + val List(c1, c2) = withClient(implicit client => getCommits(issueNbr)) assertEquals( "Represent untyped operators as Ident instead of Name", @@ -58,7 +71,7 @@ class PRServiceTests extends PullRequestService { @Test def canGetMoreThan100Commits = { val issueNbr = 1840 // has >100 commits: https://github.com/lampepfl/dotty/pull/1840/commits - val numberOfCommits = withClient(getCommits(issueNbr, _)).length + val numberOfCommits = withClient(implicit client => getCommits(issueNbr)).length assert( numberOfCommits > 100, @@ -124,4 +137,24 @@ class PRServiceTests extends PullRequestService { val killed = withClient(implicit client => Drone.stopBuild(1921, droneToken)) assert(killed, "Couldn't kill build") } + + @Test def canUnderstandWhenToRestartBuild = { + val json = getResource("/test-mention.json") + val issueComment: IssueComment = decode[IssueComment](json) match { + case Right(is: IssueComment) => is + case Left(ex) => throw ex + } + + assert(respondToComment(issueComment).run.status.code == 200) + } + + @Test def canTellUserWhenNotUnderstanding = { + val json = getResource("/test-mention-no-understandy.json") + val issueComment: IssueComment = decode[IssueComment](json) match { + case Right(is: IssueComment) => is + case Left(ex) => throw ex + } + + assert(respondToComment(issueComment).run.status.code == 200) + } } diff --git a/bot/test/resources/test-mention-no-understandy.json b/bot/test/resources/test-mention-no-understandy.json new file mode 100644 index 000000000000..0b17d83de34e --- /dev/null +++ b/bot/test/resources/test-mention-no-understandy.json @@ -0,0 +1,203 @@ +{ + "action": "created", + "issue": { + "url": "https://api.github.com/repos/lampepfl/dotty/issues/2304", + "repository_url": "https://api.github.com/repos/lampepfl/dotty", + "labels_url": "https://api.github.com/repos/lampepfl/dotty/issues/2304/labels{/name}", + "comments_url": "https://api.github.com/repos/lampepfl/dotty/issues/2304/comments", + "events_url": "https://api.github.com/repos/lampepfl/dotty/issues/2304/events", + "html_url": "https://github.com/lampepfl/dotty/pull/2304", + "id": 224071012, + "number": 2304, + "title": "Bot improvements - CLA checks, killing old jobs, nagging about form", + "user": { + "login": "felixmulder", + "id": 1530049, + "avatar_url": "https://avatars2.githubusercontent.com/u/1530049?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/felixmulder", + "html_url": "https://github.com/felixmulder", + "followers_url": "https://api.github.com/users/felixmulder/followers", + "following_url": "https://api.github.com/users/felixmulder/following{/other_user}", + "gists_url": "https://api.github.com/users/felixmulder/gists{/gist_id}", + "starred_url": "https://api.github.com/users/felixmulder/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/felixmulder/subscriptions", + "organizations_url": "https://api.github.com/users/felixmulder/orgs", + "repos_url": "https://api.github.com/users/felixmulder/repos", + "events_url": "https://api.github.com/users/felixmulder/events{/privacy}", + "received_events_url": "https://api.github.com/users/felixmulder/received_events", + "type": "User", + "site_admin": false + }, + "labels": [ + + ], + "state": "open", + "locked": false, + "assignee": null, + "assignees": [ + + ], + "milestone": null, + "comments": 0, + "created_at": "2017-04-25T09:10:15Z", + "updated_at": "2017-04-25T11:34:20Z", + "closed_at": null, + "pull_request": { + "url": "https://api.github.com/repos/lampepfl/dotty/pulls/2304", + "html_url": "https://github.com/lampepfl/dotty/pull/2304", + "diff_url": "https://github.com/lampepfl/dotty/pull/2304.diff", + "patch_url": "https://github.com/lampepfl/dotty/pull/2304.patch" + }, + "body": "Bot improvements include:\r\n\r\n- Deleting old jobs from PR, only keep the latest drone build\r\n- Check CLA and only approve last commit\r\n- Add some greetings to opened PRs helping people on where to sign CLA etc" + }, + "comment": { + "url": "https://api.github.com/repos/lampepfl/dotty/issues/comments/297001946", + "html_url": "https://github.com/lampepfl/dotty/pull/2304#issuecomment-297001946", + "issue_url": "https://api.github.com/repos/lampepfl/dotty/issues/2304", + "id": 297001946, + "user": { + "login": "felixmulder", + "id": 1530049, + "avatar_url": "https://avatars2.githubusercontent.com/u/1530049?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/felixmulder", + "html_url": "https://github.com/felixmulder", + "followers_url": "https://api.github.com/users/felixmulder/followers", + "following_url": "https://api.github.com/users/felixmulder/following{/other_user}", + "gists_url": "https://api.github.com/users/felixmulder/gists{/gist_id}", + "starred_url": "https://api.github.com/users/felixmulder/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/felixmulder/subscriptions", + "organizations_url": "https://api.github.com/users/felixmulder/orgs", + "repos_url": "https://api.github.com/users/felixmulder/repos", + "events_url": "https://api.github.com/users/felixmulder/events{/privacy}", + "received_events_url": "https://api.github.com/users/felixmulder/received_events", + "type": "User", + "site_admin": false + }, + "created_at": "2017-04-25T11:34:20Z", + "updated_at": "2017-04-25T11:34:20Z", + "body": "@dotty-bot: I don't like you." + }, + "repository": { + "id": 7035651, + "name": "dotty", + "full_name": "lampepfl/dotty", + "owner": { + "login": "lampepfl", + "id": 2684793, + "avatar_url": "https://avatars2.githubusercontent.com/u/2684793?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/lampepfl", + "html_url": "https://github.com/lampepfl", + "followers_url": "https://api.github.com/users/lampepfl/followers", + "following_url": "https://api.github.com/users/lampepfl/following{/other_user}", + "gists_url": "https://api.github.com/users/lampepfl/gists{/gist_id}", + "starred_url": "https://api.github.com/users/lampepfl/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/lampepfl/subscriptions", + "organizations_url": "https://api.github.com/users/lampepfl/orgs", + "repos_url": "https://api.github.com/users/lampepfl/repos", + "events_url": "https://api.github.com/users/lampepfl/events{/privacy}", + "received_events_url": "https://api.github.com/users/lampepfl/received_events", + "type": "Organization", + "site_admin": false + }, + "private": false, + "html_url": "https://github.com/lampepfl/dotty", + "description": "Research platform for new language concepts and compiler technologies for Scala.", + "fork": false, + "url": "https://api.github.com/repos/lampepfl/dotty", + "forks_url": "https://api.github.com/repos/lampepfl/dotty/forks", + "keys_url": "https://api.github.com/repos/lampepfl/dotty/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/lampepfl/dotty/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/lampepfl/dotty/teams", + "hooks_url": "https://api.github.com/repos/lampepfl/dotty/hooks", + "issue_events_url": "https://api.github.com/repos/lampepfl/dotty/issues/events{/number}", + "events_url": "https://api.github.com/repos/lampepfl/dotty/events", + "assignees_url": "https://api.github.com/repos/lampepfl/dotty/assignees{/user}", + "branches_url": "https://api.github.com/repos/lampepfl/dotty/branches{/branch}", + "tags_url": "https://api.github.com/repos/lampepfl/dotty/tags", + "blobs_url": "https://api.github.com/repos/lampepfl/dotty/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/lampepfl/dotty/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/lampepfl/dotty/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/lampepfl/dotty/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/lampepfl/dotty/statuses/{sha}", + "languages_url": "https://api.github.com/repos/lampepfl/dotty/languages", + "stargazers_url": "https://api.github.com/repos/lampepfl/dotty/stargazers", + "contributors_url": "https://api.github.com/repos/lampepfl/dotty/contributors", + "subscribers_url": "https://api.github.com/repos/lampepfl/dotty/subscribers", + "subscription_url": "https://api.github.com/repos/lampepfl/dotty/subscription", + "commits_url": "https://api.github.com/repos/lampepfl/dotty/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/lampepfl/dotty/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/lampepfl/dotty/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/lampepfl/dotty/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/lampepfl/dotty/contents/{+path}", + "compare_url": "https://api.github.com/repos/lampepfl/dotty/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/lampepfl/dotty/merges", + "archive_url": "https://api.github.com/repos/lampepfl/dotty/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/lampepfl/dotty/downloads", + "issues_url": "https://api.github.com/repos/lampepfl/dotty/issues{/number}", + "pulls_url": "https://api.github.com/repos/lampepfl/dotty/pulls{/number}", + "milestones_url": "https://api.github.com/repos/lampepfl/dotty/milestones{/number}", + "notifications_url": "https://api.github.com/repos/lampepfl/dotty/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/lampepfl/dotty/labels{/name}", + "releases_url": "https://api.github.com/repos/lampepfl/dotty/releases{/id}", + "deployments_url": "https://api.github.com/repos/lampepfl/dotty/deployments", + "created_at": "2012-12-06T12:57:33Z", + "updated_at": "2017-04-23T22:50:39Z", + "pushed_at": "2017-04-25T11:28:03Z", + "git_url": "git://github.com/lampepfl/dotty.git", + "ssh_url": "git@github.com:lampepfl/dotty.git", + "clone_url": "https://github.com/lampepfl/dotty.git", + "svn_url": "https://github.com/lampepfl/dotty", + "homepage": "http://dotty.epfl.ch", + "size": 38144, + "stargazers_count": 1567, + "watchers_count": 1567, + "language": "Scala", + "has_issues": true, + "has_projects": true, + "has_downloads": true, + "has_wiki": false, + "has_pages": true, + "forks_count": 233, + "mirror_url": null, + "open_issues_count": 309, + "forks": 233, + "open_issues": 309, + "watchers": 1567, + "default_branch": "master" + }, + "organization": { + "login": "lampepfl", + "id": 2684793, + "url": "https://api.github.com/orgs/lampepfl", + "repos_url": "https://api.github.com/orgs/lampepfl/repos", + "events_url": "https://api.github.com/orgs/lampepfl/events", + "hooks_url": "https://api.github.com/orgs/lampepfl/hooks", + "issues_url": "https://api.github.com/orgs/lampepfl/issues", + "members_url": "https://api.github.com/orgs/lampepfl/members{/member}", + "public_members_url": "https://api.github.com/orgs/lampepfl/public_members{/member}", + "avatar_url": "https://avatars2.githubusercontent.com/u/2684793?v=3", + "description": null + }, + "sender": { + "login": "felixmulder", + "id": 1530049, + "avatar_url": "https://avatars2.githubusercontent.com/u/1530049?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/felixmulder", + "html_url": "https://github.com/felixmulder", + "followers_url": "https://api.github.com/users/felixmulder/followers", + "following_url": "https://api.github.com/users/felixmulder/following{/other_user}", + "gists_url": "https://api.github.com/users/felixmulder/gists{/gist_id}", + "starred_url": "https://api.github.com/users/felixmulder/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/felixmulder/subscriptions", + "organizations_url": "https://api.github.com/users/felixmulder/orgs", + "repos_url": "https://api.github.com/users/felixmulder/repos", + "events_url": "https://api.github.com/users/felixmulder/events{/privacy}", + "received_events_url": "https://api.github.com/users/felixmulder/received_events", + "type": "User", + "site_admin": false + } +} diff --git a/bot/test/resources/test-mention.json b/bot/test/resources/test-mention.json new file mode 100644 index 000000000000..d888e82c7234 --- /dev/null +++ b/bot/test/resources/test-mention.json @@ -0,0 +1,203 @@ +{ + "action": "created", + "issue": { + "url": "https://api.github.com/repos/lampepfl/dotty/issues/2304", + "repository_url": "https://api.github.com/repos/lampepfl/dotty", + "labels_url": "https://api.github.com/repos/lampepfl/dotty/issues/2304/labels{/name}", + "comments_url": "https://api.github.com/repos/lampepfl/dotty/issues/2304/comments", + "events_url": "https://api.github.com/repos/lampepfl/dotty/issues/2304/events", + "html_url": "https://github.com/lampepfl/dotty/pull/2304", + "id": 224071012, + "number": 2304, + "title": "Bot improvements - CLA checks, killing old jobs, nagging about form", + "user": { + "login": "felixmulder", + "id": 1530049, + "avatar_url": "https://avatars2.githubusercontent.com/u/1530049?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/felixmulder", + "html_url": "https://github.com/felixmulder", + "followers_url": "https://api.github.com/users/felixmulder/followers", + "following_url": "https://api.github.com/users/felixmulder/following{/other_user}", + "gists_url": "https://api.github.com/users/felixmulder/gists{/gist_id}", + "starred_url": "https://api.github.com/users/felixmulder/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/felixmulder/subscriptions", + "organizations_url": "https://api.github.com/users/felixmulder/orgs", + "repos_url": "https://api.github.com/users/felixmulder/repos", + "events_url": "https://api.github.com/users/felixmulder/events{/privacy}", + "received_events_url": "https://api.github.com/users/felixmulder/received_events", + "type": "User", + "site_admin": false + }, + "labels": [ + + ], + "state": "open", + "locked": false, + "assignee": null, + "assignees": [ + + ], + "milestone": null, + "comments": 0, + "created_at": "2017-04-25T09:10:15Z", + "updated_at": "2017-04-25T11:34:20Z", + "closed_at": null, + "pull_request": { + "url": "https://api.github.com/repos/lampepfl/dotty/pulls/2304", + "html_url": "https://github.com/lampepfl/dotty/pull/2304", + "diff_url": "https://github.com/lampepfl/dotty/pull/2304.diff", + "patch_url": "https://github.com/lampepfl/dotty/pull/2304.patch" + }, + "body": "Bot improvements include:\r\n\r\n- Deleting old jobs from PR, only keep the latest drone build\r\n- Check CLA and only approve last commit\r\n- Add some greetings to opened PRs helping people on where to sign CLA etc" + }, + "comment": { + "url": "https://api.github.com/repos/lampepfl/dotty/issues/comments/297001946", + "html_url": "https://github.com/lampepfl/dotty/pull/2304#issuecomment-297001946", + "issue_url": "https://api.github.com/repos/lampepfl/dotty/issues/2304", + "id": 297001946, + "user": { + "login": "felixmulder", + "id": 1530049, + "avatar_url": "https://avatars2.githubusercontent.com/u/1530049?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/felixmulder", + "html_url": "https://github.com/felixmulder", + "followers_url": "https://api.github.com/users/felixmulder/followers", + "following_url": "https://api.github.com/users/felixmulder/following{/other_user}", + "gists_url": "https://api.github.com/users/felixmulder/gists{/gist_id}", + "starred_url": "https://api.github.com/users/felixmulder/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/felixmulder/subscriptions", + "organizations_url": "https://api.github.com/users/felixmulder/orgs", + "repos_url": "https://api.github.com/users/felixmulder/repos", + "events_url": "https://api.github.com/users/felixmulder/events{/privacy}", + "received_events_url": "https://api.github.com/users/felixmulder/received_events", + "type": "User", + "site_admin": false + }, + "created_at": "2017-04-25T11:34:20Z", + "updated_at": "2017-04-25T11:34:20Z", + "body": "@dotty-bot: could you recheck this please?" + }, + "repository": { + "id": 7035651, + "name": "dotty", + "full_name": "lampepfl/dotty", + "owner": { + "login": "lampepfl", + "id": 2684793, + "avatar_url": "https://avatars2.githubusercontent.com/u/2684793?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/lampepfl", + "html_url": "https://github.com/lampepfl", + "followers_url": "https://api.github.com/users/lampepfl/followers", + "following_url": "https://api.github.com/users/lampepfl/following{/other_user}", + "gists_url": "https://api.github.com/users/lampepfl/gists{/gist_id}", + "starred_url": "https://api.github.com/users/lampepfl/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/lampepfl/subscriptions", + "organizations_url": "https://api.github.com/users/lampepfl/orgs", + "repos_url": "https://api.github.com/users/lampepfl/repos", + "events_url": "https://api.github.com/users/lampepfl/events{/privacy}", + "received_events_url": "https://api.github.com/users/lampepfl/received_events", + "type": "Organization", + "site_admin": false + }, + "private": false, + "html_url": "https://github.com/lampepfl/dotty", + "description": "Research platform for new language concepts and compiler technologies for Scala.", + "fork": false, + "url": "https://api.github.com/repos/lampepfl/dotty", + "forks_url": "https://api.github.com/repos/lampepfl/dotty/forks", + "keys_url": "https://api.github.com/repos/lampepfl/dotty/keys{/key_id}", + "collaborators_url": "https://api.github.com/repos/lampepfl/dotty/collaborators{/collaborator}", + "teams_url": "https://api.github.com/repos/lampepfl/dotty/teams", + "hooks_url": "https://api.github.com/repos/lampepfl/dotty/hooks", + "issue_events_url": "https://api.github.com/repos/lampepfl/dotty/issues/events{/number}", + "events_url": "https://api.github.com/repos/lampepfl/dotty/events", + "assignees_url": "https://api.github.com/repos/lampepfl/dotty/assignees{/user}", + "branches_url": "https://api.github.com/repos/lampepfl/dotty/branches{/branch}", + "tags_url": "https://api.github.com/repos/lampepfl/dotty/tags", + "blobs_url": "https://api.github.com/repos/lampepfl/dotty/git/blobs{/sha}", + "git_tags_url": "https://api.github.com/repos/lampepfl/dotty/git/tags{/sha}", + "git_refs_url": "https://api.github.com/repos/lampepfl/dotty/git/refs{/sha}", + "trees_url": "https://api.github.com/repos/lampepfl/dotty/git/trees{/sha}", + "statuses_url": "https://api.github.com/repos/lampepfl/dotty/statuses/{sha}", + "languages_url": "https://api.github.com/repos/lampepfl/dotty/languages", + "stargazers_url": "https://api.github.com/repos/lampepfl/dotty/stargazers", + "contributors_url": "https://api.github.com/repos/lampepfl/dotty/contributors", + "subscribers_url": "https://api.github.com/repos/lampepfl/dotty/subscribers", + "subscription_url": "https://api.github.com/repos/lampepfl/dotty/subscription", + "commits_url": "https://api.github.com/repos/lampepfl/dotty/commits{/sha}", + "git_commits_url": "https://api.github.com/repos/lampepfl/dotty/git/commits{/sha}", + "comments_url": "https://api.github.com/repos/lampepfl/dotty/comments{/number}", + "issue_comment_url": "https://api.github.com/repos/lampepfl/dotty/issues/comments{/number}", + "contents_url": "https://api.github.com/repos/lampepfl/dotty/contents/{+path}", + "compare_url": "https://api.github.com/repos/lampepfl/dotty/compare/{base}...{head}", + "merges_url": "https://api.github.com/repos/lampepfl/dotty/merges", + "archive_url": "https://api.github.com/repos/lampepfl/dotty/{archive_format}{/ref}", + "downloads_url": "https://api.github.com/repos/lampepfl/dotty/downloads", + "issues_url": "https://api.github.com/repos/lampepfl/dotty/issues{/number}", + "pulls_url": "https://api.github.com/repos/lampepfl/dotty/pulls{/number}", + "milestones_url": "https://api.github.com/repos/lampepfl/dotty/milestones{/number}", + "notifications_url": "https://api.github.com/repos/lampepfl/dotty/notifications{?since,all,participating}", + "labels_url": "https://api.github.com/repos/lampepfl/dotty/labels{/name}", + "releases_url": "https://api.github.com/repos/lampepfl/dotty/releases{/id}", + "deployments_url": "https://api.github.com/repos/lampepfl/dotty/deployments", + "created_at": "2012-12-06T12:57:33Z", + "updated_at": "2017-04-23T22:50:39Z", + "pushed_at": "2017-04-25T11:28:03Z", + "git_url": "git://github.com/lampepfl/dotty.git", + "ssh_url": "git@github.com:lampepfl/dotty.git", + "clone_url": "https://github.com/lampepfl/dotty.git", + "svn_url": "https://github.com/lampepfl/dotty", + "homepage": "http://dotty.epfl.ch", + "size": 38144, + "stargazers_count": 1567, + "watchers_count": 1567, + "language": "Scala", + "has_issues": true, + "has_projects": true, + "has_downloads": true, + "has_wiki": false, + "has_pages": true, + "forks_count": 233, + "mirror_url": null, + "open_issues_count": 309, + "forks": 233, + "open_issues": 309, + "watchers": 1567, + "default_branch": "master" + }, + "organization": { + "login": "lampepfl", + "id": 2684793, + "url": "https://api.github.com/orgs/lampepfl", + "repos_url": "https://api.github.com/orgs/lampepfl/repos", + "events_url": "https://api.github.com/orgs/lampepfl/events", + "hooks_url": "https://api.github.com/orgs/lampepfl/hooks", + "issues_url": "https://api.github.com/orgs/lampepfl/issues", + "members_url": "https://api.github.com/orgs/lampepfl/members{/member}", + "public_members_url": "https://api.github.com/orgs/lampepfl/public_members{/member}", + "avatar_url": "https://avatars2.githubusercontent.com/u/2684793?v=3", + "description": null + }, + "sender": { + "login": "felixmulder", + "id": 1530049, + "avatar_url": "https://avatars2.githubusercontent.com/u/1530049?v=3", + "gravatar_id": "", + "url": "https://api.github.com/users/felixmulder", + "html_url": "https://github.com/felixmulder", + "followers_url": "https://api.github.com/users/felixmulder/followers", + "following_url": "https://api.github.com/users/felixmulder/following{/other_user}", + "gists_url": "https://api.github.com/users/felixmulder/gists{/gist_id}", + "starred_url": "https://api.github.com/users/felixmulder/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/felixmulder/subscriptions", + "organizations_url": "https://api.github.com/users/felixmulder/orgs", + "repos_url": "https://api.github.com/users/felixmulder/repos", + "events_url": "https://api.github.com/users/felixmulder/events{/privacy}", + "received_events_url": "https://api.github.com/users/felixmulder/received_events", + "type": "User", + "site_admin": false + } +} \ No newline at end of file From 595602d762c6271db4c71915ca5118d4e4599f4d Mon Sep 17 00:00:00 2001 From: Felix Mulder Date: Thu, 27 Apr 2017 09:49:19 +0200 Subject: [PATCH 9/9] Get rid of unnecessary `foldLeft`s and `shutdownClient` --- .../dotty/tools/bot/PullRequestService.scala | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/bot/src/dotty/tools/bot/PullRequestService.scala b/bot/src/dotty/tools/bot/PullRequestService.scala index c95254e98f46..afe5f10ce623 100644 --- a/bot/src/dotty/tools/bot/PullRequestService.scala +++ b/bot/src/dotty/tools/bot/PullRequestService.scala @@ -84,9 +84,6 @@ trait PullRequestService { def reviewUrl(issueNbr: Int): String = s"$githubUrl/repos/lampepfl/dotty/pulls/$issueNbr/reviews" - def shutdownClient(client: Client): Task[Unit] = - client.shutdownNow().pure[Task] - sealed trait CommitStatus { def commit: Commit def isValid: Boolean @@ -207,7 +204,7 @@ trait PullRequestService { => false } - wrongTense || firstLine.last == '.' || firstLine.length > 100 + wrongTense || firstLine.last == '.' || firstLine.length > 80 } def sendInitialComment(issueNbr: Int, invalidUsers: List[String], commits: List[Commit], client: Client): Task[ReviewResponse] = { @@ -235,7 +232,7 @@ trait PullRequestService { | |> 1. Separate subject from body with a blank line |> 1. When fixing an issue, start your commit message with `Fix #: ` - |> 1. Limit the subject line to 80 characters + |> 1. Limit the subject line to 72 characters |> 1. Capitalize the subject line |> 1. Do not end the subject line with a period |> 1. Use the imperative mood in the subject line ("Added" instead of "Add") @@ -288,7 +285,7 @@ trait PullRequestService { // Send positive comment: _ <- sendInitialComment(issue.number, invalidUsers, commits, httpClient) - _ <- shutdownClient(httpClient) + _ <- httpClient.shutdown resp <- Ok("Fresh PR checked") } yield resp @@ -331,10 +328,10 @@ trait PullRequestService { cancellable = statuses.filter(status => status.state == "pending" && status.context == droneContext) runningJobs = cancellable.map(_.target_url.split('/').last.toInt) cancelled <- Task.gatherUnordered(runningJobs.map(Drone.stopBuild(_, droneToken))) - } yield cancelled.foldLeft(true)(_ == _) + } yield cancelled.forall(identity) } } - .map(xs => xs.foldLeft(true)(_ == _)) + .map(_.forall(identity)) def checkSynchronize(issue: Issue): Task[Response] = { implicit val httpClient = PooledHttp1Client() @@ -353,7 +350,7 @@ trait PullRequestService { else setStatus(statuses.last, httpClient) } - _ <- shutdownClient(httpClient) + _ <- httpClient.shutdown resp <- Ok("Updated PR checked") } yield resp } @@ -421,7 +418,16 @@ trait PullRequestService { def extractMention(body: String): Option[String] = body.lines.find(_.startsWith("@dotty-bot:")) - /** TODO: The implementation here could be quite elegant if we used a trie instead */ + /** Try to make sense of what the user is requesting from the bot + * + * The bot's abilities currently only include: + * + * - Checking or re-checking the CLA + * - Restarting the CI tests + * + * @note The implementation here could be quite elegant if we used a trie + * instead + */ def interpretMention(line: String, issueComment: IssueComment): Task[Response] = { val loweredLine = line.toLowerCase if (loweredLine.contains("check cla") || loweredLine.contains("check the cla"))