Merge branch 'ce-to-ee-2018-11-27' into 'master'

CE upstream - 2018-11-27 14:23 UTC

See merge request gitlab-org/gitlab-ee!8609

app/models/project.rb

@@ -89,9 +89,6 @@ class Project < ActiveRecord::Base
 
   after_create :create_project_feature, unless: :project_feature
 
-  after_create -> { SiteStatistic.track(STATISTICS_ATTRIBUTE) }
-  before_destroy -> { SiteStatistic.untrack(STATISTICS_ATTRIBUTE) }
-
   after_create :create_ci_cd_settings,
     unless: :ci_cd_settings,
     if: proc { ProjectCiCdSetting.available? }

app/models/site_statistic.rb

-# frozen_string_literal: true
-
-class SiteStatistic < ActiveRecord::Base
-  # prevents the creation of multiple rows
-  default_value_for :id, 1
-
-  COUNTER_ATTRIBUTES = %w(repositories_count).freeze
-  REQUIRED_SCHEMA_VERSION = 20180629153018
-
-  # Tracks specific attribute
-  #
-  # @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES
-  def self.track(raw_attribute)
-    with_statistics_available(raw_attribute) do |attribute|
-      SiteStatistic.update_all(["#{attribute} = #{attribute}+1"])
-    end
-  end
-
-  # Untracks specific attribute
-  #
-  # @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES
-  def self.untrack(raw_attribute)
-    with_statistics_available(raw_attribute) do |attribute|
-      SiteStatistic.update_all(["#{attribute} = #{attribute}-1 WHERE #{attribute} > 0"])
-    end
-  end
-
-  # Wrapper for track/untrack operations with basic validations and enforced requirements
-  #
-  # @param [String] raw_attribute must be one of the values listed in COUNTER_ATTRIBUTES
-  # @yield [String] attribute quoted to be used inside SQL / Arel query
-  def self.with_statistics_available(raw_attribute)
-    unless raw_attribute.in?(COUNTER_ATTRIBUTES)
-      raise ArgumentError, "Invalid attribute: '#{raw_attribute}' to '#{caller_locations(1, 1)[0].label}' method. " \
-                           "Valid attributes are: #{COUNTER_ATTRIBUTES.join(', ')}"
-    end
-
-    return unless available?
-
-    self.fetch # make sure record exists
-
-    attribute = self.connection.quote_column_name(raw_attribute)
-
-    # will be running on its own transaction context
-    yield(attribute)
-  end
-
-  # Returns a site statistic record with tracked information
-  #
-  # @return [SiteStatistic] record with tracked information
-  def self.fetch
-    transaction(requires_new: true) do
-      SiteStatistic.first_or_create!
-    end
-  rescue ActiveRecord::RecordNotUnique
-    retry
-  end
-
-  # Return whether required schema change is available
-  #
-  # This is needed in order to degrade gracefully when testing schema migrations
-  #
-  # @return [Boolean] whether schema is available
-  def self.available?
-    @available_flag ||= ActiveRecord::Migrator.current_version >= REQUIRED_SCHEMA_VERSION
-  end
-
-  # Resets cached column information
-  #
-  # This is called during schema migration specs, in order to reset internal cache state
-  def self.reset_column_information
-    @available_flag = nil
-
-    super
-  end
-end

app/models/user.rb

@@ -363,7 +363,7 @@ class User < ActiveRecord::Base
       from_users = from_users.confirmed if confirmed
 
       from_emails = joins(:emails).where(emails: { email: emails })
-      from_emails = from_emails.confirmed if confirmed
+      from_emails = from_emails.confirmed.merge(Email.confirmed) if confirmed
 
       items = [from_users, from_emails]
 

app/services/clusters/applications/check_installation_progress_service.rb

@@ -29,17 +29,13 @@ module Clusters
       end
 
       def on_failed
-        app.make_errored!('Installation failed')
-      ensure
-        remove_installation_pod
+        app.make_errored!("Installation failed. Check pod logs for #{install_command.pod_name} for more details.")
       end
 
       def check_timeout
         if timeouted?
           begin
-            app.make_errored!('Installation timed out')
-          ensure
-            remove_installation_pod
+            app.make_errored!("Installation timed out. Check pod logs for #{install_command.pod_name} for more details.")
           end
         else
           ClusterWaitForAppInstallationWorker.perform_in(
@@ -53,9 +49,6 @@ module Clusters
 
       def remove_installation_pod
         helm_api.delete_pod!(install_command.pod_name)
-      rescue => e
-        Rails.logger.error("Kubernetes error: #{e.class.name} #{e.message}")
-        # no-op
       end
 
       def installation_phase

changelogs/unreleased/51792-dont-delete-failed-install-pods.yml

+---
+title: Don't remove failed install pods after installing GitLab managed applications
+merge_request: 23350
+author:
+type: changed

changelogs/unreleased/53778-remove-site-statistics.yml

+---
+title: Removed Site Statistics optimization as it was causing problems
+merge_request: 23314
+author:
+type: removed

changelogs/unreleased/ce-54109-fix_user_by_any_email.yml

+---
+title: Respect confirmed flag on secondary emails
+merge_request: 23181
+author:
+type: fixed

db/migrate/20181123042307_drop_site_statistics.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class DropSiteStatistics < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    drop_table :site_statistics
+  end
+
+  def down
+    create_table :site_statistics do |t|
+      t.integer :repositories_count, default: 0, null: false
+    end
+
+    execute('INSERT INTO site_statistics (id) VALUES(1)')
+  end
+end

db/schema.rb

@@ -2577,10 +2577,6 @@ ActiveRecord::Schema.define(version: 20181126153547) do
     t.index ["name"], name: "index_shards_on_name", unique: true, using: :btree
   end
 
-  create_table "site_statistics", force: :cascade do |t|
-    t.integer "repositories_count", default: 0, null: false
-  end
-
   create_table "slack_integrations", force: :cascade do |t|
     t.integer "service_id", null: false
     t.string "team_id", null: false

doc/README.md

@@ -58,7 +58,7 @@ The following sections provide links to documentation for each DevOps stage:
 | [Release](#release)     | Application release and delivery features.                  |
 | [Configure](#configure) | Application and infrastructure configuration tools.         |
 | [Monitor](#monitor)     | Application monitoring and metrics features.                |
-| [Secure](#secure)       | Security capability feature.                                |
+| [Secure](#secure)       | Security capability features.                               |
 
 <div align="right">
   <a type="button" class="btn btn-default" href="#overview">

doc/development/ui_guide.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/animation.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/foundations/motion'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com).

doc/development/ux_guide/basics.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/components.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/copy.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/features.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/illustrations.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/foundations/illustration/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/index.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/principles.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/resources.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/resources/design-resources'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/surfaces.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/tips.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
\ No newline at end of file
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

doc/development/ux_guide/users.md

@@ -2,4 +2,4 @@
 redirect_to: 'https://design.gitlab.com/getting-started/personas/'
 ---
 
-The content of this documented was moved into the [GitLab Design System](https://design.gitlab.com/).
+The content of this document was moved into the [GitLab Design System](https://design.gitlab.com/).

lib/gitlab/kubernetes/helm/api.rb

@@ -16,12 +16,16 @@ module Gitlab
           create_cluster_role_binding(command)
           create_config_map(command)
 
+          delete_pod!(command.pod_name)
           kubeclient.create_pod(command.pod_resource)
         end
 
         def update(command)
           namespace.ensure_exists!
+
           update_config_map(command)
+
+          delete_pod!(command.pod_name)
           kubeclient.create_pod(command.pod_resource)
         end
 
@@ -42,6 +46,8 @@ module Gitlab
 
         def delete_pod!(pod_name)
           kubeclient.delete_pod(pod_name, namespace.name)
+        rescue ::Kubeclient::ResourceNotFoundError
+          # no-op
         end
 
         def get_config_map(config_map_name)

lib/tasks/gitlab/site_statistics.rake

-namespace :gitlab do
-  desc "GitLab | Refresh Site Statistics counters"
-  task refresh_site_statistics: :environment do
-    puts 'Updating Site Statistics counters: '
-
-    print '* Repositories... '
-    SiteStatistic.transaction do
-      # see https://gitlab.com/gitlab-org/gitlab-ce/issues/48967
-      ActiveRecord::Base.connection.execute('SET LOCAL statement_timeout TO 0') if Gitlab::Database.postgresql?
-      SiteStatistic.update_all('repositories_count = (SELECT COUNT(*) FROM projects)')
-    end
-    puts 'OK!'.color(:green)
-    puts
-  end
-end

spec/factories/site_statistics.rb

-FactoryBot.define do
-  factory :site_statistics, class: 'SiteStatistic' do
-    id 1
-    repositories_count 999
-  end
-end

spec/lib/gitlab/kubernetes/helm/api_spec.rb

@@ -40,6 +40,7 @@ describe Gitlab::Kubernetes::Helm::Api do
       allow(client).to receive(:create_config_map).and_return(nil)
       allow(client).to receive(:create_service_account).and_return(nil)
       allow(client).to receive(:create_cluster_role_binding).and_return(nil)
+      allow(client).to receive(:delete_pod).and_return(nil)
       allow(namespace).to receive(:ensure_exists!).once
     end
 
@@ -50,6 +51,13 @@ describe Gitlab::Kubernetes::Helm::Api do
       subject.install(command)
     end
 
+    it 'removes an existing pod before installing' do
+      expect(client).to receive(:delete_pod).with('install-app-name', 'gitlab-managed-apps').once.ordered
+      expect(client).to receive(:create_pod).once.ordered
+
+      subject.install(command)
+    end
+
     context 'with a ConfigMap' do
       let(:resource) { Gitlab::Kubernetes::ConfigMap.new(application_name, files).generate }
 
@@ -180,6 +188,7 @@ describe Gitlab::Kubernetes::Helm::Api do
 
       allow(client).to receive(:update_config_map).and_return(nil)
       allow(client).to receive(:create_pod).and_return(nil)
+      allow(client).to receive(:delete_pod).and_return(nil)
     end
 
     it 'ensures the namespace exists before creating the pod' do
@@ -189,6 +198,13 @@ describe Gitlab::Kubernetes::Helm::Api do
       subject.update(command)
     end
 
+    it 'removes an existing pod before updating' do
+      expect(client).to receive(:delete_pod).with('upgrade-app-name', 'gitlab-managed-apps').once.ordered
+      expect(client).to receive(:create_pod).once.ordered
+
+      subject.update(command)
+    end
+
     it 'updates the config map on kubeclient when one exists' do
       resource = Gitlab::Kubernetes::ConfigMap.new(
         application_name, files
@@ -224,9 +240,18 @@ describe Gitlab::Kubernetes::Helm::Api do
 
   describe '#delete_pod!' do
     it 'deletes the POD from kubernetes cluster' do
-      expect(client).to receive(:delete_pod).with(command.pod_name, gitlab_namespace).once
+      expect(client).to receive(:delete_pod).with('install-app-name', 'gitlab-managed-apps').once
 
-      subject.delete_pod!(command.pod_name)
+      subject.delete_pod!('install-app-name')
+    end
+
+    context 'when the resource being deleted does not exist' do
+      it 'catches the error' do
+        expect(client).to receive(:delete_pod).with('install-app-name', 'gitlab-managed-apps')
+          .and_raise(Kubeclient::ResourceNotFoundError.new(404, 'Not found', nil))
+
+        subject.delete_pod!('install-app-name')
+      end
     end
   end
 

spec/models/project_spec.rb

@@ -110,22 +110,6 @@ describe Project do
       end
     end
 
-    context 'Site Statistics' do
-      context 'when creating a new project' do
-        it 'tracks project in SiteStatistic' do
-          expect { create(:project) }.to change { SiteStatistic.fetch.repositories_count }.by(1)
-        end
-      end
-
-      context 'when deleting a project' do
-        it 'untracks project in SiteStatistic' do
-          project = create(:project)
-
-          expect { project.destroy }.to change { SiteStatistic.fetch.repositories_count }.by(-1)
-        end
-      end
-    end
-
     context 'updating cd_cd_settings' do
       it 'does not raise an error' do
         project = create(:project)

spec/models/site_statistic_spec.rb

-require 'spec_helper'
-
-describe SiteStatistic do
-  describe '.fetch' do
-    context 'existing record' do
-      it 'returns existing SiteStatistic model' do
-        statistics = create(:site_statistics)
-
-        expect(described_class.fetch).to be_a(described_class)
-        expect(described_class.fetch).to eq(statistics)
-      end
-    end
-
-    context 'non existing record' do
-      it 'creates a new SiteStatistic model' do
-        expect(described_class.first).to be_nil
-        expect(described_class.fetch).to be_a(described_class)
-      end
-    end
-  end
-
-  describe '.track' do
-    context 'with allowed attributes' do
-      let(:statistics) { create(:site_statistics) }
-
-      it 'increases the attribute counter' do
-        expect { described_class.track('repositories_count') }.to change { statistics.reload.repositories_count }.by(1)
-      end
-
-      it 'doesnt increase the attribute counter when an exception happens during transaction' do
-        expect do
-          begin
-            described_class.transaction do
-              described_class.track('repositories_count')
-
-              raise StandardError
-            end
-          rescue StandardError
-            # no-op
-          end
-        end.not_to change { statistics.reload.repositories_count }
-      end
-    end
-
-    context 'with not allowed attributes' do
-      it 'returns error' do
-        expect { described_class.track('something_else') }.to raise_error(ArgumentError).with_message(/Invalid attribute: \'something_else\' to \'track\' method/)
-      end
-    end
-  end
-
-  describe '.untrack' do
-    context 'with allowed attributes' do
-      let(:statistics) { create(:site_statistics) }
-
-      it 'decreases the attribute counter' do
-        expect { described_class.untrack('repositories_count') }.to change { statistics.reload.repositories_count }.by(-1)
-      end
-
-      it 'doesnt decrease the attribute counter when an exception happens during transaction' do
-        expect do
-          begin
-            described_class.transaction do
-              described_class.track('repositories_count')
-
-              raise StandardError
-            end
-          rescue StandardError
-            # no-op
-          end
-        end.not_to change { described_class.fetch.repositories_count }
-      end
-    end
-
-    context 'with not allowed attributes' do
-      it 'returns error' do
-        expect { described_class.untrack('something_else') }.to raise_error(ArgumentError).with_message(/Invalid attribute: \'something_else\' to \'untrack\' method/)
-      end
-    end
-  end
-end

spec/models/user_spec.rb

@@ -1165,12 +1165,38 @@ describe User do
       expect(described_class.find_by_any_email(user.email.upcase, confirmed: true)).to eq user
     end
 
-    it 'finds by secondary email' do
-      email = create(:email, email: 'foo@example.com')
-      user  = email.user
+    context 'finds by secondary email' do
+      let(:user) { email.user }
 
-      expect(described_class.find_by_any_email(email.email)).to eq user
-      expect(described_class.find_by_any_email(email.email, confirmed: true)).to eq user
+      context 'primary email confirmed' do
+        context 'secondary email confirmed' do
+          let!(:email) { create(:email, :confirmed, email: 'foo@example.com') }
+
+          it 'finds user respecting the confirmed flag' do
+            expect(described_class.find_by_any_email(email.email)).to eq user
+            expect(described_class.find_by_any_email(email.email, confirmed: true)).to eq user
+          end
+        end
+
+        context 'secondary email not confirmed' do
+          let!(:email) { create(:email, email: 'foo@example.com') }
+
+          it 'finds user respecting the confirmed flag' do
+            expect(described_class.find_by_any_email(email.email)).to eq user
+            expect(described_class.find_by_any_email(email.email, confirmed: true)).to be_nil
+          end
+        end
+      end
+
+      context 'primary email not confirmed' do
+        let(:user) { create(:user, confirmed_at: nil) }
+        let!(:email) { create(:email, :confirmed, user: user, email: 'foo@example.com') }
+
+        it 'finds user respecting the confirmed flag' do
+          expect(described_class.find_by_any_email(email.email)).to eq user
+          expect(described_class.find_by_any_email(email.email, confirmed: true)).to be_nil
+        end
+      end
     end
 
     it 'returns nil when nothing found' do

spec/services/clusters/applications/check_installation_progress_service_spec.rb

@@ -8,14 +8,6 @@ describe Clusters::Applications::CheckInstallationProgressService do
   let(:phase) { Gitlab::Kubernetes::Pod::UNKNOWN }
   let(:errors) { nil }
 
-  shared_examples 'a terminated installation' do
-    it 'removes the installation POD' do
-      expect(service).to receive(:remove_installation_pod).once
-
-      service.execute
-    end
-  end
-
   shared_examples 'a not yet terminated installation' do |a_phase|
     let(:phase) { a_phase }
 
@@ -39,15 +31,13 @@ describe Clusters::Applications::CheckInstallationProgressService do
       context 'when timeouted' do
         let(:application) { create(:clusters_applications_helm, :timeouted) }
 
-        it_behaves_like 'a terminated installation'
-
         it 'make the application errored' do
           expect(ClusterWaitForAppInstallationWorker).not_to receive(:perform_in)
 
           service.execute
 
           expect(application).to be_errored
-          expect(application.status_reason).to match(/\btimed out\b/)
+          expect(application.status_reason).to eq("Installation timed out. Check pod logs for install-helm for more details.")
         end
       end
     end
@@ -66,7 +56,11 @@ describe Clusters::Applications::CheckInstallationProgressService do
         expect(service).to receive(:installation_phase).once.and_return(phase)
       end
 
-      it_behaves_like 'a terminated installation'
+      it 'removes the installation POD' do
+        expect(service).to receive(:remove_installation_pod).once
+
+        service.execute
+      end
 
       it 'make the application installed' do
         expect(ClusterWaitForAppInstallationWorker).not_to receive(:perform_in)
@@ -86,13 +80,11 @@ describe Clusters::Applications::CheckInstallationProgressService do
         expect(service).to receive(:installation_phase).once.and_return(phase)
       end
 
-      it_behaves_like 'a terminated installation'
-
       it 'make the application errored' do
         service.execute
 
         expect(application).to be_errored
-        expect(application.status_reason).to eq("Installation failed")
+        expect(application.status_reason).to eq("Installation failed. Check pod logs for install-helm for more details.")
       end
     end
 

spec/tasks/gitlab/site_statistics_rake_spec.rb

-# frozen_string_literal: true
-require 'rake_helper'
-
-describe 'rake gitlab:refresh_site_statistics' do
-  before do
-    Rake.application.rake_require 'tasks/gitlab/site_statistics'
-
-    create(:project)
-    SiteStatistic.fetch.update(repositories_count: 0)
-  end
-
-  let(:task) { 'gitlab:refresh_site_statistics' }
-
-  it 'recalculates existing counters' do
-    run_rake_task(task)
-
-    expect(SiteStatistic.fetch.repositories_count).to eq(1)
-  end
-
-  it 'displays message listing counters' do
-    expect { run_rake_task(task) }.to output(/Updating Site Statistics counters:.* Repositories\.\.\. OK!/m).to_stdout
-  end
-end