Merge branch...

Merge branch '297391-sync-metric-dashboards-from-repository-to-database-only-if-needed' into 'master'

Resolve "Sync metric dashboards from repository to database only if needed"

See merge request gitlab-org/gitlab!52812

app/services/git/branch_hooks_service.rb

@@ -2,6 +2,8 @@
 
 module Git
   class BranchHooksService < ::Git::BaseHooksService
+    extend ::Gitlab::Utils::Override
+
     def execute
       execute_branch_hooks
 
@@ -41,9 +43,14 @@ module Git
       super
     end
 
+    override :invalidated_file_types
     def invalidated_file_types
       return super unless default_branch? && !creating_branch?
 
+      modified_file_types
+    end
+
+    def modified_file_types
       paths = commit_paths.values.reduce(&:merge) || Set.new
 
       Gitlab::FileDetector.types_in_paths(paths)
@@ -82,6 +89,7 @@ module Git
 
     def enqueue_metrics_dashboard_sync
       return unless default_branch?
+      return unless modified_file_types.include?(:metrics_dashboard)
 
       ::Metrics::Dashboard::SyncDashboardsWorker.perform_async(project.id)
     end
@@ -210,10 +218,10 @@ module Git
 
     def commit_paths
       strong_memoize(:commit_paths) do
-        limited_commits.map do |commit|
+        limited_commits.to_h do |commit|
           paths = Set.new(commit.raw_deltas.map(&:new_path))
           [commit, paths]
-        end.to_h
+        end
       end
     end
   end

spec/services/git/branch_hooks_service_spec.rb

@@ -11,7 +11,8 @@ RSpec.describe Git::BranchHooksService do
 
   let(:branch) { project.default_branch }
   let(:ref) { "refs/heads/#{branch}" }
-  let(:commit) { project.commit(sample_commit.id) }
+  let(:commit_id) { sample_commit.id }
+  let(:commit) { project.commit(commit_id) }
   let(:oldrev) { commit.parent_id }
   let(:newrev) { commit.id }
 
@@ -227,7 +228,6 @@ RSpec.describe Git::BranchHooksService do
       )
     end
 
-    let(:commit) { project.repository.commit(commit_id) }
     let(:blank_sha) { Gitlab::Git::BLANK_SHA }
 
     def clears_cache(extended: [])
@@ -508,11 +508,7 @@ RSpec.describe Git::BranchHooksService do
   end
 
   describe 'Metrics dashboard sync' do
-    context 'with feature flag enabled' do
-      before do
-        Feature.enable(:metrics_dashboards_sync)
-      end
-
+    shared_examples 'trigger dashboard sync' do
       it 'imports metrics to database' do
         expect(Metrics::Dashboard::SyncDashboardsWorker).to receive(:perform_async)
 
@@ -520,12 +516,95 @@ RSpec.describe Git::BranchHooksService do
       end
     end
 
-    context 'with feature flag disabled' do
-      it 'imports metrics to database' do
-        expect(Metrics::Dashboard::SyncDashboardsWorker).to receive(:perform_async)
+    shared_examples 'no dashboard sync' do
+      it 'does not sync metrics to database' do
+        expect(Metrics::Dashboard::SyncDashboardsWorker).not_to receive(:perform_async)
 
         service.execute
       end
     end
+
+    def change_repository(**changes)
+      actions = changes.flat_map do |(action, paths)|
+        Array(paths).flat_map do |file_path|
+          { action: action, file_path: file_path, content: SecureRandom.hex }
+        end
+      end
+
+      project.repository.multi_action(
+        user, message: 'message', branch_name: branch, actions: actions
+      )
+    end
+
+    let(:charts) { '.gitlab/dashboards/charts.yml' }
+    let(:readme) { 'README.md' }
+    let(:commit_id) { change_repository(**commit_changes) }
+
+    context 'with default branch' do
+      context 'when adding files' do
+        let(:new_file) { 'somenewfile.md' }
+
+        context 'also related' do
+          let(:commit_changes) { { create: [charts, new_file] } }
+
+          include_examples 'trigger dashboard sync'
+        end
+
+        context 'only unrelated' do
+          let(:commit_changes) { { create: new_file } }
+
+          include_examples 'no dashboard sync'
+        end
+      end
+
+      context 'when deleting files' do
+        before do
+          change_repository(create: charts)
+        end
+
+        context 'also related' do
+          let(:commit_changes) { { delete: [charts, readme] } }
+
+          include_examples 'trigger dashboard sync'
+        end
+
+        context 'only unrelated' do
+          let(:commit_changes) { { delete: readme } }
+
+          include_examples 'no dashboard sync'
+        end
+      end
+
+      context 'when updating files' do
+        before do
+          change_repository(create: charts)
+        end
+
+        context 'also related' do
+          let(:commit_changes) { { update: [charts, readme] } }
+
+          include_examples 'trigger dashboard sync'
+        end
+
+        context 'only unrelated' do
+          let(:commit_changes) { { update: readme } }
+
+          include_examples 'no dashboard sync'
+        end
+      end
+
+      context 'without changes' do
+        let(:commit_changes) { {} }
+
+        include_examples 'no dashboard sync'
+      end
+    end
+
+    context 'with other branch' do
+      let(:branch) { 'fix' }
+      let(:commit_changes) { { create: charts } }
+
+      include_examples 'no dashboard sync'
+    end
   end
 end