miq_bot icon indicating copy to clipboard operation
miq_bot copied to clipboard

GitHub Status API Integration

Open europ opened this issue 6 years ago • 13 comments

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

good_status

  • error

bad_status

NOTE: The Details refers to the comment shown above.

europ avatar Mar 07 '18 16:03 europ

@europ please don't mark this as ready until you have tests.

@miq-bot add_label wip

skateman avatar Mar 19 '18 10:03 skateman

@miq-bot remove_label wip

europ avatar Mar 21 '18 13:03 europ

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 avatar Mar 26 '18 15:03 Fryguy

@fryguy by block do you mean red status? AFAIK we don't block PRs based on any status...

skateman avatar Mar 26 '18 15:03 skateman

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 avatar Mar 26 '18 15:03 Fryguy

@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?

skateman avatar Mar 26 '18 21:03 skateman

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)
    status_green

  • Red "error" status (at least one fatal or error offense)
    status_red

  • No status (other offense types such as warning, informing, etc.)
    status_none

europ avatar Apr 24 '18 14:04 europ

@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 avatar May 08 '18 15:05 Fryguy

@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"?

europ avatar May 08 '18 16:05 europ

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 avatar May 08 '18 16:05 Fryguy

@Fryguy The commits are from now marked every time depending of the detected offences as you requested.

There are 3 types of commit marking:

  1. zero offences: "success" status + "Everything looks fine."
  2. one or more warning offences excluding :bomb: :boom: :fire: :fire_engine: offences: "success" status + "Some offenses detected."
  3. at least one :bomb: :boom: :fire: :fire_engine: offence: "error" status + "Something went wrong."

europ avatar May 16 '18 13:05 europ

Checked commit https://github.com/europ/miq_bot/commit/1354dd8b26f2d6052cadfa0db24e460a9ec88c92 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 4 files checked, 0 offenses detected Everything looks fine. :cookie:

miq-bot avatar May 16 '18 13:05 miq-bot

This pull request is not mergeable. Please rebase and repush.

miq-bot avatar Mar 16 '20 22:03 miq-bot