Merge branch '268258-propagate-integration-to-group-descendants' into 'master'

Propagate integration to group descendants

See merge request gitlab-org/gitlab!45529

app/models/service.rb

@@ -274,6 +274,17 @@ class Service < ApplicationRecord
     end
   end
 
+  def self.inherited_descendants_from_self_or_ancestors_from(integration)
+    inherit_from_ids =
+      where(type: integration.type, group: integration.group.self_and_ancestors)
+        .or(where(type: integration.type, instance: true)).select(:id)
+
+    from_union([
+      where(type: integration.type, inherit_from_id: inherit_from_ids, group: integration.group.descendants),
+      where(type: integration.type, inherit_from_id: inherit_from_ids, project: Project.in_namespace(integration.group.self_and_descendants))
+    ])
+  end
+
   def activated?
     active
   end

app/services/admin/propagate_integration_service.rb

@@ -5,12 +5,12 @@ module Admin
     include PropagateService
 
     def propagate
-      update_inherited_integrations
-
       if integration.instance?
+        update_inherited_integrations
         create_integration_for_groups_without_integration if Feature.enabled?(:group_level_integrations)
         create_integration_for_projects_without_integration
       else
+        update_inherited_descendant_integrations
         create_integration_for_groups_without_integration_belonging_to_group
         create_integration_for_projects_without_integration_belonging_to_group
       end
@@ -18,34 +18,39 @@ module Admin
 
     private
 
-    # rubocop: disable Cop/InBatches
     def update_inherited_integrations
-      Service.by_type(integration.type).inherit_from_id(integration.id).each_batch(of: BATCH_SIZE) do |services|
-        min_id, max_id = services.pick("MIN(services.id), MAX(services.id)")
-        PropagateIntegrationInheritWorker.perform_async(integration.id, min_id, max_id)
+      propagate_integrations(
+        Service.by_type(integration.type).inherit_from_id(integration.id),
+        PropagateIntegrationInheritWorker
+      )
     end
+
+    def update_inherited_descendant_integrations
+      propagate_integrations(
+        Service.inherited_descendants_from_self_or_ancestors_from(integration),
+        PropagateIntegrationInheritDescendantWorker
+      )
     end
-    # rubocop: enable Cop/InBatches
 
     def create_integration_for_groups_without_integration
-      Group.without_integration(integration).each_batch(of: BATCH_SIZE) do |groups|
-        min_id, max_id = groups.pick("MIN(namespaces.id), MAX(namespaces.id)")
-        PropagateIntegrationGroupWorker.perform_async(integration.id, min_id, max_id)
-      end
+      propagate_integrations(
+        Group.without_integration(integration),
+        PropagateIntegrationGroupWorker
+      )
     end
 
     def create_integration_for_groups_without_integration_belonging_to_group
-      integration.group.descendants.without_integration(integration).each_batch(of: BATCH_SIZE) do |groups|
-        min_id, max_id = groups.pick("MIN(namespaces.id), MAX(namespaces.id)")
-        PropagateIntegrationGroupWorker.perform_async(integration.id, min_id, max_id)
-      end
+      propagate_integrations(
+        integration.group.descendants.without_integration(integration),
+        PropagateIntegrationGroupWorker
+      )
     end
 
     def create_integration_for_projects_without_integration_belonging_to_group
-      Project.without_integration(integration).in_namespace(integration.group.self_and_descendants).each_batch(of: BATCH_SIZE) do |projects|
-        min_id, max_id = projects.pick("MIN(projects.id), MAX(projects.id)")
-        PropagateIntegrationProjectWorker.perform_async(integration.id, min_id, max_id)
-      end
+      propagate_integrations(
+        Project.without_integration(integration).in_namespace(integration.group.self_and_descendants),
+        PropagateIntegrationProjectWorker
+      )
     end
   end
 end

app/services/bulk_create_integration_service.rb

@@ -47,7 +47,7 @@ class BulkCreateIntegrationService
     if integration.template?
       integration.to_service_hash
     else
-      integration.to_service_hash.tap { |json| json['inherit_from_id'] = integration.id }
+      integration.to_service_hash.tap { |json| json['inherit_from_id'] = integration.inherit_from_id || integration.id }
     end
   end
 

