Merge branch 'application-limits-with-defaults' into 'master'

Update ApplicationLimits to prefer defaults

See merge request gitlab-org/gitlab!27574

changelogs/unreleased/application-limits-with-defaults.yml

+---
+title: Update ApplicationLimits to prefer defaults
+merge_request: 27574
+author:
+type: changed

db/migrate/20200319123041_update_plan_limits_defaults.rb

+# frozen_string_literal: true
+
+class UpdatePlanLimitsDefaults < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    change_column_default :plan_limits, :project_hooks, 100
+    change_column_default :plan_limits, :group_hooks, 50
+    change_column_default :plan_limits, :ci_project_subscriptions, 2
+    change_column_default :plan_limits, :ci_pipeline_schedules, 10
+  end
+
+  def down
+    change_column_default :plan_limits, :project_hooks, 0
+    change_column_default :plan_limits, :group_hooks, 0
+    change_column_default :plan_limits, :ci_project_subscriptions, 0
+    change_column_default :plan_limits, :ci_pipeline_schedules, 0
+  end
+end

db/structure.sql

@@ -4508,10 +4508,10 @@ CREATE TABLE public.plan_limits (
     ci_active_pipelines integer DEFAULT 0 NOT NULL,
     ci_pipeline_size integer DEFAULT 0 NOT NULL,
     ci_active_jobs integer DEFAULT 0 NOT NULL,
-    project_hooks integer DEFAULT 0 NOT NULL,
-    group_hooks integer DEFAULT 0 NOT NULL,
-    ci_project_subscriptions integer DEFAULT 0 NOT NULL,
-    ci_pipeline_schedules integer DEFAULT 0 NOT NULL
+    project_hooks integer DEFAULT 100 NOT NULL,
+    group_hooks integer DEFAULT 50 NOT NULL,
+    ci_project_subscriptions integer DEFAULT 2 NOT NULL,
+    ci_pipeline_schedules integer DEFAULT 10 NOT NULL
 );
 
 CREATE SEQUENCE public.plan_limits_id_seq
@@ -12747,6 +12747,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200318164448'),
 ('20200318165448'),
 ('20200318175008'),
+('20200319123041'),
 ('20200319203901'),
 ('20200323075043'),
 ('20200323122201');

doc/development/application_limits.md

