GitHub Status API Integration #412

europ · 2018-03-07T16:40:43Z

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.

skateman

Looks good, but some things can be simplified and logically reorganized. Also please add some tests.

skateman · 2018-03-08T05:36:37Z

app/workers/commit_monitor_handlers/commit_range/rubocop_checker.rb

@@ -34,12 +34,14 @@ def process_branch
  end

  def rubocop_comments
-    MessageBuilder.new(results, branch).comments
+    message_builder = MessageBuilder.new(results, branch)
+    @status = message_builder.commit_status


I don't think this is The Right Way™ to pass the information, the rubocop_comments method is being used just in the replace_rubocop_comments method, so it's safe to move its content there. This way you don't need the instance variable at all. Then you can refactor or even rename the method and maybe break it into small(er) chunk.

skateman · 2018-03-08T05:36:45Z

app/workers/commit_monitor_handlers/commit_range/rubocop_checker.rb

  end

  def replace_rubocop_comments
    logger.info("Updating PR #{pr_number} with rubocop comment.")
-    GithubService.replace_comments(fq_repo_name, pr_number, rubocop_comments) do |old_comment|
+    GithubService.replace_comments(fq_repo_name, pr_number, rubocop_comments, @status) do |old_comment|


See my comment above...

skateman · 2018-03-08T05:38:57Z

lib/github_service.rb

+
+        unless commit_status.nil?
+          commit_status["options"]["target_url"] = retval["html_url"]
+          add_status(commit_status)


Adding the status is depending on the comment's ID, however, it is an operation on the same level so this method call doesn't belong here, but AFTER the add_comments is called.

skateman · 2018-03-08T05:39:27Z

lib/github_service.rb

      raise "no block given" unless block_given?

      to_delete = issue_comments(fq_repo_name, issue_number).select { |c| yield c }
      delete_comments(fq_repo_name, to_delete.map(&:id))
-      add_comments(fq_repo_name, issue_number, new_comments)
+      add_comments(fq_repo_name, issue_number, new_comments, commit_status)


I think you should put the add_status call after this line, see above...

skateman · 2018-03-08T05:40:13Z

Gemfile.lock

@@ -396,4 +396,4 @@ DEPENDENCIES
  webmock

 BUNDLED WITH
-   1.15.4
+   1.16.1


Don't forget to change this back please...

skateman · 2018-03-08T09:36:15Z

app/workers/commit_monitor_handlers/commit_range/rubocop_checker.rb

-    GithubService.replace_comments(fq_repo_name, pr_number, rubocop_comments) do |old_comment|
+    msg_builder_result = rubocop_comments
+
+    GithubService.replace_comments(fq_repo_name, pr_number, msg_builder_result[:comments], msg_builder_result[:status]) do |old_comment|


Do you really need the 2 arguments if you're taking them from the same hash? Send the whole hash instead!

skateman · 2018-03-08T09:37:34Z

app/workers/commit_monitor_handlers/commit_range/rubocop_checker/message_builder.rb

@@ -14,6 +14,20 @@ def comments
    message_builder.comments
  end

+  # requirements to "create_status(repo, sha, state, options)"
+  def commit_status


Why is this method under the MessageBuilder? Maybe this can be moved to a better place.

Agreed. This feels like it should live somewhere else.

@skateman @Fryguy The commit_status method is now in the MessageBuilder.

skateman · 2018-03-08T09:38:21Z

lib/github_service.rb

+      first_comment = add_comments(fq_repo_name, issue_number, new_comments)
+
+      # add_status creates a commit status pointing to the first comment URL
+      add_status(commit_status, first_comment)


add_status(commit_status, first_comment) unless first_comment.nil?

Then you don't need the condition in the add_status.

skateman · 2018-03-08T09:39:44Z

lib/github_service.rb

+    def add_status(comstat, comment)
+      unless comstat.nil?
+        comstat["options"]["target_url"] = comment["html_url"]
+        create_status(comstat["repo"], comstat["sha"], comstat["state"], comstat["options"])


Again, you're sending arguments from a single hash, these things can be simplified.

Fryguy · 2018-03-09T22:13:49Z

lib/github_service.rb

        add_comment(fq_repo_name, issue_number, comment)
-      end
+      end.first


I like the idea of returning the comments, but from a clean standpoint, this method should just return all of the comments. If the caller only needs the first one, they should be the one to call .first. This way you also don't need the comment describing why you need the .first.

skateman · 2018-03-12T12:59:51Z

app/workers/commit_monitor_handlers/commit_range/rubocop_checker.rb

+      "state"   => state ? "success" : "error",
+      "options" => {
+        "context"     => "miq-bot", # TODO: it should be user name variable
+        "target_url"  => nil,


Why is this nil? Can't be this parametrized as state?

@skateman The key is required and the value is added when the URL of the comment is obtained.

OK, what about the 2nd part of the question? 😉

skateman · 2018-03-19T10:47:49Z

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

@miq-bot add_label wip

europ · 2018-03-21T13:24:28Z

@miq-bot remove_label wip

Fryguy · 2018-03-26T15:21:19Z

app/workers/commit_monitor_handlers/commit_range/rubocop_checker.rb

@@ -39,7 +39,10 @@ def rubocop_comments

  def replace_rubocop_comments
    logger.info("Updating PR #{pr_number} with rubocop comment.")
-    GithubService.replace_comments(fq_repo_name, pr_number, rubocop_comments) do |old_comment|
+
+    status = results["files"].select { |f| f["offenses"].any? }.empty?


Can you just do status = results["files"].any? { |f| f["offenses"].any? } ?

It works exactly the opposite. I will have to negate it as

status = !results["files"].any? { |f| f["offenses"].any? }

otherwise I will have to remake the rest of the code. Do you agree with the negation?

if you're looking for the negation, then you can also do

results["files"].none? { |f| f["offenses"].any? }

Fryguy · 2018-03-26T15:23:38Z

lib/github_service.rb

+      first_comment = add_comments(fq_repo_name, issue_number, new_comments).first
+
+      # add_status creates a commit status pointing to the first comment URL
+      unless first_comment.nil? && status.nil?


unless nil && nil is melting my head with triple-negatives. This would be much easier to read in the positive. I think it's

if first_comment && status add_status(...) end

@Fryguy Can it be like this?

if first_comment && !status.nil? add_status(...) end

The status variable contain a boolean value so your example will not work in case of status has False value.

Ah ok, then yes, first_comment && !status.nil? is good.

Fryguy · 2018-03-26T15:24:29Z

spec/lib/github_service_spec.rb

+
+RSpec.describe GithubService do
+  let(:repo) { "owner/repository" }
+  let(:sha) { "cd16q9z48b3b0xbb91wb311" }


Pretty sure a SHA only has hexidecimal digits ;)

