Merge branch...

Merge branch 'kassio/remove-github_review_importer_query_only_unimported_merge_requests-feature-flag' into 'master'

Remove `github_review_importer_query_only_unimported_merge_requests` feature flag

See merge request gitlab-org/gitlab!65473

config/feature_flags/development/github_review_importer_query_only_unimported_merge_requests.yml

----
-name: github_review_importer_query_only_unimported_merge_requests
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/62036
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/332982
-milestone: '14.0'
-type: development
-group: group::import
-default_enabled: true

lib/gitlab/github_import/importer/pull_requests_reviews_importer.rb

@@ -37,43 +37,6 @@ module Gitlab
           review.id
         end
 
-        def each_object_to_import(&block)
-          if use_github_review_importer_query_only_unimported_merge_requests?
-            each_merge_request_to_import(&block)
-          else
-            each_merge_request_skipping_imported(&block)
-          end
-        end
-
-        private
-
-        attr_reader :merge_requests_already_imported_cache_key
-
-        # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/62036#note_587181108
-        def use_github_review_importer_query_only_unimported_merge_requests?
-          Feature.enabled?(
-            :github_review_importer_query_only_unimported_merge_requests,
-            default_enabled: :yaml
-          )
-        end
-
-        def each_merge_request_skipping_imported
-          project.merge_requests.find_each do |merge_request|
-            next if already_imported?(merge_request)
-
-            Gitlab::GithubImport::ObjectCounter.increment(project, object_type, :fetched)
-
-            client
-              .pull_request_reviews(project.import_source, merge_request.iid)
-              .each do |review|
-                review.merge_request_id = merge_request.id
-                yield(review)
-              end
-
-            mark_as_imported(merge_request)
-          end
-        end
-
         # The worker can be interrupted, by rate limit for instance,
         # in different situations. To avoid requesting already imported data,
         # if the worker is interrupted:
@@ -82,7 +45,7 @@ module Gitlab
         # - before importing all merge requests reviews
         #   Merge requests that had all the reviews imported are cached with
         #   `mark_merge_request_reviews_imported`
-        def each_merge_request_to_import
+        def each_object_to_import(&block)
           each_review_page do |page, merge_request|
             page.objects.each do |review|
               next if already_imported?(review)
@@ -97,6 +60,10 @@ module Gitlab
           end
         end
 
+        private
+
+        attr_reader :merge_requests_already_imported_cache_key
+
         def each_review_page
           merge_requests_to_import.find_each do |merge_request|
             # The page counter needs to be scoped by merge request to avoid skipping

spec/lib/gitlab/github_import/importer/pull_requests_reviews_importer_spec.rb

@@ -27,100 +27,62 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsReviewsImporter do
   end
 
   describe '#each_object_to_import', :clean_gitlab_redis_cache do
-    context 'when github_review_importer_query_only_unimported_merge_requests is enabled' do
-      before do
-        stub_feature_flags(github_review_importer_query_only_unimported_merge_requests: true)
-      end
-
-      let(:merge_request) do
-        create(
-          :merged_merge_request,
-          iid: 999,
-          source_project: project,
-          target_project: project
-        )
-      end
-
-      let(:review) { double(id: 1) }
-
-      it 'fetches the pull requests reviews data' do
-        page = double(objects: [review], number: 1)
-
-        expect(review)
-          .to receive(:merge_request_id=)
-          .with(merge_request.id)
-
-        expect(client)
-          .to receive(:each_page)
-          .exactly(:once) # ensure to be cached on the second call
-          .with(:pull_request_reviews, 'github/repo', merge_request.iid, page: 1)
-          .and_yield(page)
+    let(:merge_request) do
+      create(
+        :merged_merge_request,
+        iid: 999,
+        source_project: project,
+        target_project: project
+      )
+    end
 
-        expect { |b| subject.each_object_to_import(&b) }
-          .to yield_with_args(review)
+    let(:review) { double(id: 1) }
 
-        subject.each_object_to_import {}
-      end
+    it 'fetches the pull requests reviews data' do
+      page = double(objects: [review], number: 1)
 
-      it 'skips cached pages' do
-        Gitlab::GithubImport::PageCounter
-          .new(project, "merge_request/#{merge_request.id}/pull_request_reviews")
-          .set(2)
+      expect(review)
+        .to receive(:merge_request_id=)
+        .with(merge_request.id)
 
-        expect(review).not_to receive(:merge_request_id=)
+      expect(client)
+        .to receive(:each_page)
+        .exactly(:once) # ensure to be cached on the second call
+        .with(:pull_request_reviews, 'github/repo', merge_request.iid, page: 1)
+        .and_yield(page)
 
-        expect(client)
-          .to receive(:each_page)
-          .exactly(:once) # ensure to be cached on the second call
-          .with(:pull_request_reviews, 'github/repo', merge_request.iid, page: 2)
+      expect { |b| subject.each_object_to_import(&b) }
+        .to yield_with_args(review)
 
-        subject.each_object_to_import {}
-      end
+      subject.each_object_to_import {}
+    end
 
-      it 'skips cached merge requests' do
-        Gitlab::Cache::Import::Caching.set_add(
-          "github-importer/merge_request/already-imported/#{project.id}",
-          merge_request.id
-        )
+    it 'skips cached pages' do
+      Gitlab::GithubImport::PageCounter
+        .new(project, "merge_request/#{merge_request.id}/pull_request_reviews")
+        .set(2)
 
-        expect(review).not_to receive(:merge_request_id=)
+      expect(review).not_to receive(:merge_request_id=)
 
-        expect(client).not_to receive(:each_page)
+      expect(client)
+        .to receive(:each_page)
+        .exactly(:once) # ensure to be cached on the second call
+        .with(:pull_request_reviews, 'github/repo', merge_request.iid, page: 2)
 
-        subject.each_object_to_import {}
-      end
+      subject.each_object_to_import {}
     end
 
-    context 'when github_review_importer_query_only_unimported_merge_requests is disabled' do
-      before do
-        stub_feature_flags(github_review_importer_query_only_unimported_merge_requests: false)
-      end
-
-      it 'fetchs the merged pull requests data' do
-        merge_request = create(
-          :merged_merge_request,
-          iid: 999,
-          source_project: project,
-          target_project: project
-        )
-
-        review = double
-
-        expect(review)
-          .to receive(:merge_request_id=)
-          .with(merge_request.id)
+    it 'skips cached merge requests' do
+      Gitlab::Cache::Import::Caching.set_add(
+        "github-importer/merge_request/already-imported/#{project.id}",
+        merge_request.id
+      )
 
-        allow(client)
-          .to receive(:pull_request_reviews)
-          .exactly(:once) # ensure to be cached on the second call
-          .with('github/repo', merge_request.iid)
-          .and_return([review])
+      expect(review).not_to receive(:merge_request_id=)
 
-        expect { |b| subject.each_object_to_import(&b) }
-          .to yield_with_args(review)
+      expect(client).not_to receive(:each_page)
 
-        subject.each_object_to_import {}
-      end
+      subject.each_object_to_import {}
     end
   end
 end