From 30b36c92c386e93b432166fb6f9dd973882a6d82 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Tue, 15 Mar 2016 11:03:43 +0100
Subject: [PATCH] Use an exception to pass messages
---
app/controllers/projects_controller.rb | 15 ++++++++----
app/services/git_push_service.rb | 1 +
app/services/projects/housekeeping_service.rb | 19 ++++++++-------
spec/services/git_push_service_spec.rb | 24 ++++++++++++++-----
.../projects/housekeeping_service_spec.rb | 6 ++---
5 files changed, 42 insertions(+), 23 deletions(-)
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index 2a3dc5c79f7..36f37221c58 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -170,12 +170,17 @@ class ProjectsController < ApplicationController
end
def housekeeping
- message = ::Projects::HousekeepingService.new(@project).execute
+ ::Projects::HousekeepingService.new(@project).execute
- respond_to do |format|
- flash[:notice] = message
- format.html { redirect_to project_path(@project) }
- end
+ redirect_to(
+ project_path(@project),
+ notice: "Housekeeping successfully started"
+ )
+ rescue ::Projects::HousekeepingService::LeaseTaken => ex
+ redirect_to(
+ edit_project_path(@project),
+ alert: ex.to_s
+ )
end
def toggle_star
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index e50cbdfb602..4313de0ccab 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -79,6 +79,7 @@ class GitPushService < BaseService
housekeeping = Projects::HousekeepingService.new(@project)
housekeeping.increment!
housekeeping.execute if housekeeping.needed?
+ rescue Projects::HousekeepingService::LeaseTaken
end
def process_default_branch
diff --git a/app/services/projects/housekeeping_service.rb b/app/services/projects/housekeeping_service.rb
index 83bdedf7a8d..bccd67d3dbf 100644
--- a/app/services/projects/housekeeping_service.rb
+++ b/app/services/projects/housekeeping_service.rb
@@ -11,20 +11,22 @@ module Projects
LEASE_TIMEOUT = 3600
+ class LeaseTaken < StandardError
+ def to_s
+ "Somebody already triggered housekeeping for this project in the past #{LEASE_TIMEOUT / 60} minutes"
+ end
+ end
+
def initialize(project)
@project = project
end
def execute
- if !try_obtain_lease
- return "Housekeeping was already triggered in the past #{LEASE_TIMEOUT / 60} minutes"
- end
+ raise LeaseTaken if !try_obtain_lease
GitlabShellWorker.perform_async(:gc, @project.path_with_namespace)
- @project.pushes_since_gc = 0
- @project.save!
-
- "Housekeeping successfully started"
+ ensure
+ @project.update_column(:pushes_since_gc, 0)
end
def needed?
@@ -32,8 +34,7 @@ module Projects
end
def increment!
- @project.pushes_since_gc += 1
- @project.save!
+ @project.increment!(:pushes_since_gc)
end
private
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index ebf3ec1f5fd..145bc937560 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -402,7 +402,7 @@ describe GitPushService, services: true do
end
describe "housekeeping" do
- let(:housekeeping) { instance_double('Projects::HousekeepingService', increment!: nil, needed?: false) }
+ let(:housekeeping) { Projects::HousekeepingService.new(project) }
before do
allow(Projects::HousekeepingService).to receive(:new).and_return(housekeeping)
@@ -414,16 +414,28 @@ describe GitPushService, services: true do
execute_service(project, user, @oldrev, @newrev, @ref)
end
- it 'performs housekeeping when needed' do
- expect(housekeeping).to receive(:needed?).and_return(true)
- expect(housekeeping).to receive(:execute)
+ context 'when housekeeping is needed' do
+ before do
+ allow(housekeeping).to receive(:needed?).and_return(true)
+ end
- execute_service(project, user, @oldrev, @newrev, @ref)
+ it 'performs housekeeping' do
+ expect(housekeeping).to receive(:execute)
+
+ execute_service(project, user, @oldrev, @newrev, @ref)
+ end
+
+ it 'does not raise an exception' do
+ allow(housekeeping).to receive(:try_obtain_lease).and_return(false)
+
+ execute_service(project, user, @oldrev, @newrev, @ref)
+ end
end
+
it 'increments the push counter' do
expect(housekeeping).to receive(:increment!)
-
+
execute_service(project, user, @oldrev, @newrev, @ref)
end
end
diff --git a/spec/services/projects/housekeeping_service_spec.rb b/spec/services/projects/housekeeping_service_spec.rb
index 4c3577149f9..93bf1b81fbe 100644
--- a/spec/services/projects/housekeeping_service_spec.rb
+++ b/spec/services/projects/housekeeping_service_spec.rb
@@ -14,7 +14,7 @@ describe Projects::HousekeepingService do
expect(subject).to receive(:try_obtain_lease).and_return(true)
expect(GitlabShellWorker).to receive(:perform_async).with(:gc, project.path_with_namespace)
- expect(subject.execute).to include('successfully started')
+ subject.execute
expect(project.pushes_since_gc).to eq(0)
end
@@ -22,8 +22,8 @@ describe Projects::HousekeepingService do
expect(subject).to receive(:try_obtain_lease).and_return(false)
expect(GitlabShellWorker).not_to receive(:perform_async)
- expect(subject.execute).to include('already triggered')
- expect(project.pushes_since_gc).to eq(3)
+ expect { subject.execute }.to raise_error(Projects::HousekeepingService::LeaseTaken)
+ expect(project.pushes_since_gc).to eq(0)
end
end
--
GitLab