Add multiple rules management per branch

[achaussier]

Hello,

First, thanks for this very useful service !

Sometime, for the same branch, we want a different behavior based on some criteria (PR title, user, ...).

For example, I use Pyup to manage my Python dependencies and for these changes, I only want Travis tests pass, but use another criterias for other changes on the same destination branch.

So if it could be possible to manage multiple rules for the same branch, this would be useful.

Have a nice day

[jd]

Hi @achaussier!

What would be your other criteria to merge those Pyup PR?

[achaussier]

For this use case, it's more a filtering criteria, not a validation criteria. Perhaps PR title, PR owner, commit message, ...

An example:

• For Pyup PR: only criteria is to have Travis tests pass, because this is only dependencies changes, and I trust my tests ;)
• For other PR, about code change normally, I want Travis tests pass but also review and validate these changes before the merge

My default branch is develop for my projects, so this is the default target for merge and Pyup updates

[jd]

I see what you mean. Right now the rules are per-branch, but they could be per-PR.

Then you'd need a criterion of 0 reviewers. Not sure GitHub allows that to be declared, but Mergify could support it nonetheless.

Those are likely two different features but they could be achievable on their own and then combined to achieve what you want.

[jd]

Here's a proposal for the format implementing this:

A pull request object would be represented as a JSON/YAML object like this:

- assignees: [list of String users]
- labels: [list of String labels]
- review_requests: [list of String users/teams]
- author: <String login>
- base: <String name of the base branch>
- body: <String the pull request body>
- head: <String name of the target branch>
- locked: <Boolean if the PR is locked>
- title: <String title of the PR>
- files: [String list of filenames modified]

It'd then pass a list of filters where the first match will give the rules.
The filters are written using JSONPath and the filters provided by jsonpath-rw-ext.

rules:
   …
