diff --git a/.rubocop_todo/layout/first_array_element_indentation.yml b/.rubocop_todo/layout/first_array_element_indentation.yml index fd795c5219c..5207d493044 100644 --- a/.rubocop_todo/layout/first_array_element_indentation.yml +++ b/.rubocop_todo/layout/first_array_element_indentation.yml @@ -2,22 +2,6 @@ # Cop supports --auto-correct. Layout/FirstArrayElementIndentation: Exclude: - - 'ee/lib/ee/api/helpers/award_emoji.rb' - - 'ee/spec/graphql/mutations/incident_management/escalation_policy/create_spec.rb' - - 'ee/spec/lib/gitlab/graphql/loaders/bulk_epic_aggregate_loader_spec.rb' - - 'ee/spec/models/snippet_repository_spec.rb' - - 'ee/spec/services/protected_environments/base_service_spec.rb' - - 'ee/spec/services/search_service_spec.rb' - - 'ee/spec/services/security/ingestion/tasks/hooks_execution_spec.rb' - - 'ee/spec/services/security/security_orchestration_policies/process_scan_result_policy_service_spec.rb' - - 'ee/spec/services/security/store_findings_metadata_service_spec.rb' - - 'ee/spec/services/timebox_report_service_spec.rb' - - 'ee/spec/services/user_permissions/export_service_spec.rb' - - 'ee/spec/support/shared_examples/services/search_notes_shared_examples.rb' - - 'ee/spec/workers/geo/scheduler/scheduler_worker_spec.rb' - - 'lib/event_filter.rb' - - 'lib/gitlab/database/migration_helpers.rb' - - 'lib/gitlab/email/message/in_product_marketing/team.rb' - 'lib/gitlab/email/message/in_product_marketing/trial.rb' - 'lib/gitlab/email/message/in_product_marketing/verify.rb' - 'lib/gitlab/import_export/base/relation_factory.rb' diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 392a6981cb7..0b94efba568 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -c3718f5f9a3285ad8d99b4e3383edc13b1546fda +b4c1f29c487a41b2e69a31a99f6b0ac462c81ce4 diff --git a/app/graphql/mutations/ci/runner/bulk_delete.rb b/app/graphql/mutations/ci/runner/bulk_delete.rb index 4c1c2967799..4265099d28e 100644 --- a/app/graphql/mutations/ci/runner/bulk_delete.rb +++ b/app/graphql/mutations/ci/runner/bulk_delete.rb @@ -40,9 +40,7 @@ module Mutations private def model_ids_of(ids) - ids.map do |gid| - gid.model_id.to_i - end.compact + ids.filter_map { |gid| gid.model_id.to_i } end def find_all_runners_by_ids(ids) diff --git a/app/graphql/mutations/ci/runner/update.rb b/app/graphql/mutations/ci/runner/update.rb index 203cd44bc84..f98138646be 100644 --- a/app/graphql/mutations/ci/runner/update.rb +++ b/app/graphql/mutations/ci/runner/update.rb @@ -48,8 +48,13 @@ module Mutations description: 'Indicates the runner is able to run untagged jobs.' argument :tag_list, [GraphQL::Types::String], - required: false, - description: 'Tags associated with the runner.' + required: false, + description: 'Tags associated with the runner.' + + argument :associated_projects, [::Types::GlobalIDType[::Project]], + required: false, + description: 'Projects associated with the runner. Available only for project runners.', + prepare: -> (global_ids, ctx) { global_ids&.filter_map { |gid| gid.model_id.to_i } } field :runner, Types::Ci::RunnerType, @@ -59,15 +64,47 @@ module Mutations def resolve(id:, **runner_attrs) runner = authorized_find!(id) - result = ::Ci::Runners::UpdateRunnerService.new(runner).execute(runner_attrs) - return { runner: nil, errors: result.errors } if result.error? + associated_projects_ids = runner_attrs.delete(:associated_projects) - { runner: runner, errors: [] } + response = { runner: runner, errors: [] } + ::Ci::Runner.transaction do + associate_runner_projects(response, runner, associated_projects_ids) if associated_projects_ids.present? + update_runner(response, runner, runner_attrs) + end + + response end def find_object(id) GitlabSchema.find_by_gid(id) end + + private + + def associate_runner_projects(response, runner, associated_project_ids) + unless runner.project_type? + raise Gitlab::Graphql::Errors::ArgumentError, + "associatedProjects must not be specified for '#{runner.runner_type}' scope" + end + + result = ::Ci::Runners::SetRunnerAssociatedProjectsService.new( + runner: runner, + current_user: current_user, + project_ids: associated_project_ids + ).execute + return if result.success? + + response[:errors] = result.errors + raise ActiveRecord::Rollback + end + + def update_runner(response, runner, attrs) + result = ::Ci::Runners::UpdateRunnerService.new(runner).execute(attrs) + return if result.success? + + response[:errors] = result.errors + raise ActiveRecord::Rollback + end end end end diff --git a/app/graphql/mutations/todos/restore_many.rb b/app/graphql/mutations/todos/restore_many.rb index fe0ad6df65b..20913a9e7da 100644 --- a/app/graphql/mutations/todos/restore_many.rb +++ b/app/graphql/mutations/todos/restore_many.rb @@ -32,9 +32,7 @@ module Mutations private def model_ids_of(ids) - ids.map do |gid| - gid.model_id.to_i - end.compact + ids.filter_map { |gid| gid.model_id.to_i } end def raise_too_many_todos_requested_error diff --git a/app/services/ci/runners/set_runner_associated_projects_service.rb b/app/services/ci/runners/set_runner_associated_projects_service.rb index 759bd2ffb16..7930776749d 100644 --- a/app/services/ci/runners/set_runner_associated_projects_service.rb +++ b/app/services/ci/runners/set_runner_associated_projects_service.rb @@ -17,6 +17,8 @@ module Ci return ServiceResponse.error(message: 'user not allowed to assign runner', http_status: :forbidden) end + return ServiceResponse.success if project_ids.blank? + set_associated_projects end @@ -47,16 +49,15 @@ module Ci def associate_new_projects(new_project_ids, current_project_ids) missing_projects = Project.id_in(new_project_ids - current_project_ids) - missing_projects.each do |project| - return false unless runner.assign_to(project, current_user) - end - - true + missing_projects.all? { |project| runner.assign_to(project, current_user) } end def disassociate_old_projects(new_project_ids, current_project_ids) + projects_to_be_deleted = current_project_ids - new_project_ids + return true if projects_to_be_deleted.empty? + Ci::RunnerProject - .destroy_by(project_id: current_project_ids - new_project_ids) + .destroy_by(project_id: projects_to_be_deleted) .all?(&:destroyed?) end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 8fc09fd840c..c5b57c47c79 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -4495,6 +4495,7 @@ Input type: `RunnerUpdateInput` | ---- | ---- | ----------- | | `accessLevel` | [`CiRunnerAccessLevel`](#cirunneraccesslevel) | Access level of the runner. | | `active` **{warning-solid}** | [`Boolean`](#boolean) | **Deprecated:** This was renamed. Please use `paused`. Deprecated in 14.8. | +| `associatedProjects` | [`[ProjectID!]`](#projectid) | Projects associated with the runner. Available only for project runners. | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `description` | [`String`](#string) | Description of the runner. | | `id` | [`CiRunnerID!`](#cirunnerid) | ID of the runner to update. | diff --git a/lib/event_filter.rb b/lib/event_filter.rb index 8c3377fdb80..f14b0a6b9e7 100644 --- a/lib/event_filter.rb +++ b/lib/event_filter.rb @@ -131,18 +131,19 @@ class EventFilter finder_query = -> (id_expression) { Event.where(Event.arel_table[:id].eq(id_expression)) } if order_hint_column.present? - order = Gitlab::Pagination::Keyset::Order.build([ - Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( - attribute_name: order_hint_column, - order_expression: Event.arel_table[order_hint_column].desc, - nullable: :nulls_last, - distinct: false - ), - Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( - attribute_name: :id, - order_expression: Event.arel_table[:id].desc - ) - ]) + order = Gitlab::Pagination::Keyset::Order.build( + [ + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: order_hint_column, + order_expression: Event.arel_table[order_hint_column].desc, + nullable: :nulls_last, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: :id, + order_expression: Event.arel_table[:id].desc + ) + ]) finder_query = -> (_order_hint, id_expression) { Event.where(Event.arel_table[:id].eq(id_expression)) } end diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index db39524f4f6..e574422ce11 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -936,13 +936,14 @@ module Gitlab def revert_backfill_conversion_of_integer_to_bigint(table, columns, primary_key: :id) columns = Array.wrap(columns) - conditions = ActiveRecord::Base.sanitize_sql([ - 'job_class_name = :job_class_name AND table_name = :table_name AND column_name = :column_name AND job_arguments = :job_arguments', - job_class_name: 'CopyColumnUsingBackgroundMigrationJob', - table_name: table, - column_name: primary_key, - job_arguments: [columns, columns.map { |column| convert_to_bigint_column(column) }].to_json - ]) + conditions = ActiveRecord::Base.sanitize_sql( + [ + 'job_class_name = :job_class_name AND table_name = :table_name AND column_name = :column_name AND job_arguments = :job_arguments', + job_class_name: 'CopyColumnUsingBackgroundMigrationJob', + table_name: table, + column_name: primary_key, + job_arguments: [columns, columns.map { |column| convert_to_bigint_column(column) }].to_json + ]) execute("DELETE FROM batched_background_migrations WHERE #{conditions}") end diff --git a/lib/gitlab/email/message/in_product_marketing/team.rb b/lib/gitlab/email/message/in_product_marketing/team.rb index 6a0471ef9c5..ca99dd12c8e 100644 --- a/lib/gitlab/email/message/in_product_marketing/team.rb +++ b/lib/gitlab/email/message/in_product_marketing/team.rb @@ -42,18 +42,18 @@ module Gitlab [ s_('InProductMarketing|Did you know teams that use GitLab are far more efficient?'), list([ - s_('InProductMarketing|Goldman Sachs went from 1 build every two weeks to thousands of builds a day'), - s_('InProductMarketing|Ticketmaster decreased their CI build time by 15X') - ]) + s_('InProductMarketing|Goldman Sachs went from 1 build every two weeks to thousands of builds a day'), + s_('InProductMarketing|Ticketmaster decreased their CI build time by 15X') + ]) ].join("\n"), s_("InProductMarketing|We know a thing or two about efficiency and we don't want to keep that to ourselves. Sign up for a free trial of GitLab Ultimate and your teams will be on it from day one."), [ s_('InProductMarketing|Stop wondering and use GitLab to answer questions like:'), list([ - s_('InProductMarketing|How long does it take us to close issues/MRs by types like feature requests, bugs, tech debt, security?'), - s_('InProductMarketing|How many days does it take our team to complete various tasks?'), - s_('InProductMarketing|What does our value stream timeline look like from product to development to review and production?') - ]) + s_('InProductMarketing|How long does it take us to close issues/MRs by types like feature requests, bugs, tech debt, security?'), + s_('InProductMarketing|How many days does it take our team to complete various tasks?'), + s_('InProductMarketing|What does our value stream timeline look like from product to development to review and production?') + ]) ].join("\n") ][series] end diff --git a/qa/qa/page/component/content_editor.rb b/qa/qa/page/component/content_editor.rb index 1f022665477..f7b055b6052 100644 --- a/qa/qa/page/component/content_editor.rb +++ b/qa/qa/page/component/content_editor.rb @@ -46,8 +46,6 @@ module QA find_element(:file_upload_field, visible: false).send_keys(image_path) end - wait_for_requests - QA::Support::Retrier.retry_on_exception do source = find_element(:wiki_hidden_content, visible: false) source.value =~ %r{uploads/.*#{::File.basename(image_path)}} diff --git a/qa/qa/page/component/wiki_page_form.rb b/qa/qa/page/component/wiki_page_form.rb index 22b9a4c8b0d..9e558844469 100644 --- a/qa/qa/page/component/wiki_page_form.rb +++ b/qa/qa/page/component/wiki_page_form.rb @@ -35,6 +35,10 @@ module QA end def click_submit + # In case any changes were just made, wait for the hidden content field to be updated via a deferred call + # before clicking submit. See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/97693#note_1098728562 + sleep 0.5 + click_element(:wiki_submit_button) QA::Support::Retrier.retry_on_exception do diff --git a/qa/qa/specs/features/browser_ui/3_create/wiki/content_editor_spec.rb b/qa/qa/specs/features/browser_ui/3_create/wiki/content_editor_spec.rb index 2e8cacd8e19..4b8edf8b792 100644 --- a/qa/qa/specs/features/browser_ui/3_create/wiki/content_editor_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/wiki/content_editor_spec.rb @@ -1,10 +1,7 @@ # frozen_string_literal: true module QA - RSpec.describe 'Create', :reliable, quarantine: { - type: :investigating, - issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/373093' - } do + RSpec.describe 'Create', :reliable do context 'Content Editor' do let(:initial_wiki) { Resource::Wiki::ProjectPage.fabricate_via_api! } let(:page_title) { 'Content Editor Page' } diff --git a/spec/graphql/mutations/ci/runner/update_spec.rb b/spec/graphql/mutations/ci/runner/update_spec.rb index b8efd4213fa..39fe2a53a68 100644 --- a/spec/graphql/mutations/ci/runner/update_spec.rb +++ b/spec/graphql/mutations/ci/runner/update_spec.rb @@ -6,10 +6,13 @@ RSpec.describe Mutations::Ci::Runner::Update do include GraphqlHelpers let_it_be(:user) { create(:user) } - let_it_be(:runner) { create(:ci_runner, active: true, locked: false, run_untagged: true) } + let_it_be(:project1) { create(:project) } + let_it_be(:runner) do + create(:ci_runner, :project, projects: [project1], active: true, locked: false, run_untagged: true) + end let(:current_ctx) { { current_user: user } } - let(:mutated_runner) { subject[:runner] } + let(:mutated_runner) { response[:runner] } let(:mutation_params) do { @@ -21,14 +24,14 @@ RSpec.describe Mutations::Ci::Runner::Update do specify { expect(described_class).to require_graphql_authorizations(:update_runner) } describe '#resolve' do - subject do + subject(:response) do sync(resolve(described_class, args: mutation_params, ctx: current_ctx)) end context 'when the user cannot admin the runner' do it 'generates an error' do expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ResourceNotAvailable) do - subject + response end end end @@ -37,7 +40,7 @@ RSpec.describe Mutations::Ci::Runner::Update do let(:mutation_params) { {} } it 'raises an error' do - expect { subject }.to raise_error(ArgumentError, "Arguments must be provided: id") + expect { response }.to raise_error(ArgumentError, "Arguments must be provided: id") end end @@ -45,41 +48,150 @@ RSpec.describe Mutations::Ci::Runner::Update do let(:admin_user) { create(:user, :admin) } let(:current_ctx) { { current_user: admin_user } } - let(:mutation_params) do - { - id: runner.to_global_id, - description: 'updated description', - maintenance_note: 'updated maintenance note', - maximum_timeout: 900, - access_level: 'ref_protected', - active: false, - locked: true, - run_untagged: false, - tag_list: %w(tag1 tag2) - } - end - context 'with valid arguments' do + let(:mutation_params) do + { + id: runner.to_global_id, + description: 'updated description', + maintenance_note: 'updated maintenance note', + maximum_timeout: 900, + access_level: 'ref_protected', + active: false, + locked: true, + run_untagged: false, + tag_list: %w(tag1 tag2) + } + end + it 'updates runner with correct values' do expected_attributes = mutation_params.except(:id, :tag_list) - subject + response - expect(subject[:errors]).to be_empty - expect(subject[:runner]).to be_an_instance_of(Ci::Runner) - expect(subject[:runner]).to have_attributes(expected_attributes) - expect(subject[:runner].tag_list).to contain_exactly(*mutation_params[:tag_list]) + expect(response[:errors]).to be_empty + expect(response[:runner]).to be_an_instance_of(Ci::Runner) + expect(response[:runner]).to have_attributes(expected_attributes) + expect(response[:runner].tag_list).to contain_exactly(*mutation_params[:tag_list]) expect(runner.reload).to have_attributes(expected_attributes) expect(runner.tag_list).to contain_exactly(*mutation_params[:tag_list]) end end - context 'with out-of-range maximum_timeout and missing tag_list' do - it 'returns a descriptive error' do - mutation_params[:maximum_timeout] = 100 - mutation_params.delete(:tag_list) + context 'with associatedProjects argument' do + let_it_be(:project2) { create(:project) } - expect(subject[:errors]).to contain_exactly( + context 'with id set to project runner' do + let(:mutation_params) do + { + id: runner.to_global_id, + description: 'updated description', + associated_projects: [project2.to_global_id.to_s] + } + end + + it 'updates runner attributes and project relationships', :aggregate_failures do + expect_next_instance_of( + ::Ci::Runners::SetRunnerAssociatedProjectsService, + { + runner: runner, + current_user: admin_user, + project_ids: [project2.id] + } + ) do |service| + expect(service).to receive(:execute).and_call_original + end + + expected_attributes = mutation_params.except(:id, :associated_projects) + + response + + expect(response[:errors]).to be_empty + expect(response[:runner]).to be_an_instance_of(Ci::Runner) + expect(response[:runner]).to have_attributes(expected_attributes) + expect(runner.reload).to have_attributes(expected_attributes) + expect(runner.projects).to match_array([project1, project2]) + end + + context 'with user not allowed to assign runner' do + before do + allow(admin_user).to receive(:can?).with(:assign_runner, runner).and_return(false) + end + + it 'does not update runner', :aggregate_failures do + expect_next_instance_of( + ::Ci::Runners::SetRunnerAssociatedProjectsService, + { + runner: runner, + current_user: admin_user, + project_ids: [project2.id] + } + ) do |service| + expect(service).to receive(:execute).and_call_original + end + + expected_attributes = mutation_params.except(:id, :associated_projects) + + response + + expect(response[:errors]).to match_array(['user not allowed to assign runner']) + expect(response[:runner]).to be_an_instance_of(Ci::Runner) + expect(response[:runner]).not_to have_attributes(expected_attributes) + expect(runner.reload).not_to have_attributes(expected_attributes) + expect(runner.projects).to match_array([project1]) + end + end + end + + context 'with id set to instance runner' do + let(:instance_runner) { create(:ci_runner, :instance) } + let(:mutation_params) do + { + id: instance_runner.to_global_id, + description: 'updated description', + associated_projects: [project2.to_global_id.to_s] + } + end + + it 'raises error', :aggregate_failures do + expect_graphql_error_to_be_created(Gitlab::Graphql::Errors::ArgumentError) do + response + end + end + end + end + + context 'with non-existing project ID in associatedProjects argument' do + let(:mutation_params) do + { + id: runner.to_global_id, + associated_projects: ['gid://gitlab/Project/-1'] + } + end + + it 'does not change associated projects' do + expected_attributes = mutation_params.except(:id, :associated_projects) + + response + + expect(response[:errors]).to be_empty + expect(response[:runner]).to be_an_instance_of(Ci::Runner) + expect(response[:runner]).to have_attributes(expected_attributes) + expect(runner.reload).to have_attributes(expected_attributes) + expect(runner.projects).to match_array([project1]) + end + end + + context 'with out-of-range maximum_timeout and missing tag_list' do + let(:mutation_params) do + { + id: runner.to_global_id, + maximum_timeout: 100, + run_untagged: false + } + end + + it 'returns a descriptive error' do + expect(response[:errors]).to contain_exactly( 'Maximum timeout needs to be at least 10 minutes', 'Tags list can not be empty when runner is not allowed to pick untagged jobs' ) @@ -90,7 +202,7 @@ RSpec.describe Mutations::Ci::Runner::Update do it 'returns a descriptive error' do mutation_params[:maintenance_note] = '1' * 1025 - expect(subject[:errors]).to contain_exactly( + expect(response[:errors]).to contain_exactly( 'Maintenance note is too long (maximum is 1024 characters)' ) end diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index b3fee24e09d..0d2e237c87b 100644 --- a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -8,7 +8,8 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do let_it_be(:owner_project) { create(:project) } let_it_be(:project2) { create(:project) } let_it_be(:original_projects) { [owner_project, project2] } - let_it_be(:runner) { create(:ci_runner, :project, projects: original_projects) } + + let(:runner) { create(:ci_runner, :project, projects: original_projects) } context 'without user' do let(:user) { nil } @@ -35,35 +36,52 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do end context 'with admin user', :enable_admin_mode do - let(:user) { create_default(:user, :admin) } - let(:project_ids) { [project3.id, project4.id] } + let_it_be(:user) { create(:user, :admin) } + let(:project3) { create(:project) } let(:project4) { create(:project) } context 'with successful requests' do - it 'calls assign_to on runner and returns success response' do - expect(execute).to be_success - expect(runner.reload.projects.ids).to match_array([owner_project.id] + project_ids) + context 'when disassociating a project' do + let(:project_ids) { [project3.id, project4.id] } + + it 'reassigns associated projects and returns success response' do + expect(execute).to be_success + expect(runner.reload.projects.ids).to eq([owner_project.id] + project_ids) + end + end + + context 'when disassociating no projects' do + let(:project_ids) { [project2.id, project3.id] } + + it 'reassigns associated projects and returns success response' do + expect(execute).to be_success + expect(runner.reload.projects.ids).to eq([owner_project.id] + project_ids) + end end end context 'with failing assign_to requests' do + let(:project_ids) { [project3.id, project4.id] } + it 'returns error response and rolls back transaction' do expect(runner).to receive(:assign_to).with(project4, user).once.and_return(false) expect(execute).to be_error - expect(runner.reload.projects).to match_array(original_projects) + expect(runner.reload.projects).to eq(original_projects) end end context 'with failing destroy calls' do + let(:project_ids) { [project3.id, project4.id] } + it 'returns error response and rolls back transaction' do allow_next_found_instance_of(Ci::RunnerProject) do |runner_project| allow(runner_project).to receive(:destroy).and_return(false) end expect(execute).to be_error - expect(runner.reload.projects).to match_array(original_projects) + expect(runner.reload.projects).to eq(original_projects) end end end