summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMayra Cabrera <mcabrera@gitlab.com>2019-06-18 22:08:30 +0000
committerMayra Cabrera <mcabrera@gitlab.com>2019-06-18 22:08:30 +0000
commit57a1c98df060836ec85f9da7383acab71c6602fa (patch)
tree022132b6fa8aa927f0e5ac5cc3b1292ec4efe3f9
parent6fa900547dbd30b0db0070f87dbeb4b05d485b9b (diff)
parent34a283f90a56914306f77384e7b54c2dbd2e467a (diff)
downloadgitlab-ce-57a1c98df060836ec85f9da7383acab71c6602fa.tar.gz
Merge branch '62183-update-response-code-for-bulk-delete-api-for-container-registry' into 'master'
Update response code for container registry api bulk delete Closes #62183 See merge request gitlab-org/gitlab-ce!29448
-rw-r--r--app/workers/cleanup_container_repository_worker.rb29
-rw-r--r--changelogs/unreleased/62183-update-response-code-for-bulk-delete-api-for-container-registry.yml5
-rw-r--r--lib/api/container_registry.rb10
-rw-r--r--spec/requests/api/container_registry_spec.rb21
-rw-r--r--spec/workers/cleanup_container_repository_worker_spec.rb8
5 files changed, 39 insertions, 34 deletions
diff --git a/app/workers/cleanup_container_repository_worker.rb b/app/workers/cleanup_container_repository_worker.rb
index 974ee8c8146..0331fc7b01c 100644
--- a/app/workers/cleanup_container_repository_worker.rb
+++ b/app/workers/cleanup_container_repository_worker.rb
@@ -2,12 +2,9 @@
class CleanupContainerRepositoryWorker
include ApplicationWorker
- include ExclusiveLeaseGuard
queue_namespace :container_repository
- LEASE_TIMEOUT = 1.hour
-
attr_reader :container_repository, :current_user
def perform(current_user_id, container_repository_id, params)
@@ -16,11 +13,9 @@ class CleanupContainerRepositoryWorker
return unless valid?
- try_obtain_lease do
- Projects::ContainerRepository::CleanupTagsService
- .new(project, current_user, params)
- .execute(container_repository)
- end
+ Projects::ContainerRepository::CleanupTagsService
+ .new(project, current_user, params)
+ .execute(container_repository)
end
private
@@ -32,22 +27,4 @@ class CleanupContainerRepositoryWorker
def project
container_repository&.project
end
-
- # For ExclusiveLeaseGuard concern
- def lease_key
- @lease_key ||= "container_repository:cleanup_tags:#{container_repository.id}"
- end
-
- # For ExclusiveLeaseGuard concern
- def lease_timeout
- LEASE_TIMEOUT
- end
-
- # For ExclusiveLeaseGuard concern
- def lease_release?
- # we don't allow to execute this worker
- # more often than LEASE_TIMEOUT
- # for given container repository
- false
- end
end
diff --git a/changelogs/unreleased/62183-update-response-code-for-bulk-delete-api-for-container-registry.yml b/changelogs/unreleased/62183-update-response-code-for-bulk-delete-api-for-container-registry.yml
new file mode 100644
index 00000000000..ce75a55efdc
--- /dev/null
+++ b/changelogs/unreleased/62183-update-response-code-for-bulk-delete-api-for-container-registry.yml
@@ -0,0 +1,5 @@
+---
+title: Return 400 when deleting tags more often than once per hour.
+merge_request: 29448
+author:
+type: changed
diff --git a/lib/api/container_registry.rb b/lib/api/container_registry.rb
index 7d9b5e1a598..7dad20a822a 100644
--- a/lib/api/container_registry.rb
+++ b/lib/api/container_registry.rb
@@ -68,6 +68,9 @@ module API
delete ':id/registry/repositories/:repository_id/tags', requirements: REGISTRY_ENDPOINT_REQUIREMENTS do
authorize_admin_container_image!
+ message = 'This request has already been made. You can run this at most once an hour for a given container repository'
+ render_api_error!(message, 400) unless obtain_new_cleanup_container_lease
+
CleanupContainerRepositoryWorker.perform_async(current_user.id, repository.id,
declared_params.except(:repository_id)) # rubocop: disable CodeReuse/ActiveRecord
@@ -123,6 +126,13 @@ module API
authorize! :admin_container_image, repository
end
+ def obtain_new_cleanup_container_lease
+ Gitlab::ExclusiveLease
+ .new("container_repository:cleanup_tags:#{repository.id}",
+ timeout: 1.hour)
+ .try_obtain
+ end
+
def repository
@repository ||= user_project.container_repositories.find(params[:repository_id])
end
diff --git a/spec/requests/api/container_registry_spec.rb b/spec/requests/api/container_registry_spec.rb
index 4ad15ed6bea..b64f3ea1081 100644
--- a/spec/requests/api/container_registry_spec.rb
+++ b/spec/requests/api/container_registry_spec.rb
@@ -1,6 +1,8 @@
require 'spec_helper'
describe API::ContainerRegistry do
+ include ExclusiveLeaseHelpers
+
set(:project) { create(:project, :private) }
set(:maintainer) { create(:user) }
set(:developer) { create(:user) }
@@ -155,7 +157,10 @@ describe API::ContainerRegistry do
older_than: '1 day' }
end
+ let(:lease_key) { "container_repository:cleanup_tags:#{root_repository.id}" }
+
it 'schedules cleanup of tags repository' do
+ stub_exclusive_lease(lease_key, timeout: 1.hour)
expect(CleanupContainerRepositoryWorker).to receive(:perform_async)
.with(maintainer.id, root_repository.id, worker_params)
@@ -163,6 +168,22 @@ describe API::ContainerRegistry do
expect(response).to have_gitlab_http_status(:accepted)
end
+
+ context 'called multiple times in one hour' do
+ it 'returns 400 with an error message' do
+ stub_exclusive_lease_taken(lease_key, timeout: 1.hour)
+ subject
+
+ expect(response).to have_gitlab_http_status(400)
+ expect(response.body).to include('This request has already been made.')
+ end
+
+ it 'executes service only for the first time' do
+ expect(CleanupContainerRepositoryWorker).to receive(:perform_async).once
+
+ 2.times { subject }
+ end
+ end
end
end
end
diff --git a/spec/workers/cleanup_container_repository_worker_spec.rb b/spec/workers/cleanup_container_repository_worker_spec.rb
index 5bee7294010..9be8f882785 100644
--- a/spec/workers/cleanup_container_repository_worker_spec.rb
+++ b/spec/workers/cleanup_container_repository_worker_spec.rb
@@ -35,13 +35,5 @@ describe CleanupContainerRepositoryWorker, :clean_gitlab_redis_shared_state do
subject.perform(user.id, -1, params)
end.not_to raise_error
end
-
- context 'when executed twice in short period' do
- it 'executes service only for the first time' do
- expect(service).to receive(:execute).once
-
- 2.times { subject.perform(user.id, repository.id, params) }
- end
- end
end
end