From db5b4b8b1a9b8aa07c8310dde53b7c3ed391bafd Mon Sep 17 00:00:00 2001 From: Andre Guedes Date: Wed, 22 Feb 2017 11:19:23 -0300 Subject: Creates specs for destroy service and improves namespace container image query performance --- app/models/namespace.rb | 2 +- app/services/container_images/destroy_service.rb | 26 ++--------------- app/views/admin/container_registry/show.html.haml | 2 +- .../20161031013926_create_container_image.rb | 16 ---------- ...egistry_access_token_to_application_settings.rb | 16 ---------- db/schema.rb | 10 +++---- lib/api/registry_events.rb | 4 +-- .../container_images/destroy_service_spec.rb | 34 ++++++++++++++++++++++ 8 files changed, 45 insertions(+), 65 deletions(-) create mode 100644 spec/services/container_images/destroy_service_spec.rb diff --git a/app/models/namespace.rb b/app/models/namespace.rb index c8e329044e0..a803be2e780 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -155,7 +155,7 @@ class Namespace < ActiveRecord::Base end def any_project_has_container_registry_images? - projects.any? { |project| project.container_images.present? } + projects.joins(:container_images).any? end def send_update_instructions diff --git a/app/services/container_images/destroy_service.rb b/app/services/container_images/destroy_service.rb index c73b6cfefba..15dca227291 100644 --- a/app/services/container_images/destroy_service.rb +++ b/app/services/container_images/destroy_service.rb @@ -1,31 +1,9 @@ module ContainerImages class DestroyService < BaseService - class DestroyError < StandardError; end - def execute(container_image) - @container_image = container_image - - return false unless can?(current_user, :remove_project, project) - - ContainerImage.transaction do - container_image.destroy! - - unless remove_container_image_tags - raise_error('Failed to remove container image tags. Please try again or contact administrator') - end - end - - true - end - - private - - def raise_error(message) - raise DestroyError.new(message) - end + return false unless can?(current_user, :update_container_image, project) - def remove_container_image_tags - container_image.delete_tags + container_image.destroy! end end end diff --git a/app/views/admin/container_registry/show.html.haml b/app/views/admin/container_registry/show.html.haml index 8803eddda69..ffaa7736d65 100644 --- a/app/views/admin/container_registry/show.html.haml +++ b/app/views/admin/container_registry/show.html.haml @@ -17,7 +17,7 @@ X-Registry-Token: [#{@access_token}] %br Access token is - %code{ id: 'registry-token' } #{@access_token} + %code{ id: 'registry-token' }= @access_token .bs-callout.clearfix .pull-left diff --git a/db/migrate/20161031013926_create_container_image.rb b/db/migrate/20161031013926_create_container_image.rb index 85c0913b8f3..884c78880eb 100644 --- a/db/migrate/20161031013926_create_container_image.rb +++ b/db/migrate/20161031013926_create_container_image.rb @@ -7,22 +7,6 @@ class CreateContainerImage < ActiveRecord::Migration # Set this constant to true if this migration requires downtime. DOWNTIME = false - # When a migration requires downtime you **must** uncomment the following - # constant and define a short and easy to understand explanation as to why the - # migration requires downtime. - # DOWNTIME_REASON = '' - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def change create_table :container_images do |t| t.integer :project_id diff --git a/db/migrate/20161213212947_add_container_registry_access_token_to_application_settings.rb b/db/migrate/20161213212947_add_container_registry_access_token_to_application_settings.rb index f89f9b00a5f..23d87cc6d0a 100644 --- a/db/migrate/20161213212947_add_container_registry_access_token_to_application_settings.rb +++ b/db/migrate/20161213212947_add_container_registry_access_token_to_application_settings.rb @@ -7,22 +7,6 @@ class AddContainerRegistryAccessTokenToApplicationSettings < ActiveRecord::Migra # Set this constant to true if this migration requires downtime. DOWNTIME = false - # When a migration requires downtime you **must** uncomment the following - # constant and define a short and easy to understand explanation as to why the - # migration requires downtime. - # DOWNTIME_REASON = '' - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def change add_column :application_settings, :container_registry_access_token, :string end diff --git a/db/schema.rb b/db/schema.rb index 36df20fc8f2..08d11546800 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -61,6 +61,7 @@ ActiveRecord::Schema.define(version: 20170215200045) do t.boolean "shared_runners_enabled", default: true, null: false t.integer "max_artifacts_size", default: 100, null: false t.string "runners_registration_token" + t.integer "max_pages_size", default: 100, null: false t.boolean "require_two_factor_authentication", default: false t.integer "two_factor_grace_period", default: 48 t.boolean "metrics_enabled", default: false @@ -107,10 +108,9 @@ ActiveRecord::Schema.define(version: 20170215200045) do t.string "sidekiq_throttling_queues" t.decimal "sidekiq_throttling_factor" t.boolean "html_emails_enabled", default: true + t.string "container_registry_access_token" t.string "plantuml_url" t.boolean "plantuml_enabled" - t.string "container_registry_access_token" - t.integer "max_pages_size", default: 100, null: false t.integer "terminal_max_session_time", default: 0, null: false end @@ -586,9 +586,9 @@ ActiveRecord::Schema.define(version: 20170215200045) do end add_index "labels", ["group_id", "project_id", "title"], name: "index_labels_on_group_id_and_project_id_and_title", unique: true, using: :btree - add_index "labels", ["type", "project_id"], name: "index_labels_on_type_and_project_id", using: :btree add_index "labels", ["project_id"], name: "index_labels_on_project_id", using: :btree add_index "labels", ["title"], name: "index_labels_on_title", using: :btree + add_index "labels", ["type", "project_id"], name: "index_labels_on_type_and_project_id", using: :btree create_table "lfs_objects", force: :cascade do |t| t.string "oid", null: false @@ -761,8 +761,8 @@ ActiveRecord::Schema.define(version: 20170215200045) do t.integer "visibility_level", default: 20, null: false t.boolean "request_access_enabled", default: false, null: false t.datetime "deleted_at" - t.boolean "lfs_enabled" t.text "description_html" + t.boolean "lfs_enabled" t.integer "parent_id" end @@ -1283,8 +1283,8 @@ ActiveRecord::Schema.define(version: 20170215200045) do t.datetime "otp_grace_period_started_at" t.boolean "ldap_email", default: false, null: false t.boolean "external", default: false - t.string "organization" t.string "incoming_email_token" + t.string "organization" t.boolean "authorized_projects_populated" t.boolean "notified_of_own_activity", default: false, null: false end diff --git a/lib/api/registry_events.rb b/lib/api/registry_events.rb index 12305a49f0f..fc6fc0b97e0 100644 --- a/lib/api/registry_events.rb +++ b/lib/api/registry_events.rb @@ -39,9 +39,9 @@ module API params['events'].each do |event| repository = event['target']['repository'] - if event['action'] == 'push' and !!event['target']['tag'] + if event['action'] == 'push' && !!event['target']['tag'] namespace, container_image_name = ContainerImage::split_namespace(repository) - project = Project::find_with_namespace(namespace) + project = Project::find_by_full_path(namespace) if project container_image = project.container_images.find_or_create_by(name: container_image_name) diff --git a/spec/services/container_images/destroy_service_spec.rb b/spec/services/container_images/destroy_service_spec.rb new file mode 100644 index 00000000000..5b4dbaa7934 --- /dev/null +++ b/spec/services/container_images/destroy_service_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe ContainerImages::DestroyService, services: true do + describe '#execute' do + let(:user) { create(:user) } + let(:container_image) { create(:container_image, name: '') } + let(:project) { create(:project, path: 'test', namespace: user.namespace, container_images: [container_image]) } + let(:example_host) { 'example.com' } + let(:registry_url) { 'http://' + example_host } + + it { expect(container_image).to be_valid } + it { expect(project.container_images).not_to be_empty } + + context 'when container image has tags' do + before do + project.team << [user, :master] + end + + it 'removes all tags before destroy' do + service = described_class.new(project, user) + + expect(container_image).to receive(:delete_tags).and_return(true) + expect { service.execute(container_image) }.to change(project.container_images, :count).by(-1) + end + + it 'fails when tags are not removed' do + service = described_class.new(project, user) + + expect(container_image).to receive(:delete_tags).and_return(false) + expect { service.execute(container_image) }.to raise_error(ActiveRecord::RecordNotDestroyed) + end + end + end +end -- cgit v1.2.1