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

[ mergify ] require 2 days to pass after last update of PR to merge it #8285

Merged
merged 2 commits into from
Jul 21, 2022

Conversation

ulysses4ever
Copy link
Collaborator

@ulysses4ever ulysses4ever commented Jul 12, 2022

Per discussion on IRC. Some members of the team expressed a concern that PRs sometimes get merged too fast. A chillout time after review was proposed as a countermeasure. This PR implements 2 day chillout period after last update of any PR against master (so, backports are not included in this new policy).


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@Mikolaj
Copy link
Member

Mikolaj commented Jul 12, 2022

I wonder if this change affects the very PR. Does mergify take its config from master or from current branch?

@ulysses4ever
Copy link
Collaborator Author

@Mikolaj I'd expect from master

@Mikolaj
Copy link
Member

Mikolaj commented Jul 12, 2022

BTW, if this works, let's not forget to add in the next PR an explanation to CONTRIBUTE so that people puzzled by new behaviour have a fighting chance. Also, ask devs there to use the mergify labels instead of big red buttons.

@Mikolaj
Copy link
Member

Mikolaj commented Jul 12, 2022

@Mikolaj I'd expect from master

OK, so we will learn by observing if this PR broke everything.

@ulysses4ever
Copy link
Collaborator Author

Yes, unfortunately, I don't see any other way.

@jneira
Copy link
Member

jneira commented Jul 12, 2022

Yes, unfortunately, I don't see any other way.

well making the experiment in a fork before merging upstream 😄
i usually pollute my fork master to do ci experiments

hmm maybe making pr against the branch with the mergify config could work too (but in the fork in any case)

@Mikolaj
Copy link
Member

Mikolaj commented Jul 12, 2022

@jneira: don't kill the suspense ;P

But isn't mergify installation somehow org-wide (Haskell org in this case)? Does it really work fine in a fork? I've never tried.

@ulysses4ever
Copy link
Collaborator Author

Right. I can set up Mergify against my fork, I guess.

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Jul 12, 2022

Can't self approve... I invited @jneira to help with that: ...

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Jul 12, 2022

@jneira actually, this one: ulysses4ever#3

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Jul 12, 2022

Actually, disregard that, I'll just kill the line about number of approvers in the Margify config.

@ulysses4ever
Copy link
Collaborator Author

Hmm, the experiment failed spectacularly: it merged it right away without waiting ulysses4ever#5 Will have to look into it...

@ulysses4ever
Copy link
Collaborator Author

Okay, I think I got it: I got the inequality in "updated-at>=2 days ago" the other way round (>= means after that moment in time, and we want before). Waiting for the new test PR to be merged in 30 minutes after merge-me label applied ulysses4ever#7 after I will say I'm confident this should work.

@ulysses4ever
Copy link
Collaborator Author

Yes, worked fine (only I used 30 mins instead of 2 days chillout)

2022-07-12_20-07-1657673448

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we already know this works, could we add the CONTRIBUTING explanations already in this PR? I'd help.

@ulysses4ever ulysses4ever force-pushed the mergify-two-days-chillout branch from 88ff5d0 to bd387af Compare July 13, 2022 13:40
@ulysses4ever
Copy link
Collaborator Author

@Mikolaj CHANGELOG updated

@ulysses4ever
Copy link
Collaborator Author

Arghhh, not changelog, but contributing.md

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot. My suggestions. Feel free to ignore/edit.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@ulysses4ever
Copy link
Collaborator Author

I forgot to mention the squash+merge me label. Will have to get back to it later today.

@ulysses4ever ulysses4ever force-pushed the mergify-two-days-chillout branch 2 times, most recently from ccdfcc0 to e630f04 Compare July 13, 2022 14:47
@ulysses4ever ulysses4ever force-pushed the mergify-two-days-chillout branch from e630f04 to b54e00a Compare July 13, 2022 14:48
@ulysses4ever
Copy link
Collaborator Author

Added a sentence about squash, check it out...

@Mikolaj
Copy link
Member

Mikolaj commented Jul 13, 2022

Added a sentence about squash, check it out...

It doesn't sound as an absolute requirement, so it's fine IMHO.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

many thanks, I did not follow the IRC discussion, could we paste/mention it briefly? i cant think why we want to delay merging even more (they usually takes quite time!)

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Jul 14, 2022

@jneira some members of the team expressed a concern that PRs sometimes get merged too fast, and the chillout time after review was proposed as a countermeasure.

@Mikolaj Mikolaj requested a review from gbaz July 15, 2022 15:02
@Mikolaj Mikolaj requested a review from emilypi July 18, 2022 13:18
Copy link
Member

@emilypi emilypi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is pretty fair

@Mikolaj
Copy link
Member

Mikolaj commented Jul 21, 2022

2 days have passed. Time to merge?

@ulysses4ever ulysses4ever added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Jul 21, 2022
@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Jul 21, 2022

Oh wow

Error: All install methods for ghc 8.0.2.2 failed

https://github.com/haskell/cabal/runs/7448278695?check_suite_focus=true

@ulysses4ever
Copy link
Collaborator Author

Mergify says config-changing PRs have to be merged manually. Merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants