miq_bot
miq_bot copied to clipboard
GitHub Status API Integration
This feature will allow miq-bot to mark the latest commit with a status which depends from the offence(s).
Commits are marked with success
or error
state based on offence(s) detection via create_status
. If there are any offences in the code, the latest commit is marked with error
state, otherwise with success
state. It is reflected at the end of pull request in the checks
section. The status also involves a reference (URL) to the comment which has the description about the offences.
Detailed description about the creation of a commit status.
The commit state is stated in rubocop_checker.rb
which will be used as a marking state for the last commit. This state is passed to the replace_comments
method in github_service.rb
as an optional parameter also with the hash (sha) of the last commit which is going to be marked with this state. In replace_comments
the comment(s) writing is firstly proceed as it was originally. After writing the comment(s) about the offence(s) the id of each comment is preserved due to the missing URL which will be used in the commit status as a reference to the comment including the commit status description including the offence(s). The first comment id is selected and a payload is created which is required for the create_status
. Obtaining these information, the last commit is marked with the adequate state, a little state description and link to the comment including detailed description.
\cc @skateman @romanblanco
Preview
-
success
-
error
NOTE: The Details
refers to the comment shown above.
@europ please don't mark this as ready until you have tests.
@miq-bot add_label wip
@miq-bot remove_label wip
high level I like the ability to use the github api status, but I'm not sure I want to block the PR expect in specific cases (like :bomb: :boom: :fire: :fire_engine:)
@fryguy by block do you mean red status? AFAIK we don't block PRs based on any status...
Sorry, yes, red status. If too many PRs go "red" then it's harder for reviewers to differentiate between actual failed PRs (i.e. travis didn't pass) vs not actually failed PRs (like how when code climate goes red on 1 new issue).
@Fryguy this makes sense, so let's add a green status when all is good and a red when :bomb: :boom: :fire: :fire_engine: happens. For other cases I would just simply not set the status...what do you think?
Based on the pull request review from @skateman and @Fryguy this feature was a little bit changed.
There are now 3 types of statuses:
-
Green "success" status (zero offenses)
-
Red "error" status (at least one fatal or error offense)
-
No status (other offense types such as warning, informing, etc.)
@europ I'm thinking the third case
No status (other offense types such as warning, informing, etc.)
should still show the status API entry, but it should be a) green and b) say something like "Changes requested"
@Fryguy There is a possibility to use "yellow" status - the pending status.
There are 4 available statuses:
- failure - this means an internal error (red) - this will not be used
- error - at leas one :bomb: :boom: :fire: :fire_engine: offense (red)
- pending - detected offenses required to fix (yellow)
- success - no offense detected (green)
Would you like to use the "yellow" one (pending status) for this case with a message "detected offenses required to fix"?
The problem there is that when PR reviewers see "yellow" they think "the CI for this PR is still running", which is not what we want. I'm thinking just use green.
@Fryguy The commits are from now marked every time depending of the detected offences as you requested.
There are 3 types of commit marking:
- zero offences: "success" status + "Everything looks fine."
- one or more warning offences excluding :bomb: :boom: :fire: :fire_engine: offences: "success" status + "Some offenses detected."
- at least one :bomb: :boom: :fire: :fire_engine: offence: "error" status + "Something went wrong."