app/services/bulk_update_integration_service.rb

@@ -23,7 +23,7 @@ class BulkUpdateIntegrationService
   attr_reader :integration, :batch
 
   def service_hash
-    integration.to_service_hash.tap { |json| json['inherit_from_id'] = integration.id }
+    integration.to_service_hash.tap { |json| json['inherit_from_id'] = integration.inherit_from_id || integration.id }
   end
 
   def data_fields_hash

app/services/concerns/admin/propagate_service.rb

@@ -21,9 +21,16 @@ module Admin
     attr_reader :integration
 
     def create_integration_for_projects_without_integration
-      Project.without_integration(integration).each_batch(of: BATCH_SIZE) do |projects|
-        min_id, max_id = projects.pick("MIN(projects.id), MAX(projects.id)")
-        PropagateIntegrationProjectWorker.perform_async(integration.id, min_id, max_id)
+      propagate_integrations(
+        Project.without_integration(integration),
+        PropagateIntegrationProjectWorker
+      )
+    end
+
+    def propagate_integrations(relation, worker_class)
+      relation.each_batch(of: BATCH_SIZE) do |records|
+        min_id, max_id = records.pick("MIN(#{relation.table_name}.id), MAX(#{relation.table_name}.id)")
+        worker_class.perform_async(integration.id, min_id, max_id)
       end
     end
   end

app/workers/all_queues.yml

@@ -1847,6 +1847,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: propagate_integration_inherit_descendant
+  :feature_category: :integrations
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: propagate_integration_project
   :feature_category: :integrations
   :has_external_dependencies:

app/workers/propagate_integration_inherit_descendant_worker.rb

+# frozen_string_literal: true
+
+class PropagateIntegrationInheritDescendantWorker
+  include ApplicationWorker
+
+  feature_category :integrations
+  idempotent!
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def perform(integration_id, min_id, max_id)
+    integration = Service.find_by_id(integration_id)
+    return unless integration
+
+    batch = Service.inherited_descendants_from_self_or_ancestors_from(integration).where(id: min_id..max_id)
+
+    BulkUpdateIntegrationService.new(integration, batch).execute
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end

app/workers/propagate_integration_inherit_worker.rb

@@ -11,9 +11,9 @@ class PropagateIntegrationInheritWorker
     integration = Service.find_by_id(integration_id)
     return unless integration
 
-    services = Service.where(id: min_id..max_id).by_type(integration.type).inherit_from_id(integration.id)
+    batch = Service.where(id: min_id..max_id).by_type(integration.type).inherit_from_id(integration.id)
 
-    BulkUpdateIntegrationService.new(integration, services).execute
+    BulkUpdateIntegrationService.new(integration, batch).execute
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end

config/sidekiq_queues.yml

@@ -254,6 +254,8 @@
   - 1
 - - propagate_integration_inherit
   - 1
+- - propagate_integration_inherit_descendant
+  - 1
 - - propagate_integration_project
   - 1
 - - propagate_service_template

spec/models/service_spec.rb

@@ -599,6 +599,23 @@ RSpec.describe Service do
     end
   end
 
+  describe '.inherited_descendants_from_self_or_ancestors_from' do
+    let_it_be(:subgroup1) { create(:group, parent: group) }
+    let_it_be(:subgroup2) { create(:group, parent: group) }
+    let_it_be(:project1) { create(:project, group: subgroup1) }
+    let_it_be(:project2) { create(:project, group: subgroup2) }
+    let_it_be(:group_integration) { create(:prometheus_service, group: group, project: nil) }
+    let_it_be(:subgroup_integration1) { create(:prometheus_service, group: subgroup1, project: nil, inherit_from_id: group_integration.id) }
+    let_it_be(:subgroup_integration2) { create(:prometheus_service, group: subgroup2, project: nil) }
+    let_it_be(:project_integration1) { create(:prometheus_service, group: nil, project: project1, inherit_from_id: group_integration.id) }
+    let_it_be(:project_integration2) { create(:prometheus_service, group: nil, project: project2, inherit_from_id: subgroup_integration2.id) }
+
+    it 'returns the groups and projects inheriting from integration ancestors', :aggregate_failures do
+      expect(described_class.inherited_descendants_from_self_or_ancestors_from(group_integration)).to eq([subgroup_integration1, project_integration1])
+      expect(described_class.inherited_descendants_from_self_or_ancestors_from(subgroup_integration2)).to eq([project_integration2])
+    end
+  end
+
   describe "{property}_changed?" do
     let(:service) do
       BambooService.create(

spec/services/admin/propagate_integration_service_spec.rb

@@ -67,7 +67,7 @@ RSpec.describe Admin::PropagateIntegrationService do
         end
       end
 
-      context 'with a group without integration' do
+      context 'with a subgroup without integration' do
         let(:subgroup) { create(:group, parent: group) }
 
         it 'calls to PropagateIntegrationGroupWorker' do
@@ -77,6 +77,18 @@ RSpec.describe Admin::PropagateIntegrationService do
           described_class.propagate(group_integration)
         end
       end
+
+      context 'with a subgroup with integration' do
+        let(:subgroup) { create(:group, parent: group) }
+        let(:subgroup_integration) { create(:jira_service, group: subgroup, project: nil, inherit_from_id: group_integration.id) }
+
+        it 'calls to PropagateIntegrationInheritDescendantWorker' do
+          expect(PropagateIntegrationInheritDescendantWorker).to receive(:perform_async)
+            .with(group_integration.id, subgroup_integration.id, subgroup_integration.id)
+
+          described_class.propagate(group_integration)
+        end
+      end
     end
   end
 end

spec/services/bulk_create_integration_service_spec.rb

@@ -9,9 +9,9 @@ RSpec.describe BulkCreateIntegrationService do
     stub_jira_service_test
   end
 
+  let_it_be(:instance_integration) { create(:jira_service, :instance) }
+  let_it_be(:template_integration) { create(:jira_service, :template) }
   let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] }
-  let!(:instance_integration) { create(:jira_service, :instance) }
-  let!(:template_integration) { create(:jira_service, :template) }
 
   shared_examples 'creates integration from batch ids' do
     it 'updates the inherited integrations' do
@@ -37,7 +37,7 @@ RSpec.describe BulkCreateIntegrationService do
     it 'updates inherit_from_id attributes' do
       described_class.new(integration, batch, association).execute
 
-      expect(created_integration.reload.inherit_from_id).to eq(integration.id)
+      expect(created_integration.reload.inherit_from_id).to eq(inherit_from_id)
     end
   end
 
@@ -79,6 +79,7 @@ RSpec.describe BulkCreateIntegrationService do
 
   context 'with an instance-level integration' do
     let(:integration) { instance_integration }
+    let(:inherit_from_id) { integration.id }
 
     context 'with a project association' do
       let!(:project) { create(:project) }
@@ -100,13 +101,26 @@ RSpec.describe BulkCreateIntegrationService do
     end
 
     context 'with a group association' do
-      let!(:group) { create(:group) }
+      let_it_be(:group) { create(:group) }
       let(:created_integration) { Service.find_by(group: group) }
       let(:batch) { Group.all }
       let(:association) { 'group' }
 
       it_behaves_like 'creates integration from batch ids'
       it_behaves_like 'updates inherit_from_id'
+
+      context 'with a subgroup association' do
+        let_it_be(:group_integration) { create(:jira_service, group: group, project: nil, inherit_from_id: instance_integration.id) }
+        let_it_be(:subgroup) { create(:group, parent: group) }
+        let(:integration) { group_integration }
+        let(:created_integration) { Service.find_by(group: subgroup) }
+        let(:batch) { Group.all }
+        let(:association) { 'group' }
+        let(:inherit_from_id) { instance_integration.id }
+
+        it_behaves_like 'creates integration from batch ids'
+        it_behaves_like 'updates inherit_from_id'
+      end
     end
   end
 
@@ -118,6 +132,7 @@ RSpec.describe BulkCreateIntegrationService do
       let(:created_integration) { project.jira_service }
       let(:batch) { Project.all }
       let(:association) { 'project' }
+      let(:inherit_from_id) { integration.id }
 
       it_behaves_like 'creates integration from batch ids'
       it_behaves_like 'updates project callbacks'

spec/services/bulk_update_integration_service_spec.rb

@@ -5,14 +5,28 @@ require 'spec_helper'
 RSpec.describe BulkUpdateIntegrationService do
   include JiraServiceHelper
 
-  before do
+  before_all do
     stub_jira_service_test
   end
 
   let(:excluded_attributes) { %w[id project_id group_id inherit_from_id instance template created_at updated_at] }
-  let!(:instance_integration) do
+
+  let_it_be(:group) { create(:group) }
+  let_it_be(:group_integration) do
+    JiraService.create!(
+      group: group,
+      active: true,
+      push_events: true,
+      url: 'http://update-jira.instance.com',
+      username: 'user',
+      password: 'secret'
+    )
+  end
+
+  let_it_be(:subgroup_integration) do
     JiraService.create!(
-      instance: true,
+      inherit_from_id: group_integration.id,
+      group: create(:group, parent: group),
       active: true,
       push_events: true,
       url: 'http://update-jira.instance.com',
@@ -21,10 +35,9 @@ RSpec.describe BulkUpdateIntegrationService do
     )
   end
 
-  let!(:integration) do
+  let_it_be(:integration) do
     JiraService.create!(
       project: create(:project),
-      inherit_from_id: instance_integration.id,
       instance: false,
       active: true,
       push_events: false,
@@ -36,21 +49,21 @@ RSpec.describe BulkUpdateIntegrationService do
 
   context 'with inherited integration' do
     it 'updates the integration' do
-      described_class.new(instance_integration, Service.inherit_from_id(instance_integration.id)).execute
+      described_class.new(subgroup_integration, Service.where.not(project: nil)).execute
 
-      expect(integration.reload.inherit_from_id).to eq(instance_integration.id)
+      expect(integration.reload.inherit_from_id).to eq(group_integration.id)
       expect(integration.attributes.except(*excluded_attributes))
-        .to eq(instance_integration.attributes.except(*excluded_attributes))
+        .to eq(subgroup_integration.attributes.except(*excluded_attributes))
     end
 
     context 'with integration with data fields' do
       let(:excluded_attributes) { %w[id service_id created_at updated_at] }
 
       it 'updates the data fields from the integration' do
-        described_class.new(instance_integration, Service.inherit_from_id(instance_integration.id)).execute
+        described_class.new(subgroup_integration, Service.where.not(project: nil)).execute
 
         expect(integration.reload.data_fields.attributes.except(*excluded_attributes))
-          .to eq(instance_integration.data_fields.attributes.except(*excluded_attributes))
+          .to eq(subgroup_integration.data_fields.attributes.except(*excluded_attributes))
       end
     end
   end

spec/workers/propagate_integration_inherit_descendant_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe PropagateIntegrationInheritDescendantWorker do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:subgroup) { create(:group, parent: group) }
+  let_it_be(:group_integration) { create(:redmine_service, group: group, project: nil) }
+  let_it_be(:subgroup_integration) { create(:redmine_service, group: subgroup, project: nil, inherit_from_id: group_integration.id) }
+
+  it_behaves_like 'an idempotent worker' do
+    let(:job_args) { [group_integration.id, subgroup_integration.id, subgroup_integration.id] }
+
+    it 'calls to BulkUpdateIntegrationService' do
+      expect(BulkUpdateIntegrationService).to receive(:new)
+        .with(group_integration, match_array(subgroup_integration)).twice
+        .and_return(double(execute: nil))
+
+      subject
+    end
+  end
+
+  context 'with an invalid integration id' do
+    it 'returns without failure' do
+      expect(BulkUpdateIntegrationService).not_to receive(:new)
+
+      subject.perform(0, subgroup_integration.id, subgroup_integration.id)
+    end
+  end
+end

spec/workers/propagate_integration_inherit_worker_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe PropagateIntegrationInheritWorker do
     it_behaves_like 'an idempotent worker' do
       let(:job_args) { [integration.id, integration1.id, integration3.id] }
 
-      it 'calls to BulkCreateIntegrationService' do
+      it 'calls to BulkUpdateIntegrationService' do
         expect(BulkUpdateIntegrationService).to receive(:new)
           .with(integration, match_array(integration1)).twice
           .and_return(double(execute: nil))