| 8 | 8 | | //! branch's rules ask for is met, attributed to `g1t` (`POLICY_ACTOR_ID`). |
| 9 | 9 | | //! |
| 10 | 10 | | //! A merge asked for while a pull request is behind is recorded here |
| 11 | | − | //! (`request_landing`) and lands once the push that brings it up to date |
| 12 | | − | //! arrives (`land_if_requested`). |
| 11 | + | //! (`request_landing`). Once the push that brings it up to date arrives |
| 12 | + | //! (`land_if_requested`), it lands when the checks the branch requires |
| 13 | + | //! pass on that new commit, as they must for any merge (`land_when_checked`, |
| 14 | + | //! also called as each of the commit's checks finishes). |
| 13 | 15 | | |
| 14 | 16 | | use g1t_contracts::repos::{GetByIdArgs, Repo, RepoPath}; |
| 15 | 17 | | use g1t_contracts::time::rfc3339; |
| ⋯ |
| 31 | 33 | | /// How long a claimed step is waited for before it may be taken again. |
| 32 | 34 | | const CATCH_UP_MINUTES: u64 = 30; |
| 33 | 35 | | const MERGE_MINUTES: u64 = 2; |
| 36 | + | /// How long a merge that was brought up to date waits for the required |
| 37 | + | /// checks on the new commit; after that it is no longer pending. |
| 38 | + | const CHECKS_MINUTES: u64 = 180; |
| 39 | + | /// The step under way while it does (`working_on`). |
| 40 | + | const WAITING_FOR_CHECKS: &str = "checks"; |
| 41 | + | |
| 42 | + | /// How long a merge waits on the repository service for one write (the |
| 43 | + | /// catch-up push, the landing): long enough for a large merge, short |
| 44 | + | /// enough that the person who clicked hears back well before the site |
| 45 | + | /// stops waiting on the merge (apps/web/app/lib/deadline.ts, 60 s). |
| 46 | + | pub(crate) const REPOS_WRITE_WITHIN: std::time::Duration = std::time::Duration::from_secs(25); |
| 47 | + | |
| 48 | + | /// What a merge says when the repository service did not answer in time. |
| 49 | + | pub(crate) fn repository_unreachable<T>() -> Outcome<T> { |
| 50 | + | Outcome::fail( |
| 51 | + | FailureCode::Unavailable, |
| 52 | + | "g1t couldn't reach the repository to merge this. Try again in a moment.", |
| 53 | + | ) |
| 54 | + | } |
| 55 | + | |
| 56 | + | /// What becomes of a merge that was brought up to date, by the checks the |
| 57 | + | /// branch requires on the commit that did it. |
| 58 | + | #[derive(Debug, PartialEq)] |
| 59 | + | pub(crate) enum Landing { |
| 60 | + | /// None are required, or all of them passed. |
| 61 | + | Now, |
| 62 | + | /// Some are still running or have not reported yet. |
| 63 | + | Wait, |
| 64 | + | /// One failed: a person has to push a fix. |
| 65 | + | Stall(String), |
| 66 | + | } |
| 67 | + | |
| 68 | + | /// Whether a merge that was brought up to date lands now, by its new |
| 69 | + | /// commit's checks. The same checks hold it as hold any merge |
| 70 | + | /// (`merge_gate`): landing it before they report would only be refused. |
| 71 | + | pub(crate) fn landing_step(facts: &WorkflowFacts) -> Landing { |
| 72 | + | let failed = facts.required_failed(); |
| 73 | + | if !failed.is_empty() { |
| 74 | + | let one = failed.len() == 1; |
| 75 | + | return Landing::Stall(format!( |
| 76 | + | "It was brought up to date, but the required {} {} failed on that commit, so it was not merged. Push a fix, then merge again.", |
| 77 | + | if one { "check" } else { "checks" }, |
| 78 | + | statuses::list(&failed), |
| 79 | + | )); |
| 80 | + | } |
| 81 | + | if facts.required.iter().any(|check| check.state != RequiredState::Success) { |
| 82 | + | return Landing::Wait; |
| 83 | + | } |
| 84 | + | Landing::Now |
| 85 | + | } |
| 86 | + | |
| 34 | 87 | | /// Who a merge made by a repository's settings is attributed to. Not an |
| 35 | 88 | | /// account: `g1t` cannot be registered. |
| 36 | 89 | | pub(crate) const POLICY_ACTOR_ID: &str = "g1t_policy"; |
| ⋯ |
| 59 | 112 | | /// when absent. |
| 60 | 113 | | #[serde(default)] |
| 61 | 114 | | delete_branch: Option<bool>, |
| 115 | + | /// The merger asked to merge past the checks, and the repository |
| 116 | + | /// allows that: it lands without waiting for them. |
| 117 | + | #[serde(default)] |
| 118 | + | ignore_checks: bool, |
| 62 | 119 | | } |
| 63 | 120 | | |
| 64 | 121 | | #[derive(Deserialize)] |
| ⋯ |
| 66 | 123 | | land_requested: Option<String>, |
| 67 | 124 | | land_requested_at: Option<String>, |
| 68 | 125 | | stalled: Option<String>, |
| 126 | + | #[serde(default)] |
| 127 | + | working_on: Option<String>, |
| 69 | 128 | | } |
| 70 | 129 | | |
| 71 | 130 | | /// What should happen next, if it is g1t's turn. |
| ⋯ |
| 163 | 222 | | return wait(Stage::CatchingUp, "Bringing it up to date with the branch it will land on."); |
| 164 | 223 | | } |
| 165 | 224 | | Some("merge") => return wait(Stage::Ready, "Merging."), |
| 225 | + | Some(WAITING_FOR_CHECKS) => { |
| 226 | + | return wait( |
| 227 | + | Stage::Checking, |
| 228 | + | "Brought up to date. It lands once the required checks pass on that commit.", |
| 229 | + | ); |
| 230 | + | } |
| 166 | 231 | | _ => {} |
| 167 | 232 | | } |
| 168 | 233 | | if let Some(name) = &facts.person_request { |
| ⋯ |
| 526 | 591 | | actor: &User, |
| 527 | 592 | | keep_issue_open: bool, |
| 528 | 593 | | delete_branch: Option<bool>, |
| 594 | + | ignore_checks: bool, |
| 529 | 595 | | ) -> Result<()> { |
| 530 | 596 | | let now = now_ms(); |
| 531 | 597 | | let request = serde_json::to_string(&LandRequest { |
| 532 | 598 | | actor: actor.clone(), |
| 533 | 599 | | keep_issue_open, |
| 534 | 600 | | delete_branch, |
| 601 | + | ignore_checks, |
| 535 | 602 | | })?; |
| 536 | 603 | | self.db |
| 537 | 604 | | .prepare( |
| ⋯ |
| 561 | 628 | | /// enough to still stand. |
| 562 | 629 | | async fn land_request(&self, pull_id: &str) -> Result<Option<LandRequest>> { |
| 563 | 630 | | let row = self.land_row(pull_id).await?; |
| 564 | | − | let oldest = rfc3339(now_ms().saturating_sub(CATCH_UP_MINUTES * 60 * 1000)); |
| 631 | + | let now = now_ms(); |
| 565 | 632 | | Ok(row |
| 566 | 633 | | .filter(|row| { |
| 634 | + | // Waiting for checks on the caught-up commit stands longer |
| 635 | + | // than waiting for the push. |
| 636 | + | let minutes = if row.working_on.as_deref() == Some(WAITING_FOR_CHECKS) { |
| 637 | + | CHECKS_MINUTES |
| 638 | + | } else { |
| 639 | + | CATCH_UP_MINUTES |
| 640 | + | }; |
| 641 | + | let oldest = rfc3339(now.saturating_sub(minutes * 60 * 1000)); |
| 567 | 642 | | row.land_requested_at |
| 568 | 643 | | .as_deref() |
| 569 | 644 | | .is_some_and(|at| at >= oldest.as_str()) |
| ⋯ |
| 586 | 661 | | return found.first::<LandRow>(Slot::Pull); |
| 587 | 662 | | } |
| 588 | 663 | | self.db |
| 589 | | − | .prepare("SELECT land_requested, land_requested_at, stalled FROM pulls WHERE id = ?") |
| 664 | + | .prepare("SELECT land_requested, land_requested_at, stalled, working_on FROM pulls WHERE id = ?") |
| 590 | 665 | | .bind(&[pull_id.into()])? |
| 591 | 666 | | .first::<LandRow>(None) |
| 592 | 667 | | .await |
| 593 | 668 | | } |
| 594 | 669 | | |
| 595 | 670 | | /// Forgets a merge that was waiting on a pull request, and the step |
| 596 | | − | /// that was bringing it up to date. |
| 671 | + | /// that was bringing it up to date or waiting for its checks. |
| 597 | 672 | | pub(crate) async fn forget_landing(&self, pull_id: &str) -> Result<()> { |
| 598 | | − | self.db |
| 673 | + | self.take_landing(pull_id).await?; |
| 674 | + | Ok(()) |
| 675 | + | } |
| 676 | + | |
| 677 | + | /// Forgets a merge that was waiting on a pull request and returns it, |
| 678 | + | /// in one statement, so that of two callers deciding to land it at |
| 679 | + | /// once (two checks finishing together) only one does. |
| 680 | + | async fn take_landing(&self, pull_id: &str) -> Result<Option<LandRequest>> { |
| 681 | + | // SET sees the row as it was: the CASEs read the old `working_on`. |
| 682 | + | let taken = self |
| 683 | + | .db |
| 599 | 684 | | .prepare( |
| 600 | 685 | | "UPDATE pulls SET land_requested = NULL, land_requested_at = NULL, |
| 601 | | − | working_on = CASE WHEN working_on = 'catch_up' THEN NULL ELSE working_on END, |
| 602 | | − | working_until = CASE WHEN working_on = 'catch_up' THEN NULL ELSE working_until END |
| 603 | | − | WHERE id = ?", |
| 686 | + | working_on = CASE WHEN working_on IN ('catch_up', 'checks') THEN NULL ELSE working_on END, |
| 687 | + | working_until = CASE WHEN working_on IN ('catch_up', 'checks') THEN NULL ELSE working_until END |
| 688 | + | WHERE id = ? AND land_requested IS NOT NULL |
| 689 | + | RETURNING land_requested AS value", |
| 604 | 690 | | ) |
| 605 | 691 | | .bind(&[pull_id.into()])? |
| 692 | + | .first::<ValueRow>(None) |
| 693 | + | .await?; |
| 694 | + | Ok(taken.and_then(|row| serde_json::from_str(&row.value).ok())) |
| 695 | + | } |
| 696 | + | |
| 697 | + | /// Notes that a merge which was brought up to date waits for the |
| 698 | + | /// required checks on its new commit, from now: on the first look at |
| 699 | + | /// this commit (a later push clears the step and starts it again). |
| 700 | + | async fn wait_for_checks(&self, pull_id: &str) -> Result<()> { |
| 701 | + | let now = now_ms(); |
| 702 | + | self.db |
| 703 | + | .prepare( |
| 704 | + | "UPDATE pulls SET working_on = 'checks', working_until = ?1, land_requested_at = ?2 |
| 705 | + | WHERE id = ?3 AND land_requested IS NOT NULL AND status = 'open' |
| 706 | + | AND (working_on IS NULL OR working_on <> 'checks')", |
| 707 | + | ) |
| 708 | + | .bind(&[ |
| 709 | + | rfc3339(now + CHECKS_MINUTES * 60 * 1000).into(), |
| 710 | + | rfc3339(now).into(), |
| 711 | + | pull_id.into(), |
| 712 | + | ])? |
| 606 | 713 | | .run() |
| 607 | 714 | | .await?; |
| 608 | 715 | | Ok(()) |
| 609 | 716 | | } |
| 610 | 717 | | |
| 611 | | − | /// Lands a pull request whose head has just moved, if a merge of it was |
| 612 | | − | /// waiting for exactly that. What it was caught up to was already |
| 613 | | − | /// checked and reviewed apart from the merge, so the checks are not |
| 614 | | − | /// waited for again; a repository that wants them rerun turns on |
| 615 | | − | /// "require up to date", and then nothing is landed this way. |
| 718 | + | /// Moves on a merge that was waiting for a pull request's head to move |
| 719 | + | /// (the push that brought it up to date): it lands once the checks the |
| 720 | + | /// branch requires pass on that commit ([`Work::land_when_checked`]). |
| 721 | + | /// The commit is new, so they are waited for, as for any merge: a |
| 722 | + | /// ruleset that requires them refuses a merge that skips them. |
| 616 | 723 | | pub(crate) async fn land_if_requested(&self, pull_id: &str) -> Result<()> { |
| 724 | + | if self.land_request(pull_id).await?.is_none() { |
| 725 | + | return Ok(()); |
| 726 | + | } |
| 727 | + | self.land_when_checked(pull_id).await |
| 728 | + | } |
| 729 | + | |
| 730 | + | /// Lands a pull request that was brought up to date for a merge, once |
| 731 | + | /// the checks the branch requires pass on its head: at once when none |
| 732 | + | /// are required, never when one failed (it stops and says which), and |
| 733 | + | /// otherwise when the last of them reports (statuses.rs `store_status` |
| 734 | + | /// calls this as the commit's checks finish). |
| 735 | + | pub(crate) async fn land_when_checked(&self, pull_id: &str) -> Result<()> { |
| 617 | 736 | | let Some(request) = self.land_request(pull_id).await? else { |
| 618 | 737 | | return Ok(()); |
| 619 | 738 | | }; |
| 620 | | − | self.forget_landing(pull_id).await?; |
| 621 | 739 | | let Some(pull) = self.pull_by_id(pull_id).await? else { |
| 622 | 740 | | return Ok(()); |
| 623 | 741 | | }; |
| 742 | + | if pull.status != PullStatus::Open { |
| 743 | + | return self.forget_landing(pull_id).await; |
| 744 | + | } |
| 624 | 745 | | let repo: Outcome<Repo> = g1t_kit::call( |
| 625 | 746 | | &self.repos, |
| 626 | 747 | | "get_by_id", |
| ⋯ |
| 631 | 752 | | ) |
| 632 | 753 | | .await?; |
| 633 | 754 | | let Outcome::Ok(repo) = crate::retired::unless_archived(repo) else { |
| 755 | + | return self.forget_landing(pull_id).await; |
| 756 | + | }; |
| 757 | + | let step = if request.ignore_checks { |
| 758 | + | Landing::Now |
| 759 | + | } else { |
| 760 | + | let settings = self.settings_on(&repo, &pull).await?; |
| 761 | + | let statuses = self.statuses(&pull.repo_id, pull.head_commit.as_deref()).await?; |
| 762 | + | let facts = self |
| 763 | + | .workflow_facts(&pull.repo_id, pull.head_commit.as_deref(), &statuses, &settings.required_checks) |
| 764 | + | .await?; |
| 765 | + | landing_step(&facts) |
| 766 | + | }; |
| 767 | + | match step { |
| 768 | + | Landing::Wait => return self.wait_for_checks(pull_id).await, |
| 769 | + | Landing::Stall(reason) => { |
| 770 | + | self.stall(StallArgs { |
| 771 | + | pull_id: pull.id, |
| 772 | + | reason, |
| 773 | + | by: None, |
| 774 | + | }) |
| 775 | + | .await?; |
| 776 | + | return Ok(()); |
| 777 | + | } |
| 778 | + | Landing::Now => {} |
| 779 | + | } |
| 780 | + | // Taken by one caller: two checks finishing together land it once. |
| 781 | + | let Some(request) = self.take_landing(pull_id).await? else { |
| 634 | 782 | | return Ok(()); |
| 635 | 783 | | }; |
| 636 | 784 | | let merged = self |
| ⋯ |
| 643 | 791 | | number: pull.number, |
| 644 | 792 | | summary: String::new(), |
| 645 | 793 | | keep_issue_open: request.keep_issue_open, |
| 646 | | − | ignore_checks: true, |
| 794 | + | ignore_checks: request.ignore_checks, |
| 647 | 795 | | bypass_rules: false, |
| 648 | 796 | | // Only a fast-forward waits to be brought up to date. |
| 649 | 797 | | merge_method: Some(g1t_contracts::rules::MergeMethod::FastForward), |
| ⋯ |
| 870 | 1018 | | } |
| 871 | 1019 | | |
| 872 | 1020 | | #[test] |
| 1021 | + | fn a_merge_brought_up_to_date_lands_once_its_required_checks_pass_on_the_new_commit() { |
| 1022 | + | // The catch-up's merge commit, just pushed: nothing has reported yet. |
| 1023 | + | assert_eq!(landing_step(&checks(&[], &["CI"])), Landing::Wait); |
| 1024 | + | assert_eq!(landing_step(&checks(&[("CI / pull_request", "pending")], &["CI"])), Landing::Wait); |
| 1025 | + | assert_eq!(landing_step(&checks(&[("CI / pull_request", "success")], &["CI"])), Landing::Now); |
| 1026 | + | // Every required one, not just the first to report. |
| 1027 | + | assert_eq!( |
| 1028 | + | landing_step(&checks(&[("CI / pull_request", "success")], &["CI", "Deploy"])), |
| 1029 | + | Landing::Wait |
| 1030 | + | ); |
| 1031 | + | // Nothing required: it lands at once, whatever else runs or failed. |
| 1032 | + | assert_eq!(landing_step(&checks(&[], &[])), Landing::Now); |
| 1033 | + | assert_eq!(landing_step(&checks(&[("Lint / pull_request", "failure")], &[])), Landing::Now); |
| 1034 | + | assert_eq!( |
| 1035 | + | landing_step(&checks(&[("CI / pull_request", "success"), ("Lint / pull_request", "failure")], &["CI"])), |
| 1036 | + | Landing::Now |
| 1037 | + | ); |
| 1038 | + | // A required one that failed stops it, saying which. |
| 1039 | + | let Landing::Stall(reason) = landing_step(&checks(&[("CI / pull_request", "failure")], &["CI"])) else { |
| 1040 | + | panic!("a failing required check must stop it"); |
| 1041 | + | }; |
| 1042 | + | assert_eq!( |
| 1043 | + | reason, |
| 1044 | + | "It was brought up to date, but the required check CI failed on that commit, so it was not merged. Push a fix, then merge again." |
| 1045 | + | ); |
| 1046 | + | let Landing::Stall(reason) = |
| 1047 | + | landing_step(&checks(&[("CI / push", "error"), ("Deploy", "failure")], &["CI", "Deploy"])) |
| 1048 | + | else { |
| 1049 | + | panic!("failing required checks must stop it"); |
| 1050 | + | }; |
| 1051 | + | assert!(reason.contains("required checks CI and Deploy failed"), "{reason}"); |
| 1052 | + | } |
| 1053 | + | |
| 1054 | + | #[test] |
| 1055 | + | fn a_merge_that_cannot_reach_the_repository_says_so() { |
| 1056 | + | let Outcome::Fail(failure) = repository_unreachable::<()>() else { |
| 1057 | + | panic!("it must fail"); |
| 1058 | + | }; |
| 1059 | + | assert_eq!(failure.code, FailureCode::Unavailable); |
| 1060 | + | assert!(failure.message.contains("couldn't reach the repository")); |
| 1061 | + | assert!(failure.message.contains("Try again")); |
| 1062 | + | // Well inside the site's own 60 s wait on a merge, catch-up and landing both. |
| 1063 | + | assert!(REPOS_WRITE_WITHIN.as_secs() * 2 < 60); |
| 1064 | + | } |
| 1065 | + | |
| 1066 | + | #[test] |
| 1067 | + | fn a_landing_asked_for_before_it_carried_ignore_checks_still_reads() { |
| 1068 | + | let old: LandRequest = serde_json::from_value(serde_json::json!({ |
| 1069 | + | "actor": serde_json::to_value(User::default()).unwrap(), |
| 1070 | + | "keepIssueOpen": true, |
| 1071 | + | })) |
| 1072 | + | .unwrap(); |
| 1073 | + | assert!(old.keep_issue_open && !old.ignore_checks); |
| 1074 | + | } |
| 1075 | + | |
| 1076 | + | #[test] |
| 873 | 1077 | | fn drafts_steps_under_way_and_stops_wait() { |
| 1078 | + | assert_eq!( |
| 1079 | + | outcome(Facts { working_on: Some(WAITING_FOR_CHECKS.into()), auto_merge: true, ..facts() }), |
| 1080 | + | (Stage::Checking, "wait") |
| 1081 | + | ); |
| 874 | 1082 | | assert_eq!(outcome(Facts { draft: true, ..facts() }), (Stage::Working, "wait")); |
| 875 | 1083 | | assert_eq!(outcome(Facts { working_on: Some("catch_up".into()), auto_merge: true, ..facts() }), (Stage::CatchingUp, "wait")); |
| 876 | 1084 | | assert_eq!(outcome(Facts { working_on: Some("merge".into()), auto_merge: true, ..facts() }), (Stage::Ready, "wait")); |
