From 67806750d0471034f6df614147874314a69d2390 Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Wed, 24 Jan 2024 20:00:53 +0100 Subject: [PATCH 01/10] Move order of collaborators import This change moves github collaborators import out of an advance worker stage and before pull requests are imported. Changelog: changed --- .../github_import/advance_stage_worker.rb | 4 ++-- .../stage/import_base_data_worker.rb | 2 +- .../stage/import_collaborators_worker.rb | 18 +++++------------- .../stage/import_pull_requests_worker.rb | 18 +++++++++++++----- .../stage/import_base_data_worker_spec.rb | 2 +- .../stage/import_collaborators_worker_spec.rb | 17 +++++++++-------- .../stage/import_pull_requests_worker_spec.rb | 4 ++-- 7 files changed, 33 insertions(+), 32 deletions(-) diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 316035fd683af4..81a12c7f2d5b91 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -17,13 +17,13 @@ class AdvanceStageWorker # rubocop:disable Scalability/IdempotentWorker # The known importer stages and their corresponding Sidekiq workers. # - # Note: AdvanceStageWorker is not used for the repository, base_data, and pull_requests stages. + # Note: AdvanceStageWorker is not used for the repository, base_data, collaborators, and pull_requests stages. # They are included in the list for us to easily see all stage workers and the order in which they are executed. STAGES = { repository: Stage::ImportRepositoryWorker, base_data: Stage::ImportBaseDataWorker, - pull_requests: Stage::ImportPullRequestsWorker, collaborators: Stage::ImportCollaboratorsWorker, + pull_requests: Stage::ImportPullRequestsWorker, pull_requests_merged_by: Stage::ImportPullRequestsMergedByWorker, # Skipped on extended_events pull_request_review_requests: Stage::ImportPullRequestsReviewRequestsWorker, # Skipped on extended_events pull_request_reviews: Stage::ImportPullRequestsReviewsWorker, # Skipped on extended_events diff --git a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb index d965c1ae847064..ff30e72e3a7ea6 100644 --- a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb @@ -26,7 +26,7 @@ def import(client, project) klass.new(project, client).execute end - ImportPullRequestsWorker.perform_async(project.id) + ImportCollaboratorsWorker.perform_async(project.id) end end end diff --git a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb index 38e1fd52889c7b..c1b6710e4faf3b 100644 --- a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb @@ -18,9 +18,9 @@ def import(client, project) info(project.id, message: 'starting importer', importer: 'Importer::CollaboratorsImporter') - waiter = Importer::CollaboratorsImporter.new(project, client).execute + Importer::CollaboratorsImporter.new(project, client).execute - move_to_next_stage(project, { waiter.key => waiter.jobs_remaining }) + move_to_next_stage(project) end private @@ -37,19 +37,11 @@ def skip_to_next_stage(project) importer: 'Importer::CollaboratorsImporter' ) ) - move_to_next_stage(project, {}) + move_to_next_stage(project) end - def move_to_next_stage(project, waiters = {}) - AdvanceStageWorker.perform_async( - project.id, waiters.deep_stringify_keys, next_stage(project) - ) - end - - def next_stage(project) - return 'issues_and_diff_notes' if import_settings(project).extended_events? - - 'pull_requests_merged_by' + def move_to_next_stage(project) + ImportPullRequestsWorker.perform_async(project.id) end end end diff --git a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb index bcc39b169afeb8..6de8ad072d2478 100644 --- a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb @@ -26,11 +26,7 @@ def import(client, project) .new(project, client) .execute - AdvanceStageWorker.perform_async( - project.id, - { waiter.key => waiter.jobs_remaining }, - 'collaborators' - ) + move_to_next_stage(project, { waiter.key => waiter.jobs_remaining }) end private @@ -45,6 +41,18 @@ def allocate_merge_requests_internal_id!(project, client) MergeRequest.track_target_project_iid!(project, last_github_pull_request[:number]) end + + def move_to_next_stage(project, waiters = {}) + AdvanceStageWorker.perform_async( + project.id, waiters.deep_stringify_keys, next_stage(project) + ) + end + + def next_stage(project) + return 'issues_and_diff_notes' if import_settings(project).extended_events? + + 'pull_requests_merged_by' + end end end end diff --git a/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb index 49dc905f430079..6eb636cb2815b5 100644 --- a/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb @@ -23,7 +23,7 @@ expect(importer).to receive(:execute) end - expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker) + expect(Gitlab::GithubImport::Stage::ImportCollaboratorsWorker) .to receive(:perform_async) .with(project.id) diff --git a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb index 7a085227b367d8..3faf9b129fa285 100644 --- a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb @@ -25,17 +25,18 @@ context 'when user has push access for this repo' do it 'imports all collaborators' do - waiter = Gitlab::JobWaiter.new(2, '123') + # waiter = Gitlab::JobWaiter.new(2, '123') expect(Gitlab::GithubImport::Importer::CollaboratorsImporter) .to receive(:new) .with(project, client) .and_return(importer) - expect(importer).to receive(:execute).and_return(waiter) - expect(Gitlab::GithubImport::AdvanceStageWorker) + expect(importer).to receive(:execute) + + expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') + .with(project.id) worker.import(client, project) end @@ -47,9 +48,9 @@ it 'skips stage' do expect(Gitlab::GithubImport::Importer::CollaboratorsImporter).not_to receive(:new) - expect(Gitlab::GithubImport::AdvanceStageWorker) + expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker) .to receive(:perform_async) - .with(project.id, {}, 'pull_requests_merged_by') + .with(project.id) worker.import(client, project) end @@ -61,9 +62,9 @@ it 'skips collaborators import and calls next stage' do expect(Gitlab::GithubImport::Importer::CollaboratorsImporter).not_to receive(:new) - expect(Gitlab::GithubImport::AdvanceStageWorker) + expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker) .to receive(:perform_async) - .with(project.id, {}, 'pull_requests_merged_by') + .with(project.id) worker.import(client, project) end diff --git a/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb index 2c1beb29fa19c7..be99a0bdf9fe26 100644 --- a/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb @@ -35,7 +35,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'collaborators') + .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') expect(MergeRequest).to receive(:track_target_project_iid!) @@ -64,7 +64,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'collaborators') + .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') expect(MergeRequest).not_to receive(:track_target_project_iid!) -- GitLab From 594b4cc1f5e3f73703b713c7d4df3e30e02794c2 Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Mon, 29 Jan 2024 18:11:12 +0100 Subject: [PATCH 02/10] Trigger pull requests with advance stage worker --- .../gitlab/github_import/advance_stage_worker.rb | 2 +- .../stage/import_collaborators_worker.rb | 12 +++++++----- .../stage/import_collaborators_worker_spec.rb | 16 ++++++++-------- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 81a12c7f2d5b91..537ac0e3105e4c 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -17,7 +17,7 @@ class AdvanceStageWorker # rubocop:disable Scalability/IdempotentWorker # The known importer stages and their corresponding Sidekiq workers. # - # Note: AdvanceStageWorker is not used for the repository, base_data, collaborators, and pull_requests stages. + # Note: AdvanceStageWorker is not used for the repository, base_data, and collaborators stages. # They are included in the list for us to easily see all stage workers and the order in which they are executed. STAGES = { repository: Stage::ImportRepositoryWorker, diff --git a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb index c1b6710e4faf3b..70c0ceb6d2960e 100644 --- a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb @@ -18,9 +18,9 @@ def import(client, project) info(project.id, message: 'starting importer', importer: 'Importer::CollaboratorsImporter') - Importer::CollaboratorsImporter.new(project, client).execute + waiter = Importer::CollaboratorsImporter.new(project, client).execute - move_to_next_stage(project) + move_to_next_stage(project, { waiter.key => waiter.jobs_remaining }) end private @@ -37,11 +37,13 @@ def skip_to_next_stage(project) importer: 'Importer::CollaboratorsImporter' ) ) - move_to_next_stage(project) + move_to_next_stage(project, {}) end - def move_to_next_stage(project) - ImportPullRequestsWorker.perform_async(project.id) + def move_to_next_stage(project, waiters = {}) + AdvanceStageWorker.perform_async( + project.id, waiters.deep_stringify_keys, 'pull_requests' + ) end end end diff --git a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb index 3faf9b129fa285..4828f78ef19320 100644 --- a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb @@ -25,18 +25,18 @@ context 'when user has push access for this repo' do it 'imports all collaborators' do - # waiter = Gitlab::JobWaiter.new(2, '123') + waiter = Gitlab::JobWaiter.new(2, '123') expect(Gitlab::GithubImport::Importer::CollaboratorsImporter) .to receive(:new) .with(project, client) .and_return(importer) - expect(importer).to receive(:execute) + expect(importer).to receive(:execute).and_return(waiter) - expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker) + expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id) + .with(project.id, { '123' => 2 }, 'pull_requests') worker.import(client, project) end @@ -48,9 +48,9 @@ it 'skips stage' do expect(Gitlab::GithubImport::Importer::CollaboratorsImporter).not_to receive(:new) - expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker) + expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id) + .with(project.id, {}, 'pull_requests') worker.import(client, project) end @@ -62,9 +62,9 @@ it 'skips collaborators import and calls next stage' do expect(Gitlab::GithubImport::Importer::CollaboratorsImporter).not_to receive(:new) - expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker) + expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id) + .with(project.id, {}, 'pull_requests') worker.import(client, project) end -- GitLab From da9a7c76f429e767a7efb618984da432acb12c0f Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Tue, 30 Jan 2024 06:12:26 +0100 Subject: [PATCH 03/10] Update documentation and comment --- .../gitlab/github_import/advance_stage_worker.rb | 2 +- doc/development/github_importer.md | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 537ac0e3105e4c..9312a6125a467e 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -17,7 +17,7 @@ class AdvanceStageWorker # rubocop:disable Scalability/IdempotentWorker # The known importer stages and their corresponding Sidekiq workers. # - # Note: AdvanceStageWorker is not used for the repository, base_data, and collaborators stages. + # Note: AdvanceStageWorker is not used for the repository and base_data stages. # They are included in the list for us to easily see all stage workers and the order in which they are executed. STAGES = { repository: Stage::ImportRepositoryWorker, diff --git a/doc/development/github_importer.md b/doc/development/github_importer.md index c3fad50f2a62d2..c31d2fbef75962 100644 --- a/doc/development/github_importer.md +++ b/doc/development/github_importer.md @@ -82,12 +82,7 @@ This worker imports base data such as labels, milestones, and releases. This work is done in a single thread because it can be performed fast enough that we don't need to perform this work in parallel. -### 4. Stage::ImportPullRequestsWorker - -This worker imports all pull requests. For every pull request a job for the -`Gitlab::GithubImport::ImportPullRequestWorker` worker is scheduled. - -### 5. Stage::ImportCollaboratorsWorker +### 4. Stage::ImportCollaboratorsWorker This worker imports only direct repository collaborators who are not outside collaborators. For every collaborator, we schedule a job for the `Gitlab::GithubImport::ImportCollaboratorWorker` worker. @@ -95,6 +90,11 @@ For every collaborator, we schedule a job for the `Gitlab::GithubImport::ImportC NOTE: This stage is optional (controlled by `Gitlab::GithubImport::Settings`) and is selected by default. +### 5. Stage::ImportPullRequestsWorker + +This worker imports all pull requests. For every pull request a job for the +`Gitlab::GithubImport::ImportPullRequestWorker` worker is scheduled. + ### 6. Stage::ImportPullRequestsMergedByWorker (deprecated) This worker imports the pull requests' _merged-by_ user information. The -- GitLab From 0e9086afa42233dc24d48a4d86d6a7aeb66e90cd Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Tue, 30 Jan 2024 18:02:00 +0100 Subject: [PATCH 04/10] Move changes behind a feature flag --- .../github_import/advance_stage_worker.rb | 54 +++++++++++++------ .../stage/import_base_data_worker.rb | 6 ++- .../stage/import_collaborators_worker.rb | 13 ++++- .../stage/import_pull_requests_worker.rb | 8 ++- ...github_import_prioritize_collaborators.yml | 8 +++ doc/development/github_importer.md | 12 ++--- .../stage/import_base_data_worker_spec.rb | 53 +++++++++++++----- .../stage/import_collaborators_worker_spec.rb | 30 +++++++++-- .../stage/import_pull_requests_worker_spec.rb | 41 +++++++++++++- 9 files changed, 180 insertions(+), 45 deletions(-) create mode 100644 config/feature_flags/development/github_import_prioritize_collaborators.yml diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 9312a6125a467e..5ebf00d88616bf 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -17,24 +17,44 @@ class AdvanceStageWorker # rubocop:disable Scalability/IdempotentWorker # The known importer stages and their corresponding Sidekiq workers. # - # Note: AdvanceStageWorker is not used for the repository and base_data stages. + # Note: AdvanceStageWorker is not used to initiate the repository, base_data, and collaborators stages. # They are included in the list for us to easily see all stage workers and the order in which they are executed. - STAGES = { - repository: Stage::ImportRepositoryWorker, - base_data: Stage::ImportBaseDataWorker, - collaborators: Stage::ImportCollaboratorsWorker, - pull_requests: Stage::ImportPullRequestsWorker, - pull_requests_merged_by: Stage::ImportPullRequestsMergedByWorker, # Skipped on extended_events - pull_request_review_requests: Stage::ImportPullRequestsReviewRequestsWorker, # Skipped on extended_events - pull_request_reviews: Stage::ImportPullRequestsReviewsWorker, # Skipped on extended_events - issues_and_diff_notes: Stage::ImportIssuesAndDiffNotesWorker, - issue_events: Stage::ImportIssueEventsWorker, - notes: Stage::ImportNotesWorker, # Skipped on extended_events - attachments: Stage::ImportAttachmentsWorker, - protected_branches: Stage::ImportProtectedBranchesWorker, - lfs_objects: Stage::ImportLfsObjectsWorker, - finish: Stage::FinishImportWorker - }.freeze + + if Feature.enabled?(:github_import_prioritize_collaborators) + STAGES = { + repository: Stage::ImportRepositoryWorker, + base_data: Stage::ImportBaseDataWorker, + collaborators: Stage::ImportCollaboratorsWorker, + pull_requests: Stage::ImportPullRequestsWorker, + pull_requests_merged_by: Stage::ImportPullRequestsMergedByWorker, # Skipped on extended_events + pull_request_review_requests: Stage::ImportPullRequestsReviewRequestsWorker, # Skipped on extended_events + pull_request_reviews: Stage::ImportPullRequestsReviewsWorker, # Skipped on extended_events + issues_and_diff_notes: Stage::ImportIssuesAndDiffNotesWorker, + issue_events: Stage::ImportIssueEventsWorker, + notes: Stage::ImportNotesWorker, # Skipped on extended_events + attachments: Stage::ImportAttachmentsWorker, + protected_branches: Stage::ImportProtectedBranchesWorker, + lfs_objects: Stage::ImportLfsObjectsWorker, + finish: Stage::FinishImportWorker + }.freeze + else + STAGES = { + repository: Stage::ImportRepositoryWorker, + base_data: Stage::ImportBaseDataWorker, + pull_requests: Stage::ImportPullRequestsWorker, + collaborators: Stage::ImportCollaboratorsWorker, + pull_requests_merged_by: Stage::ImportPullRequestsMergedByWorker, # Skipped on extended_events + pull_request_review_requests: Stage::ImportPullRequestsReviewRequestsWorker, # Skipped on extended_events + pull_request_reviews: Stage::ImportPullRequestsReviewsWorker, # Skipped on extended_events + issues_and_diff_notes: Stage::ImportIssuesAndDiffNotesWorker, + issue_events: Stage::ImportIssueEventsWorker, + notes: Stage::ImportNotesWorker, # Skipped on extended_events + attachments: Stage::ImportAttachmentsWorker, + protected_branches: Stage::ImportProtectedBranchesWorker, + lfs_objects: Stage::ImportLfsObjectsWorker, + finish: Stage::FinishImportWorker + }.freeze + end def find_import_state_jid(project_id) ProjectImportState.jid_by(project_id: project_id, status: :started) diff --git a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb index ff30e72e3a7ea6..48ab3ec9eba5c5 100644 --- a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb @@ -26,7 +26,11 @@ def import(client, project) klass.new(project, client).execute end - ImportCollaboratorsWorker.perform_async(project.id) + if Feature.enabled?(:github_import_prioritize_collaborators) + ImportCollaboratorsWorker.perform_async(project.id) + else + ImportPullRequestsWorker.perform_async(project.id) + end end end end diff --git a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb index 70c0ceb6d2960e..ca593c9cbcb8d4 100644 --- a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb @@ -42,9 +42,20 @@ def skip_to_next_stage(project) def move_to_next_stage(project, waiters = {}) AdvanceStageWorker.perform_async( - project.id, waiters.deep_stringify_keys, 'pull_requests' + project.id, waiters.deep_stringify_keys, next_stage(project) ) end + + def next_stage(project) + if Feature.enabled?(:github_import_prioritize_collaborators) + 'pull_requests' + else + + return 'issues_and_diff_notes' if import_settings(project).extended_events? + + 'pull_requests_merged_by' + end + end end end end diff --git a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb index 6de8ad072d2478..820c652a235e3c 100644 --- a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb @@ -49,9 +49,13 @@ def move_to_next_stage(project, waiters = {}) end def next_stage(project) - return 'issues_and_diff_notes' if import_settings(project).extended_events? + if Feature.enabled?(:github_import_prioritize_collaborators) + return 'issues_and_diff_notes' if import_settings(project).extended_events? - 'pull_requests_merged_by' + 'pull_requests_merged_by' + else + 'collaborators' + end end end end diff --git a/config/feature_flags/development/github_import_prioritize_collaborators.yml b/config/feature_flags/development/github_import_prioritize_collaborators.yml new file mode 100644 index 00000000000000..f323a83a40779c --- /dev/null +++ b/config/feature_flags/development/github_import_prioritize_collaborators.yml @@ -0,0 +1,8 @@ +--- +name: github_import_prioritize_collaborators +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139410 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439742 +milestone: '16.9' +type: development +group: group::import and integrate +default_enabled: false diff --git a/doc/development/github_importer.md b/doc/development/github_importer.md index c31d2fbef75962..c3fad50f2a62d2 100644 --- a/doc/development/github_importer.md +++ b/doc/development/github_importer.md @@ -82,7 +82,12 @@ This worker imports base data such as labels, milestones, and releases. This work is done in a single thread because it can be performed fast enough that we don't need to perform this work in parallel. -### 4. Stage::ImportCollaboratorsWorker +### 4. Stage::ImportPullRequestsWorker + +This worker imports all pull requests. For every pull request a job for the +`Gitlab::GithubImport::ImportPullRequestWorker` worker is scheduled. + +### 5. Stage::ImportCollaboratorsWorker This worker imports only direct repository collaborators who are not outside collaborators. For every collaborator, we schedule a job for the `Gitlab::GithubImport::ImportCollaboratorWorker` worker. @@ -90,11 +95,6 @@ For every collaborator, we schedule a job for the `Gitlab::GithubImport::ImportC NOTE: This stage is optional (controlled by `Gitlab::GithubImport::Settings`) and is selected by default. -### 5. Stage::ImportPullRequestsWorker - -This worker imports all pull requests. For every pull request a job for the -`Gitlab::GithubImport::ImportPullRequestWorker` worker is scheduled. - ### 6. Stage::ImportPullRequestsMergedByWorker (deprecated) This worker imports the pull requests' _merged-by_ user information. The diff --git a/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb index 6eb636cb2815b5..50c33b99800fb2 100644 --- a/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb @@ -13,21 +13,50 @@ it_behaves_like Gitlab::GithubImport::StageMethods describe '#import' do - it 'imports the base data of a project' do - described_class::IMPORTERS.each do |klass| - expect(klass) - .to receive(:new) - .with(project, client) - .and_return(importer) - - expect(importer).to receive(:execute) + context 'when the prioritize_collaborators feature flag is enabled' do + before do + stub_feature_flags(github_import_prioritize_collaborators: true) end - expect(Gitlab::GithubImport::Stage::ImportCollaboratorsWorker) - .to receive(:perform_async) - .with(project.id) + it 'imports the base data of a project' do + described_class::IMPORTERS.each do |klass| + expect(klass) + .to receive(:new) + .with(project, client) + .and_return(importer) - worker.import(client, project) + expect(importer).to receive(:execute) + end + + expect(Gitlab::GithubImport::Stage::ImportCollaboratorsWorker) + .to receive(:perform_async) + .with(project.id) + + worker.import(client, project) + end + end + + context 'when the prioritize_collaborators feature flag is disabled' do + before do + stub_feature_flags(github_import_prioritize_collaborators: false) + end + + it 'imports the base data of a project' do + described_class::IMPORTERS.each do |klass| + expect(klass) + .to receive(:new) + .with(project, client) + .and_return(importer) + + expect(importer).to receive(:execute) + end + + expect(Gitlab::GithubImport::Stage::ImportPullRequestsWorker) + .to receive(:perform_async) + .with(project.id) + + worker.import(client, project) + end end end end diff --git a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb index 4828f78ef19320..ac7ed96c674320 100644 --- a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb @@ -21,6 +21,7 @@ settings.write({ optional_stages: { collaborators_import: stage_enabled } }) allow(client).to receive(:repository).with(project.import_source) .and_return({ permissions: { push: push_rights_granted } }) + stub_feature_flags(github_import_prioritize_collaborators: false) end context 'when user has push access for this repo' do @@ -31,12 +32,11 @@ .to receive(:new) .with(project, client) .and_return(importer) - expect(importer).to receive(:execute).and_return(waiter) expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'pull_requests') + .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') worker.import(client, project) end @@ -50,7 +50,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, {}, 'pull_requests') + .with(project.id, {}, 'pull_requests_merged_by') worker.import(client, project) end @@ -64,7 +64,29 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, {}, 'pull_requests') + .with(project.id, {}, 'pull_requests_merged_by') + + worker.import(client, project) + end + end + + context 'when github_importer_collaborators_import feature flag is enabled' do + before do + stub_feature_flags(github_import_prioritize_collaborators: true) + end + + it 'imports all collaborators and advances to the correct next stage' do + waiter = Gitlab::JobWaiter.new(2, '123') + + expect(Gitlab::GithubImport::Importer::CollaboratorsImporter) + .to receive(:new) + .with(project, client) + .and_return(importer) + expect(importer).to receive(:execute).and_return(waiter) + + expect(Gitlab::GithubImport::AdvanceStageWorker) + .to receive(:perform_async) + .with(project.id, { '123' => 2 }, 'pull_requests') worker.import(client, project) end diff --git a/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb index be99a0bdf9fe26..5a3cf83767822f 100644 --- a/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb @@ -14,6 +14,10 @@ it_behaves_like Gitlab::GithubImport::StageMethods describe '#import' do + before do + stub_feature_flags(github_import_prioritize_collaborators: false) + end + context 'with pull requests' do it 'imports all the pull requests and allocates internal iids' do waiter = Gitlab::JobWaiter.new(2, '123') @@ -35,7 +39,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') + .with(project.id, { '123' => 2 }, 'collaborators') expect(MergeRequest).to receive(:track_target_project_iid!) @@ -64,7 +68,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') + .with(project.id, { '123' => 2 }, 'collaborators') expect(MergeRequest).not_to receive(:track_target_project_iid!) @@ -93,5 +97,38 @@ worker.import(client, project) end end + + context 'when the :github_import_prioritize_collaborators feature flag is enabled' do + before do + stub_feature_flags(github_import_prioritize_collaborators: true) + end + + it 'advances to the correct next stage' do + waiter = Gitlab::JobWaiter.new(2, '123') + + expect(Gitlab::GithubImport::Importer::PullRequestsImporter) + .to receive(:new) + .with(project, client) + .and_return(importer) + + expect(importer) + .to receive(:execute) + .and_return(waiter) + + expect(InternalId).to receive(:exists?).and_return(false) + + expect(client).to receive(:each_object).with( + :pulls, project.import_source, options + ).and_return([{ number: 4 }].each) + + expect(Gitlab::GithubImport::AdvanceStageWorker) + .to receive(:perform_async) + .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') + + expect(MergeRequest).to receive(:track_target_project_iid!) + + worker.import(client, project) + end + end end end -- GitLab From 150098d9bbd116073c93f9fa2f908efecd9c4bb4 Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Wed, 31 Jan 2024 17:38:38 +0100 Subject: [PATCH 05/10] Use import setting to determine ff status --- app/services/import/github_service.rb | 3 +- .../github_import/advance_stage_worker.rb | 51 ++++++------------- .../stage/import_base_data_worker.rb | 2 +- .../stage/import_collaborators_worker.rb | 2 +- spec/services/import/github_service_spec.rb | 21 +++++--- .../stage/import_base_data_worker_spec.rb | 38 +++++++------- .../stage/import_collaborators_worker_spec.rb | 22 ++++---- 7 files changed, 64 insertions(+), 75 deletions(-) diff --git a/app/services/import/github_service.rb b/app/services/import/github_service.rb index 7771af18cf5741..7245b697beb9a6 100644 --- a/app/services/import/github_service.rb +++ b/app/services/import/github_service.rb @@ -142,7 +142,8 @@ def store_import_settings(project) .write( timeout_strategy: params[:timeout_strategy] || ProjectImportData::PESSIMISTIC_TIMEOUT, optional_stages: params[:optional_stages], - extended_events: Feature.enabled?(:github_import_extended_events, current_user) + extended_events: Feature.enabled?(:github_import_extended_events, current_user), + prioritize_collaborators: Feature.enabled?(:github_import_prioritize_collaborators, current_user) ) end end diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 5ebf00d88616bf..4f7fbb3c81a4d8 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -20,41 +20,22 @@ class AdvanceStageWorker # rubocop:disable Scalability/IdempotentWorker # Note: AdvanceStageWorker is not used to initiate the repository, base_data, and collaborators stages. # They are included in the list for us to easily see all stage workers and the order in which they are executed. - if Feature.enabled?(:github_import_prioritize_collaborators) - STAGES = { - repository: Stage::ImportRepositoryWorker, - base_data: Stage::ImportBaseDataWorker, - collaborators: Stage::ImportCollaboratorsWorker, - pull_requests: Stage::ImportPullRequestsWorker, - pull_requests_merged_by: Stage::ImportPullRequestsMergedByWorker, # Skipped on extended_events - pull_request_review_requests: Stage::ImportPullRequestsReviewRequestsWorker, # Skipped on extended_events - pull_request_reviews: Stage::ImportPullRequestsReviewsWorker, # Skipped on extended_events - issues_and_diff_notes: Stage::ImportIssuesAndDiffNotesWorker, - issue_events: Stage::ImportIssueEventsWorker, - notes: Stage::ImportNotesWorker, # Skipped on extended_events - attachments: Stage::ImportAttachmentsWorker, - protected_branches: Stage::ImportProtectedBranchesWorker, - lfs_objects: Stage::ImportLfsObjectsWorker, - finish: Stage::FinishImportWorker - }.freeze - else - STAGES = { - repository: Stage::ImportRepositoryWorker, - base_data: Stage::ImportBaseDataWorker, - pull_requests: Stage::ImportPullRequestsWorker, - collaborators: Stage::ImportCollaboratorsWorker, - pull_requests_merged_by: Stage::ImportPullRequestsMergedByWorker, # Skipped on extended_events - pull_request_review_requests: Stage::ImportPullRequestsReviewRequestsWorker, # Skipped on extended_events - pull_request_reviews: Stage::ImportPullRequestsReviewsWorker, # Skipped on extended_events - issues_and_diff_notes: Stage::ImportIssuesAndDiffNotesWorker, - issue_events: Stage::ImportIssueEventsWorker, - notes: Stage::ImportNotesWorker, # Skipped on extended_events - attachments: Stage::ImportAttachmentsWorker, - protected_branches: Stage::ImportProtectedBranchesWorker, - lfs_objects: Stage::ImportLfsObjectsWorker, - finish: Stage::FinishImportWorker - }.freeze - end + STAGES = { + repository: Stage::ImportRepositoryWorker, + base_data: Stage::ImportBaseDataWorker, + pull_requests: Stage::ImportPullRequestsWorker, + collaborators: Stage::ImportCollaboratorsWorker, + pull_requests_merged_by: Stage::ImportPullRequestsMergedByWorker, # Skipped on extended_events + pull_request_review_requests: Stage::ImportPullRequestsReviewRequestsWorker, # Skipped on extended_events + pull_request_reviews: Stage::ImportPullRequestsReviewsWorker, # Skipped on extended_events + issues_and_diff_notes: Stage::ImportIssuesAndDiffNotesWorker, + issue_events: Stage::ImportIssueEventsWorker, + notes: Stage::ImportNotesWorker, # Skipped on extended_events + attachments: Stage::ImportAttachmentsWorker, + protected_branches: Stage::ImportProtectedBranchesWorker, + lfs_objects: Stage::ImportLfsObjectsWorker, + finish: Stage::FinishImportWorker + }.freeze def find_import_state_jid(project_id) ProjectImportState.jid_by(project_id: project_id, status: :started) diff --git a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb index 48ab3ec9eba5c5..82e5f396238b7e 100644 --- a/app/workers/gitlab/github_import/stage/import_base_data_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_base_data_worker.rb @@ -26,7 +26,7 @@ def import(client, project) klass.new(project, client).execute end - if Feature.enabled?(:github_import_prioritize_collaborators) + if import_settings(project).prioritize_collaborators? ImportCollaboratorsWorker.perform_async(project.id) else ImportPullRequestsWorker.perform_async(project.id) diff --git a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb index ca593c9cbcb8d4..fd8b494f9372ec 100644 --- a/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_collaborators_worker.rb @@ -47,7 +47,7 @@ def move_to_next_stage(project, waiters = {}) end def next_stage(project) - if Feature.enabled?(:github_import_prioritize_collaborators) + if import_settings(project).prioritize_collaborators? 'pull_requests' else diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 06ce00260edcdd..7be2351fa6a15a 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -33,7 +33,8 @@ .with( extended_events: true, optional_stages: optional_stages, - timeout_strategy: timeout_strategy + timeout_strategy: timeout_strategy, + prioritize_collaborators: true ) end @@ -94,7 +95,8 @@ .to have_received(:write) .with(optional_stages: nil, extended_events: true, - timeout_strategy: timeout_strategy + timeout_strategy: timeout_strategy, + prioritize_collaborators: true ) expect_snowplow_event( category: 'Import::GithubService', @@ -120,7 +122,8 @@ .with( optional_stages: nil, extended_events: true, - timeout_strategy: timeout_strategy + timeout_strategy: timeout_strategy, + prioritize_collaborators: true ) expect_snowplow_event( category: 'Import::GithubService', @@ -153,7 +156,8 @@ .with( optional_stages: nil, extended_events: true, - timeout_strategy: timeout_strategy + timeout_strategy: timeout_strategy, + prioritize_collaborators: true ) expect_snowplow_event( category: 'Import::GithubService', @@ -190,7 +194,8 @@ .with( optional_stages: optional_stages, extended_events: true, - timeout_strategy: timeout_strategy + timeout_strategy: timeout_strategy, + prioritize_collaborators: true ) end end @@ -206,7 +211,8 @@ .with( optional_stages: optional_stages, extended_events: true, - timeout_strategy: timeout_strategy + timeout_strategy: timeout_strategy, + prioritize_collaborators: true ) end end @@ -220,7 +226,8 @@ .with( optional_stages: optional_stages, extended_events: true, - timeout_strategy: timeout_strategy + timeout_strategy: timeout_strategy, + prioritize_collaborators: true ) end end diff --git a/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb index 50c33b99800fb2..081aa507763a92 100644 --- a/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_base_data_worker_spec.rb @@ -13,35 +13,33 @@ it_behaves_like Gitlab::GithubImport::StageMethods describe '#import' do - context 'when the prioritize_collaborators feature flag is enabled' do - before do - stub_feature_flags(github_import_prioritize_collaborators: true) + it 'imports the base data of a project' do + allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting| + allow(setting).to receive(:prioritize_collaborators?).and_return(true) end - it 'imports the base data of a project' do - described_class::IMPORTERS.each do |klass| - expect(klass) - .to receive(:new) - .with(project, client) - .and_return(importer) + described_class::IMPORTERS.each do |klass| + expect(klass) + .to receive(:new) + .with(project, client) + .and_return(importer) - expect(importer).to receive(:execute) - end + expect(importer).to receive(:execute) + end - expect(Gitlab::GithubImport::Stage::ImportCollaboratorsWorker) - .to receive(:perform_async) - .with(project.id) + expect(Gitlab::GithubImport::Stage::ImportCollaboratorsWorker) + .to receive(:perform_async) + .with(project.id) - worker.import(client, project) - end + worker.import(client, project) end context 'when the prioritize_collaborators feature flag is disabled' do - before do - stub_feature_flags(github_import_prioritize_collaborators: false) - end - it 'imports the base data of a project' do + allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting| + allow(setting).to receive(:prioritize_collaborators?).and_return(false) + end + described_class::IMPORTERS.each do |klass| expect(klass) .to receive(:new) diff --git a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb index ac7ed96c674320..7f1ef5c726f558 100644 --- a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb @@ -21,7 +21,9 @@ settings.write({ optional_stages: { collaborators_import: stage_enabled } }) allow(client).to receive(:repository).with(project.import_source) .and_return({ permissions: { push: push_rights_granted } }) - stub_feature_flags(github_import_prioritize_collaborators: false) + allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting| + allow(setting).to receive(:prioritize_collaborators?).and_return(true) + end end context 'when user has push access for this repo' do @@ -36,7 +38,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') + .with(project.id, { '123' => 2 }, 'pull_requests') worker.import(client, project) end @@ -50,7 +52,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, {}, 'pull_requests_merged_by') + .with(project.id, {}, 'pull_requests') worker.import(client, project) end @@ -64,18 +66,18 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, {}, 'pull_requests_merged_by') + .with(project.id, {}, 'pull_requests') worker.import(client, project) end end - context 'when github_importer_collaborators_import feature flag is enabled' do - before do - stub_feature_flags(github_import_prioritize_collaborators: true) - end - + context 'when github_importer_collaborators_import feature flag is disabled' do it 'imports all collaborators and advances to the correct next stage' do + allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting| + allow(setting).to receive(:prioritize_collaborators?).and_return(false) + end + waiter = Gitlab::JobWaiter.new(2, '123') expect(Gitlab::GithubImport::Importer::CollaboratorsImporter) @@ -86,7 +88,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'pull_requests') + .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') worker.import(client, project) end -- GitLab From 8e81812bee1eb102b4c54f6e6a72c13c522b7281 Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Wed, 31 Jan 2024 21:19:58 +0100 Subject: [PATCH 06/10] Edit comment from collaborators to prs --- app/workers/gitlab/github_import/advance_stage_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/gitlab/github_import/advance_stage_worker.rb b/app/workers/gitlab/github_import/advance_stage_worker.rb index 4f7fbb3c81a4d8..82b3303e28cf81 100644 --- a/app/workers/gitlab/github_import/advance_stage_worker.rb +++ b/app/workers/gitlab/github_import/advance_stage_worker.rb @@ -17,7 +17,7 @@ class AdvanceStageWorker # rubocop:disable Scalability/IdempotentWorker # The known importer stages and their corresponding Sidekiq workers. # - # Note: AdvanceStageWorker is not used to initiate the repository, base_data, and collaborators stages. + # Note: AdvanceStageWorker is not used to initiate the repository, base_data, and pull_requests stages. # They are included in the list for us to easily see all stage workers and the order in which they are executed. STAGES = { -- GitLab From 0f42fffbad7f23cfdd3e4030367400977cd28c1d Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Thu, 1 Feb 2024 16:09:23 +0100 Subject: [PATCH 07/10] Add prioritize collabs method to settings class --- .../stage/import_pull_requests_worker.rb | 2 +- lib/gitlab/github_import/settings.rb | 7 ++++++- .../lib/gitlab/github_import/settings_spec.rb | 20 +++++++++++++++++++ spec/services/import/github_service_spec.rb | 16 ++++++++++++++- .../stage/import_pull_requests_worker_spec.rb | 12 +++++------ 5 files changed, 48 insertions(+), 9 deletions(-) diff --git a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb index 820c652a235e3c..0ff7af6b83ed7f 100644 --- a/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb +++ b/app/workers/gitlab/github_import/stage/import_pull_requests_worker.rb @@ -49,7 +49,7 @@ def move_to_next_stage(project, waiters = {}) end def next_stage(project) - if Feature.enabled?(:github_import_prioritize_collaborators) + if import_settings(project).prioritize_collaborators? return 'issues_and_diff_notes' if import_settings(project).extended_events? 'pull_requests_merged_by' diff --git a/lib/gitlab/github_import/settings.rb b/lib/gitlab/github_import/settings.rb index da5833df3a1d13..f99cd6c6931c32 100644 --- a/lib/gitlab/github_import/settings.rb +++ b/lib/gitlab/github_import/settings.rb @@ -67,7 +67,8 @@ def write(user_settings) data: { optional_stages: optional_stages, timeout_strategy: user_settings[:timeout_strategy], - extended_events: user_settings[:extended_events] + extended_events: user_settings[:extended_events], + prioritize_collaborators: user_settings[:prioritize_collaborators] }, credentials: project.import_data&.credentials ) @@ -87,6 +88,10 @@ def extended_events? !!project.import_data&.data&.dig('extended_events') end + def prioritize_collaborators? + !!project.import_data&.data&.dig('prioritize_collaborators') + end + private attr_reader :project diff --git a/spec/lib/gitlab/github_import/settings_spec.rb b/spec/lib/gitlab/github_import/settings_spec.rb index d268f3a8650950..022fde057cebda 100644 --- a/spec/lib/gitlab/github_import/settings_spec.rb +++ b/spec/lib/gitlab/github_import/settings_spec.rb @@ -137,4 +137,24 @@ expect(settings.extended_events?).to eq(false) end end + + describe '#prioritize_collaborators?' do + it 'when prioritize_collaborators is set to true' do + project.build_or_assign_import_data(data: { prioritize_collaborators: true }) + + expect(settings.prioritize_collaborators?).to eq(true) + end + + it 'when prioritize_collaborators is set to false' do + project.build_or_assign_import_data(data: { prioritize_collaborators: false }) + + expect(settings.prioritize_collaborators?).to eq(false) + end + + it 'when prioritize_collaborators is not present' do + project.build_or_assign_import_data(data: {}) + + expect(settings.prioritize_collaborators?).to eq(false) + end + end end diff --git a/spec/services/import/github_service_spec.rb b/spec/services/import/github_service_spec.rb index 7be2351fa6a15a..f4045318c1630b 100644 --- a/spec/services/import/github_service_spec.rb +++ b/spec/services/import/github_service_spec.rb @@ -232,7 +232,7 @@ end end - context 'when `github_import_extended_events`` feature flag is disabled' do + context 'when `github_import_extended_events` feature flag is disabled' do before do stub_feature_flags(github_import_extended_events: false) end @@ -245,6 +245,20 @@ subject.execute(access_params, :github) end end + + context 'when `prioritize_collaborators` feature flag is disabled' do + before do + stub_feature_flags(github_import_prioritize_collaborators: false) + end + + it 'saves prioritize_collaborators to import_data' do + expect(settings) + .to receive(:write) + .with(a_hash_including(prioritize_collaborators: false)) + + subject.execute(access_params, :github) + end + end end context 'when import source is disabled' do diff --git a/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb index 5a3cf83767822f..d9f174fea822b4 100644 --- a/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_pull_requests_worker_spec.rb @@ -15,7 +15,7 @@ describe '#import' do before do - stub_feature_flags(github_import_prioritize_collaborators: false) + project.build_or_assign_import_data(data: { prioritize_collaborators: true }) end context 'with pull requests' do @@ -39,7 +39,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'collaborators') + .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') expect(MergeRequest).to receive(:track_target_project_iid!) @@ -68,7 +68,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'collaborators') + .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') expect(MergeRequest).not_to receive(:track_target_project_iid!) @@ -98,9 +98,9 @@ end end - context 'when the :github_import_prioritize_collaborators feature flag is enabled' do + context 'when prioritize_collaborators import setting is disabled' do before do - stub_feature_flags(github_import_prioritize_collaborators: true) + project.build_or_assign_import_data(data: { prioritize_collaborators: false }) end it 'advances to the correct next stage' do @@ -123,7 +123,7 @@ expect(Gitlab::GithubImport::AdvanceStageWorker) .to receive(:perform_async) - .with(project.id, { '123' => 2 }, 'pull_requests_merged_by') + .with(project.id, { '123' => 2 }, 'collaborators') expect(MergeRequest).to receive(:track_target_project_iid!) -- GitLab From 4d54ff4c45048a3992aac247b83403f0ad415ff4 Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Fri, 2 Feb 2024 04:21:47 +0100 Subject: [PATCH 08/10] Update test name --- .../github_import/stage/import_collaborators_worker_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb index 7f1ef5c726f558..152b70a9567e44 100644 --- a/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb +++ b/spec/workers/gitlab/github_import/stage/import_collaborators_worker_spec.rb @@ -72,7 +72,7 @@ end end - context 'when github_importer_collaborators_import feature flag is disabled' do + context 'when prioritize_collaborators import setting is disabled' do it 'imports all collaborators and advances to the correct next stage' do allow_next_instance_of(Gitlab::GithubImport::Settings) do |setting| allow(setting).to receive(:prioritize_collaborators?).and_return(false) -- GitLab From 46d811de69869a3050fc4df2a1daaa11b6a99160 Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Fri, 2 Feb 2024 13:20:14 +0100 Subject: [PATCH 09/10] Update ff yml with new rules --- .../development/github_import_prioritize_collaborators.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/config/feature_flags/development/github_import_prioritize_collaborators.yml b/config/feature_flags/development/github_import_prioritize_collaborators.yml index f323a83a40779c..8178219fe40ffa 100644 --- a/config/feature_flags/development/github_import_prioritize_collaborators.yml +++ b/config/feature_flags/development/github_import_prioritize_collaborators.yml @@ -1,8 +1,9 @@ --- name: github_import_prioritize_collaborators +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/436320 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139410 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439742 milestone: '16.9' -type: development +type: gitlab_com_derisk group: group::import and integrate default_enabled: false -- GitLab From 48713d6a3db3e034eae9c2a210d70e6b2a3f2eb9 Mon Sep 17 00:00:00 2001 From: carlad-gl Date: Fri, 2 Feb 2024 14:01:45 +0100 Subject: [PATCH 10/10] Revert ff definition --- .../development/github_import_prioritize_collaborators.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/config/feature_flags/development/github_import_prioritize_collaborators.yml b/config/feature_flags/development/github_import_prioritize_collaborators.yml index 8178219fe40ffa..f323a83a40779c 100644 --- a/config/feature_flags/development/github_import_prioritize_collaborators.yml +++ b/config/feature_flags/development/github_import_prioritize_collaborators.yml @@ -1,9 +1,8 @@ --- name: github_import_prioritize_collaborators -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/436320 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139410 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439742 milestone: '16.9' -type: gitlab_com_derisk +type: development group: group::import and integrate default_enabled: false -- GitLab