@@ -20,40 +20,45 @@ limits](https://about.gitlab.com/handbook/product/#introducing-application-limit
 In the `plan_limits` table, you have to create a new column and insert the
 limit values. It's recommended to create separate migration script files.
 
-1. Add new column to the `plan_limits` table with non-null default value 0, eg:
-
-   ```ruby
-   add_column(:plan_limits, :project_hooks, :integer, default: 0, null: false)
-   ```
-
-   NOTE: **Note:** Plan limits entries set to `0` mean that limits are not
-   enabled.
-
-1. Insert plan limits values into the database using
-   `create_or_update_plan_limit` migration helper, eg:
-
-   ```ruby
-   def up
-     return unless Gitlab.com?
-
-     create_or_update_plan_limit('project_hooks', 'free', 100)
-     create_or_update_plan_limit('project_hooks', 'bronze', 100)
-     create_or_update_plan_limit('project_hooks', 'silver', 100)
-     create_or_update_plan_limit('project_hooks', 'gold', 100)
-   end
-
-   def down
-     return unless Gitlab.com?
-
-     create_or_update_plan_limit('project_hooks', 'free', 0)
-     create_or_update_plan_limit('project_hooks', 'bronze', 0)
-     create_or_update_plan_limit('project_hooks', 'silver', 0)
-     create_or_update_plan_limit('project_hooks', 'gold', 0)
-   end
-   ```
-
-NOTE: **Note:** Some plans exist only on GitLab.com. You can check if the
-migration is running on GitLab.com with `Gitlab.com?`.
+1. Add new column to the `plan_limits` table with non-null default value
+   that represents desired limit, eg:
+
+  ```ruby
+  add_column(:plan_limits, :project_hooks, :integer, default: 100, null: false)
+  ```
+
+  NOTE: **Note:** Plan limits entries set to `0` mean that limits are not
+  enabled. You should use this setting only in special and documented circumstances.
+
+1. (Optionally) Create the database migration that fine-tunes each level with
+    a desired limit using `create_or_update_plan_limit` migration helper, eg:
+
+  ```ruby
+  class InsertProjectHooksPlanLimits < ActiveRecord::Migration[5.2]
+    include Gitlab::Database::MigrationHelpers
+
+    DOWNTIME = false
+
+    def up
+      create_or_update_plan_limit('project_hooks', 'default', 0)
+      create_or_update_plan_limit('project_hooks', 'free', 10)
+      create_or_update_plan_limit('project_hooks', 'bronze', 20)
+      create_or_update_plan_limit('project_hooks', 'silver', 30)
+      create_or_update_plan_limit('project_hooks', 'gold', 100)
+    end
+
+    def down
+      create_or_update_plan_limit('project_hooks', 'default', 0)
+      create_or_update_plan_limit('project_hooks', 'free', 0)
+      create_or_update_plan_limit('project_hooks', 'bronze', 0)
+      create_or_update_plan_limit('project_hooks', 'silver', 0)
+      create_or_update_plan_limit('project_hooks', 'gold', 0)
+    end
+  end
+  ```
+
+NOTE: **Note:** Some plans exist only on GitLab.com. This will be no-op
+for plans that do not exist.
 
 ### Plan limits validation
 

ee/spec/models/ee/plan_limits_spec.rb

@@ -45,4 +45,30 @@ describe PlanLimits do
       end
     end
   end
+
+  context 'validates default values' do
+    let(:columns_with_zero) do
+      %w[
+        ci_active_pipelines
+        ci_pipeline_size
+        ci_active_jobs
+      ]
+    end
+
+    it "has positive values for enabled limits" do
+      attributes = plan_limits.attributes
+      attributes = attributes.except(described_class.primary_key)
+      attributes = attributes.except(described_class.reflections.values.map(&:foreign_key))
+      attributes = attributes.except(*columns_with_zero)
+
+      expect(attributes).to all(include(be_positive))
+    end
+
+    it "has zero values for disabled limits" do
+      attributes = plan_limits.attributes
+      attributes = attributes.slice(*columns_with_zero)
+
+      expect(attributes).to all(include(be_zero))
+    end
+  end
 end

ee/spec/models/namespace_spec.rb

@@ -310,10 +310,10 @@ describe Namespace do
         expect(subject).to be_kind_of(PlanLimits)
       end
 
-      it 'has all limits disabled' do
+      it 'has all limits defined' do
         limits = subject.attributes.except('id', 'plan_id')
         limits.each do |_attribute, limit|
-          expect(limit).to be_zero
+          expect(limit).not_to be_nil
         end
       end
     end

lib/gitlab/database/migration_helpers.rb

@@ -1121,11 +1121,14 @@ into similar problems in the future (e.g. when new tables are created).
       end
 
       def create_or_update_plan_limit(limit_name, plan_name, limit_value)
+        limit_name_quoted = quote_column_name(limit_name)
+        plan_name_quoted = quote(plan_name)
+        limit_value_quoted = quote(limit_value)
+
         execute <<~SQL
-          INSERT INTO plan_limits (plan_id, #{quote_column_name(limit_name)})
-          VALUES
-            ((SELECT id FROM plans WHERE name = #{quote(plan_name)} LIMIT 1), #{quote(limit_value)})
-          ON CONFLICT (plan_id) DO UPDATE SET #{quote_column_name(limit_name)} = EXCLUDED.#{quote_column_name(limit_name)};
+          INSERT INTO plan_limits (plan_id, #{limit_name_quoted})
+          SELECT id, #{limit_value_quoted} FROM plans WHERE name = #{plan_name_quoted} LIMIT 1
+          ON CONFLICT (plan_id) DO UPDATE SET #{limit_name_quoted} = EXCLUDED.#{limit_name_quoted};
         SQL
       end
 

spec/lib/gitlab/database/migration_helpers_spec.rb

@@ -1542,16 +1542,54 @@ describe Gitlab::Database::MigrationHelpers do
   end
 
   describe '#create_or_update_plan_limit' do
-    it 'creates or updates plan limits' do
+    class self::Plan < ActiveRecord::Base
+      self.table_name = 'plans'
+    end
+
+    class self::PlanLimits < ActiveRecord::Base
+      self.table_name = 'plan_limits'
+    end
+
+    it 'properly escapes names' do
       expect(model).to receive(:execute).with <<~SQL
         INSERT INTO plan_limits (plan_id, "project_hooks")
-        VALUES
-          ((SELECT id FROM plans WHERE name = 'free' LIMIT 1), '10')
+        SELECT id, '10' FROM plans WHERE name = 'free' LIMIT 1
         ON CONFLICT (plan_id) DO UPDATE SET "project_hooks" = EXCLUDED."project_hooks";
       SQL
 
       model.create_or_update_plan_limit('project_hooks', 'free', 10)
     end
+
+    context 'when plan does not exist' do
+      it 'does not create any plan limits' do
+        expect { model.create_or_update_plan_limit('project_hooks', 'plan_name', 10) }
+          .not_to change { self.class::PlanLimits.count }
+      end
+    end
+
+    context 'when plan does exist' do
+      let!(:plan) { self.class::Plan.create!(name: 'plan_name') }
+
+      context 'when limit does not exist' do
+        it 'inserts a new plan limits' do
+          expect { model.create_or_update_plan_limit('project_hooks', 'plan_name', 10) }
+            .to change { self.class::PlanLimits.count }.by(1)
+
+          expect(self.class::PlanLimits.pluck(:project_hooks)).to contain_exactly(10)
+        end
+      end
+
+      context 'when limit does exist' do
+        let!(:plan_limit) { self.class::PlanLimits.create!(plan_id: plan.id) }
+
+        it 'updates an existing plan limits' do
+          expect { model.create_or_update_plan_limit('project_hooks', 'plan_name', 999) }
+            .not_to change { self.class::PlanLimits.count }
+
+          expect(plan_limit.reload.project_hooks).to eq(999)
+        end
+      end
+    end
   end
 
   describe '#with_lock_retries' do

spec/requests/api/pipeline_schedules_spec.rb

@@ -46,7 +46,7 @@ describe API::PipelineSchedules do
           get api("/projects/#{project.id}/pipeline_schedules", developer)
         end.count
 
-        create_pipeline_schedules(10)
+        create_pipeline_schedules(5)
 
         expect do
           get api("/projects/#{project.id}/pipeline_schedules", developer)