Remove unnecessary unmergeable comments #415

europ · 2018-03-22T13:03:24Z

Except of label deletion the bot also deletes the comments describing unmergeable status of the pull request.

Comments to delete (example):

skateman

Almost perfect, great job! 👍

skateman · 2018-03-26T12:17:57Z

app/workers/pr_mergeability_checker.rb

@@ -19,6 +19,10 @@ def tag
    "<pr-mergeability-checker />"
  end

+  def unmergeable_comment
+    "#{tag}This pull request is not mergeable.  Please rebase and repush."


t(w)oo (many) spaces here

The change of this string will affect older pull requests - ignore them. It will be working only for new ones which will have this "new" string without "two" spaces due to the string comparison at this line in remove_comments method.

@europ I'd rather fix the line here, and leave the older PR comments as they are. But try asking in https://gitter.im/ManageIQ/miq_bot

I'm fine with orphaning old comments but I am also fine with one or two spaces after a period. It's cute to have the legacy of typewriters still around via the two space after periods 😆

In other words, I'm more in favor of getting this merged than worrying about semantics.

Agreed...only worry about the "new" comments moving forward.

skateman · 2018-03-26T12:19:28Z

app/workers/pr_mergeability_checker.rb

+      GithubService.issue_comments(fq_repo_name, branch.pr_number).select do |com|
+        com.user.login == Settings.github_credentials.username && com.body.eql?(unmergeable_comment)
+      end.map(&:id)
+    )


Can you move out the block above to a variable so you don't have the weird ) at the end?

I think the usage of this "one" variable at this point is useless. It will just help with the code readability.

skateman

Fryguy · 2018-03-26T14:34:10Z

app/workers/pr_mergeability_checker.rb

      remove_label
    end

    # Update columns directly to avoid collisions wrt the serialized column issue
    branch.update_columns(:mergeable => currently_mergeable)
  end

+  def remove_comments
+    comment_ids = GithubService.issue_comments(fq_repo_name, branch.pr_number).select do |com|
+      com.user.login == Settings.github_credentials.username && com.body.eql?(unmergeable_comment)


Only look for the leading tag and not the entire string. The purpose of the tag is to allow us to change the string at any time while still allowing the comment finder to work.

Fryguy

See above

Except for label deletion the bot deletes the comments describing unmergeable status.

europ · 2018-03-26T21:02:54Z

@Fryguy I keep the unmergeable_comment method, but I changed the the original string — 2 spaces to 1 space. The remove_comments now search for a comment whose body content starts with the tag.

miq-bot · 2018-03-26T21:17:12Z

Checked commit europ@665c5fc with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

Fryguy

LGTM

Fryguy · 2018-04-23T18:47:30Z

app/workers/pr_mergeability_checker.rb

+      com.user.login == Settings.github_credentials.username && com.body.start_with?(tag)
+    end.map(&:id)
+
+    GithubService.delete_comments(fq_repo_name, comment_ids)


One quick comment...what happens if there aren't any ids (i.e. it didn't find any comments). Does this just ignore that?

No action is performed.

From: /root/miq_bot/app/workers/pr_mergeability_checker.rb @ line 46 PrMergeabilityChecker#remove_comments: 45: def remove_comments => 46: binding.pry 47: comment_ids = GithubService.issue_comments(fq_repo_name, branch.pr_number).select do |com| 48: com.user.login == Settings.github_credentials.username && com.body.start_with?(tag) 49: end.map(&:id) 50: 51: GithubService.delete_comments(fq_repo_name, comment_ids) 52: end [22] pry(#<PrMergeabilityChecker>)> comment_ids = GithubService.issue_comments(fq_repo_name, branch.pr_number).select do |com| [22] pry(#<PrMergeabilityChecker>)* com.user.login == Settings.github_credentials.username && com.body.start_with?(tag) [22] pry(#<PrMergeabilityChecker>)* end.map(&:id) I, [2018-04-21T22:59:51.679438 #13620] INFO -- : Executed GET https://api.github.com/repos/xtest123/testrepo/issues/4/comments?per_page=100...api calls remaining 4912 => [383854765] [23] pry(#<PrMergeabilityChecker>)> comment_ids => [383854765] [24] pry(#<PrMergeabilityChecker>)> GithubService.delete_comments(fq_repo_name, comment_ids) I, [2018-04-21T23:00:01.528455 #13620] INFO -- : Executed DELETE https://api.github.com/repos/xtest123/testrepo/issues/comments/383854765...api calls remaining 4911 => [383854765] [25] pry(#<PrMergeabilityChecker>)> comment_ids = GithubService.issue_comments(fq_repo_name, branch.pr_number).select do |com| [25] pry(#<PrMergeabilityChecker>)* com.user.login == Settings.github_credentials.username && com.body.start_with?(tag) [25] pry(#<PrMergeabilityChecker>)* end.map(&:id) I, [2018-04-21T23:00:21.684062 #13620] INFO -- : Executed GET https://api.github.com/repos/xtest123/testrepo/issues/4/comments?per_page=100...api calls remaining 4909 => [] [26] pry(#<PrMergeabilityChecker>)> comment_ids => [] [27] pry(#<PrMergeabilityChecker>)> GithubService.delete_comments(fq_repo_name, comment_ids) => []

