diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-06-20 06:06:58 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-06-20 06:06:58 +0300 |
commit | accf0d7db3d58a62212125703df39e341d327ec6 (patch) | |
tree | 7ea5e828e0350f9ecd3fd9c8d6daee02849ab2ef | |
parent | d7511e6d2f947dbae4b19947b746fdabb0897d92 (diff) |
Add latest changes from gitlab-org/gitlab@master
13 files changed, 395 insertions, 31 deletions
diff --git a/Gemfile.checksum b/Gemfile.checksum index 6ae52adfb06..84f65c2f348 100644 --- a/Gemfile.checksum +++ b/Gemfile.checksum @@ -569,7 +569,7 @@ {"name":"sentry-raven","version":"3.1.2","platform":"ruby","checksum":"103d3b122958810d34898ce2e705bcf549ddb9d855a70ce9a3970ee2484f364a"}, {"name":"sentry-ruby","version":"5.8.0","platform":"ruby","checksum":"caeb121433be379fb94e991a45265a287b13a9a9083e7264f539752369d37110"}, {"name":"sentry-sidekiq","version":"5.8.0","platform":"ruby","checksum":"90d1123d16a9fc5fd99dbad190b766dd189eaf9e2baddad641f1334e1877c779"}, -{"name":"set","version":"1.0.1","platform":"ruby","checksum":"d169fe8df4738e9da1118199429a9cf1ce0ac5e8a3cacc481e2ed24d585419dd"}, +{"name":"set","version":"1.0.2","platform":"ruby","checksum":"02ffa4de1f2621495e05b72326040dd014d7abbcb02fea698bc600a389992c02"}, {"name":"sexp_processor","version":"4.16.1","platform":"ruby","checksum":"5caadbf4bbe5ab539cb892a5bcf74ca33a2f2a897cecafdee4a63be79b4819dc"}, {"name":"shellany","version":"0.0.1","platform":"ruby","checksum":"0e127a9132698766d7e752e82cdac8250b6adbd09e6c0a7fbbb6f61964fedee7"}, {"name":"shoulda-matchers","version":"5.1.0","platform":"ruby","checksum":"a01d20589989e9653ab4a28c67d9db2b82bcf0a2496cf01d5e1a95a4aaaf5b07"}, diff --git a/Gemfile.lock b/Gemfile.lock index 06a3d1e70e3..ccda09fd2c7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1425,7 +1425,7 @@ GEM sentry-sidekiq (5.8.0) sentry-ruby (~> 5.8.0) sidekiq (>= 3.0) - set (1.0.1) + set (1.0.2) sexp_processor (4.16.1) shellany (0.0.1) shoulda-matchers (5.1.0) diff --git a/app/services/merge_requests/mergeability_check_batch_service.rb b/app/services/merge_requests/mergeability_check_batch_service.rb new file mode 100644 index 00000000000..7697b596a83 --- /dev/null +++ b/app/services/merge_requests/mergeability_check_batch_service.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module MergeRequests + class MergeabilityCheckBatchService + def initialize(merge_requests, user) + @merge_requests = merge_requests + @user = user + end + + def execute + return unless merge_requests.present? + + MergeRequests::MergeabilityCheckBatchWorker.perform_async(merge_requests.map(&:id), user&.id) + end + + private + + attr_reader :merge_requests, :user + end +end diff --git a/app/workers/merge_requests/mergeability_check_batch_worker.rb b/app/workers/merge_requests/mergeability_check_batch_worker.rb index cbe34ac3790..f48e9c234ab 100644 --- a/app/workers/merge_requests/mergeability_check_batch_worker.rb +++ b/app/workers/merge_requests/mergeability_check_batch_worker.rb @@ -15,10 +15,16 @@ module MergeRequests @logger ||= Sidekiq.logger end - def perform(merge_request_ids) + def perform(merge_request_ids, user_id) merge_requests = MergeRequest.id_in(merge_request_ids) + user = User.find_by_id(user_id) merge_requests.each do |merge_request| + # Skip projects that user doesn't have update_merge_request access + next if merge_status_recheck_not_allowed?(merge_request, user) + + merge_request.mark_as_checking + result = merge_request.check_mergeability next unless result&.error? @@ -30,5 +36,12 @@ module MergeRequests ) end end + + private + + def merge_status_recheck_not_allowed?(merge_request, user) + ::Feature.enabled?(:restrict_merge_status_recheck, merge_request.project) && + !Ability.allowed?(user, :update_merge_request, merge_request.project) + end end end diff --git a/config/feature_flags/development/batched_api_mergeability_checks.yml b/config/feature_flags/development/batched_api_mergeability_checks.yml new file mode 100644 index 00000000000..2a5e85f7566 --- /dev/null +++ b/config/feature_flags/development/batched_api_mergeability_checks.yml @@ -0,0 +1,8 @@ +--- +name: batched_api_mergeability_checks +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121980 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/413232 +milestone: '16.1' +type: development +group: group::code review +default_enabled: false diff --git a/doc/administration/audit_event_streaming/graphql_api.md b/doc/administration/audit_event_streaming/graphql_api.md index f5a31f073dc..c1e24fae901 100644 --- a/doc/administration/audit_event_streaming/graphql_api.md +++ b/doc/administration/audit_event_streaming/graphql_api.md @@ -144,6 +144,39 @@ mutation { The header is created if the returned `errors` object is empty. +### Google Cloud Logging streaming + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/409422) in GitLab 16.1. + +Prerequisites: + +- Owner role for a top-level group. +- A Google Cloud project with the necessary permissions to create service accounts and enable Google Cloud Logging. + +To enable streaming and add a configuration, use the +`googleCloudLoggingConfigurationCreate` mutation in the GraphQL API. + +```graphql +mutation { + googleCloudLoggingConfigurationCreate(input: { groupPath: "my-group", googleProjectIdName: "my-google-project", clientEmail: "my-email@my-google-project.iam.gservice.account.com", privateKey: "YOUR_PRIVATE_KEY", logIdName: "audit-events" } ) { + errors + googleCloudLoggingConfiguration { + id + googleProjectIdName + logIdName + privateKey + clientEmail + } + errors + } +} +``` + +Event streaming is enabled if: + +- The returned `errors` object is empty. +- The API responds with `200 OK`. + ## List streaming destinations List new streaming destinations for top-level groups or an entire instance. @@ -220,6 +253,37 @@ If the resulting list is empty, then audit streaming is not enabled for the inst You need the ID values returned by this query for the update and delete mutations. +### Google Cloud Logging configurations + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/409422) in GitLab 16.1. + +Prerequisite: + +- Owner role for a top-level group. + +You can view a list of streaming configurations for a top-level group using the `googleCloudLoggingConfigurations` query +type. + +```graphql +query { + group(fullPath: "my-group") { + id + googleCloudLoggingConfigurations { + nodes { + id + logIdName + googleProjectIdName + privateKey + } + } + } +} +``` + +If the resulting list is empty, then audit streaming is not enabled for the group. + +You need the ID values returned by this query for the update and delete mutations. + ## Update streaming destinations Update streaming destinations for a top-level group or an entire instance. @@ -313,6 +377,39 @@ mutation { The header is updated if the returned `errors` object is empty. +### Google Cloud Logging configurations + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/409422) in GitLab 16.1. + +Prerequisite: + +- Owner role for a top-level group. + +To update streaming configuration for a top-level group, use the +`googleCloudLoggingConfigurationUpdate` mutation type. You can retrieve the configuration ID +by [listing all the external destinations](#list-streaming-destinations). + +```graphql +mutation { + googleCloudLoggingConfigurationUpdate( + input: {id: "gid://gitlab/AuditEvents::GoogleCloudLoggingConfiguration/1", groupPath: "my-group", googleProjectIdName: "my-google-project", clientEmail: "my-email@my-google-project.iam.gservice.account.com", privateKey: "YOUR_PRIVATE_KEY", logIdName: "audit-events"} + ) { + errors + googleCloudLoggingConfiguration { + id + logIdName + privateKey + googleProjectIdName + } + } +} +``` + +Streaming configuration is updated if: + +- The returned `errors` object is empty. +- The API responds with `200 OK`. + ## Delete streaming destinations Delete streaming destinations for a top-level group or an entire instance. @@ -384,6 +481,31 @@ Streaming destination is deleted if: - The returned `errors` object is empty. - The API responds with `200 OK`. +### Google Cloud Logging configurations + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/409422) in GitLab 16.1. + +Prerequisite: + +- Owner role for a top-level group. + +Users with the Owner role for a group can delete streaming configurations using the +`googleCloudLoggingConfigurationDestroy` mutation type. You can retrieve the configurations ID +by [listing all the streaming destinations](#list-streaming-destinations) for the group. + +```graphql +mutation { + googleCloudLoggingConfigurationDestroy(input: { id: "gid://gitlab/AuditEvents::GoogleCloudLoggingConfiguration/1" }) { + errors + } +} +``` + +Streaming configuration is deleted if: + +- The returned `errors` object is empty. +- The API responds with `200 OK`. + ## Event type filters > Event type filters API [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/344845) in GitLab 15.7. diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 4412eb56c82..d9f6f10ea8a 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -853,6 +853,8 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="queryworkspacesids"></a>`ids` | [`[RemoteDevelopmentWorkspaceID!]`](#remotedevelopmentworkspaceid) | Array of global workspace IDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`. | +| <a id="queryworkspacesincludeactualstates"></a>`includeActualStates` | [`[String!]`](#string) | Includes all workspaces that match any of the actual states. | +| <a id="queryworkspacesprojectids"></a>`projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project id. | ## `Mutation` type @@ -17932,6 +17934,8 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="mergerequestassigneeworkspacesids"></a>`ids` | [`[RemoteDevelopmentWorkspaceID!]`](#remotedevelopmentworkspaceid) | Array of global workspace IDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`. | +| <a id="mergerequestassigneeworkspacesincludeactualstates"></a>`includeActualStates` | [`[String!]`](#string) | Includes all workspaces that match any of the actual states. | +| <a id="mergerequestassigneeworkspacesprojectids"></a>`projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project id. | ### `MergeRequestAuthor` @@ -18205,6 +18209,8 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="mergerequestauthorworkspacesids"></a>`ids` | [`[RemoteDevelopmentWorkspaceID!]`](#remotedevelopmentworkspaceid) | Array of global workspace IDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`. | +| <a id="mergerequestauthorworkspacesincludeactualstates"></a>`includeActualStates` | [`[String!]`](#string) | Includes all workspaces that match any of the actual states. | +| <a id="mergerequestauthorworkspacesprojectids"></a>`projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project id. | ### `MergeRequestDiffLlmSummary` @@ -18512,6 +18518,8 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="mergerequestparticipantworkspacesids"></a>`ids` | [`[RemoteDevelopmentWorkspaceID!]`](#remotedevelopmentworkspaceid) | Array of global workspace IDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`. | +| <a id="mergerequestparticipantworkspacesincludeactualstates"></a>`includeActualStates` | [`[String!]`](#string) | Includes all workspaces that match any of the actual states. | +| <a id="mergerequestparticipantworkspacesprojectids"></a>`projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project id. | ### `MergeRequestPermissions` @@ -18804,6 +18812,8 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="mergerequestreviewerworkspacesids"></a>`ids` | [`[RemoteDevelopmentWorkspaceID!]`](#remotedevelopmentworkspaceid) | Array of global workspace IDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`. | +| <a id="mergerequestreviewerworkspacesincludeactualstates"></a>`includeActualStates` | [`[String!]`](#string) | Includes all workspaces that match any of the actual states. | +| <a id="mergerequestreviewerworkspacesprojectids"></a>`projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project id. | ### `Metadata` @@ -23319,6 +23329,8 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="usercoreworkspacesids"></a>`ids` | [`[RemoteDevelopmentWorkspaceID!]`](#remotedevelopmentworkspaceid) | Array of global workspace IDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`. | +| <a id="usercoreworkspacesincludeactualstates"></a>`includeActualStates` | [`[String!]`](#string) | Includes all workspaces that match any of the actual states. | +| <a id="usercoreworkspacesprojectids"></a>`projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project id. | ### `UserMergeRequestInteraction` @@ -28339,6 +28351,8 @@ four standard [pagination arguments](#connection-pagination-arguments): | Name | Type | Description | | ---- | ---- | ----------- | | <a id="userworkspacesids"></a>`ids` | [`[RemoteDevelopmentWorkspaceID!]`](#remotedevelopmentworkspaceid) | Array of global workspace IDs. For example, `["gid://gitlab/RemoteDevelopment::Workspace/1"]`. | +| <a id="userworkspacesincludeactualstates"></a>`includeActualStates` | [`[String!]`](#string) | Includes all workspaces that match any of the actual states. | +| <a id="userworkspacesprojectids"></a>`projectIds` | [`[ProjectID!]`](#projectid) | Filter workspaces by project id. | #### `WorkItemWidget` diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index c29a7eee923..ff9d0e2c371 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -124,6 +124,10 @@ module API merge_requests.each { |mr| mr.check_mergeability(async: true) } end + def batch_process_mergeability_checks(merge_requests) + ::MergeRequests::MergeabilityCheckBatchService.new(merge_requests, current_user).execute + end + params :merge_requests_params do use :merge_requests_base_params use :optional_merge_requests_search_params @@ -177,8 +181,16 @@ module API get ":id/merge_requests", feature_category: :code_review_workflow, urgency: :low do validate_search_rate_limit! if declared_params[:search].present? merge_requests = find_merge_requests(group_id: user_group.id, include_subgroups: true) + options = serializer_options_for(merge_requests).merge(group: user_group) + + if !options[:skip_merge_status_recheck] && ::Feature.enabled?(:batched_api_mergeability_checks, user_group) + batch_process_mergeability_checks(merge_requests) + + # NOTE: skipping individual mergeability checks in the presenter + options[:skip_merge_status_recheck] = true + end - present merge_requests, serializer_options_for(merge_requests).merge(group: user_group) + present merge_requests, options end end diff --git a/qa/qa/specs/features/api/3_create/merge_request/push_options_mwps_spec.rb b/qa/qa/specs/features/api/3_create/merge_request/push_options_mwps_spec.rb index 9fed6787ade..0817d2f4870 100644 --- a/qa/qa/specs/features/api/3_create/merge_request/push_options_mwps_spec.rb +++ b/qa/qa/specs/features/api/3_create/merge_request/push_options_mwps_spec.rb @@ -74,7 +74,14 @@ module QA end end - it 'merges when pipeline succeeds', testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347842' do + it( + 'merges when pipeline succeeds', + testcase: 'https://gitlab.com/gitlab-org/gitlab/-/quality/test_cases/347842', + quarantine: { + type: :flaky, + issue: "https://gitlab.com/gitlab-org/gitlab/-/issues/346425" + } + ) do Resource::Repository::Commit.fabricate_via_api! do |commit| commit.project = project commit.commit_message = 'Add .gitlab-ci.yml' diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 50e70a9dc0f..0ca621fd9ea 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -60,13 +60,14 @@ RSpec.describe API::MergeRequests, :aggregate_failures, feature_category: :sourc end context 'with merge status recheck projection' do - it 'does not enqueue a merge status recheck' do + it 'does not check mergeability', :sidekiq_inline do expect(check_service_class).not_to receive(:new) - get(api(endpoint_path), params: { with_merge_status_recheck: true }) + get(api(endpoint_path, user2), params: { with_merge_status_recheck: true }) expect_successful_response_with_paginated_array expect(mr_entity['merge_status']).to eq('unchecked') + expect(merge_request.reload.merge_status).to eq('unchecked') end end end @@ -112,16 +113,32 @@ RSpec.describe API::MergeRequests, :aggregate_failures, feature_category: :sourc end context 'with merge status recheck projection' do - it 'checks mergeability asynchronously' do - expect_next_instances_of(check_service_class, (1..2)) do |service| - expect(service).not_to receive(:execute) - expect(service).to receive(:async_execute).and_call_original + context 'with batched_api_mergeability_checks FF on' do + it 'checks mergeability asynchronously in batch', :sidekiq_inline do + get(api(endpoint_path, user2), params: { with_merge_status_recheck: true }) + + expect_successful_response_with_paginated_array + + expect(merge_request.reload.merge_status).to eq('can_be_merged') end + end - get(api(endpoint_path, user2), params: { with_merge_status_recheck: true }) + context 'with batched_api_mergeability_checks FF off' do + before do + stub_feature_flags(batched_api_mergeability_checks: false) + end - expect_successful_response_with_paginated_array - expect(mr_entity['merge_status']).to eq('checking') + it 'checks mergeability asynchronously' do + expect_next_instances_of(check_service_class, (1..2)) do |service| + expect(service).not_to receive(:execute) + expect(service).to receive(:async_execute).and_call_original + end + + get(api(endpoint_path, user2), params: { with_merge_status_recheck: true }) + + expect_successful_response_with_paginated_array + expect(mr_entity['merge_status']).to eq('checking') + end end end @@ -139,13 +156,14 @@ RSpec.describe API::MergeRequests, :aggregate_failures, feature_category: :sourc context 'with a reporter role' do context 'with merge status recheck projection' do - it 'does not enqueue a merge status recheck' do + it 'does not check mergeability', :sidekiq_inline do expect(check_service_class).not_to receive(:new) get(api(endpoint_path, user2), params: { with_merge_status_recheck: true }) expect_successful_response_with_paginated_array expect(mr_entity['merge_status']).to eq('unchecked') + expect(merge_request.reload.merge_status).to eq('unchecked') end end @@ -154,17 +172,33 @@ RSpec.describe API::MergeRequests, :aggregate_failures, feature_category: :sourc stub_feature_flags(restrict_merge_status_recheck: false) end - context 'with merge status recheck projection' do - it 'does enqueue a merge status recheck' do - expect_next_instances_of(check_service_class, (1..2)) do |service| - expect(service).not_to receive(:execute) - expect(service).to receive(:async_execute).and_call_original - end - + context 'with batched_api_mergeability_checks FF on' do + it 'checks mergeability asynchronously in batch', :sidekiq_inline do get(api(endpoint_path, user2), params: { with_merge_status_recheck: true }) expect_successful_response_with_paginated_array - expect(mr_entity['merge_status']).to eq('checking') + + expect(merge_request.reload.merge_status).to eq('can_be_merged') + end + end + + context 'with batched_api_mergeability_checks FF off' do + before do + stub_feature_flags(batched_api_mergeability_checks: false) + end + + context 'with merge status recheck projection' do + it 'does enqueue a merge status recheck' do + expect_next_instances_of(check_service_class, (1..2)) do |service| + expect(service).not_to receive(:execute) + expect(service).to receive(:async_execute).and_call_original + end + + get(api(endpoint_path, user2), params: { with_merge_status_recheck: true }) + + expect_successful_response_with_paginated_array + expect(mr_entity['merge_status']).to eq('checking') + end end end end diff --git a/spec/services/merge_requests/mergeability_check_batch_service_spec.rb b/spec/services/merge_requests/mergeability_check_batch_service_spec.rb new file mode 100644 index 00000000000..099b8039f3e --- /dev/null +++ b/spec/services/merge_requests/mergeability_check_batch_service_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::MergeabilityCheckBatchService, feature_category: :code_review_workflow do + describe '#execute' do + subject { described_class.new(merge_requests, user).execute } + + let(:merge_requests) { [] } + let_it_be(:user) { create(:user) } + + context 'when merge_requests are not empty' do + let_it_be(:merge_request_1) { create(:merge_request) } + let_it_be(:merge_request_2) { create(:merge_request) } + let_it_be(:merge_requests) { [merge_request_1, merge_request_2] } + + it 'triggers batch mergeability checks' do + expect(MergeRequests::MergeabilityCheckBatchWorker).to receive(:perform_async) + .with([merge_request_1.id, merge_request_2.id], user.id) + + subject + end + + context 'when user is nil' do + let(:user) { nil } + + it 'trigger mergeability checks with nil user_id' do + expect(MergeRequests::MergeabilityCheckBatchWorker).to receive(:perform_async) + .with([merge_request_1.id, merge_request_2.id], nil) + + subject + end + end + end + + context 'when merge_requests is empty' do + let(:merge_requests) { MergeRequest.none } + + it 'does not trigger mergeability checks' do + expect(MergeRequests::MergeabilityCheckBatchWorker).not_to receive(:perform_async) + + subject + end + end + end +end diff --git a/spec/support/shared_examples/requests/api/graphql/remote_development_shared_examples.rb b/spec/support/shared_examples/requests/api/graphql/remote_development_shared_examples.rb new file mode 100644 index 00000000000..7c32c7bf2a9 --- /dev/null +++ b/spec/support/shared_examples/requests/api/graphql/remote_development_shared_examples.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'workspaces query in licensed environment and with feature flag on' do + describe 'when licensed and remote_development_feature_flag feature flag is enabled' do + before do + stub_licensed_features(remote_development: true) + + post_graphql(query, current_user: current_user) + end + + it_behaves_like 'a working graphql query' + + # noinspection RubyResolve + it { is_expected.to match_array(a_hash_including('name' => workspace.name)) } + # noinspection RubyResolve + + context 'when user is not authorized' do + let(:current_user) { create(:user) } + + it { is_expected.to eq([]) } + end + end +end + +RSpec.shared_examples 'workspaces query in unlicensed environment and with feature flag off' do + describe 'when remote_development feature is unlicensed' do + before do + stub_licensed_features(remote_development: false) + post_graphql(query, current_user: current_user) + end + + it 'returns an error' do + expect(subject).to be_nil + expect_graphql_errors_to_include(/'remote_development' licensed feature is not available/) + end + end + + describe 'when remote_development_feature_flag feature flag is disabled' do + before do + stub_licensed_features(remote_development: true) + stub_feature_flags(remote_development_feature_flag: false) + post_graphql(query, current_user: current_user) + end + + it 'returns an error' do + expect(subject).to be_nil + expect_graphql_errors_to_include(/'remote_development_feature_flag' feature flag is disabled/) + end + end +end diff --git a/spec/workers/merge_requests/mergeability_check_batch_worker_spec.rb b/spec/workers/merge_requests/mergeability_check_batch_worker_spec.rb index 2c429ed62fb..828ffb0c811 100644 --- a/spec/workers/merge_requests/mergeability_check_batch_worker_spec.rb +++ b/spec/workers/merge_requests/mergeability_check_batch_worker_spec.rb @@ -5,6 +5,8 @@ require 'spec_helper' RSpec.describe MergeRequests::MergeabilityCheckBatchWorker, feature_category: :code_review_workflow do subject { described_class.new } + let_it_be(:user) { create(:user) } + describe '#perform' do context 'when some merge_requests do not exist' do it 'ignores unknown merge request ids' do @@ -12,26 +14,49 @@ RSpec.describe MergeRequests::MergeabilityCheckBatchWorker, feature_category: :c expect(Sidekiq.logger).not_to receive(:error) - subject.perform([1234, 5678]) + subject.perform([1234, 5678], user.id) end end context 'when some merge_requests needs mergeability checks' do let(:merge_request_1) { create(:merge_request, merge_status: :unchecked) } - let(:merge_request_2) { create(:merge_request, merge_status: :cannot_be_merged_rechecking) } + let(:merge_request_2) { create(:merge_request, merge_status: :unchecked) } let(:merge_request_3) { create(:merge_request, merge_status: :can_be_merged) } + before do + merge_request_1.project.add_developer(user) + merge_request_2.project.add_reporter(user) + merge_request_3.project.add_developer(user) + end + it 'executes MergeabilityCheckService on merge requests that needs to be checked' do expect_next_instance_of(MergeRequests::MergeabilityCheckService, merge_request_1) do |service| expect(service).to receive(:execute).and_return(ServiceResponse.success) end - expect_next_instance_of(MergeRequests::MergeabilityCheckService, merge_request_2) do |service| - expect(service).to receive(:execute).and_return(ServiceResponse.success) - end + expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(merge_request_2.id) expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(merge_request_3.id) expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(1234) - subject.perform([merge_request_1.id, merge_request_2.id, merge_request_3.id, 1234]) + subject.perform([merge_request_1.id, merge_request_2.id, merge_request_3.id, 1234], user.id) + end + + context 'when restrict_merge_status_recheck FF is off' do + before do + stub_feature_flags(restrict_merge_status_recheck: false) + end + + it 'executes MergeabilityCheckService on merge requests that needs to be checked' do + expect_next_instance_of(MergeRequests::MergeabilityCheckService, merge_request_1) do |service| + expect(service).to receive(:execute).and_return(ServiceResponse.success) + end + expect_next_instance_of(MergeRequests::MergeabilityCheckService, merge_request_2) do |service| + expect(service).to receive(:execute).and_return(ServiceResponse.success) + end + expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(merge_request_3.id) + expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(1234) + + subject.perform([merge_request_1.id, merge_request_2.id, merge_request_3.id, 1234], user.id) + end end it 'structurally logs a failed mergeability check' do @@ -45,13 +70,26 @@ RSpec.describe MergeRequests::MergeabilityCheckBatchWorker, feature_category: :c worker: described_class.to_s, message: 'Failed to check mergeability of merge request: solar flares') - subject.perform([merge_request_1.id]) + subject.perform([merge_request_1.id], user.id) + end + + context 'when user is nil' do + let(:user) { nil } + + it 'does not run any mergeability checks' do + expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(merge_request_1.id) + expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(merge_request_2.id) + expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(merge_request_3.id) + expect(MergeRequests::MergeabilityCheckService).not_to receive(:new).with(1234) + + subject.perform([merge_request_1.id, merge_request_2.id, merge_request_3.id, 1234], user&.id) + end end end it_behaves_like 'an idempotent worker' do let(:merge_request) { create(:merge_request) } - let(:job_args) { [merge_request.id] } + let(:job_args) { [[merge_request.id], user.id] } it 'is mergeable' do subject |