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

fix(mergify): restrict merges with unresolved threads #3453

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

gustavovalverde
Copy link
Member

Motivation

We've had a few cases of Mergify merging reviewed PR's with unresolved comments.

Solution

  • Added "#review-threads-unresolved=0" to all pull_request_rules which will prevent merges with unresolved threads.

Review

@dconnolly

@gustavovalverde gustavovalverde added A-devops Area: Pipelines, CI/CD and Dockerfiles P-Critical 🚑 labels Feb 1, 2022
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

Nice! I was looking for something like this in the Mergify docs and just did not have the grep-fu to find it 🙏🙏🙏

@dconnolly dconnolly enabled auto-merge (rebase) February 1, 2022 23:33
@teor2345 teor2345 disabled auto-merge February 2, 2022 00:02
@teor2345
Copy link
Contributor

teor2345 commented Feb 2, 2022

@dconnolly we don't need to set GitHub auto-merge any more, because mergify will do the merge.

@dconnolly
Copy link
Contributor

@dconnolly we don't need to set GitHub auto-merge any more, because mergify will do the merge.

Not for changes to Mergify config, which makes sense:

Screenshot_20220201-190503.png

@teor2345 teor2345 enabled auto-merge (squash) February 2, 2022 00:06
@teor2345
Copy link
Contributor

teor2345 commented Feb 2, 2022

Ah right, I just read that:

For security reason, Mergify will never automatically merge a pull request if it changes the Mergify configuration file.

https://docs.mergify.com/getting-started/#configuration

@codecov
Copy link

codecov bot commented Feb 2, 2022

Codecov Report

Merging #3453 (a079d7b) into main (499ae89) will decrease coverage by 0.29%.
The diff coverage is 78.91%.

@@            Coverage Diff             @@
##             main    #3453      +/-   ##
==========================================
- Coverage   78.34%   78.04%   -0.30%     
==========================================
  Files         267      272       +5     
  Lines       31526    31532       +6     
==========================================
- Hits        24698    24609      -89     
- Misses       6828     6923      +95     

@teor2345 teor2345 merged commit 4d32f9c into main Feb 2, 2022
@teor2345 teor2345 deleted the mergify-fix-comments branch February 2, 2022 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants