[IOSP-196] Include config to only consider *required* checks to merge (…

…#31)

* Original Implementation (pending tests)

* [Tests] makeState() -> MergeService.State.stub()

For consistency with other test files

* Avoid fetching required statuses if all success

* Create more stub methods for GitHub models

Fix tests

* Code cleanup and documentation

* Fix logic, using MergeState.unstable

* sendStatusEvent test helper

* Add tests for new feature

* 🧹

* Address PR feedback

* Rename `strict` to `isStrict`

Sources/App/Extensions/EnvironmentProperties.swift

@@ -19,6 +19,13 @@ extension Environment {
         return try Environment.get("GITHUB_REPOSITORY")
     }
 
+    static func requiresAllGitHubStatusChecks() throws -> Bool {
+        guard let stringValue: String = try? Environment.get("REQUIRES_ALL_STATUS_CHECKS") else {
+            return false // defaults to only consider required checks
+        }
+        return ["yes", "1", "true"].contains(stringValue.lowercased())
+    }
+
     static func mergeLabel() throws -> PullRequest.Label {
         return PullRequest.Label(name: try Environment.get("MERGE_LABEL"))
     }

Sources/App/configure.swift

@@ -42,6 +42,7 @@ private func makeMergeService(with logger: LoggerProtocol, _ gitHubEventsService
     return MergeService(
         integrationLabel: try Environment.mergeLabel(),
         topPriorityLabels: try Environment.topPriorityLabels(),
+        requiresAllStatusChecks: try Environment.requiresAllGitHubStatusChecks(), 
         logger: logger,
         gitHubAPI: gitHubAPI,
         gitHubEvents: gitHubEventsService

Sources/Bot/API/GitHubAPIProtocol.swift

@@ -10,6 +10,8 @@ public protocol GitHubAPIProtocol {
 
     func fetchCommitStatus(for pullRequest: PullRequest) -> SignalProducer<CommitState, AnyError>
 
+    func fetchRequiredStatusChecks(for branch: PullRequest.Branch) -> SignalProducer<RequiredStatusChecks, AnyError>
+
     /// Merges one branch into another.
     ///
     /// - SeeAlso: https://developer.github.com/v3/repos/merging/

Sources/Bot/API/Repository.swift

@@ -51,6 +51,14 @@ extension Repository {
         )
     }
 
+    func requiredStatusChecks(branch: PullRequest.Branch) -> Resource<RequiredStatusChecks> {
+        return Resource(
+            method: .GET,
+            path: path(for: "branches/\(branch.ref)/protection/required_status_checks"),
+            decoder: decode
+        )
+    }
+
     func deleteBranch(branch: PullRequest.Branch) -> Resource<Void> {
         return Resource(
             method: .DELETE,

Sources/Bot/API/RepositoryAPI.swift

@@ -29,6 +29,11 @@ public struct RepositoryAPI: GitHubAPIProtocol {
             .mapError(AnyError.init)
     }
 
+    public func fetchRequiredStatusChecks(for branch: PullRequest.Branch) -> SignalProducer<RequiredStatusChecks, AnyError> {
+        return client.request(repository.requiredStatusChecks(branch: branch))
+            .mapError(AnyError.init)
+    }
+
     public func merge(head: PullRequest.Branch, into base: PullRequest.Branch) -> SignalProducer<MergeResult, AnyError> {
         return client.request(repository.merge(head: head, into: base))
             .mapError(AnyError.init)

Sources/Bot/Models/CommitState.swift

@@ -14,6 +14,38 @@ public struct CommitState: Decodable, Equatable {
         public let context: String
     }
 
+    /// Combined state for _all_ the checks
+    /// - `.failure` if any of the status checks (`statuses.map(\.state)`) reports an error or `.failure`
+    /// - `.pending` if there are no statuses, or a status check is `.pending`
+    /// - `.success` if the latest status for all checks is `.success`
+    /// See https://developer.github.com/v3/repos/statuses/#get-the-combined-status-for-a-specific-ref
     public let state: State
+
+    /// Individual status of each check
     public let statuses: [Status]
 }
+
+
+extension CommitState.State {
+
+    /// Compute the combined state for a set of states.
+    /// This logic replicates the behavior provided by the GitHub API (see `CommitState.state` property provided by GitHub API above)
+    ///
+    /// We use this method ourselves to:
+    ///  - compute a similar combined state as GitHub API provides, but for only a subset of checks (especially only considering _required_ checks)
+    ///  - create stubs for `CommitState` in the tests to replicate the GitHub API behavior
+    /// - Parameter states: List of states to compute the combined state for
+    /// - Returns
+    ///   - `.failure` if any of the status checks reports a `.failure`
+    ///   - `.pending` if there are no statuses, or a status check is `.pending`
+    ///   - `.success` if the latest status for all checks is `.success`
+    static func combinedState(for states: [CommitState.State]) -> CommitState.State {
+        if states.contains(.failure) {
+            return .failure
+        } else if states.contains(.pending) {
+            return .pending
+        } else {
+            return .success
+        }
+    }
+}

Sources/Bot/Models/RequiredStatusCheck.swift

@@ -0,0 +1,13 @@
+import Foundation
+
+public struct RequiredStatusChecks: Decodable, Equatable {
+    /// Require branches to be up to date before merging?
+    public let isStrict: Bool
+    /// Names of status checks marked as Required
+    public let contexts: [String]
+
+    enum CodingKeys: String, CodingKey {
+        case isStrict = "strict"
+        case contexts
+    }
+}

Sources/Bot/Services/MergeService.swift

@@ -21,6 +21,7 @@ public final class MergeService {
     public init(
         integrationLabel: PullRequest.Label,
         topPriorityLabels: [PullRequest.Label],
+        requiresAllStatusChecks: Bool,
         statusChecksTimeout: TimeInterval = 60.minutes,
         logger: LoggerProtocol,
         gitHubAPI: GitHubAPIProtocol,
@@ -43,8 +44,8 @@ public final class MergeService {
             feedbacks: [
                 Feedbacks.whenStarting(github: self.gitHubAPI, scheduler: scheduler),
                 Feedbacks.whenReady(github: self.gitHubAPI, scheduler: scheduler),
-                Feedbacks.whenIntegrating(github: self.gitHubAPI, pullRequestChanges: pullRequestChanges, scheduler: scheduler),
-                Feedbacks.whenRunningStatusChecks(github: self.gitHubAPI, logger: logger, statusChecksCompletion: statusChecksCompletion, scheduler: scheduler),
+                Feedbacks.whenIntegrating(github: self.gitHubAPI, requiresAllStatusChecks: requiresAllStatusChecks, pullRequestChanges: pullRequestChanges, scheduler: scheduler),
+                Feedbacks.whenRunningStatusChecks(github: self.gitHubAPI, logger: logger, requiresAllStatusChecks: requiresAllStatusChecks, statusChecksCompletion: statusChecksCompletion, scheduler: scheduler),
                 Feedbacks.whenIntegrationFailed(github: self.gitHubAPI, logger: logger, scheduler: scheduler),
                 Feedbacks.pullRequestChanges(pullRequestChanges: pullRequestChanges, scheduler: scheduler),
                 Feedbacks.whenAddingPullRequests(github: self.gitHubAPI, scheduler: scheduler)
@@ -175,6 +176,7 @@ extension MergeService {
 
     fileprivate static func whenIntegrating(
         github: GitHubAPIProtocol,
+        requiresAllStatusChecks: Bool,
         pullRequestChanges: Signal<(PullRequestMetadata, PullRequest.Action), NoError>,
         scheduler: DateScheduler
     ) -> Feedback<State, Event> {
@@ -190,7 +192,8 @@ extension MergeService {
                 else { return .value(.integrationDidChangeStatus(.done, metadata)) }
 
             switch metadata.mergeState {
-            case .clean:
+            case .clean,
+                 .unstable where !requiresAllStatusChecks:
                 return github.mergePullRequest(metadata.reference)
                     .flatMap(.latest) { () -> SignalProducer<(), NoError> in
                         github.deleteBranch(named: metadata.reference.source)
@@ -234,7 +237,7 @@ extension MergeService {
                         case .pending:
                             return .value(.integrationDidChangeStatus(.updating, metadata))
                         case .failure:
-                            return .value(.integrationDidChangeStatus(.failed(.checkingCommitChecksFailed), metadata))
+                            return .value(.integrationDidChangeStatus(.failed(.checksFailing), metadata))
                         case  .success:
                             return github.fetchPullRequest(number: metadata.reference.number)
                                 .map { metadata in
@@ -280,6 +283,7 @@ extension MergeService {
     fileprivate static func whenRunningStatusChecks(
         github: GitHubAPIProtocol,
         logger: LoggerProtocol,
+        requiresAllStatusChecks: Bool,
         statusChecksCompletion: Signal<StatusEvent, NoError>,
         scheduler: DateScheduler
     ) -> Feedback<State, Event> {
@@ -315,13 +319,19 @@ extension MergeService {
                 // window where all checks have passed but just until the next check is added and stars running. This
                 // hopefully prevents those false positives by making sure we wait some time before checking if all
                 // checks have passed
-                .debounce(60, on: scheduler)
+                .debounce(60.0, on: scheduler)
                 .flatMap(.latest) { change in
                     github.fetchPullRequest(number: pullRequest.number)
                         .flatMap(.latest) { github.fetchCommitStatus(for: $0.reference).zip(with: .value($0)) }
+                        .flatMap(.latest) { commitStatus, pullRequestMetadataRefreshed -> SignalProducer<(CommitState.State, PullRequestMetadata), AnyError> in
+                            let requiredStateProducer = requiresAllStatusChecks
+                                ? .value(commitStatus.state)
+                                : getRequiredChecksState(github: github, targetBranch: pullRequest.target, commitState: commitStatus)
+                            return requiredStateProducer.zip(with: .value(pullRequestMetadataRefreshed))
+                        }
                         .flatMapError { _ in .empty }
-                        .filterMap { commitStatus, pullRequestMetadataRefreshed in
-                            switch commitStatus.state {
+                        .filterMap { state, pullRequestMetadataRefreshed in
+                            switch state {
                             case .pending:
                                 return nil
                             case .failure:
@@ -402,6 +412,25 @@ extension MergeService {
                 .map(Event.pullRequestDidChange)
         }
     }
+
+    /// Returns the consolidated status of all _required_ checks only
+    /// i.e. returns .failure or .pending if one of the required check is in .failure or .pending respectively
+    /// and return .success only if all required states are .success
+    fileprivate static func getRequiredChecksState(
+        github: GitHubAPIProtocol,
+        targetBranch: PullRequest.Branch,
+        commitState: CommitState
+    ) -> SignalProducer<CommitState.State, AnyError> {
+        if commitState.state == .success {
+            return .value(.success)
+        }
+        return github.fetchRequiredStatusChecks(for: targetBranch).map { (requiredStatusChecks) -> CommitState.State in
+            let requiredStates = requiredStatusChecks.contexts.map { requiredContext in
+                commitState.statuses.first(where: { $0.context == requiredContext })?.state ?? .pending
+            }
+            return CommitState.State.combinedState(for: requiredStates)
+        }
+    }
 }
 
 // MARK: - System types

Tests/BotTests/Canned Data/GitHubPullRequest.swift

@@ -1,5 +1,3 @@
-import Foundation
-
 let GitHubPullRequest = """
 {
     "id": 1,
@@ -429,4 +427,4 @@ let GitHubPullRequest = """
     "changed_files": 5,
     "maintainer_can_modify": true
 }
-"""
+"""

Tests/BotTests/Canned Data/GitHubPullRequestEvent.swift

@@ -1,5 +1,3 @@
-import Foundation
-
 let GitHubPullRequestEvent: String = """
 {
     "action": "closed",

Tests/BotTests/Canned Data/GitHubRequiredStatusChecks.swift

@@ -0,0 +1,17 @@
+let GitHubRequiredStatusChecks: String = """
+{
+    "url": "https://api.github.com/repos/babylonhealth/babylon-ios/branches/develop/protection/required_status_checks",
+    "strict": true,
+    "contexts": [
+        "ci/circleci: Build: SDK",
+        "ci/circleci: UnitTests: Ascension",
+        "ci/circleci: UnitTests: BabylonKSA",
+        "ci/circleci: UnitTests: BabylonUS",
+        "ci/circleci: UnitTests: Telus",
+        "ci/circleci: SnapshotTests: BabylonChatBotUI",
+        "ci/circleci: SnapshotTests: BabylonUI",
+        "ci/circleci: SnapshotTests: Babylon"
+    ],
+    "contexts_url": "https://api.github.com/repos/babylonhealth/babylon-ios/branches/develop/protection/required_status_checks/contexts"
+}
+"""

Tests/BotTests/Canned Data/GitHubStatusEvent.swift

@@ -1,5 +1,3 @@
-import Foundation
-
 let GitHubStatusEvent: String = """
 {
   "id": 5018968172,

Tests/BotTests/GitHub/GitHubAPITests.swift

@@ -93,6 +93,41 @@ class GitHubAPITests: XCTestCase {
         }
     }
 
+    func test_fetch_required_status_checks() {
+        perform(stub:
+            Interceptor.load(
+                stubs: [
+                    Interceptor.Stub(
+                        response: Interceptor.Stub.Response(
+                            url: URL(string: "https://api.github.com/repos/babylonhealth/babylon-ios/branches/develop/protection/required_status_checks")!,
+                            statusCode: 200,
+                            body: GitHubRequiredStatusChecks.data(using: .utf8)!
+                        )
+                    )]
+            )
+        ) { client in
+
+            let api = RepositoryAPI(client: client, repository: .init(owner: "golang", name: "go"))
+
+            let result = api.fetchRequiredStatusChecks(for: target.target).first()?.value
+
+            expect(result).toNot(beNil())
+            expect(result) == RequiredStatusChecks(
+                isStrict: true,
+                contexts: [
+                    "ci/circleci: Build: SDK",
+                    "ci/circleci: UnitTests: Ascension",
+                    "ci/circleci: UnitTests: BabylonKSA",
+                    "ci/circleci: UnitTests: BabylonUS",
+                    "ci/circleci: UnitTests: Telus",
+                    "ci/circleci: SnapshotTests: BabylonChatBotUI",
+                    "ci/circleci: SnapshotTests: BabylonUI",
+                    "ci/circleci: SnapshotTests: Babylon"
+                ]
+            )
+        }
+    }
+
     func test_delete_branch() {
 
         perform(stub:

Tests/BotTests/MergeService/MergeServiceHealthcheckTests.swift

@@ -12,27 +12,27 @@ class MergeServiceHealthcheckTests: XCTestCase {
             when: { input, scheduler in
                 scheduler.advance()
 
-                input.send(value: makeState(status: .starting))
+                input.send(value: MergeService.State.stub(status: .starting))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .idle))
+                input.send(value: MergeService.State.stub(status: .idle))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .ready))
+                input.send(value: MergeService.State.stub(status: .ready))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .integrating(MergeServiceFixture.defaultTarget)))
+                input.send(value: MergeService.State.stub(status: .integrating(MergeServiceFixture.defaultTarget)))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .ready))
+                input.send(value: MergeService.State.stub(status: .ready))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .idle))
+                input.send(value: MergeService.State.stub(status: .idle))
 
                 scheduler.advance()
             },
@@ -49,28 +49,28 @@ class MergeServiceHealthcheckTests: XCTestCase {
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .starting))
+                input.send(value: MergeService.State.stub(status: .starting))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .idle))
+                input.send(value: MergeService.State.stub(status: .idle))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .ready))
+                input.send(value: MergeService.State.stub(status: .ready))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .runningStatusChecks(MergeServiceFixture.defaultTarget)))
+                input.send(value: MergeService.State.stub(status: .runningStatusChecks(MergeServiceFixture.defaultTarget)))
 
                 scheduler.advance(by: .minutes(2 * MergeServiceFixture.defaultStatusChecksTimeout))
 
-                input.send(value: makeState(status: .integrationFailed(MergeServiceFixture.defaultTarget, .checksFailing)))
+                input.send(value: MergeService.State.stub(status: .integrationFailed(MergeServiceFixture.defaultTarget, .checksFailing)))
 
                 scheduler.advance()
 
-                input.send(value: makeState(status: .ready))
-                input.send(value: makeState(status: .idle))
+                input.send(value: MergeService.State.stub(status: .ready))
+                input.send(value: MergeService.State.stub(status: .idle))
 
                 scheduler.advance()