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.

Sign up for GitHub

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

Jump to bottom

fix: the clean-up process not to log error for 401 on webhook removal #884

Merged
merged 1 commit into from
Feb 28, 2022
Merged

fix: the clean-up process not to log error for 401 on webhook removal #884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions ...ventlog/events/categories/statuschange/projectCleaner/ProjectWebhookAndTokenRemover.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import io.renku.graph.webhookservice.WebhookServiceUrl
import io.renku.http.client.{AccessToken, RestClient}
import org.http4s.EntityDecoder
import org.http4s.Method.DELETE
import org.http4s.Status.{NoContent, NotFound, Ok}
import org.http4s.Status.{Forbidden, NoContent, NotFound, Ok, Unauthorized}
import org.http4s.circe.jsonOf
import org.typelevel.log4cats.Logger

Expand All @@ -49,9 +49,10 @@ private class ProjectWebhookAndTokenRemoverImpl[F[_]: Async: Logger](accessToken
) extends RestClient[F, ProjectWebhookAndTokenRemover[F]](Throttler.noThrottling[F])
with ProjectWebhookAndTokenRemover[F] {

import AccessTokenFinder.projectIdToPath

override def removeWebhookAndToken(project: Project): F[Unit] =
accessTokenFinder
.findAccessToken(project.id)(AccessTokenFinder.projectIdToPath) flatMap {
accessTokenFinder.findAccessToken(project.id) >>= {
case Some(token) => removeProjectWebhook(project, token) >> removeProjectTokens(project)
case None => ().pure[F]
}
Expand All @@ -69,9 +70,9 @@ private class ProjectWebhookAndTokenRemoverImpl[F[_]: Async: Logger](accessToken
private implicit val tokenEntityDecoder: EntityDecoder[F, AccessToken] = jsonOf[F, AccessToken]

private def mapWebhookResponse(project: Project): ResponseMapping[Unit] = {
case (Ok | NotFound, _, _) => ().pure[F]
case (Ok | NotFound | Unauthorized | Forbidden, _, _) => ().pure[F]
case (status, _, _) =>
new Exception(s"Removing project webhook failed with status: $status for project: ${project.show}")
new Exception(show"Removing project webhook failed with status: $status for project: $project")
.raiseError[F, Unit]
}

Expand Down
30 changes: 26 additions & 4 deletions ...log/events/categories/statuschange/projectCleaner/ProjectWebhookAndTokenRemoverSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import io.renku.interpreters.TestLogger
import io.renku.stubbing.ExternalServiceStubbing
import io.renku.testtools.IOSpec
import org.http4s.Status
import org.http4s.Status.{InternalServerError, Unauthorized}
import org.http4s.Status.{Forbidden, InternalServerError, NotFound, ServiceUnavailable, Unauthorized}
import org.scalamock.scalatest.MockFactory
import org.scalatest.matchers.should
import org.scalatest.wordspec.AnyWordSpec
Expand All @@ -47,7 +47,7 @@ class ProjectWebhookAndTokenRemoverSpec

"removeWebhookAndToken" should {

"remove the token and the web hook of the specified project" in new TestCase {
"remove the token and the webhook of the specified project" in new TestCase {
(accesTokenFinder
.findAccessToken[projects.Id](_: projects.Id)(_: projects.Id => String))
.expects(project.id, AccessTokenFinder.projectIdToPath)
Expand All @@ -65,6 +65,28 @@ class ProjectWebhookAndTokenRemoverSpec
webhookAndTokenRemover.removeWebhookAndToken(project).unsafeRunSync() shouldBe ()
}

NotFound :: Unauthorized :: Forbidden :: Nil foreach { status =>
s"remove the token even if the webhook removal returned $status" in new TestCase {
(accesTokenFinder
.findAccessToken[projects.Id](_: projects.Id)(_: projects.Id => String))
.expects(project.id, AccessTokenFinder.projectIdToPath)
.returns(accessToken.some.pure[IO])
stubFor {
delete(s"/projects/${project.id}/webhooks")
.withAccessToken(accessToken.some)
.willReturn(aResponse().withStatus(status.code))
}
stubFor {
delete(s"/projects/${project.id}/tokens")
.willReturn(noContent())
}

webhookAndTokenRemover.removeWebhookAndToken(project).unsafeRunSync() shouldBe ()

logger.expectNoLogs()
}
}

"do nothing when the tokenFinder returns no token for the project" in new TestCase {
(accesTokenFinder
.findAccessToken[projects.Id](_: projects.Id)(_: projects.Id => String))
Expand Down Expand Up @@ -93,11 +115,11 @@ class ProjectWebhookAndTokenRemoverSpec
stubFor {
delete(s"/projects/${project.id}/webhooks")
.withAccessToken(accessToken.some)
.willReturn(unauthorized())
.willReturn(serviceUnavailable())
}
intercept[Exception] {
webhookAndTokenRemover.removeWebhookAndToken(project).unsafeRunSync()
}.getMessage shouldBe s"DELETE $webhookServiceUrl/projects/${project.id}/webhooks returned ${Status.Unauthorized}; error: Removing project webhook failed with status: $Unauthorized for project: ${project.show}"
}.getMessage shouldBe s"DELETE $webhookServiceUrl/projects/${project.id}/webhooks returned $ServiceUnavailable; error: Removing project webhook failed with status: $ServiceUnavailable for project: ${project.show}"
}

"fail when the token deletion fails" in new TestCase {
Expand Down