Auto merge of #35 - troy/fix-merge-status, r=lennartkloock

fix: pr temporarily going into unknown status canceling runs
After a change in BASE github puts a PR in an `unknown` mergeable state, we shouldn't cancel the run if its in this state.

ScuffleCloud/scuffle#244 (comment)

Is where it was observed when merging multiple PRs concurrently.

Requested-by: lennartkloock <[email protected]>
Reviewed-by: lennartkloock <[email protected]>

server/src/database/pr.rs

@@ -103,15 +103,14 @@ fn pr_status(pr: &PullRequest) -> GithubPrStatus {
     }
 }
 
-fn pr_merge_status(pr: &PullRequest) -> GithubPrMergeStatus {
+fn pr_merge_status(pr: &PullRequest) -> Option<GithubPrMergeStatus> {
     match pr.mergeable_state {
-        _ if pr.merged_at.is_some() => GithubPrMergeStatus::Merged,
-        Some(MergeableState::Behind | MergeableState::Blocked | MergeableState::Clean) => GithubPrMergeStatus::Ready,
-        Some(MergeableState::Dirty) => GithubPrMergeStatus::Conflict,
-        Some(MergeableState::Unstable) => GithubPrMergeStatus::CheckFailure,
-        Some(MergeableState::Unknown) => GithubPrMergeStatus::NotReady,
-        Some(MergeableState::Draft) => GithubPrMergeStatus::NotReady,
-        _ => GithubPrMergeStatus::NotReady,
+        _ if pr.merged_at.is_some() => Some(GithubPrMergeStatus::Merged),
+        Some(MergeableState::Behind | MergeableState::Blocked | MergeableState::Clean) => Some(GithubPrMergeStatus::Ready),
+        Some(MergeableState::Dirty) => Some(GithubPrMergeStatus::Conflict),
+        Some(MergeableState::Unstable) => Some(GithubPrMergeStatus::CheckFailure),
+        Some(MergeableState::Draft) => Some(GithubPrMergeStatus::NotReady),
+        _ => None,
     }
 }
 
@@ -130,7 +129,7 @@ impl<'a> Pr<'a> {
             github_pr_number: pr.number as i32,
             title: Cow::Borrowed(&pr.title),
             body: Cow::Borrowed(&pr.body),
-            merge_status: pr_merge_status(pr),
+            merge_status: pr_merge_status(pr).unwrap_or(GithubPrMergeStatus::NotReady),
             author_id: pr.user.as_ref().map(|u| u.id.0 as i64).unwrap_or(user_id.0 as i64),
             assigned_ids: pr_assigned_ids(pr),
             status: pr_status(pr),
@@ -201,7 +200,9 @@ impl<'a> Pr<'a> {
         UpdatePr::builder(self.github_repo_id, self.github_pr_number)
             .maybe_title(self.title.ne(&title).then_some(title))
             .maybe_body(self.body.ne(&body).then_some(body))
-            .maybe_merge_status(self.merge_status.ne(&merge_status).then_some(merge_status))
+            .maybe_merge_status(
+                merge_status.and_then(|merge_status| self.merge_status.ne(&merge_status).then_some(merge_status)),
+            )
             .maybe_assigned_ids(self.assigned_ids.ne(&assigned_ids).then_some(assigned_ids))
             .maybe_status(self.status.ne(&status).then_some(status))
             .maybe_merge_commit_sha(self.merge_commit_sha.ne(&merge_commit_sha).then_some(merge_commit_sha))
@@ -669,13 +670,13 @@ mod tests {
     #[test]
     fn test_pr_merge_status_from_pr() {
         let cases = [
-            (MergeableState::Behind, GithubPrMergeStatus::Ready),
-            (MergeableState::Blocked, GithubPrMergeStatus::Ready),
-            (MergeableState::Clean, GithubPrMergeStatus::Ready),
-            (MergeableState::Dirty, GithubPrMergeStatus::Conflict),
-            (MergeableState::Unstable, GithubPrMergeStatus::CheckFailure),
-            (MergeableState::Unknown, GithubPrMergeStatus::NotReady),
-            (MergeableState::Draft, GithubPrMergeStatus::NotReady),
+            (MergeableState::Behind, Some(GithubPrMergeStatus::Ready)),
+            (MergeableState::Blocked, Some(GithubPrMergeStatus::Ready)),
+            (MergeableState::Clean, Some(GithubPrMergeStatus::Ready)),
+            (MergeableState::Dirty, Some(GithubPrMergeStatus::Conflict)),
+            (MergeableState::Unstable, Some(GithubPrMergeStatus::CheckFailure)),
+            (MergeableState::Draft, Some(GithubPrMergeStatus::NotReady)),
+            (MergeableState::Unknown, None),
         ];
 
         for (state, status) in cases {
@@ -693,7 +694,7 @@ mod tests {
                 merged_at: Some(chrono::Utc::now()),
                 ..Default::default()
             }),
-            GithubPrMergeStatus::Merged
+            Some(GithubPrMergeStatus::Merged)
         );
     }
 

server/src/webhook/pull_request.rs

@@ -81,6 +81,7 @@ mod tests {
     use std::sync::Arc;
 
     use chrono::Utc;
+    use octocrab::models::pulls::MergeableState;
     use octocrab::models::UserId;
 
     use super::*;
@@ -374,4 +375,67 @@ mod tests {
         assert!(run.is_some(), "Run was cancelled");
         assert!(!AtomicBool::load(&mock.cancel, std::sync::atomic::Ordering::Relaxed));
     }
+
+    #[tokio::test]
+    async fn test_pr_merge_status_unknown() {
+        let mut conn = get_test_connection().await;
+        let mock = MockMergeWorkFlow::default();
+        let (client, _) = MockRepoClient::new(mock.clone());
+
+        let pr = PullRequest {
+            number: 1,
+            title: "test".to_string(),
+            body: "test".to_string(),
+            mergeable_state: Some(MergeableState::Clean),
+            ..Default::default()
+        };
+
+        Pr::new(&pr, UserId(1), client.id())
+            .insert()
+            .execute(&mut conn)
+            .await
+            .unwrap();
+
+        CiRun::insert(client.id(), pr.number)
+            .base_ref(Base::from_sha("base"))
+            .head_commit_sha(Cow::Borrowed("head"))
+            .ci_branch(Cow::Borrowed("ci"))
+            .requested_by_id(1)
+            .approved_by_ids(vec![])
+            .is_dry_run(false)
+            .build()
+            .query()
+            .get_result(&mut conn)
+            .await
+            .unwrap();
+
+        let task = tokio::spawn(async move {
+            handle_with_pr(
+                &client,
+                &mut conn,
+                PullRequest {
+                    number: 1,
+                    title: "test".to_string(),
+                    body: "test".to_string(),
+                    mergeable_state: Some(MergeableState::Unknown),
+                    ..Default::default()
+                },
+                User {
+                    id: UserId(1),
+                    login: "test".to_string(),
+                },
+            )
+            .await
+            .unwrap();
+
+            (conn, client)
+        });
+
+        let (mut conn, client) = task.await.unwrap();
+
+        let run = CiRun::active(client.id(), 1).get_result(&mut conn).await.optional().unwrap();
+
+        assert!(run.is_some(), "Run was cancelled");
+        assert!(!AtomicBool::load(&mock.cancel, std::sync::atomic::Ordering::Relaxed));
+    }
 }