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

Feature: assign author and comment on PR if a merge conflict occurs after a correct pr is merged #64

Closed
ghost opened this issue Jun 13, 2017 · 11 comments

Comments

@ghost
Copy link

ghost commented Jun 13, 2017

When a PR has "awaiting-review" & merge conflict then remove awaiting review label, add "resolving-merge-conflict" label, assign the author of the PR and add comment on the issue with "@username before proceeding with review, please resolve the merge conflict and then reassign."

@ghost ghost added the priority-2 label Jun 13, 2017
@ghost ghost assigned SimonLab Jun 13, 2017
@SimonLab SimonLab added the T2h label Jun 13, 2017
@SimonLab
Copy link
Member

Hopefully Github will trigger an event when a merge conflict occurs. If it's the case this should be quiet straightforward to implement as it follow the same login as in #58

@ghost ghost modified the milestone: Sprint 1 Jun 13, 2017
@SimonLab
Copy link
Member

Unfortunately Github doesn't seem to trigger any events when a PR has a merge conflict.
However we could catch the event when a PR is merged, get the list of all the open PR and check if the PRs with "awaiting-review" has any merge conflicts

The rule merge-conflict is triggered by the event: pull request closed
do get the list of open PRs
If PRs have "awaiting-review" label AND PRs can't be merged
Then

  • add a comment on the PR: "@{author_PR}, the pull request has some merge conflict, please resolved them and reassign when ready, thanks 👍"
  • remove "awaiting-review" label
  • add "merge-conflict" label

@SimonLab SimonLab added T4h and removed T2h labels Jun 16, 2017
@SimonLab
Copy link
Member

changing time estimate to 4h. I've already spend 1h looking for a github event when a merge conflict occurs (doesn't exist). Compare to the other rules implemented so far, this rule require a bit more logic on the backend (fetching all the open PRs, checking for errors on all these PRs, reporting for erorr for all the PRs).
Also on this specific rule we shouldn't have dwylbot to wait before commenting on the PR. The error report should be direct. So I will need first to implement #74 (define waiting time per rule) first

@SimonLab
Copy link
Member

Unfortunately a github app doesn't seem to have the right to remove reviewers when a PR has some merge conlflicts:

%HTTPoison.Response{body: "{\"message\":\"Validation Failed\",\"errors\":[\"Must have push access to this repository to add review requests.\"],\"documentation_url\":\"https://developer.github.com/v3/schema/pull-request-review-request\"}"

However I don't think this is a big blocker as we already remove the assignees.

SimonLab added a commit that referenced this issue Jun 19, 2017
SimonLab added a commit that referenced this issue Jun 19, 2017
@SimonLab SimonLab assigned ghost and unassigned SimonLab Jun 20, 2017
ghost pushed a commit that referenced this issue Jun 21, 2017
Merge Conflict Test #64
@ghost
Copy link
Author

ghost commented Jun 21, 2017

@SimonLab these parts don't happen:

remove "awaiting-review" label
add "merge-conflict" label

#118

also note your own app is posting too

@ghost ghost removed the please-test label Jun 21, 2017
@ghost ghost removed their assignment Jun 21, 2017
@ghost ghost assigned SimonLab Jun 21, 2017
@SimonLab
Copy link
Member

myapp has been removed, I'll have a look if I can reproduce the error

@SimonLab
Copy link
Member

SimonLab commented Jun 21, 2017

The merge conflict can only be checked when a PR is merged:

  • a correct PR is merged
  • dwylbot get all the other open PRs and check for merge conflict
  • for each of the PRs with merge conflict add an error commnent.

Agree that it would be great to have a more dynamic check. I think this will be a similar rule but the trigger is different. We need a rule that is trigger when a PR is created/opened also, I'll create an issue for that, thanks for testing the rule 👍
You can leave your test PR open and as soon as another PR is merged dwylbot should report the merge conflict. If this happen I think this issue can be close and I will work on the other one.

@SimonLab SimonLab mentioned this issue Jun 21, 2017
@SimonLab SimonLab assigned ghost and unassigned SimonLab Jun 21, 2017
@ghost ghost changed the title Feature: assign author and comment on PR if a merge conflict occurs Feature: assign author and comment on PR if a merge conflict occurs after a correct pr is merged Jun 21, 2017
@ghost
Copy link
Author

ghost commented Aug 9, 2017

Waiting on #118 (comment)

@ghost ghost added dependency and removed please-test T4h labels Aug 9, 2017
@ghost
Copy link
Author

ghost commented Aug 9, 2017

@SimonLab dwylbot has not reported a merge conflict here: dwyl/dwylbot-test#1

@ghost ghost added bug and removed dependency labels Aug 9, 2017
@ghost ghost assigned SimonLab and unassigned ghost Aug 9, 2017
@naazy
Copy link
Member

naazy commented Aug 18, 2017

@markwilliamfirth I believe dwylbot did not report a merge conflict due to @SimonLab's comment #64 (comment) . We only check for a merge conflicts when a (different) PR is merged.

@ghost ghost added please-test and removed bug labels Aug 18, 2017
@ghost ghost assigned ghost and unassigned SimonLab Aug 18, 2017
@ghost
Copy link
Author

ghost commented Sep 20, 2017

Replaced by #64

@ghost ghost closed this as completed Sep 20, 2017
@jammur jammur unassigned ghost Dec 14, 2017
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants