Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ulysses4ever opened this issue Aug 25, 2022 · 2 comments
Open

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

ulysses4ever opened this issue Aug 25, 2022 · 2 comments

Comments

@ulysses4ever
Copy link

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
Copy link
Member

jd commented Aug 29, 2022

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
Copy link
Author

@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.

phlogistonjohn added a commit to phlogistonjohn/samba-operator that referenced this issue Mar 31, 2023
Previously, the mergify rules tried to define a window of time where
contributions from maintainers would not sit indefinitely waiting for
two reviews (as the maintainer contributing the patch might normally be
the one reviewing these things). Instead, we added a short cut where a
single review on a maintainer's PR would be auto merged after 15 days.
Unfortunately, this approach didn't take into account the fact that
reacting to feedback and pushing new patches "resets" the 'updated-at'
timer.

This change adds a new mergify rule to label any non-draft PR that has
been sitting unchanged for the 15 day window. If this label is applied
to a PR from a maintainer and the PR has one approving review the auto
merge will be initiated. This has a nice side benefit that the label can
be applied to important bugfixes manually to accelerate them.
Non-maintainer contributions will not automerge because of the label but
it can be used to help reviewers decide what to look at first. This
label can also be removed manually in the case the submitter or reviewer
decides that, for example, an update to the PR was big enough to warrant
resetting the time window or requiring two reviews.

Idea based on the discussion in:
Mergifyio/mergify#5036

Signed-off-by: John Mulligan <[email protected]>
phlogistonjohn added a commit to phlogistonjohn/samba-operator that referenced this issue Mar 31, 2023
Previously, the mergify rules tried to define a window of time where
contributions from maintainers would not sit indefinitely waiting for
two reviews (as the maintainer contributing the patch might normally be
the one reviewing these things). Instead, we added a short cut where a
single review on a maintainer's PR would be auto merged after 15 days.
Unfortunately, this approach didn't take into account the fact that
reacting to feedback and pushing new patches "resets" the 'updated-at'
timer.

This change adds a new mergify rule to label any non-draft PR that has
been sitting unchanged for the 15 day window. If this label is applied
to a PR from a maintainer and the PR has one approving review the auto
merge will be initiated. This has a nice side benefit that the label can
be applied to important bugfixes manually to accelerate them.
Non-maintainer contributions will not automerge because of the label but
it can be used to help reviewers decide what to look at first. This
label can also be removed manually in the case the submitter or reviewer
decides that, for example, an update to the PR was big enough to warrant
resetting the time window or requiring two reviews.

Idea based on the discussion in:
Mergifyio/mergify#5036

Signed-off-by: John Mulligan <[email protected]>
phlogistonjohn added a commit to samba-in-kubernetes/samba-operator that referenced this issue Apr 2, 2023
Previously, the mergify rules tried to define a window of time where
contributions from maintainers would not sit indefinitely waiting for
two reviews (as the maintainer contributing the patch might normally be
the one reviewing these things). Instead, we added a short cut where a
single review on a maintainer's PR would be auto merged after 15 days.
Unfortunately, this approach didn't take into account the fact that
reacting to feedback and pushing new patches "resets" the 'updated-at'
timer.

This change adds a new mergify rule to label any non-draft PR that has
been sitting unchanged for the 15 day window. If this label is applied
to a PR from a maintainer and the PR has one approving review the auto
merge will be initiated. This has a nice side benefit that the label can
be applied to important bugfixes manually to accelerate them.
Non-maintainer contributions will not automerge because of the label but
it can be used to help reviewers decide what to look at first. This
label can also be removed manually in the case the submitter or reviewer
decides that, for example, an update to the PR was big enough to warrant
resetting the time window or requiring two reviews.

Idea based on the discussion in:
Mergifyio/mergify#5036

Signed-off-by: John Mulligan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants