diff --git a/src/bors/approval_queue.rs b/src/bors/approval_queue.rs index 1fd17721..c4b70789 100644 --- a/src/bors/approval_queue.rs +++ b/src/bors/approval_queue.rs @@ -1,8 +1,7 @@ use crate::BorsContext; use crate::bors::approval::{PrCiStatus, get_pr_ci_status}; use crate::bors::comment::{ - tentative_approval_pr_ci_not_started, tentative_approval_removed_comment, - tentative_approval_timed_out_comment, + tentative_approval_removed_comment, tentative_approval_timed_out_comment, }; use crate::bors::event::WorkflowRunCompleted; use crate::bors::handlers::unapprove_pr; @@ -168,30 +167,34 @@ async fn process_tentative_approval( match pr_ci_status { PrCiStatus::NotStarted => { // Either no PR CI workflows have started yet, or GitHub is full of crap, and it claims - // that there are no workflows even if there are some. In any case, if it was too long - // since the last push, we time out the build. - let Some(head_update_time) = repo - .client - .get_pull_request_head_update_time(&gh_pr) - .await? - else { - tracing::warn!( - "Could not find the last head update for PR #{} at commit {}", - pr.number, - gh_pr.head.sha - ); - return Ok(()); - }; - if elapsed_time_since(head_update_time) >= timeout { - unapprove_pr(repo, &ctx.db, pr, &PullRequestInfo::from(gh_pr.clone())).await?; - repo.client - .post_comment( - pr.number, - tentative_approval_pr_ci_not_started(&gh_pr.head.sha, timeout), - &ctx.db, - ) - .await?; - } + // that there are no workflows even if there are some. Both cases are problematic: + // + // If no PR CI workflows have started yet, we can hit a race condition where this + // background check runs *just* after the PR was approved, and if the PR has a HEAD + // commit that was pushed a long time ago, we can immediately time out the PR CI build + // by mistake. We could alleviate that by storing the time of the last approval into the + // DB, and measuring timeouts against that, rather than against the latest push. + // But we could still be timing out by mistake if GitHub returns us wrong data (see + // below). + // + // If some PR CI workflows have started, but GitHub has some issues, it can tell us that + // there are no workflows. In this case, we can time out PR CI by mistake. It seems that + // this situation is recoverable though, so waiting for GitHub to recover might not be + // a bad thing. + // + // Rather than timing out tentatively approved PRs by mistake (which we used to do + // before), we choose the opposite trade-off: we simply don't do anything if GitHub + // returns us an empty list of workflows. + // This means that if for some reason there are absolutely no workflows started on the + // pushed commit, or if GitHub gets stuck and never returns us any workflows for the + // given HEAD SHA, the PR could stay tentatively approved forever. + // But given that such a PR will be shown on the queue page, which is checked by people + // multiple times daily, that would likely be pretty obvious, and people could then + // repush a commit to it. + // + // If this turns out to be problematic, we could start recording the approval date into + // the DB, and then time out the tentative approval if no workflows appear e.g. a whole + // day after the approval, or something like that. } PrCiStatus::Pending { started_at } => { // A pending workflow has been running for too long @@ -333,7 +336,7 @@ mod tests { } #[sqlx::test(migrator = "crate::MIGRATOR")] - async fn refresh_rejects_tentative_approval_when_ci_does_not_start(pool: sqlx::PgPool) { + async fn refresh_keeps_tentative_approval_when_ci_does_not_start(pool: sqlx::PgPool) { run_test(pool, async |ctx: &mut BorsTester| { ctx.modify_repo((), |repo| repo.default_pr_ci = false); ctx.approve(()).await?; @@ -343,31 +346,13 @@ mod tests { }) .await; - insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Commit pr-1-sha has been unapproved because PR CI has not started after `7200s`."); - ctx.pr(()).await.expect_unapproved(); + // Don't do anything, because it could be a false positive from GitHub + ctx.pr(()).await.expect_tentative_approval(); Ok(()) }) .await; } - #[sqlx::test(migrator = "crate::MIGRATOR")] - async fn refresh_confirms_tentative_approval_when_ci_is_restarted(pool: sqlx::PgPool) { - run_test(pool, async |ctx: &mut BorsTester| { - ctx.modify_repo((), |repo| repo.default_pr_ci = false); - ctx.approve(()).await?; - - with_mocked_time(Duration::from_secs(8000), async { - ctx.refresh_tentative_approvals().await; - }) - .await; - - insta::assert_snapshot!(ctx.get_next_comment_text(()).await?, @":x: Commit pr-1-sha has been unapproved because PR CI has not started after `7200s`."); - ctx.pr(()).await.expect_unapproved(); - Ok(()) - }) - .await; - } - #[sqlx::test(migrator = "crate::MIGRATOR")] async fn refresh_rejects_tentative_approval_when_ci_is_too_long(pool: sqlx::PgPool) { run_test(pool, async |ctx: &mut BorsTester| { diff --git a/src/bors/comment.rs b/src/bors/comment.rs index 4f0b2b5d..52daf04f 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -393,13 +393,6 @@ pub fn tentative_approval_timed_out_comment(commit_sha: &CommitSha, timeout: Dur )) } -pub fn tentative_approval_pr_ci_not_started(commit_sha: &CommitSha, timeout: Duration) -> Comment { - Comment::new(format!( - ":x: Commit {commit_sha} has been unapproved because PR CI has not started after `{}s`.", - timeout.as_secs() - )) -} - pub fn tentative_approval_failed_comment(commit_sha: &CommitSha) -> Comment { Comment::new(format!( ":x: Cannot approve commit {commit_sha}, because CI currently fails on this PR. Use `@bors r+ force` to override the PR CI check." diff --git a/src/bors/handlers/info.rs b/src/bors/handlers/info.rs index 8445fd49..22845a56 100644 --- a/src/bors/handlers/info.rs +++ b/src/bors/handlers/info.rs @@ -1,6 +1,7 @@ -use crate::bors::Comment; use crate::bors::RepositoryState; +use crate::bors::approval::{PrCiStatus, get_pr_ci_status}; use crate::bors::handlers::PullRequestData; +use crate::bors::{Comment, now}; use crate::database::{ApprovalStatus, MergeableState}; use crate::database::{BuildStatus, PgDbClient}; use std::sync::Arc; @@ -16,18 +17,55 @@ pub(super) async fn command_info( // Approval info match &pr.db.approval_status { - ApprovalStatus::Approved(info) => writeln!(message, "- Approved by: `{}`", info.approver)?, - ApprovalStatus::TentativelyApproved(info) => { - writeln!(message, "- Tentatively approved by: `{}`", info.approver)? - } + ApprovalStatus::Approved(info) => writeln!( + message, + "- Commit {} approved by: `{}`", + info.sha, info.approver + )?, + ApprovalStatus::TentativelyApproved(info) => writeln!( + message, + "- Commit {} tentatively approved by: `{}`", + info.sha, info.approver + )?, ApprovalStatus::NotApproved => writeln!(message, "- Not Approved")?, } + // PR CI status + let pr_ci_status = get_pr_ci_status(&repo, pr.github).await?; + write!(message, "- PR CI status: ")?; + match pr_ci_status { + PrCiStatus::NotStarted => { + writeln!(message, "not started")?; + } + PrCiStatus::Pending { started_at } => { + let now = now(); + writeln!( + message, + "pending (elapsed: {:.0}s)", + now.signed_duration_since(started_at).as_seconds_f64() + )?; + } + PrCiStatus::Success => { + writeln!(message, "successful")?; + } + PrCiStatus::Failed => { + writeln!(message, "failed")?; + } + } + // Priority info if let Some(priority) = pr.db.priority { writeln!(message, "- Priority: {priority}")?; - } else { - writeln!(message, "- Priority: unset")?; + } + + // Rollup mode + if let Some(rollup) = pr.db.rollup { + writeln!(message, "- Rollup mode: {rollup}")?; + } + + // Note + if let Some(note) = pr.db.note() { + writeln!(message, "- Note: {note}")?; } // Mergeability state @@ -42,12 +80,26 @@ pub(super) async fn command_info( )?; // Try build status - if let Some(try_build) = &pr.db.try_build - && try_build.status == BuildStatus::Pending + if let Some(build) = &pr.db.try_build + && build.status == BuildStatus::Pending { writeln!(message, "- Try build is in progress")?; - if let Ok(urls) = db.get_workflow_urls_for_build(try_build).await { + if let Ok(urls) = db.get_workflow_urls_for_build(build).await { + message.extend( + urls.into_iter() + .map(|url| format!("\t- Workflow URL: {url}")), + ); + } + } + + // Unrolled build status + if let Some(build) = &pr.db.unrolled_build + && build.status == BuildStatus::Pending + { + writeln!(message, "- Unrolled build is in progress")?; + + if let Ok(urls) = db.get_workflow_urls_for_build(build).await { message.extend( urls.into_iter() .map(|url| format!("\t- Workflow URL: {url}")), @@ -102,7 +154,7 @@ mod tests { @" ## Status of PR `1` - Not Approved - - Priority: unset + - PR CI status: successful - Mergeable: yes " ); @@ -121,8 +173,8 @@ mod tests { ctx.get_next_comment_text(()).await?, @" ## Status of PR `1` - - Approved by: `default-user` - - Priority: unset + - Commit pr-1-sha approved by: `default-user` + - PR CI status: successful - Mergeable: yes " ); @@ -146,8 +198,8 @@ mod tests { ctx.get_next_comment_text(()).await?, @" ## Status of PR `1` - - Tentatively approved by: `default-user` - - Priority: unset + - Commit pr-1-sha tentatively approved by: `default-user` + - PR CI status: pending (elapsed: 0s) - Mergeable: yes " ); @@ -168,6 +220,7 @@ mod tests { @" ## Status of PR `1` - Not Approved + - PR CI status: successful - Priority: 5 - Mergeable: yes " @@ -189,7 +242,7 @@ mod tests { @" ## Status of PR `1` - Not Approved - - Priority: unset + - PR CI status: successful - Mergeable: yes - Try build is in progress " @@ -216,7 +269,8 @@ mod tests { ctx.get_next_comment_text(()).await?, @" ## Status of PR `1` - - Approved by: `default-user` + - Commit pr-1-sha approved by: `default-user` + - PR CI status: successful - Priority: 10 - Mergeable: yes - Try build is in progress @@ -240,8 +294,8 @@ mod tests { ctx.get_next_comment_text(()).await?, @" ## Status of PR `1` - - Approved by: `default-user` - - Priority: unset + - Commit pr-1-sha approved by: `default-user` + - PR CI status: successful - Mergeable: yes - Auto build has been cancelled " diff --git a/src/github/api/client.rs b/src/github/api/client.rs index 5e8cb0b4..58f2b640 100644 --- a/src/github/api/client.rs +++ b/src/github/api/client.rs @@ -5,9 +5,7 @@ use chrono::{DateTime, Utc}; use octocrab::Octocrab; use octocrab::models::checks::CheckRun; use octocrab::models::pulls::MergeableState; -use octocrab::models::repos::ActivityType; use octocrab::models::{CheckRunId, Repository, RunId, RunnerGroupId, UserId}; -use octocrab::params::Direction; use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus}; use serde::{Deserialize, Serialize}; use std::fmt::Debug; @@ -266,50 +264,6 @@ impl GithubRepositoryClient { Ok(prs) } - /// Return when the pull request's current head was last updated. - pub async fn get_pull_request_head_update_time( - &self, - pr: &PullRequest, - ) -> anyhow::Result>> { - let head_repository = pr.head_repository.as_ref().unwrap_or(&self.repo_name); - let head_branch = pr.head.name.clone(); - let head_sha = pr.head.sha.clone(); - - let update_time = perform_retryable( - "get_pull_request_head_update_time", - RetryMethod::default(), - || async { - let activities = self - .client - .repos(head_repository.owner(), head_repository.name()) - .list_activities() - .direction(Direction::Descending) - .per_page(100) - .git_ref(&head_branch) - .send() - .await?; - - anyhow::Ok( - activities - .items - .into_iter() - .filter(|activity| { - matches!( - activity.activity_type, - ActivityType::Push - | ActivityType::ForcePush - | ActivityType::BranchCreation - ) && activity.after == head_sha.as_ref() - }) - .map(|activity| activity.timestamp) - .max(), - ) - }, - ) - .await?; - Ok(update_time) - } - pub async fn get_pull_request_commits( &self, number: PullRequestNumber, @@ -401,7 +355,7 @@ impl GithubRepositoryClient { branch: &str, sha: &CommitSha, force: ForcePush, - ) -> Result<(), crate::github::api::operations::BranchUpdateError> { + ) -> Result<(), BranchUpdateError> { perform_retryable("set_branch_to_sha", RetryMethod::default(), || async { set_branch_to_commit(self, branch.to_string(), sha, force) .await