Fryguy · 2018-03-26T15:28:13Z

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 💣 💥 🔥 🚒)

skateman · 2018-03-26T15:39:49Z

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

Fryguy · 2018-03-26T15:43:48Z

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

skateman · 2018-03-26T21:15:04Z

@Fryguy this makes sense, so let's add a green status when all is good and a red when 💣 💥 🔥 🚒 happens. For other cases I would just simply not set the status...what do you think?

europ · 2018-04-24T14:05:51Z

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

Fryguy · 2018-05-08T15:42:37Z

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

europ · 2018-05-08T16:15:07Z

@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 💣 💥 🔥 🚒 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"?

Fryguy · 2018-05-08T16:16:16Z

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.

Commits are marked with [success/error] state depending on offence detection. If there are any offences in the code, the commit is marked with error state otherwise with success state. It is reflected at the end of PR in the check conclusion part.

europ · 2018-05-16T13:54:47Z

@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 💣 💥 🔥 🚒 offences:
"success" status + "Some offenses detected."
at least one 💣 💥 🔥 🚒 offence:
"error" status + "Something went wrong."

miq-bot · 2018-05-16T13:56:21Z

Checked commit europ@1354dd8 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. 🍪

miq-bot · 2020-03-16T22:05:26Z

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

europ changed the title ~~Integration of GitHub Status API~~ GitHub Status API Integration Mar 7, 2018

europ force-pushed the commit_status branch 3 times, most recently from 0f24fbc to cfe0348 Compare March 7, 2018 17:58

skateman suggested changes Mar 8, 2018

View reviewed changes

europ force-pushed the commit_status branch 3 times, most recently from ef1f877 to bb0aee2 Compare March 8, 2018 09:21

skateman suggested changes Mar 8, 2018

View reviewed changes

europ force-pushed the commit_status branch 2 times, most recently from 4395636 to d59d903 Compare March 8, 2018 10:31

Fryguy reviewed Mar 9, 2018

View reviewed changes

europ force-pushed the commit_status branch 2 times, most recently from 9f39949 to 85b2f5a Compare March 12, 2018 12:42

skateman reviewed Mar 12, 2018

View reviewed changes

europ force-pushed the commit_status branch 4 times, most recently from 42e8f34 to 9699f2d Compare March 12, 2018 14:09

miq-bot changed the title ~~GitHub Status API Integration~~ [WIP] GitHub Status API Integration Mar 19, 2018

miq-bot added the wip label Mar 19, 2018

europ force-pushed the commit_status branch 3 times, most recently from 56fc858 to 20a574f Compare March 21, 2018 12:06

europ changed the title ~~[WIP] GitHub Status API Integration~~ GitHub Status API Integration Mar 21, 2018

miq-bot removed the wip label Mar 21, 2018

europ force-pushed the commit_status branch 2 times, most recently from caad3ad to b0e6911 Compare March 21, 2018 14:09

Fryguy reviewed Mar 26, 2018

View reviewed changes

europ force-pushed the commit_status branch from b0e6911 to 4544a5e Compare March 26, 2018 19:57

europ force-pushed the commit_status branch from 4544a5e to dbe592e Compare April 24, 2018 13:59

skateman approved these changes Apr 25, 2018

View reviewed changes

europ force-pushed the commit_status branch from dbe592e to 70618ba Compare May 16, 2018 13:44

GitHub Status API Integration

1354dd8

Commits are marked with [success/error] state depending on offence detection. If there are any offences in the code, the commit is marked with error state otherwise with success state. It is reflected at the end of PR in the check conclusion part.

europ force-pushed the commit_status branch from 70618ba to 1354dd8 Compare May 16, 2018 13:51

mfeifer added help wanted enhancement labels Feb 6, 2020

miq-bot added the unmergeable label Mar 16, 2020

This was referenced May 7, 2020

miqbot CLI #499

Open

GitHub Status API Integration (rebased) #501

Open

GitHub Status API Integration #412

Are you sure you want to change the base?

GitHub Status API Integration #412

Conversation

europ commented Mar 7, 2018 • edited Loading

Preview

success

error

skateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented Mar 19, 2018

europ commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Mar 26, 2018

skateman commented Mar 26, 2018

Fryguy commented Mar 26, 2018

skateman commented Mar 26, 2018

europ commented Apr 24, 2018 • edited Loading

There are now 3 types of statuses:

Fryguy commented May 8, 2018

europ commented May 8, 2018 • edited Loading

Fryguy commented May 8, 2018

europ commented May 16, 2018

miq-bot commented May 16, 2018

miq-bot commented Mar 16, 2020

europ commented Mar 7, 2018 •

edited

Loading

europ commented Apr 24, 2018 •

edited

Loading

europ commented May 8, 2018 •

edited

Loading