Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make watching base branch changes and waiting for idle jenkins job optional #51

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions config/sample-config.yaml.erb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
# This makes sure we don't end up spamming GitHub's servers.
:github_polling_interval_seconds: 60

# By default Jently reschedules open pull request if base branch has been updated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may need a bit more explanation as I'm not sure how many people are familiar with the term 'base brach'

:github_watch_base_branch_update: true

# If access to your Jenkins CI is restricted with a login and password,
# you can enther these here. Otherwise you should comment out or delete these lines.
:jenkins_login: jenkins_login
Expand All @@ -54,6 +57,9 @@
# order to query the state of the test job.
:jenkins_polling_interval_seconds: 60

# Wait while jenkins job becomes idle
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs a bit more explanation as well. Not everyone knows about Jenkins executors :/

:jenkins_wait_for_idle_executor: true

# By default, Jently tests all pull requests. If you only want Jently to test pull requests that
# are to be merged into specific branches, uncomment the following, and specify these branches, one per line.
#:whitelist_branches:
Expand Down
5 changes: 3 additions & 2 deletions lib/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def Core.test_pull_request(pull_request_id)
end

if pull_request[:mergeable] == true
Jenkins.wait_for_idle_executor
Jenkins.wait_for_idle_executor if config.fetch(:jenkins_wait_for_idle_executor, true)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you use the config[:jenkins_wait_for_idle_executor] syntax instead? If there is a need for defaults, those should be set in https://github.com/vaneyckt/Jently/blob/master/lib/helpers/config_file.rb.


thr = Thread.new do
Github.set_pull_request_status(pull_request_id, {:status => 'pending', :description => 'Started work on pull request.'})
Expand All @@ -28,12 +28,13 @@ def Core.test_pull_request(pull_request_id)
end

def Core.poll_pull_requests_and_queue_next_job
config = ConfigFile.read
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no real need for all those extra spaces here :)

open_pull_requests_ids = Github.get_open_pull_requests_ids
PullRequestsData.remove_dead_pull_requests(open_pull_requests_ids)

open_pull_requests_ids.each do |pull_request_id|
pull_request = Github.get_pull_request(pull_request_id)
if PullRequestsData.outdated_success_status?(pull_request)
if PullRequestsData.outdated_success_status?(pull_request) && config.fetch(:github_watch_base_branch_update, true)
Github.set_pull_request_status(pull_request[:id], {:status => 'success', :description => "This has been rescheduled for testing as the '#{pull_request[:base_branch]}' branch has been updated."})
end
PullRequestsData.update(pull_request)
Expand Down
3 changes: 2 additions & 1 deletion lib/helpers/pull_requests_data.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def PullRequestsData.get_new_priority(pull_request)
def PullRequestsData.test_required?(pull_request)
return false if pull_request[:merged]

config = ConfigFile.read
data = read
is_new = !data.has_key?(pull_request[:id])

Expand All @@ -74,7 +75,7 @@ def PullRequestsData.test_required?(pull_request)
has_valid_status = ['success', 'failure'].include?(pull_request[:status])

was_updated = (is_new) ? false : (data[pull_request[:id]][:head_sha] != pull_request[:head_sha]) ||
(data[pull_request[:id]][:base_sha] != pull_request[:base_sha])
(data[pull_request[:id]][:base_sha] != pull_request[:base_sha] && config.fetch(:github_watch_base_branch_update, true))

is_test_required = is_new || is_waiting_to_be_tested || has_inconsistent_status || has_invalid_status || (has_valid_status && was_updated)
end
Expand Down
1 change: 1 addition & 0 deletions lib/jenkins.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def Jenkins.start_job(pull_request_id)
req.params[:id] = job_id
req.params[:branch] = "origin/pr/#{pull_request_id}/merge"
req.params[:repository] = config[:github_ssh_repository]
req.params[:pull_request_id] = pull_request_id
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you line up all the '=' signs here? It makes the code easier on the eyes

req.params[:random] = Time.now.to_i
end
job_id
Expand Down
38 changes: 38 additions & 0 deletions spec/core_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
PullRequestsData.stub(:update)
PullRequestsData.stub(:get_pull_request_id_to_test).and_return(nil)
Core.stub(:test_pull_request)
ConfigFile.stub(:read).and_return({})
end

it 'retrieves a list of open pull request ids from Github' do
Expand Down Expand Up @@ -69,6 +70,29 @@
Core.poll_pull_requests_and_queue_next_job
end
end

context 'when base branch has been updated and github_watch_base_branch_update is false' do
it 'does not trigger any new pull request tests' do
PullRequestsData.stub(:outdated_success_status?).and_return(true)
ConfigFile.stub(:read).and_return(:github_watch_base_branch_update => false)
Github.should_not_receive(:set_pull_request_status)

Core.poll_pull_requests_and_queue_next_job
end
end

context 'when base branch has been updated and github_watch_base_branch_update is true' do
it 'triggers any new pull request tests' do
PullRequestsData.stub(:outdated_success_status?).and_return(true)
ConfigFile.stub(:read).and_return(:github_watch_base_branch_update => true)
Github.should_receive(:set_pull_request_status)

Core.poll_pull_requests_and_queue_next_job
end
end


context
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this 'context' an oversight?

end

describe '.test_pull_request' do
Expand Down Expand Up @@ -126,6 +150,20 @@
Core.test_pull_request(pull_request_id)
end

it 'should not wait for an idle executor if jenkins_wait_for_idle_executor is false' do
Jenkins.should_not_receive(:wait_for_idle_executor)
ConfigFile.stub(:read).and_return(:jenkins_wait_for_idle_executor => false)

Core.test_pull_request(pull_request_id)
end

it 'should wait for an idle executor if jenkins_wait_for_idle_executor is false' do
Jenkins.should_not_receive(:wait_for_idle_executor)
ConfigFile.stub(:read).and_return(:jenkins_wait_for_idle_executor => false)

Core.test_pull_request(pull_request_id)
end

context 'when the Jenkins job takes less time than the jenkins_job_timeout_seconds value' do
it 'does not tell Github to mark the pull request status as timed out' do
Jenkins.stub(:wait_on_job) do |job|
Expand Down
22 changes: 22 additions & 0 deletions spec/helpers/pull_requests_data_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@

before do
PullRequestsData.write( id => stored_pr )
ConfigFile.stub(:read).and_return({})
end

it 'is true if the stored pr and specified pr have different a head sha' do
Expand All @@ -282,6 +283,7 @@

before do
PullRequestsData.write( id => stored_pr )
ConfigFile.stub(:read).and_return({})
end

it 'is true if the stored pr and specified pr have different a head sha' do
Expand All @@ -296,6 +298,26 @@
PullRequestsData.test_required?( stored_pr ).should be_false
end
end

context 'when base sha has changed' do
let(:head_sha) { 'abc123' }
let(:base_sha) { 'def456' }
let(:stored_pr) { {:id => id, :merged => false, :status => 'success', :head_sha => head_sha, :base_sha => base_sha } }

before do
PullRequestsData.write( id => stored_pr )
end

it 'is true when github_watch_base_branch_update set to true' do
ConfigFile.stub(:read).and_return(:github_watch_base_branch_update => true)
PullRequestsData.test_required?( stored_pr.merge(:base_sha => 'othersha') ).should be_true
end

it 'is false when github_watch_base_branch_update set to false' do
ConfigFile.stub(:read).and_return(:github_watch_base_branch_update => false)
PullRequestsData.test_required?( stored_pr.merge(:base_sha => 'othersha') ).should be_false
end
end
end
end
end
Expand Down