Commit f93e436d authored by Dylan Griffith's avatar Dylan Griffith

Merge branch '338659-refactor-cross-joins-from-runner-projects-association' into 'master'

Remove runner <-> projects cross-joins where possible

See merge request gitlab-org/gitlab!71811
parents 3155685a a7dab8b6
...@@ -28,7 +28,7 @@ module Mutations ...@@ -28,7 +28,7 @@ module Mutations
def authenticate_delete_runner!(runner) def authenticate_delete_runner!(runner)
return if current_user.can_admin_all_resources? return if current_user.can_admin_all_resources?
"Runner #{runner.to_global_id} associated with more than one project" if runner.projects.count > 1 "Runner #{runner.to_global_id} associated with more than one project" if runner.runner_projects.count > 1
end end
def find_object(id) def find_object(id)
......
...@@ -246,7 +246,7 @@ module Ci ...@@ -246,7 +246,7 @@ module Ci
begin begin
transaction do transaction do
self.projects << project self.runner_projects << ::Ci::RunnerProject.new(project: project, runner: self)
self.save! self.save!
end end
rescue ActiveRecord::RecordInvalid => e rescue ActiveRecord::RecordInvalid => e
...@@ -280,9 +280,7 @@ module Ci ...@@ -280,9 +280,7 @@ module Ci
end end
def belongs_to_more_than_one_project? def belongs_to_more_than_one_project?
::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659') do runner_projects.limit(2).count(:all) > 1
self.projects.limit(2).count(:all) > 1
end
end end
def assigned_to_group? def assigned_to_group?
...@@ -432,10 +430,8 @@ module Ci ...@@ -432,10 +430,8 @@ module Ci
end end
def no_projects def no_projects
::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659') do if runner_projects.any?
if projects.any? errors.add(:runner, 'cannot have projects assigned')
errors.add(:runner, 'cannot have projects assigned')
end
end end
end end
...@@ -448,10 +444,8 @@ module Ci ...@@ -448,10 +444,8 @@ module Ci
end end
def any_project def any_project
::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659') do unless runner_projects.any?
unless projects.any? errors.add(:runner, 'needs to be assigned to at least one project')
errors.add(:runner, 'needs to be assigned to at least one project')
end
end end
end end
......
...@@ -72,7 +72,7 @@ module Clusters ...@@ -72,7 +72,7 @@ module Clusters
if cluster.group_type? if cluster.group_type?
attributes[:groups] = [group] attributes[:groups] = [group]
elsif cluster.project_type? elsif cluster.project_type?
attributes[:projects] = [project] attributes[:runner_projects] = [::Ci::RunnerProject.new(project: project)]
end end
attributes attributes
......
...@@ -9,7 +9,7 @@ ...@@ -9,7 +9,7 @@
.row .row
.col-md-6 .col-md-6
%h4= _('Restrict projects for this runner') %h4= _('Restrict projects for this runner')
- if @runner.projects.any? - if @runner.runner_projects.any?
%table.table{ data: { testid: 'assigned-projects' } } %table.table{ data: { testid: 'assigned-projects' } }
%thead %thead
%tr %tr
......
...@@ -39,7 +39,7 @@ ...@@ -39,7 +39,7 @@
- if runner.group_type? - if runner.group_type?
= _('n/a') = _('n/a')
- else - else
= runner.projects.count(:all) = runner.runner_projects.count(:all)
.table-section.section-5 .table-section.section-5
.table-mobile-header{ role: 'rowheader' }= _('Jobs') .table-mobile-header{ role: 'rowheader' }= _('Jobs')
......
...@@ -204,7 +204,7 @@ module API ...@@ -204,7 +204,7 @@ module API
not_found!('Runner') unless runner_project not_found!('Runner') unless runner_project
runner = runner_project.runner runner = runner_project.runner
forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.projects.count == 1 forbidden!("Only one project associated with the runner. Please remove the runner instead") if runner.runner_projects.count == 1
destroy_conditionally!(runner_project) destroy_conditionally!(runner_project)
end end
...@@ -331,7 +331,7 @@ module API ...@@ -331,7 +331,7 @@ module API
def authenticate_delete_runner!(runner) def authenticate_delete_runner!(runner)
return if current_user.admin? return if current_user.admin?
forbidden!("Runner associated with more than one project") if runner.projects.count > 1 forbidden!("Runner associated with more than one project") if runner.runner_projects.count > 1
forbidden!("No access granted") unless can?(current_user, :delete_runner, runner) forbidden!("No access granted") unless can?(current_user, :delete_runner, runner)
end end
......
...@@ -2,7 +2,14 @@ ...@@ -2,7 +2,14 @@
FactoryBot.define do FactoryBot.define do
factory :ci_runner_project, class: 'Ci::RunnerProject' do factory :ci_runner_project, class: 'Ci::RunnerProject' do
runner factory: [:ci_runner, :project]
project project
after(:build) do |runner_project, evaluator|
unless runner_project.runner.present?
runner_project.runner = build(
:ci_runner, :project, runner_projects: [runner_project]
)
end
end
end end
end end
...@@ -10,6 +10,16 @@ FactoryBot.define do ...@@ -10,6 +10,16 @@ FactoryBot.define do
runner_type { :instance_type } runner_type { :instance_type }
transient do
projects { [] }
end
after(:build) do |runner, evaluator|
evaluator.projects.each do |proj|
runner.runner_projects << build(:ci_runner_project, project: proj)
end
end
trait :online do trait :online do
contacted_at { Time.now } contacted_at { Time.now }
end end
...@@ -30,7 +40,9 @@ FactoryBot.define do ...@@ -30,7 +40,9 @@ FactoryBot.define do
runner_type { :project_type } runner_type { :project_type }
after(:build) do |runner, evaluator| after(:build) do |runner, evaluator|
runner.projects << build(:project) if runner.projects.empty? if runner.runner_projects.empty?
runner.runner_projects << build(:ci_runner_project)
end
end end
end end
......
...@@ -271,7 +271,7 @@ RSpec.describe Ci::Runner do ...@@ -271,7 +271,7 @@ RSpec.describe Ci::Runner do
expect(subject).to be_truthy expect(subject).to be_truthy
expect(runner).to be_project_type expect(runner).to be_project_type
expect(runner.projects).to eq([project]) expect(runner.runner_projects.pluck(:project_id)).to match_array([project.id])
expect(runner.only_for?(project)).to be_truthy expect(runner.only_for?(project)).to be_truthy
end end
end end
...@@ -735,7 +735,7 @@ RSpec.describe Ci::Runner do ...@@ -735,7 +735,7 @@ RSpec.describe Ci::Runner do
context 'with invalid runner' do context 'with invalid runner' do
before do before do
runner.projects = [] runner.runner_projects.delete_all
end end
it 'still updates redis cache and database' do it 'still updates redis cache and database' do
......
...@@ -96,8 +96,9 @@ RSpec.describe Clusters::Applications::Runner do ...@@ -96,8 +96,9 @@ RSpec.describe Clusters::Applications::Runner do
it 'creates a project runner' do it 'creates a project runner' do
subject subject
runner_projects = Project.where(id: runner.runner_projects.pluck(:project_id))
expect(runner).to be_project_type expect(runner).to be_project_type
expect(runner.projects).to eq [project] expect(runner_projects).to match_array [project]
end end
end end
......
...@@ -250,7 +250,7 @@ RSpec.describe 'Query.runner(id)' do ...@@ -250,7 +250,7 @@ RSpec.describe 'Query.runner(id)' do
end end
before do before do
project_runner2.projects.clear project_runner2.runner_projects.clear
post_graphql(query, current_user: user) post_graphql(query, current_user: user)
end end
......
...@@ -41,8 +41,8 @@ ...@@ -41,8 +41,8 @@
- "./ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb" - "./ee/spec/services/ee/merge_requests/create_pipeline_service_spec.rb"
- "./ee/spec/services/ee/merge_requests/refresh_service_spec.rb" - "./ee/spec/services/ee/merge_requests/refresh_service_spec.rb"
- "./ee/spec/workers/scan_security_report_secrets_worker_spec.rb" - "./ee/spec/workers/scan_security_report_secrets_worker_spec.rb"
- "./spec/controllers/admin/runners_controller_spec.rb"
- "./spec/controllers/groups/settings/ci_cd_controller_spec.rb" - "./spec/controllers/groups/settings/ci_cd_controller_spec.rb"
- "./spec/controllers/admin/runners_controller_spec.rb"
- "./spec/controllers/projects/merge_requests_controller_spec.rb" - "./spec/controllers/projects/merge_requests_controller_spec.rb"
- "./spec/controllers/projects/settings/ci_cd_controller_spec.rb" - "./spec/controllers/projects/settings/ci_cd_controller_spec.rb"
- "./spec/features/admin/admin_runners_spec.rb" - "./spec/features/admin/admin_runners_spec.rb"
...@@ -56,7 +56,6 @@ ...@@ -56,7 +56,6 @@
- "./spec/finders/ci/pipelines_for_merge_request_finder_spec.rb" - "./spec/finders/ci/pipelines_for_merge_request_finder_spec.rb"
- "./spec/finders/ci/runners_finder_spec.rb" - "./spec/finders/ci/runners_finder_spec.rb"
- "./spec/frontend/fixtures/runner.rb" - "./spec/frontend/fixtures/runner.rb"
- "./spec/graphql/mutations/ci/runner/delete_spec.rb"
- "./spec/graphql/resolvers/ci/group_runners_resolver_spec.rb" - "./spec/graphql/resolvers/ci/group_runners_resolver_spec.rb"
- "./spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb" - "./spec/graphql/resolvers/merge_request_pipelines_resolver_spec.rb"
- "./spec/lib/api/entities/package_spec.rb" - "./spec/lib/api/entities/package_spec.rb"
...@@ -71,13 +70,11 @@ ...@@ -71,13 +70,11 @@
- "./spec/models/ci/job_artifact_spec.rb" - "./spec/models/ci/job_artifact_spec.rb"
- "./spec/models/ci/pipeline_spec.rb" - "./spec/models/ci/pipeline_spec.rb"
- "./spec/models/ci/runner_spec.rb" - "./spec/models/ci/runner_spec.rb"
- "./spec/models/clusters/applications/runner_spec.rb"
- "./spec/models/deployment_spec.rb" - "./spec/models/deployment_spec.rb"
- "./spec/models/environment_spec.rb" - "./spec/models/environment_spec.rb"
- "./spec/models/merge_request_spec.rb" - "./spec/models/merge_request_spec.rb"
- "./spec/models/project_spec.rb" - "./spec/models/project_spec.rb"
- "./spec/models/user_spec.rb" - "./spec/models/user_spec.rb"
- "./spec/presenters/ci/build_runner_presenter_spec.rb"
- "./spec/presenters/ci/pipeline_presenter_spec.rb" - "./spec/presenters/ci/pipeline_presenter_spec.rb"
- "./spec/presenters/packages/detail/package_presenter_spec.rb" - "./spec/presenters/packages/detail/package_presenter_spec.rb"
- "./spec/requests/api/ci/runner/runners_post_spec.rb" - "./spec/requests/api/ci/runner/runners_post_spec.rb"
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment