fix: Extend checks supported events #700
Conversation
jeremieguichard
changed the title
jeremieguichard
force-pushed
the
jg/checks-events
branch
from
d255dbb
to
2041b34
Compare
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## master #700 +/- ##
=======================================
Coverage 93.10% 93.10%
=======================================
Files 110 110
Lines 2493 2493
Branches 448 448
=======================================
Hits 2321 2321
Misses 153 153
Partials 19 19
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
lib/actions/checks.js
Outdated
| 'pull_request.synchronize', | ||
| 'pull_request.push_synchronize' | ||
| 'pull_request.*', | ||
| 'pull_request_review.*' |
There was a problem hiding this comment.
IIRC, there was a reason why we decided to list the pull_request out instead of the wildcard because some of the specific were causing infinite loop. It would be better if you just add the specific event you want
There was a problem hiding this comment.
Hey @shine2lay there was no specific comments about that loop issue in code or documenataion
and since it was stated that all pull_request events are supported for this actions https://github.com/mergeability/mergeable/blame/master/docs/actions/check.rst#L74
it thought that the check code had simply been missed during some refactoring round...
The fact that check action (alone) could cause a loop (on any of the pull_request events) is kind of surprising to me, specifically since (to my understanding) there is no pull_request event being triggered when votes are made via check API. So I do not see how using the check API on any of these events could result in a loop... do you recall
what was the problematic event? (maybe there is a trace of the discussion on a PR?)
If I would really go for just addding some of the events I need, I woulld like as well to add a comment explaning what events cannot be enabled (and why) and certainly update documentation accordingly, so next guy coming to that file will not like me spend some time updating the fine and testing the change to finally realize the change will not be accepted ;)
So in order to figure what event could have been causing this issue, I gave it a try with a separate repository and pull request and a local install that use the updated code. See the pull request here, it contains the config I used that is basically
giving a separate check vote for every separate pull_request events:
During my test, none of the newly added events seemed to caused a loop.
So could this loop issue be a old problem that if not reproducible anymore?
Or could it be that issue was actually the result of using different actions
in combination with check?
I do see one argument I would find valid for not using *: in context of check action, certain of the events
are not really useful to handle. E.g. why would someone want to add a check vote on a pull request being closed?
So I could understand we would be tempted to limit the handled events to the ones that "make sense". But then it is a different topic and would certainly make sense here as well to document why specific events were not added to support.
There was a problem hiding this comment.
@dje29 tbh, it has been a long time since I remember having this discussion so I can't remember the reason for the decision entirely. I do agree that it would've been nice to have the decision documented.
If i recall, the last argument about the pull_request.closed not being useful was the reason why decided to use specific events instead of *. Another possible reason (that i can guess) is that mergeable may have thrown error on some events like pull_request.closed because checks can't be created once pull_request are closed.
Re Loop: that may have been on a different validator
There was a problem hiding this comment.
Hey @shine2lay here is now updated fix adding all events but pull_request.closed. I updated documentation too
The checks action should be able to support most pull_request and pull_request_review events. Only the `pull_request.closed` event is not enabled since it does not have meaningful use in the context of the GitHub checks API.
jeremieguichard
force-pushed
the
jg/checks-events
branch
from
2041b34
to
c0e54f6
Compare
|
🎉 This PR is included in version 2.17.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The checks action should be able to support every
pull_request and pull_request_review events like stated
in the documentation. For example support for some events
like pull_request.ready_for_review was not supported with
current implementation.