Improve PR search when gitarro is running with cron-like tools to avoid exceeding GitHub API limits #47
Conversation
juliogonzalez
changed the title
|
In the future we can also consider using Conditional Requests. At this moment I didn't find how to use them with octokit. |
|
And just as an example of a test hitting the limit: https://travis-ci.org/openSUSE/gitarro/builds/280934686?utm_source=github_status&utm_medium=notification :-) We now need to wait so the test can pass. |
juliogonzalez
force-pushed
the
FIX/limit-rate-new
branch
from
12f4569
to
2a986e9
Compare
|
@juliogonzalez lthx ooks really cool. i will review it with calm tomorrow |
| @@ -84,8 +90,7 @@ def retrigger_check(pr) | |||
|
|
|||
| # 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 | |||
There was a problem hiding this comment.
Small refactor to avoid rubocop complains.
lib/gitarro/backend.rb
Outdated
| @@ -117,8 +122,7 @@ def reviewed_pr_test(comm_st, pr) | |||
| 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) | |||
| @check ? exit(1) : launch_test_and_setup_status(@repo, pr) | |||
There was a problem hiding this comment.
Small refactor to avoid rubocop complains.
lib/gitarro/backend.rb
Outdated
| # a pr contain always a comments, cannot be nil | ||
| pr_comment.each do |com| | ||
| @client.issue_comments(repo, pr_number).each do |com| |
There was a problem hiding this comment.
Small refactor to avoid rubocop complains.
lib/gitarro/backend.rb
Outdated
| @@ -160,8 +163,7 @@ def magicword(repo, pr_number, context) | |||
| # 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) | |||
| files.each do |file| | |||
| @client.pull_request_files(repo, pr_number).each do |file| | |||
There was a problem hiding this comment.
Small refactor to avoid rubocop complains.
lib/gitarro/backend.rb
Outdated
| @@ -259,8 +261,7 @@ def do_changelog_test(repo, pr) | |||
| def changelog_changed(repo, 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) | |||
| return false if failed_status?(comm_st) || success_status?(comm_st) | |||
There was a problem hiding this comment.
Small refactor to avoid rubocop complains.
juliogonzalez
force-pushed
the
FIX/limit-rate-new
branch
from
2a986e9
to
33133da
Compare
tests/unit_tests/t_backend.rb
Outdated
| end | ||
|
|
||
| def test_get_no_prs | ||
| @full_hash[:repo] = 'openSUSE/gitbot' |
tests/unit_tests/t_backend.rb
Outdated
| @@ -48,4 +49,19 @@ def assert_file_non_ex(gbex, test_file) | |||
| assert_equal("'#{test_file}\' doesn't exists.Enter valid file, -t option", | |||
| ex.message) | |||
| end | |||
|
|
|||
| def test_get_all_prs | |||
| @full_hash[:repo] = 'openSUSE/gitbot' | |||
juliogonzalez
force-pushed
the
FIX/limit-rate-new
branch
from
33133da
to
f357508
Compare
gitarro.rb
Outdated
| prs = b.open_newer_prs | ||
|
|
||
| # Exit without errors if no PRs were found | ||
| if prs.empty? |
There was a problem hiding this comment.
even if rubocop will complain, is good to move this function to a class to backend.rb.
I would put it in the general class, or we could create a module which is included by the main class of backend.
There was a problem hiding this comment.
Ok, maybe we should call it inside Backend.open_newer_prs() and exit if the array is empty?
lib/gitarro/backend.rb
Outdated
| def open_newer_prs | ||
| prs = [] | ||
| @client.pull_requests(@repo, state: 'open').each do |pr| | ||
| if Time.now.utc - pr.updated_at < @options[:change_newer] * 60 || |
There was a problem hiding this comment.
i think we need here a private method for 2 reasons:
something like: ( naming could be improved)
def pr_comment_commit_changed_less_then(pr, min)
return true if Time.now.utc - pr.updated_at < @options[:change_newer] * 60 || ..
end- for making the code more abstract from the algorithm itself, and more maintenable.
- it will remove the the if and end block, making a inline if (more compact)
There was a problem hiding this comment.
Ok, but maybe pr_changed_less_than(pr, min) will be a easier name. What do you think?
There was a problem hiding this comment.
Yop, but if so i would make a comment above the function saying what trigger tje change
lib/gitarro/opt_parser.rb
Outdated
| change_newer_desc = 'If present, will only check PRs with a change in ' \ | ||
| 'the last X minutes' | ||
| opt.on("--change_newer 'MINUTES'", change_newer_desc) do |change_newer| | ||
| @options[:change_newer] = Integer(change_newer) |
There was a problem hiding this comment.
as the first glance i was wondering why there is an Integer and not a boolean here.
I think that the name change_newer_since can remove all doubts, and we can accept the integer.
Otherwise, as an option i would expect a boolean for activating something.
There was a problem hiding this comment.
Sounds good. I will change it.
juliogonzalez
force-pushed
the
FIX/limit-rate-new
branch
3 times, most recently
from
7b99a0a
to
d8621fc
Compare
tests/spec/cmdline_spec.rb
Outdated
| it 'gitarro should see at least PR #30 changed in the last 60 seconds' do | ||
| context = 'changed-since' | ||
| desc = 'changed-since' | ||
| @comment = @rgit.create_comment(@pr, "gitarro rerun #{context} !!!") |
There was a problem hiding this comment.
you need here to target explicetly the PR number 30, you will write on the first pr open which is not the 30.
i
juliogonzalez
force-pushed
the
FIX/limit-rate-new
branch
5 times, most recently
from
9bd3b20
to
660bec5
Compare
tests/spec/cmdline_spec.rb
Outdated
| context = 'changed-since' | ||
| desc = 'changed-since' | ||
| @pr = @rgit.pr_by_number(PR_NUMBER) | ||
| @comment = @rgit.create_comment(@pr, "gitarro rerun #{context} !!!") |
There was a problem hiding this comment.
here i would write instead of @comment comment, since we don't need a class var
There was a problem hiding this comment.
Done, pending test and push.
tests/spec/cmdline_spec.rb
Outdated
| 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) |
There was a problem hiding this comment.
Done, pending test and push.
|
About the refactor: I can't do nothing but fully agree with you. I always say the same thing and this time I even broke my own rules. My mistake :-) That said, I think we should open an issue to refactor Backend class as a priority. It's growing and most probably we can simplify some stuff and move other stuff to a new class. In the end most of the refactors we are adding are to prevent rubocop from complaining about methods or classes being bigger than the rules allow. About all other changes: I will have a look and will fix. |
gitarro.rb
Outdated
| @@ -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.check_pr_list_empty(prs) | |||
There was a problem hiding this comment.
following issue #64 we can rename this as
prs_list_empty?
There was a problem hiding this comment.
Done, pending test and push.
lib/gitarro/backend.rb
Outdated
| # 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 check_pr_list_empty(prs) |
There was a problem hiding this comment.
Done, pending test and push.
lib/gitarro/backend.rb
Outdated
| def open_newer_prs | ||
| prs = [] | ||
| @client.pull_requests(@repo, state: 'open').each do |pr| | ||
| prs << pr if pr_last_update_less_than(pr, @options[:changed_since]) |
There was a problem hiding this comment.
cosmetical :
https://stackoverflow.com/questions/10569529/ruby-difference-between-array-and-arraypush
I tend to prefer prs.push(pr) (since it is more verbose) but << it is ok
There was a problem hiding this comment.
Done, pending test and push.
lib/gitarro/backend.rb
Outdated
| client.create_status(@repo, pr.head.sha, status, context: @context, | ||
| description: @description, | ||
| target_url: @target_url) | ||
| true |
There was a problem hiding this comment.
i think this true should go on the respectives public method
retrigger_pr_number etc
There was a problem hiding this comment.
I think I placed this at the wrong place (create_status)
Shouldn't we place it a launch_test_and_setup_status since it's in use by all the public methods? In the end it's being used by all of them (with the exception of changelog) that are being used at /gitarro.rb
juliogonzalez
force-pushed
the
FIX/limit-rate-new
branch
from
660bec5
to
04555a6
Compare
|
Done. Unit and acceptance tests passed locally. |
lib/gitarro/backend.rb
Outdated
| return false | ||
| end | ||
| pr_all_files_type(@repo, pr.number, @file_type) | ||
| return false unless @pr_files.any? | ||
| return false unless pr_all_files_type(pr.number, @file_type).any? |
There was a problem hiding this comment.
nitpick: imho we can refactor this;
return pr_all_files_type(pr.number, @file_type).any?Which will return false if there is no file and true if any
and we can remove line 294 true
There was a problem hiding this comment.
I think we can just refactor as:
pr_all_files_type(pr.number, @file_type).any?
Can't we?
juliogonzalez
force-pushed
the
FIX/limit-rate-new
branch
from
04555a6
to
55b34ef
Compare
lib/gitarro/backend.rb
Outdated
| # public method for get prs opened and matching the changed_since | ||
| # condition | ||
| def open_newer_prs | ||
| prs = [] |
There was a problem hiding this comment.
Ntipick: we can refactor this with a beatiful ruby enumerable class.
prs = @client.pull_requests(@repo, state: 'open').select do
pr_last_update_less_than(pr, @options[:changed_since])
endThere was a problem hiding this comment.
$ irb
irb(main):001:0> [1,2,3].select {|n| n > 4 }
=> []http://ruby-doc.org/core-2.2.2/Enumerable.html#method-i-select
…id exceeding GitHub API limits
juliogonzalez
force-pushed
the
FIX/limit-rate-new
branch
from
55b34ef
to
5fa3a03
Compare
|
|
|
@juliogonzalez i think we can upload the new version in production with this. |
|
Sure, can you update the gem while I update the docker image? |
|
Maybe we should also create a release for each time we update the Gem |
|
yop, imho i would wait to create the gem until a week/3-4 days of running in production. ( in case there is a bug that we overlooked and so on that we need to fix) so in this way we prevent to publish buggy gems and releases. |
|
Ok, sounds fine. Updating Docker image and changing jobs. |
|
|
Since some people (including us) is using a cron-like pull instead of GitHub Webhooks, knowing which PRs were updated since the last time the cron ran a pull can avoid some request to the API (otherwise it is easy to hit the request limit easily -as we do now- and get failed tests just because we can't contact GitHub).
New parameter is
--changed_since <SECONDS>If present it will consider only PRs with changes newer than X seconds.
So for example, if we look for PRs requiring tests each 5 minutes, and we set here
--changed_since=60it will ignore open PRs unless there were changes in the last 60 seconss.A change is considered a commit or a comment, and it will update the field
updated_atat GitHub API (if the PR was just opened thenupdated_athas the same value ascreated_at.If the parameter is NOT present, gitarro will work as usual.
If
SECONDSis 0 then it will ignore all PRs. And negative values is the same as not using the parameters. These last two options are useful for testing, including unit tests.