miq-bot added the wip label Mar 22, 2018

europ force-pushed the unmerg_coms branch from fde3bf5 to 59962ea Compare March 22, 2018 13:37

europ changed the title ~~[WIP] Remove unnecessary unmergeable comments~~ Remove unnecessary unmergeable comments Mar 22, 2018

miq-bot removed the wip label Mar 22, 2018

europ force-pushed the unmerg_coms branch 2 times, most recently from 6d9bdfb to 2f55f55 Compare March 22, 2018 14:51

europ changed the title ~~Remove unnecessary unmergeable comments~~ [WIP] Remove unnecessary unmergeable comments Mar 22, 2018

miq-bot added the wip label Mar 22, 2018

europ force-pushed the unmerg_coms branch from 2f55f55 to 54bb3dd Compare March 22, 2018 14:53

europ changed the title ~~[WIP] Remove unnecessary unmergeable comments~~ Remove unnecessary unmergeable comments Mar 22, 2018

miq-bot removed the wip label Mar 22, 2018

europ force-pushed the unmerg_coms branch 2 times, most recently from 3db4203 to 23e98a0 Compare March 26, 2018 12:07

skateman suggested changes Mar 26, 2018

View reviewed changes

europ force-pushed the unmerg_coms branch from 23e98a0 to e50c07b Compare March 26, 2018 13:19

skateman approved these changes Mar 26, 2018

View reviewed changes

Fryguy reviewed Mar 26, 2018

View reviewed changes

Fryguy requested changes Mar 26, 2018

View reviewed changes

europ force-pushed the unmerg_coms branch from e50c07b to 0777aaa Compare March 26, 2018 20:48

Remove unnecessary unmergeable comments

665c5fc

Except for label deletion the bot deletes the comments describing unmergeable status.

europ force-pushed the unmerg_coms branch from 0777aaa to 665c5fc Compare March 26, 2018 20:59

Fryguy approved these changes Apr 23, 2018

View reviewed changes

Fryguy reviewed Apr 23, 2018

View reviewed changes

Fryguy merged commit 44a5441 into ManageIQ:master May 8, 2018

Fryguy added the enhancement label May 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary unmergeable comments #415

Remove unnecessary unmergeable comments #415

europ commented Mar 22, 2018 •

edited

Loading

skateman left a comment

skateman Mar 26, 2018

europ Mar 26, 2018 •

edited

Loading

romanblanco Mar 26, 2018

jrafanie Mar 26, 2018

jrafanie Mar 26, 2018

Fryguy Mar 26, 2018

skateman Mar 26, 2018

europ Mar 26, 2018 •

edited

Loading

skateman left a comment

Fryguy Mar 26, 2018

Fryguy left a comment

europ commented Mar 26, 2018

miq-bot commented Mar 26, 2018

Fryguy left a comment

Fryguy Apr 23, 2018

europ Apr 24, 2018

Remove unnecessary unmergeable comments #415

Remove unnecessary unmergeable comments #415

Conversation

europ commented Mar 22, 2018 • edited Loading

Comments to delete (example):

skateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

europ Mar 26, 2018 • edited Loading

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

europ Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

skateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy left a comment

Choose a reason for hiding this comment

europ commented Mar 26, 2018

miq-bot commented Mar 26, 2018

Fryguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

europ commented Mar 22, 2018 •

edited

Loading

europ Mar 26, 2018 •

edited

Loading

europ Mar 26, 2018 •

edited

Loading