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.

Sign up for GitHub

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

Jump to bottom

auto addon reviewers - cond. not recognise team #876

Closed
Borda opened this issue Mar 27, 2020 · 19 comments · Fixed by #930
Closed

auto addon reviewers - cond. not recognise team #876

Borda opened this issue Mar 27, 2020 · 19 comments · Fixed by #930

Comments

@Borda
Copy link
Contributor

Borda commented Mar 27, 2020

We are facing an issue hot to recognise a team in review-requested!=core-contributors

Our situation is that have a rule that PR can be merged after 3 approvals, to do so we assign the review to a group core-contributors but Github allows to have the ethe group just once and we a member of this group does review it adds his approval and removes a team from request... So we want to make a simple rule if the number of approvals is lower then required and the team is not among requested, add the ream...

I have drafted this but it does not recognise the lean in condition

  - name: add core reviewer
    conditions:
      # number of review approvals
      - "#approved-reviews-by<3"
      # team is not requested yet
      - review-requested!=@PyTorchLightning/core-contributors
    actions:
      request_reviews:
        teams:
          - "@PyTorchLightning/core-contributors"
@Borda Borda mentioned this issue Mar 27, 2020
Merged
@sileht
Copy link
Member

sileht commented Mar 27, 2020

it's should be just review-requested!=@core-contributors

@sileht
Copy link
Member

sileht commented Mar 27, 2020

You don't need to filter out review-requested, Mergify have this protection built-in. The configuration your need is just:

  - name: add core reviewer
    conditions:
      - "#approved-reviews-by<3"
    actions:
      request_reviews:
        teams:
          - core-contributors

We use the same here https://github.com/Mergifyio/mergify-engine/blob/master/.mergify.yml#L60-L67

@Borda
Copy link
Contributor Author

Borda commented Mar 27, 2020

@sileht for the fast reply, do I have to write it as it was in examples "@PyTorchLightning/core-contributors" or core-contributors is enough?
https://doc.mergify.io/examples.html#flexible-reviewers-assignement

@sileht
Copy link
Member

sileht commented Mar 27, 2020

When the option is only teams related all syntax works (with or without @, with the organization or not).
If the options is about teams and users, @ is used to differentiate team slug from user login.

@Borda Borda closed this as completed Mar 27, 2020
@Borda
Copy link
Contributor Author

Borda commented Mar 31, 2020

I am wondering about triggering the rules... We have added the rule as discussed above...
when the PR was created the rule was applied, then a member of the assigned team did the review, so the team was "removed" from requests but then the rule did nit assigned it there again...
see the record in this PR Lightning-AI/pytorch-lightning#1312

@Borda Borda reopened this Mar 31, 2020
@jd
Copy link
Member

jd commented Mar 31, 2020

This seems to be because of the request review action reporting itself as a success to the engine, and therefore is only run once — and never again.
@sileht that seems to be a different behaviour from the one that is wanted here. Should we switch the action to always_run = True?

@Borda
Copy link
Contributor Author

Borda commented Mar 31, 2020

sounds like what I need, but don't know where to write it, to config condition?
tried to search the keyword in docs and no match... :{

@jd
Copy link
Member

jd commented Mar 31, 2020

No, that's currently a config option in the actions we run in the engine. Nothing to do from your side for now.

@Borda
Copy link
Contributor Author

Borda commented Mar 31, 2020

well thinking if turning it on won't have a negative effect on the merging and conflict check...
we need it just to this review assignment as I described above...
I would be great to have it ass option in config for particular rules though :]

@sileht
Copy link
Member

sileht commented Apr 1, 2020

At the beginning, we put always_run=True and people get spamed by Github to review the pull request on each pull request event.

But we add a some protection against that recently, we can retry it.

@Borda
Copy link
Contributor Author

Borda commented Apr 1, 2020 •
edited
Loading

so what about alow it as part of config?
like for sure I don't want to get a message about conflicting with target branch multiple time... but having assigned reviewer if he is missing any time...

@Borda
Copy link
Contributor Author

Borda commented Apr 5, 2020 •
edited
Loading

@sileht @jd any thought on having the always_run=True in the config, otherwise there is no way how to get recurrent reviewer assignment... or shall I open a new issue for it?
I assume that it is a very common situation when you need M approval from N-size team (M < N)

@jd jd mentioned this issue Apr 9, 2020
Merged
@mergify mergify bot closed this as completed in #930 Apr 9, 2020
mergify bot pushed a commit that referenced this issue Apr 9, 2020
@jd @mergify
fix(request_review): always run the action
b1c6e7e 
This makes sure the review are always requested until the conditions are not
matched anymore.

Closes #876
@Borda
Copy link
Contributor Author

Borda commented Apr 10, 2020

not sure if it is the fix, I would prefer to have it as an option in config...

@jd
Copy link
Member

jd commented Apr 11, 2020

We'd prefer to keep the number of options as low as possible to keep things simple.

What is wrong with the current fix?

@Borda
Copy link
Contributor Author

Borda commented Apr 11, 2020

Not sure how the always run behaves, is it described somewhere?

@jd
Copy link
Member

jd commented Apr 11, 2020

Not really, this is internal stuff. It just defines how an action behaves internally compared to the state of the pull request.

In that case, it means that the engine will try to re-apply the review request on each opportunity to make sure the state remains the same all along the life of the PR — as long as it matches the rule.

Some actions, e.g. merge, are not "always run" because they change the state of the PR: once a PR is merged, it can't be merged again. 😅

@Borda
Copy link
Contributor Author

Borda commented Jan 21, 2021

Hello @jd, I am suspicious if the request really works, we have re-enabled merify in our repo after some time, and seems that this reviewer assignment did nothing... When I open it in your editors and debug conditions on sample PR all condition are checked, but still no team request... From PyTorchLightning/pytorch-lightning on pull request #3256

#### Rule: add core reviewer (request_reviews)
- [X] `-conflict`
- [X] `-draft`
- [X] `label=0:] Ready-To-Go`
- [X] `#approved-reviews-by<3`

@Borda Borda mentioned this issue Jan 21, 2021
Merged
12 tasks
@jd
Copy link
Member

jd commented Jan 22, 2021

It seems to work, right?
Screenshot 2021-01-22 at 08 31 51

@Borda
Copy link
Contributor Author

Borda commented Jan 22, 2021

ok, it seems to work now, not sure what was wrong before... @jd thank you :]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(request_review): always run the action
3 participants
@jd @sileht @Borda