Creates specs for destroy service and improves namespace container image query performance

8 files changed, 45 insertions, 65 deletions

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