pull_requests_rules:
  - conditions:
      - $[?(author.login == \"sileht\")]
       - $[*].files[?(filename == \"README.rst\")]
    delete_branch: false

This is quite powerful, but not that user-friendly. So far I'm unable to find something that would be a good compromise between powerfulness and user-friendlyness.

As a second pass, we could also implement GitHub syntax for searching pull requests and issues though I'm not sure everything is useful and the syntax is quite limited.

PoC

import jsonpath_rw_ext

pull_request = {
    "assignees": [
        {"login": "jd"},
    ],
    "labels": [
        {"name": "mergemeplease"},
    ],
    "review_requests": [],
    "author": {"login": "sileht"},
    "base": "master",
    "body": "here some text",
    "head": "jd:master",
    "locked": False,
    "title": "My best PR",
    "files": [
        {"filename": "setup.py"},
        {"filename": "README.rst"},
    ],
}

pull_requests_rules = [
    {
        "conditions": [
            "$[?(author.login == \"sileht\")]",
        ],
        "delete_branch": True,
    },
    {
        "conditions": [
            "$[*].files[?(filename == \"README.rst\")]",
        ],
        "delete_branch": True,
    },
    {
        "conditions": [
            "$[?(author.login == \"sileht\")]",
            "$[*].files[?(filename == \"README.rst\")]",
        ],
        "delete_branch": True,
    },
    {
        "conditions": [
            "$[?(author.login == \"jd\")]",
            "$[*].files[?(filename == \"README.rst\")]",
        ],
        "delete_branch": True,
    },
]


for rule in pull_requests_rules:
    print(rule)
    for condition in rule["conditions"]:
        if not jsonpath_rw_ext.parse(condition).find([pull_request]):
            print("  -> %s does not match" % condition)
            break
    else:
        print("  -> match!")

Output

{'conditions': ['$[?(author.login == "sileht")]'], 'delete_branch': True}
  -> match!
{'conditions': ['$[*].files[?(filename == "README.rst")]'], 'delete_branch': True}
  -> match!
{'conditions': ['$[?(author.login == "sileht")]', '$[*].files[?(filename == "README.rst")]'], 'delete_branch': True}
  -> match!
{'conditions': ['$[?(author.login == "jd")]', '$[*].files[?(filename == "README.rst")]'], 'delete_branch': True}
  -> $[?(author.login == "jd")] does not match

[sileht]

One think bother me, people have chance to be confuse about why some PR are merged with some condition and some other not.

What about manning pull_requests_rules (with a mandatory name attribute, because we can't use dict the rules ordering matter) ? This will at least allow just before merging to post a comment saying This pull request just match the rule XXXXX and will be merged automatically.

[sileht]

Also with this new system, we can't really known in advance which rule will be used. In the future we will not be able to says Waiting for 0/1 Reviews and travis CI to finish anymore.

[jd]

Sure we can add a rule name. That'd be easier for debugging and printing which rule is being used.

I think you know which rule is being used, you just have to take into account that it can change during the lifetime of the PR, and maybe update the check status more often. Or do I miss something?

[sileht]

During one second, I have imagined, conditions will in the future also contain current 'protection' attributes, but that a bad idea.

In the future we will be more something like this ?

pull_requests_rules:
  - conditions:
       - $[?(author.login == \"sileht\")]
       - $[*].files[?(filename == \"README.rst\")]
    delete_branch: false
    protection:
      required_status_checks:
        strict: True
        contexts:
          - continuous-integration/travis-ci
      required_pull_request_reviews:
        required_approving_review_count: 2
    merge_strategy:
      method: rebase
    automated_backport_labels:
      backport-to-4.3: stable/4.3
      backport-to-4.2: stable/4.2

[sileht]

That makes me think about do we will merge 'all protections and things to do' when multiple rules match ?

I think it would simplify a lot complicated configuration, example:

pull_requests_rules:
  - name: default protection for master
    conditions:
       - $.base.ref == "master"
    protection:
      required_status_checks:
        strict: True
        contexts:
          - continuous-integration/travis-ci
      required_pull_request_reviews:
        required_approving_review_count: 2

  - name: always delete feature branch pull request
    conditions:
       - $.base.fullname == $.head.fullname
       - $.head.ref ~= "^features/"
    delete_branch: true

  - name: 4.3 backports
    conditions:
      - "backport-to-4.3" in $.labels
    backport_to: stable/4.3

[sileht]

Another idea, should be group 'thing todo after merge' into a dedicated section like
:

pull_requests_rules:
  - name: random thing
    conditions:
       - XXXXXX
    protection:
      ....
    post_merge_actions:
      delete_branch: true
      backport_to: ["stable/4.3"]

[jd]

You cannot have protections (in the GitHub sense) per pull-request, as you know. So that setting should stay where it is. We should add new settings to define custom merge barriers (that might overlap with branch protection).

Not sure about grouping, but having something like backport_to is a good idea though.

[sileht]

Of course, I called it 'protection', but I'm talking of our future per pull request merge barriers.

[jd]

In that case, +1 :D

[jd]

After some more thought, here's another draft:

pull_requests_rules:
  - name: default protection for master
    conditions:
       - base=master
    automerge:
      strict: True
      required_status_checks:
          - continuous-integration/travis-ci
      required_approving_review_count: 2

  - name: always delete feature branch pull request
    conditions:
       - head~="^features/"
    delete_branch: true

  - name: 4.3 backports
    conditions:
      - labels="backport-to-4.3"
    backport_to: stable/4.3

  - name: 4.4 backports
    conditions:
       - labels="backport-to-4.4"
       - base=master
    backport_to: stable/4.4

  - name: 4.5 backports
    conditions:
      - or:
       - labels="backport-to-4.5"
       - base=master
    backport_to: stable/4.5

Knowing that:

• each rule matching will be applied (there's no stop on first match) — so you can have a default with a wild rule first, and then make it finer
• the first keyword conditions is equivalent to a and
• the matching syntax is basically <attribute><operator><value>
• a match can have - in front of it to exclude it
• the operator : is equivalent to = (to mimic github syntax) or to in (for arrays)

[sileht]

I'm just unsure about the readability of "a match can have - in front of it to exclude it". This will show up as a double dash per line. The rest work for me.

    conditions:
      - - or:
       - - labels="backport-to-4.5"
       - - base=master

[jd]

That would rather be:

conditions:
  - -labels="backport-to-4.5"
  - -base=master

Knowing the - does not work in front of or and and.

I can also add ¬ support ;)

[achaussier]

Hello @jd & @sileht , sorry for my late answer on this topic :)

This draft open really useful opportunities on merge request management !

I've the same remark about the negative match, perhaps use strings beginning with ! or the != operator.

[jd]

This has been implemented and is available in v2.