Feature Request to add finer-grained version of updated-at

[ulysses4ever]

Hello and thanks for the amazing project! I'm working with the Cabal (Haskell package manager) maintainers team to improve Mergify-based CI. Here's our current config.

Problem

Some time ago we decided we want to implement some kind of chill-out period after a PR has been "essentially accepted" and before merging. Meaning, after a discussion on a PR finishes, all suggested updates were applied and we added the "merge me" label (we use the label in our Mergify config), we wanted to give a room of, say, 2 days to any last-minute comments and concerns.

What we did is we added the updated-at<2 days ago condition. It comes close to what we want but there's an issue when a PR has to be delayed indefinitely because of rebases needed before a merge. I'm not sure how Mergify proceeds exactly, but it feels like many PRs sit for ~2 days and then some time close to the end of the 2-days waiting period Mergify rebases it according to updates on master, and the 2-days timer resets much to our despair. Here's an example of PR that has been suffered from this kind of thing recently. Don't know if it's relevant, but we sometimes have to merge manually to overcome the waiting period for some critical PRs -- I/m guessing it may add to the problem of repeated automatic rebases.

Feature Request

What may help to avoid our problem, I think, is a more fine-grained attribute than updated-at that wouldn't care about the rebases (or any updates generally, as long as they pass the workflow checks). I have two options in mind:

• a condition saying that a certain label was applied certain time in the past; something like: label-applied(merge me)<2 days ago
• a condition saying that the last comment was made at a certain period in the past, e.g.: last-commented-at<2 days ago

I think the first idea goes a bit beyond what the condition grammar currently allows. The second one though looks perfectly in line with what is currently available. I think either would work for us (better than updated-at).

Does it make sense?

[jd]

Thanks for your report @ulysses4ever The updated-at field comes from GitHub API, and indeed it gets updated when the PR is touched by Mergify, so that's probably not a good usage for this field.

I'd try this in your case: implement a 2 level approach, where you first validate the delay with a label, and then use that label as the condition to trigger the merge.

--- orig.yml    2022-08-29 13:43:24.000000000 +0200
+++ target.yml  2022-08-29 13:43:12.000000000 +0200
@@ -1,4 +1,12 @@
 pull_request_rules:
+  - actions:
+      label:
+        add:
+          - merge delay passed
+    name: Wait for 2 days before validating merge
+    conditions:
+      - updated-at<2 days ago
+
   # rebase+merge strategy
   - actions:
       queue:
@@ -11,8 +19,9 @@
     conditions:
       - base=master
       - label=merge me
+      - label=merge delay passed
       - '#approved-reviews-by>=2'
-      - updated-at<2 days ago
+
   # merge+squash strategy
   - actions:
       queue:
@@ -25,8 +34,9 @@
     conditions:
       - base=master
       - label=squash+merge me
+      - label=merge delay passed
       - '#approved-reviews-by>=2'
-      - updated-at<2 days ago
+
   # rebase+merge strategy for backports: require 1 approver instead of 2
   - actions:
       queue:

[ulysses4ever]

@jd thanks a lot for the detailed response! I love it and think that we'll give it a try (pending approval from the rest of the team).

Please, feel free to act on the issue as you see fit. I'd be fine with closing. But if you think it may be useful to leave it open, that's fine with me too.