Improve PR search when gitarro is running with cron-like tools to avoid exceeding GitHub API limits

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2017-09-30 17:37:11 +0200 using RuboCop version 0.50.0.
+# on 2017-10-02 15:36:34 +0200 using RuboCop version 0.50.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
@@ -9,9 +9,9 @@
 # Offense count: 2
 # Configuration parameters: CountComments, ExcludedMethods.
 Metrics/BlockLength:
-  Max: 32
+  Max: 43
 
 # Offense count: 2
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 169
+  Max: 166

doc/BASICS.md

@@ -29,6 +29,8 @@ There are two basic ways of using it:
 
   It works this way so if you are using Jenkins pulling the repository, you can have one job build for each gitarro execution.
 
+  It is also posible instruct gitarro to scan only the Pull Requests changed during the last X seconds (--changed_since). From GitHub API perspective, and gitarro's perspective a change is either a new commit or a new comment at the PR.
+
 * If you are using [webhooks](https://developer.github.com/webhooks/), then you just need to specify the ID of the Pull Requests that started the hook (--P or --PR)
 
 It is also posible to tell gitarro to ignore PRs unless specific files are changed (by path or by extension with -f or --file), and specify a URL to added to the Pull Requests with the link to the log with the test output, for example to a Jenkins log (-u o --url).
@@ -53,6 +55,7 @@ Optional options:
     -P                               '--PR 'NUMBER'
                                      Specify the PR number instead of checking all of them. This will force gitarro to run the against a specific PR number,even if it is not needed (useful for using Jenkins with GitHub webhooks).
         --https                      If present, use https instead of ssh for git operations
+        --changed_since 'SECONDS'    If present, will only check PRs with a change in the last X seconds
 
 Help:
     -h, --help                       help

gitarro.rb

@@ -8,7 +8,9 @@
 require_relative 'lib/gitarro/backend'
 
 b = Backend.new
-prs = b.open_prs
+prs = b.open_newer_prs
+exit 0 if b.pr_list_empty?(prs)
+
 prs.each do |pr|
   puts '=' * 30 + "\n" + "TITLE_PR: #{pr.title}, NR: #{pr.number}\n" + '=' * 30
   # this check the last commit state, catch for review or not reviewd status.

lib/gitarro/backend.rb

@@ -2,6 +2,7 @@
 
 require 'octokit'
 require 'optparse'
+require 'time'
 require 'English'
 require_relative 'opt_parser'
 require_relative 'git_op'
@@ -47,14 +48,13 @@ def script_exists?(script)
 # this the public class is the backend of gitarro,
 # were we execute the tests and so on
 class Backend
-  attr_accessor :j_status, :options, :client, :pr_files, :gbexec
+  attr_accessor :j_status, :options, :client, :gbexec
   # public method of backend
   def initialize(option = nil)
     Octokit.auto_paginate = true
     @client = Octokit::Client.new(netrc: true)
     @options = option.nil? ? OptParser.new.cmdline_options : option
     @j_status = ''
-    @pr_files = []
     # each options will generate a object variable dinamically
     @options.each do |key, value|
       instance_variable_set("@#{key}", value)
@@ -63,105 +63,117 @@ def initialize(option = nil)
     @gbexec = TestExecutor.new(@options)
   end
 
-  # public method for get prs opens
-  # given a repo
-  def open_prs
-    prs = @client.pull_requests(@repo, state: 'open')
-    puts 'no Pull request OPEN on the REPO!' unless prs.any?
+  # Check if a PR list is empty
+  # If it is and we do want to consider the update time, return true
+  # Otherwise return false
+  def pr_list_empty?(prs)
+    prs.empty? && @options[:changed_since] >= 0 ? true : false
+  end
+
+  # public method for get prs opened and matching the changed_since
+  # condition
+  def open_newer_prs
+    prs = @client.pull_requests(@repo, state: 'open').select do |pr|
+      pr_last_update_less_than(pr, @options[:changed_since])
+    end
+    if pr_list_empty?(prs)
+      puts 'There are no Pull Requests opened or with changes newer than ' \
+           "#{options[:changed_since]} seconds"
+    end
     prs
   end
 
   # public for etrigger the test
   def retrigger_check(pr)
     return unless retrigger_needed?(pr)
-    client.create_status(@repo, pr.head.sha, 'pending',
-                         context: @context, description: @description,
-                         target_url: @target_url)
+    create_status(pr, 'pending')
     exit 1 if @check
-    launch_test_and_setup_status(@repo, pr)
+    launch_test_and_setup_status(pr)
     j_status == 'success' ? exit(0) : exit(1)
   end
 
   # public always rerun tests against the pr number if this exists
   def trigger_by_pr_number(pr)
-    return false if @pr_number.nil?
-    return false if @pr_number != pr.number
+    return false if @pr_number.nil? || @pr_number != pr.number
     puts "Got triggered by PR_NUMBER OPTION, rerunning on #{@pr_number}"
-    launch_test_and_setup_status(@repo, pr)
-    true
+    launch_test_and_setup_status(pr)
   end
 
   # public method, trigger changelogtest if option active
   def changelog_active(pr, comm_st)
     return false unless @changelog_test
-    return false unless changelog_changed(@repo, pr, comm_st)
+    return false unless changelog_changed(pr, comm_st)
     true
   end
 
   def unreviewed_pr_test(pr, comm_st)
     return unless unreviewed_pr_ck(comm_st)
-    pr_all_files_type(@repo, pr.number, @file_type)
-    return if empty_files_changed_by_pr
+    pr_all_files_type(pr.number, @file_type)
+    return if empty_files_changed_by_pr(pr)
     # gb.check is true when there is a job running as scheduler
     # which doesn't execute the test but trigger another job
     return false if @check
-    launch_test_and_setup_status(@repo, pr)
-    true
+    launch_test_and_setup_status(pr)
   end
 
   def reviewed_pr_test(comm_st, pr)
     # if PR status is not on pending and the context is not set,
     #  we dont run the tests
     return false unless context_pr(comm_st) == false ||
                         pending_pr(comm_st) == true
-    pr_all_files_type(@repo, pr.number, @file_type)
     return true if changelog_active(pr, comm_st)
-    return false unless @pr_files.any?
-    exit 1 if @check
-    launch_test_and_setup_status(@repo, pr)
-    true
-  end
-
-  private
-
-  # this function setup first pending to PR, then execute the tests
-  # then set the status according to the results of script executed.
-  # pr_head = is the PR branch
-  # base = is a the upstream branch, where the pr targets
-  def launch_test_and_setup_status(repo, pr)
-    # pending
-    @client.create_status(repo, pr.head.sha, 'pending',
-                          context: @context, description: @description,
-                          target_url: @target_url)
-    # do tests
-    @j_status = gbexec.pr_test(pr)
-    # set status
-    @client.create_status(repo, pr.head.sha, @j_status,
-                          context: @context, description: @description,
-                          target_url: @target_url)
+    return false unless pr_all_files_type(pr.number, @file_type).any?
+    @check ? exit(1) : launch_test_and_setup_status(pr)
   end
 
   # this function will check if the PR contains in comment the magic word
   # # for retrigger all the tests.
-  def magicword(repo, pr_number, context)
+  def magicword(pr_number, context)
     magic_word_trigger = "gitarro rerun #{context} !!!"
-    pr_comment = @client.issue_comments(repo, pr_number)
     # a pr contain always a comments, cannot be nil
-    pr_comment.each do |com|
+    @client.issue_comments(@repo, pr_number).each do |com|
       # delete comment otherwise it will be retrigger infinetely
       if com.body.include? magic_word_trigger
-        @client.delete_comment(repo, com.id)
+        @client.delete_comment(@repo, com.id)
         return true
       end
     end
     false
   end
 
+  private
+
+  # Create a status for a PR
+  def create_status(pr, status)
+    client.create_status(@repo, pr.head.sha, status, context: @context,
+                                                     description: @description,
+                                                     target_url: @target_url)
+  end
+
+  # Return true if the PR was updated in less than the value of variable sec
+  # or if sec < 0 (the check was disabled)
+  # GitHub considers a PR updated when there is a new commit or a new comment
+  def pr_last_update_less_than(pr, sec)
+    Time.now.utc - pr.updated_at < sec || sec < 0 ? true : false
+  end
+
+  # this function setup first pending to PR, then execute the tests
+  # then set the status according to the results of script executed.
+  # pr_head = is the PR branch
+  # base = is a the upstream branch, where the pr targets
+  def launch_test_and_setup_status(pr)
+    # pending
+    create_status(pr, 'pending')
+    # do tests
+    @j_status = gbexec.pr_test(pr)
+    # set status
+    create_status(pr, @j_status)
+  end
+
   # check all files of a Prs Number if they are a specific type
   # EX: Pr 56, we check if files are '.rb'
-  def pr_all_files_type(repo, pr_number, type)
-    files = @client.pull_request_files(repo, pr_number)
-    @pr_files = filter_files_by_type(files, type)
+  def pr_all_files_type(pr_number, type)
+    filter_files_by_type(@client.pull_request_files(@repo, pr_number), type)
   end
 
   # by default type is 'notype', which imply we get all files
@@ -192,8 +204,8 @@ def pending_pr(comm_st)
 
   # if the Pr contains magic word, test changelog
   # is true
-  def magic_comment(repo, pr_num)
-    @client.issue_comments(repo, pr_num).each do |com|
+  def magic_comment(pr_num)
+    @client.issue_comments(@repo, pr_num).each do |com|
       if com.body.include?('no changelog needed!')
         @j_status = 'success'
         break
@@ -245,45 +257,38 @@ def failed_status?(comm_st)
 
   # control if the pr change add any files, specified
   # it can be also a dir
-  def empty_files_changed_by_pr
-    return if pr_files.any?
+  def empty_files_changed_by_pr(pr)
+    return if pr_all_files_type(pr.number, @file_type).any?
     puts "no files of type #{@file_type} found! skipping"
     true
   end
 
-  def do_changelog_test(repo, pr)
+  def do_changelog_test(pr)
     @j_status = 'failure'
-    pr_all_files_type(repo, pr.number, @file_type)
     # if the pr contains changes on .changes file, test ok
-    @j_status = 'success' if @pr_files.any?
-    magic_comment(repo, pr.number)
-    @client.create_status(repo, pr.head.sha, @j_status,
-                          context: @context, description: @description,
-                          target_url: @target_url)
-    true
+    @j_status = 'success' if pr_all_files_type(pr.number, @file_type).any?
+    magic_comment(pr.number)
+    create_status(pr, @j_status)
   end
 
   # do the changelog test and set status
-  def changelog_changed(repo, pr, comm_st)
+  def changelog_changed(pr, comm_st)
     return false unless @changelog_test
     # only execute 1 time, don"t run if test is failed, or ok
-    return false if failed_status?(comm_st)
-    return false if success_status?(comm_st)
-    do_changelog_test(repo, pr)
+    return false if failed_status?(comm_st) || success_status?(comm_st)
+    do_changelog_test(pr)
   end
 
   def retrigger_needed?(pr)
     # we want redo sometimes tests
-    return false unless magicword(@repo, pr.number, @context)
+    return false unless magicword(pr.number, @context)
     # changelog trigger
     if @changelog_test
-      do_changelog_test(@repo, pr)
+      do_changelog_test(pr)
       return false
     end
-    pr_all_files_type(@repo, pr.number, @file_type)
-    return false unless @pr_files.any?
     # if check is set, the comment in the trigger job will be del.
     # so setting it to pending, it will be remembered
-    true
+    pr_all_files_type(pr.number, @file_type).any?
   end
 end

lib/gitarro/opt_parser.rb

@@ -90,6 +90,15 @@ def pr_number(opt)
     end
   end
 
+  def changed_since(opt)
+    changed_since_desc = 'If present, will only check PRs with a ' \
+                       'change in the last X seconds'
+    opt.on("--changed_since 'SECONDS'",
+           changed_since_desc) do |changed_since|
+      @options[:changed_since] = Integer(changed_since)
+    end
+  end
+
   def optional_options(opt)
     opt.separator ''
     opt.separator 'Optional options:'
@@ -100,6 +109,7 @@ def optional_options(opt)
     url_opt(opt)
     pr_number(opt)
     https_opt(opt)
+    changed_since(opt)
   end
 end
 
@@ -164,6 +174,7 @@ def defaults_false
     @options[:changelog_test] = false if @options[:changelog_test].nil?
     @options[:target_url] = '' if @options[:target_url].nil?
     @options[:https] = false if @options[:https].nil?
+    @options[:changed_since] = -1 if @options[:changed_since].nil?
   end
 
   def defaults_to_text

tests/spec/cmdline_spec.rb

@@ -97,4 +97,16 @@ def self.pr_number
       expect(result).to be true
     end
   end
+
+  describe '.changed_since' do
+    it 'gitarro should see at least PR #30 changed in the last 60 seconds' do
+      context = 'changed-since'
+      desc = 'changed-since'
+      pr = @rgit.pr_by_number(PR_NUMBER)
+      comment = @rgit.create_comment(pr, "gitarro rerun #{context} !!!")
+      result = @test.changed_since_should_find(@comm_st, 60, context, desc)
+      @rgit.delete_c(comment.id)
+      expect(result).to be true
+    end
+  end
 end

tests/spec/test_lib.rb

@@ -31,6 +31,10 @@ def create_comment(pr, comment)
   def delete_c(comment_id)
     client.delete_comment(repo, comment_id)
   end
+
+  def pr_by_number(num)
+    client.pull_request(repo, num)
+  end
 end
 
 # gitarro functional tests
@@ -120,6 +124,17 @@ def file_type_unset(comm_st, cont)
     true
   end
 
+  def changed_since_should_find(com_st, sec, context, desc)
+    `echo '#! /bin/bash' > #{valid_test}`
+    `chmod +x #{valid_test}`
+    gitarro = "#{script} -r #{repo} -c #{context} -d #{desc} -g #{git_dir}" \
+                   " -t #{valid_test} -f #{ftype} -u #{url}" \
+                   " --changed_since #{sec}"
+    puts `ruby #{gitarro}`
+    return false if failed_status(com_st, context)
+    true
+  end
+
   private
 
   def create_test_script(script)