From fa50bd13deee2f36bb7745a04dda478e583c601f Mon Sep 17 00:00:00 2001 From: Ana Rosas Date: Thu, 26 May 2016 16:23:35 +0200 Subject: [PATCH 1/5] Abstract enqueue restart service, to use it with Jobs and Builds --- lib/travis/api/app/endpoint/builds.rb | 4 +-- .../{enqueue_build.rb => restart_model.rb} | 29 ++++++++++++++----- 2 files changed, 23 insertions(+), 10 deletions(-) rename lib/travis/api/enqueue/services/{enqueue_build.rb => restart_model.rb} (56%) diff --git a/lib/travis/api/app/endpoint/builds.rb b/lib/travis/api/app/endpoint/builds.rb index ffa6b90a..e04dc25f 100644 --- a/lib/travis/api/app/endpoint/builds.rb +++ b/lib/travis/api/app/endpoint/builds.rb @@ -1,7 +1,7 @@ require 'travis/api/app' require 'travis/api/workers/build_cancellation' require 'travis/api/workers/build_restart' -require 'travis/api/enqueue/services/enqueue_build' +require 'travis/api/enqueue/services/restart_model' class Travis::Api::App class Endpoint @@ -50,7 +50,7 @@ class Travis::Api::App post '/:id/restart' do Metriks.meter("api.request.restart_build").mark if Travis::Features.owner_active?(:enqueue_to_hub, current_user) - service = Travis::Enqueue::Services::EnqueueBuild.new(current_user, params[:id]) + service = Travis::Enqueue::Services::RestartModel.new(current_user, { build_id: params[:id] }) if !service.accept? status 400 result = false diff --git a/lib/travis/api/enqueue/services/enqueue_build.rb b/lib/travis/api/enqueue/services/restart_model.rb similarity index 56% rename from lib/travis/api/enqueue/services/enqueue_build.rb rename to lib/travis/api/enqueue/services/restart_model.rb index 8776a5e6..7b7ae649 100644 --- a/lib/travis/api/enqueue/services/enqueue_build.rb +++ b/lib/travis/api/enqueue/services/restart_model.rb @@ -2,12 +2,13 @@ module Travis module Enqueue module Services - class EnqueueBuild - attr_reader :current_user, :build + class RestartModel + attr_reader :current_user, :target - def initialize(current_user, build_id) + def initialize(current_user, params) @current_user = current_user - @build = Build.find(build_id) + @params = params + target end def push(event, payload) @@ -24,20 +25,32 @@ module Travis def messages messages = [] - messages << { notice: "The build was successfully restarted." } if accept? + messages << { notice: "The #{type} was successfully restarted." } if accept? messages << { error: 'You do not seem to have sufficient permissions.' } unless permission? - messages << { error: "This build currently can not be restarted." } unless resetable? + messages << { error: "This #{type} currently can not be restarted." } unless resetable? messages end + def type + @type ||= @params[:build_id] ? :build : :job + end + + def target + if type == :build + @target = Build.find(@params[:build_id]) + else + @target = Job.find(@params[:job_id]) + end + end + private def permission? - current_user.permission?(required_role, repository_id: build.repository_id) + current_user.permission?(required_role, repository_id: target.repository_id) end def resetable? - build.resetable? + target.resetable? end def required_role From 6de524d84cbf9f76089f5bda906a060cc4b54607 Mon Sep 17 00:00:00 2001 From: Ana Rosas Date: Thu, 26 May 2016 17:08:22 +0200 Subject: [PATCH 2/5] Enqueue restarting jobs for the Hub --- lib/travis/api/app/endpoint/jobs.rb | 29 +++++++--- .../api/enqueue/services/restart_model.rb | 12 +++-- spec/integration/v2/builds_spec.rb | 2 +- spec/integration/v2/jobs_spec.rb | 53 ++++++++++++++----- 4 files changed, 69 insertions(+), 27 deletions(-) diff --git a/lib/travis/api/app/endpoint/jobs.rb b/lib/travis/api/app/endpoint/jobs.rb index 9bb2fbc9..e8a0e1e9 100644 --- a/lib/travis/api/app/endpoint/jobs.rb +++ b/lib/travis/api/app/endpoint/jobs.rb @@ -1,6 +1,7 @@ require 'travis/api/app' require 'travis/api/workers/job_cancellation' require 'travis/api/workers/job_restart' +require 'travis/api/enqueue/services/restart_model' class Travis::Api::App class Endpoint @@ -55,15 +56,27 @@ class Travis::Api::App post '/:id/restart' do Metriks.meter("api.request.restart_job").mark - - service = self.service(:reset_model, job_id: params[:id]) - if !service.accept? - status 400 - result = false + if Travis::Features.owner_active?(:enqueue_to_hub, current_user) + service = Travis::Enqueue::Services::RestartModel.new(current_user, { job_id: params[:id] }) + if !service.accept? + status 400 + result = false + else + payload = {id: params[:id], user_id: current_user.id} + service.push("job:restart", payload) + status 202 + result = true + end else - Travis::Sidekiq::JobRestart.perform_async(id: params[:id], user_id: current_user.id) - status 202 - result = true + service = self.service(:reset_model, job_id: params[:id]) + if !service.accept? + status 400 + result = false + else + Travis::Sidekiq::JobRestart.perform_async(id: params[:id], user_id: current_user.id) + status 202 + result = true + end end respond_with(result: result, flash: service.messages) end diff --git a/lib/travis/api/enqueue/services/restart_model.rb b/lib/travis/api/enqueue/services/restart_model.rb index 7b7ae649..ff9f5fec 100644 --- a/lib/travis/api/enqueue/services/restart_model.rb +++ b/lib/travis/api/enqueue/services/restart_model.rb @@ -12,11 +12,13 @@ module Travis end def push(event, payload) - ::Sidekiq::Client.push( - 'queue' => 'hub', - 'class' => 'Travis::Hub::Sidekiq::Worker', - 'args' => [event, payload] - ) + if current_user && target && accept? + ::Sidekiq::Client.push( + 'queue' => 'hub', + 'class' => 'Travis::Hub::Sidekiq::Worker', + 'args' => [event, payload] + ) + end end def accept? diff --git a/spec/integration/v2/builds_spec.rb b/spec/integration/v2/builds_spec.rb index 98a57e26..c4e78d44 100644 --- a/spec/integration/v2/builds_spec.rb +++ b/spec/integration/v2/builds_spec.rb @@ -117,7 +117,7 @@ describe 'Builds' do build.update_attribute(:state, 'passed') end - describe 'Enqueues restart event to the Hub' do + describe 'Enqueues restart event for the Hub' do before { Travis::Features.activate_owner(:enqueue_to_hub, repo.owner) } it 'restarts the build' do diff --git a/spec/integration/v2/jobs_spec.rb b/spec/integration/v2/jobs_spec.rb index 81486845..72dac3d0 100644 --- a/spec/integration/v2/jobs_spec.rb +++ b/spec/integration/v2/jobs_spec.rb @@ -274,24 +274,51 @@ describe 'Jobs' do response = post "/jobs/#{job.id}/restart", {}, headers response.status.should == 400 end + + context 'when enqueueing for the Hub' do + before { Travis::Features.activate_owner(:enqueue_to_hub, job.repository.owner) } + + it 'responds with 400' do + response = post "/jobs/#{job.id}/restart", {}, headers + response.status.should == 400 + end + end end context 'when job passed' do - before do - Travis::Sidekiq::JobCancellation.stubs(:perform_async) - job.update_attribute(:state, 'passed') + before { job.update_attribute(:state, 'passed') } + + context 'Restart from travis-core' do + before { Travis::Sidekiq::JobCancellation.stubs(:perform_async) } + + it 'restarts the job' do + Travis::Sidekiq::JobRestart.expects(:perform_async).with(id: job.id.to_s, user_id: user.id) + response = post "/jobs/#{job.id}/restart", {}, headers + response.status.should == 202 + end + it 'sends the correct response body' do + Travis::Sidekiq::JobRestart.expects(:perform_async).with(id: job.id.to_s, user_id: user.id) + response = post "/jobs/#{job.id}/restart", {}, headers + body = JSON.parse(response.body) + body.should == {"result"=>true, "flash"=>[{"notice"=>"The job was successfully restarted."}]} + end end - it 'restarts the job' do - Travis::Sidekiq::JobRestart.expects(:perform_async).with(id: job.id.to_s, user_id: user.id) - response = post "/jobs/#{job.id}/restart", {}, headers - response.status.should == 202 - end - it 'sends the correct response body' do - Travis::Sidekiq::JobRestart.expects(:perform_async).with(id: job.id.to_s, user_id: user.id) - response = post "/jobs/#{job.id}/restart", {}, headers - body = JSON.parse(response.body) - body.should == {"result"=>true, "flash"=>[{"notice"=>"The job was successfully restarted."}]} + context 'Enqueues restart event for the Hub' do + before { Travis::Features.activate_owner(:enqueue_to_hub, job.repository.owner) } + + it 'restarts the job' do + ::Sidekiq::Client.expects(:push) + response = post "/jobs/#{job.id}/restart", {}, headers + response.status.should == 202 + end + it 'sends the correct response body' do + ::Sidekiq::Client.expects(:push) + response = post "/jobs/#{job.id}/restart", {}, headers + body = JSON.parse(response.body) + body.should == {"result"=>true, "flash"=>[{"notice"=>"The job was successfully restarted."}]} + end + end end end From 8655fd19ce743ab4f28e5a9327913e4987d8ca3f Mon Sep 17 00:00:00 2001 From: Ana Rosas Date: Mon, 30 May 2016 15:46:59 +0200 Subject: [PATCH 3/5] Refacto endpoint --- lib/travis/api/app/endpoint/builds.rb | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/lib/travis/api/app/endpoint/builds.rb b/lib/travis/api/app/endpoint/builds.rb index e04dc25f..3a4243a2 100644 --- a/lib/travis/api/app/endpoint/builds.rb +++ b/lib/travis/api/app/endpoint/builds.rb @@ -51,25 +51,22 @@ class Travis::Api::App Metriks.meter("api.request.restart_build").mark if Travis::Features.owner_active?(:enqueue_to_hub, current_user) service = Travis::Enqueue::Services::RestartModel.new(current_user, { build_id: params[:id] }) - if !service.accept? - status 400 - result = false - else - payload = {id: params[:id], user_id: current_user.id} - service.push("build:restart", payload) - status 202 - result = true - end else service = self.service(:reset_model, build_id: params[:id]) - if !service.accept? - status 400 - result = false + end + + if !service.accept? + status 400 + result = false + else + if service.respond_to?(:push) + payload = {id: params[:id], user_id: current_user.id} + service.push("build:restart", payload) else Travis::Sidekiq::BuildRestart.perform_async(id: params[:id], user_id: current_user.id) - status 202 - result = true end + status 202 + result = true end respond_with(result: result, flash: service.messages) From 0038197aab4db5f1befa8a307c2f774a280ae9a7 Mon Sep 17 00:00:00 2001 From: Ana Rosas Date: Mon, 30 May 2016 18:10:11 +0200 Subject: [PATCH 4/5] Assign variable before if statement --- lib/travis/api/app/endpoint/builds.rb | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/travis/api/app/endpoint/builds.rb b/lib/travis/api/app/endpoint/builds.rb index 3a4243a2..cf6a39e0 100644 --- a/lib/travis/api/app/endpoint/builds.rb +++ b/lib/travis/api/app/endpoint/builds.rb @@ -49,24 +49,24 @@ class Travis::Api::App post '/:id/restart' do Metriks.meter("api.request.restart_build").mark - if Travis::Features.owner_active?(:enqueue_to_hub, current_user) - service = Travis::Enqueue::Services::RestartModel.new(current_user, { build_id: params[:id] }) + service = if Travis::Features.owner_active?(:enqueue_to_hub, current_user) + Travis::Enqueue::Services::RestartModel.new(current_user, build_id: params[:id]) else - service = self.service(:reset_model, build_id: params[:id]) + self.service(:reset_model, build_id: params[:id]) end - if !service.accept? + result = if !service.accept? status 400 - result = false - else - if service.respond_to?(:push) - payload = {id: params[:id], user_id: current_user.id} - service.push("build:restart", payload) - else - Travis::Sidekiq::BuildRestart.perform_async(id: params[:id], user_id: current_user.id) - end + false + elsif service.respond_to?(:push) + payload = { id: params[:id], user_id: current_user.id } + service.push("build:restart", payload) status 202 - result = true + true + else + Travis::Sidekiq::BuildRestart.perform_async(id: params[:id], user_id: current_user.id) + status 202 + true end respond_with(result: result, flash: service.messages) From 1c9212ef089e5df3d6b903056f6f8fbe4bc6a611 Mon Sep 17 00:00:00 2001 From: Ana Rosas Date: Wed, 1 Jun 2016 14:57:55 +0200 Subject: [PATCH 5/5] Refacto on restart endpoint for Jobs --- lib/travis/api/app/endpoint/jobs.rb | 38 ++++++++++++++--------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/lib/travis/api/app/endpoint/jobs.rb b/lib/travis/api/app/endpoint/jobs.rb index e8a0e1e9..c4d77228 100644 --- a/lib/travis/api/app/endpoint/jobs.rb +++ b/lib/travis/api/app/endpoint/jobs.rb @@ -56,28 +56,26 @@ class Travis::Api::App post '/:id/restart' do Metriks.meter("api.request.restart_job").mark - if Travis::Features.owner_active?(:enqueue_to_hub, current_user) - service = Travis::Enqueue::Services::RestartModel.new(current_user, { job_id: params[:id] }) - if !service.accept? - status 400 - result = false - else - payload = {id: params[:id], user_id: current_user.id} - service.push("job:restart", payload) - status 202 - result = true - end + service = if Travis::Features.owner_active?(:enqueue_to_hub, current_user) + Travis::Enqueue::Services::RestartModel.new(current_user, { job_id: params[:id] }) else - service = self.service(:reset_model, job_id: params[:id]) - if !service.accept? - status 400 - result = false - else - Travis::Sidekiq::JobRestart.perform_async(id: params[:id], user_id: current_user.id) - status 202 - result = true - end + self.service(:reset_model, job_id: params[:id]) end + + result = if !service.accept? + status 400 + false + elsif service.respond_to?(:push) + payload = {id: params[:id], user_id: current_user.id} + service.push("job:restart", payload) + status 202 + true + else + Travis::Sidekiq::JobRestart.perform_async(id: params[:id], user_id: current_user.id) + status 202 + result = true + end + respond_with(result: result, flash: service.messages) end