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

Show warning if assignee is trying to merge a PR without reviews #5126

Open
HaruChebrolu opened this issue Oct 3, 2024 · 11 comments
Open

Show warning if assignee is trying to merge a PR without reviews #5126

HaruChebrolu opened this issue Oct 3, 2024 · 11 comments

Comments

@HaruChebrolu
Copy link

Expected Behavior

PR shouldn't be merged when PR has no reviews or if assignee is trying to merge it.

Actual Behavior

PR was getting merged

Steps to Reproduce the Problem

  1. We have below configuration in mergify.yaml
  ---
pull_request_rules:
  - name: automatic merge
    conditions:
      - label!=DNM
      - label!=work-in-progress
      - base=master
      - "approved-reviews-by=@red-hat-storage/cephci-top-level-reviewers"
      - "check-success=tox (3.9.18)"
      - check-success=WIP
    actions:
      merge:
        method: merge
  - name: ask to resolve conflict
    conditions:
      - conflict
    actions:
      comment:
        message: >
          "This pull request now has conflicts with the target branch.
          Could you please resolve conflicts and force push the corrected
          changes?"
  1. So anyone can have a label approved in labels, so its getting merged.
  2. Can we have a condition in above configuration file in such a way only when reviewers keep label as approved, PR should get merged.

Specifications

  • Pull Request URL:
  • Mergify Config URL:
@jd
Copy link
Member

jd commented Oct 3, 2024

Hi @HaruChebrolu,

There are no label conditions in your configuration above. I imagine you want to add one? In that case you'd need:

pull_request_rules:
  - name: automatic merge
    conditions:
      - label!=DNM
      - label!=work-in-progress
      - label = approved
      - base=master
      - "approved-reviews-by=@red-hat-storage/cephci-top-level-reviewers"
      - "check-success=tox (3.9.18)"
      - check-success=WIP
    actions:
      merge:
        method: merge

This means the label approved will be required to merge the PR.

@HaruChebrolu
Copy link
Author

yes @jd , you are right
I also need a condition only when reviewers add a label, only when PR should get merged. Could you please help me with that.

@Syffe
Copy link
Contributor

Syffe commented Oct 3, 2024

Hi @HaruChebrolu ,

Thanks for taking the time to formulate your issue,

If we understand correctly, you want to block the merge if the label approved is not set by someone whose a reviewer on the PR, is that right?

In other words, the PR, once approved by reviewers, should only be merged if the label approved is set by one of those said reviewers, is that right?

If that's the case, there's no way to know in Mergify who added the label so you can't write a condition for that.

@jd
Copy link
Member

jd commented Oct 3, 2024

A good question for @HaruChebrolu would be: what are you trying to implement/achieve in term of workflow?
As in, what's the problem with the current rule that you have?

@HaruChebrolu
Copy link
Author

yes @Syffe , you are right! Thats what we are trying to implement in our workflow

@HaruChebrolu
Copy link
Author

@Syffe @jd Could you please provide an update on this?

@jd
Copy link
Member

jd commented Oct 9, 2024

@HaruChebrolu If you don't answer our questions, it's impossible to help you.

@HaruChebrolu
Copy link
Author

@jd I provided answer for @Syffe's question, the understanding of @Syffe is right.
We want to implement the workflow in such a way that only when the PR, once approved by reviewers, should only be merged if the label approved is set by one of those said reviewers

@jd
Copy link
Member

jd commented Oct 9, 2024

@HaruChebrolu Right but you did not answer mine. 😢

To reply to your initial question, this is simply not possible; Mergify does not expose the information of who sets a label.
That's why I'm asking you why and what you want to achieve. You're coming up with a solution which might be the good one, cf https://en.wikipedia.org/wiki/XY_problem

@HaruChebrolu
Copy link
Author

@jd The current problem with our workflow is anyone can assign a label as "approved"(even without having valid approvals from reviewers) and then mergify would merge it based on label "approved".

@jd
Copy link
Member

jd commented Oct 9, 2024

Hi @HaruChebrolu,

I understand your concern, but I'm a bit unclear about what you mean by "anyone" being able to assign the "approved" label. Typically, only users with the necessary permissions can add labels to a repository.

It would be helpful if you could provide more details about the specific issue you're encountering. Could you explain the real problem you're trying to solve? Understanding the root of the issue will allow us to find the best possible solution.

You mentioned wanting a condition where only when reviewers add the "approved" label should the PR be merged. Unfortunately, Mergify doesn't have the capability to detect who added a label, so we can't implement that exact solution.

Let's take a step back to understand your workflow needs. Are you concerned that pull requests are being merged without proper approval because labels are being added by unauthorized users? If so, we might be able to explore alternative approaches to ensure that only appropriately reviewed PRs are merged.

Please share more details about the challenges you're facing, and we'll do our best to assist you.

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

3 participants