From 5fa3a03cb87ce0c3abcbb161566f997eff6a4e9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julio=20Gonz=C3=A1lez=20Gil?= Date: Thu, 28 Sep 2017 17:54:35 +0200 Subject: [PATCH] Improve PR search when gitarro is running with cron-like tools to avoid exceeding GitHub API limits --- .rubocop_todo.yml | 6 +- doc/BASICS.md | 3 + gitarro.rb | 4 +- lib/gitarro/backend.rb | 149 ++++++++++++++++++---------------- lib/gitarro/opt_parser.rb | 11 +++ tests/spec/cmdline_spec.rb | 12 +++ tests/spec/test_lib.rb | 15 ++++ tests/unit_tests/t_backend.rb | 27 ++++-- 8 files changed, 146 insertions(+), 81 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index b0d9463..4e120b2 100644 --- a/.rubocop_todo.yml +++ b/.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 diff --git a/doc/BASICS.md b/doc/BASICS.md index 143d266..b593791 100644 --- a/doc/BASICS.md +++ b/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 diff --git a/gitarro.rb b/gitarro.rb index 9e1fd19..c158bb9 100755 --- a/gitarro.rb +++ b/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. diff --git a/lib/gitarro/backend.rb b/lib/gitarro/backend.rb index bdd0ac0..bb631e1 100755 --- a/lib/gitarro/backend.rb +++ b/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,50 +63,57 @@ 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) @@ -114,54 +121,59 @@ def reviewed_pr_test(comm_st, pr) # 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 diff --git a/lib/gitarro/opt_parser.rb b/lib/gitarro/opt_parser.rb index 663b39e..cede553 100755 --- a/lib/gitarro/opt_parser.rb +++ b/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 diff --git a/tests/spec/cmdline_spec.rb b/tests/spec/cmdline_spec.rb index 2845c69..3248cd6 100755 --- a/tests/spec/cmdline_spec.rb +++ b/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 diff --git a/tests/spec/test_lib.rb b/tests/spec/test_lib.rb index 93126d2..eb22253 100755 --- a/tests/spec/test_lib.rb +++ b/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) diff --git a/tests/unit_tests/t_backend.rb b/tests/unit_tests/t_backend.rb index 058797f..2fe4697 100755 --- a/tests/unit_tests/t_backend.rb +++ b/tests/unit_tests/t_backend.rb @@ -5,10 +5,13 @@ Dir.chdir Dir.pwd # Test the option parser class BackendTest2 < Minitest::Test - def test_full_option_import2 + def setup @full_hash = { repo: 'gino/gitarro', context: 'python-t', description: 'functional', test_file: 'gino.sh', file_type: '.sh', - git_dir: 'gitty' } + git_dir: 'gitty', changed_since: -1 } + end + + def test_full_option_import2 gitarro = Backend.new(@full_hash) puts gitarro.j_status gitarro.j_status = 'foo' @@ -25,9 +28,7 @@ def gitarro_assert(gitarro) end def test_run_script - @full_hash = { repo: 'gino/gitarro', context: 'python-t', description: - 'functional', test_file: 'test_data/script_ok.sh', - file_type: '.sh', git_dir: 'gitty' } + @full_hash[:test_file] = 'test_data/script_ok.sh' gbex = TestExecutor.new(@full_hash) ck_files(gbex) test_file = 'nofile.txt' @@ -48,4 +49,20 @@ def assert_file_non_ex(gbex, test_file) assert_equal("'#{test_file}\' doesn't exists.Enter valid file, -t option", ex.message) end + + # We always have PR #30 opened + def test_get_all_prs + @full_hash[:repo] = 'openSUSE/gitarro' + gitarro = Backend.new(@full_hash) + prs = gitarro.open_newer_prs + assert(true, prs.any?) + end + + def test_get_no_prs + @full_hash[:repo] = 'openSUSE/gitarro' + @full_hash[:changed_since] = 0 + gitarro = Backend.new(@full_hash) + prs = gitarro.open_newer_prs + assert(0, prs.count) + end end