Commit 051f385e authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Dylan Griffith

Refactor validations and make runner factory by default to be instance-wide runner

parent 5c34c3fc
......@@ -56,12 +56,15 @@ module Ci
end
validate :tag_constraints
validate :no_projects, unless: :project_type?
validate :no_groups, unless: :group_type?
validate :only_one_group, if: :group_type?
validates :access_level, presence: true
validates :runner_type, presence: true
validate :no_projects, unless: :project_type?
validate :no_groups, unless: :group_type?
validate :any_project, if: :project_type?
validate :exactly_one_group, if: :group_type?
validate :is_shared_is_valid
acts_as_taggable
after_destroy :cleanup_runner_queue
......@@ -117,8 +120,8 @@ module Ci
raise ArgumentError, 'Transitioning a group runner to a project runner is not supported'
end
self.save
project.runner_projects.create(runner_id: self.id)
self.projects << project
self.save!
end
def display_name
......@@ -257,19 +260,31 @@ module Ci
def no_projects
if projects.any?
errors.add(:runner, 'cannot assign project to a non-project runner')
errors.add(:runner, 'cannot have projects assigned')
end
end
def no_groups
if groups.any?
errors.add(:runner, 'cannot assign group to a non-group runner')
errors.add(:runner, 'cannot have groups assigned')
end
end
def any_project
unless projects.any?
errors.add(:runner, 'needs to be assigned to at least one project')
end
end
def exactly_one_group
unless groups.one?
errors.add(:runner, 'needs to be assigned to exactly one group')
end
end
def only_one_group
if groups.many?
errors.add(:runner, 'can only be assigned to one group')
def is_shared_is_valid
unless is_shared? == instance_type?
errors.add(:is_shared, 'is not equal to instance_type?')
end
end
......
......@@ -3,7 +3,7 @@ require 'spec_helper'
describe Groups::RunnersController do
let(:user) { create(:user) }
let(:group) { create(:group) }
let(:runner) { create(:ci_runner, :group) }
let(:runner) { create(:ci_runner, :group, groups: [group]) }
let(:params) do
{
......@@ -15,7 +15,6 @@ describe Groups::RunnersController do
before do
sign_in(user)
group.add_master(user)
group.runners << runner
end
describe '#update' do
......
......@@ -3,7 +3,7 @@ require 'spec_helper'
describe Projects::RunnersController do
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:runner) { create(:ci_runner) }
let(:runner) { create(:ci_runner, :project, projects: [project]) }
let(:params) do
{
......@@ -16,7 +16,6 @@ describe Projects::RunnersController do
before do
sign_in(user)
project.add_master(user)
project.runners << runner
end
describe '#update' do
......
......@@ -19,12 +19,12 @@ describe Projects::Settings::CiCdController do
end
context 'with group runners' do
let(:group_runner) { create(:ci_runner, runner_type: :group_type) }
let(:parent_group) { create(:group) }
let(:group) { create(:group, runners: [group_runner], parent: parent_group) }
let(:group) { create(:group, parent: parent_group) }
let(:group_runner) { create(:ci_runner, :group, groups: [group]) }
let(:other_project) { create(:project, group: group) }
let!(:project_runner) { create(:ci_runner, projects: [other_project], runner_type: :project_type) }
let!(:shared_runner) { create(:ci_runner, :shared) }
let!(:project_runner) { create(:ci_runner, :project, projects: [other_project]) }
let!(:shared_runner) { create(:ci_runner, :instance) }
it 'sets assignable project runners only' do
group.add_master(user)
......
FactoryBot.define do
factory :ci_runner_project, class: Ci::RunnerProject do
runner factory: :ci_runner
runner factory: [:ci_runner, :project]
project
end
end
......@@ -3,37 +3,27 @@ FactoryBot.define do
sequence(:description) { |n| "My runner#{n}" }
platform "darwin"
is_shared false
active true
access_level :not_protected
runner_type :project_type
trait :online do
contacted_at Time.now
end
trait :shared do
is_shared true
runner_type :instance_type
end
trait :specific do
is_shared false
trait :instance do
is_shared true
runner_type :instance_type
end
trait :group do
is_shared false
runner_type :group_type
end
trait :project do
is_shared false
runner_type :project_type
end
trait :instance do
is_shared true
runner_type :instance_type
end
trait :inactive do
active false
end
......
......@@ -76,7 +76,7 @@ describe "Admin Runners" do
context 'shared runner' do
it 'shows the label and does not show the project count' do
runner = create :ci_runner, :shared
runner = create :ci_runner, :instance
visit admin_runners_path
......@@ -90,7 +90,7 @@ describe "Admin Runners" do
context 'specific runner' do
it 'shows the label and the project count' do
project = create :project
runner = create :ci_runner, projects: [project]
runner = create :ci_runner, :project, projects: [project]
visit admin_runners_path
......@@ -149,8 +149,9 @@ describe "Admin Runners" do
end
context 'with specific runner' do
let(:runner) { create(:ci_runner, :project, projects: [@project1]) }
before do
@project1.runners << runner
visit admin_runner_path(runner)
end
......@@ -158,9 +159,9 @@ describe "Admin Runners" do
end
context 'with locked runner' do
let(:runner) { create(:ci_runner, :project, projects: [@project1], locked: true) }
before do
runner.update(locked: true)
@project1.runners << runner
visit admin_runner_path(runner)
end
......@@ -168,9 +169,10 @@ describe "Admin Runners" do
end
context 'with shared runner' do
let(:runner) { create(:ci_runner, :instance) }
before do
@project1.destroy
runner.update(is_shared: true)
visit admin_runner_path(runner)
end
......@@ -179,8 +181,9 @@ describe "Admin Runners" do
end
describe 'disable/destroy' do
let(:runner) { create(:ci_runner, :project, projects: [@project1]) }
before do
@project1.runners << runner
visit admin_runner_path(runner)
end
......
......@@ -29,11 +29,7 @@ feature 'Runners' do
end
context 'when a project_type runner is activated on the project' do
given(:specific_runner) { create(:ci_runner, :project) }
background do
project.runners << specific_runner
end
given(:specific_runner) { create(:ci_runner, :project, projects: [specific_runner]) }
scenario 'user sees the specific runner' do
visit project_runners_path(project)
......@@ -126,11 +122,10 @@ feature 'Runners' do
context 'when a specific runner exists in another project' do
given(:another_project) { create(:project) }
given(:specific_runner) { create(:ci_runner, :project) }
given(:specific_runner) { create(:ci_runner, :project, projects: [another_project]) }
background do
another_project.add_master(user)
another_project.runners << specific_runner
end
scenario 'user enables and disables a specific runner' do
......
......@@ -2,7 +2,7 @@ require 'spec_helper'
describe RunnerJobsFinder do
let(:project) { create(:project) }
let(:runner) { create(:ci_runner, :shared) }
let(:runner) { create(:ci_runner, :instance) }
subject { described_class.new(runner, params).execute }
......
......@@ -148,10 +148,9 @@ describe Ci::Build do
end
context 'when there are runners' do
let(:runner) { create(:ci_runner) }
let(:runner) { create(:ci_runner, :project, projects: [build.project]) }
before do
build.project.runners << runner
runner.update_attributes(contacted_at: 1.second.ago)
end
......@@ -1388,12 +1387,7 @@ describe Ci::Build do
it { is_expected.to be_truthy }
context "and there are specific runner" do
let(:runner) { create(:ci_runner, contacted_at: 1.second.ago) }
before do
build.project.runners << runner
runner.save
end
let!(:runner) { create(:ci_runner, :project, projects: [build.project], contacted_at: 1.second.ago) }
it { is_expected.to be_falsey }
end
......
......@@ -1589,7 +1589,7 @@ describe Ci::Pipeline, :mailer do
context 'when pipeline is not stuck' do
before do
create(:ci_runner, :shared, :online)
create(:ci_runner, :instance, :online)
end
it 'is not stuck' do
......
This diff is collapsed.
......@@ -1177,8 +1177,8 @@ describe Project do
describe '#any_runners?' do
context 'shared runners' do
let(:project) { create(:project, shared_runners_enabled: shared_runners_enabled) }
let(:specific_runner) { create(:ci_runner) }
let(:shared_runner) { create(:ci_runner, :shared) }
let(:specific_runner) { create(:ci_runner, :project, projects: [project]) }
let(:shared_runner) { create(:ci_runner, :instance) }
context 'for shared runners disabled' do
let(:shared_runners_enabled) { false }
......@@ -1188,7 +1188,7 @@ describe Project do
end
it 'has a specific runner' do
project.runners << specific_runner
specific_runner
expect(project.any_runners?).to be_truthy
end
......@@ -1200,13 +1200,13 @@ describe Project do
end
it 'checks the presence of specific runner' do
project.runners << specific_runner
specific_runner
expect(project.any_runners? { |runner| runner == specific_runner }).to be_truthy
end
it 'returns false if match cannot be found' do
project.runners << specific_runner
specific_runner
expect(project.any_runners? { false }).to be_falsey
end
......
......@@ -1858,8 +1858,7 @@ describe User do
describe '#ci_owned_runners' do
let(:user) { create(:user) }
let(:runner_1) { create(:ci_runner) }
let(:runner_2) { create(:ci_runner) }
let(:runner) { create(:ci_runner, :project, projects: [project]) }
context 'without any projects nor groups' do
let!(:project) { create(:project, runners: [runner_1]) }
......
......@@ -262,16 +262,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
describe '/api/v4/jobs' do
let(:project) { create(:project, shared_runners_enabled: false) }
let(:pipeline) { create(:ci_pipeline_without_jobs, project: project, ref: 'master') }
let(:runner) { create(:ci_runner) }
let(:runner) { create(:ci_runner, :project, projects: [project]) }
let(:job) do
create(:ci_build, :artifacts, :extended_options,
pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0, commands: "ls\ndate")
end
before do
project.runners << runner
end
describe 'POST /api/v4/jobs/request' do
let!(:last_update) {}
let!(:new_update) { }
......@@ -379,7 +375,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
end
context 'when shared runner requests job for project without shared_runners_enabled' do
let(:runner) { create(:ci_runner, :shared) }
let(:runner) { create(:ci_runner, :instance) }
it_behaves_like 'no jobs available'
end
......
......@@ -11,7 +11,7 @@ describe API::Runners do
let(:group) { create(:group).tap { |group| group.add_owner(user) } }
let(:group2) { create(:group).tap { |group| group.add_owner(user) } }
let!(:shared_runner) { create(:ci_runner, :shared, description: 'Shared runner') }
let!(:shared_runner) { create(:ci_runner, :instance, description: 'Shared runner') }
let!(:unused_project_runner) { create(:ci_runner) }
let!(:project_runner) do
......
require 'spec_helper'
describe RunnerEntity do
let(:runner) { create(:ci_runner, :specific) }
let(:project) { create(:project) }
let(:runner) { create(:ci_runner, :project, projects: [project]) }
let(:entity) { described_class.new(runner, request: request, current_user: user) }
let(:request) { double('request') }
let(:project) { create(:project) }
let(:user) { create(:admin) }
before do
......
......@@ -292,7 +292,7 @@ module Ci
end
context 'when access_level of runner is not_protected' do
let!(:specific_runner) { create(:ci_runner, :specific) }
let!(:specific_runner) { create(:ci_runner, :project, projects: [project]) }
context 'when a job is protected' do
let!(:pending_job) { create(:ci_build, :protected, pipeline: pipeline) }
......
......@@ -6,13 +6,9 @@ describe Ci::UpdateBuildQueueService do
let(:pipeline) { create(:ci_pipeline, project: project) }
context 'when updating specific runners' do
let(:runner) { create(:ci_runner, :project) }
let(:runner) { create(:ci_runner, :project, projects: [project]) }
context 'when there is a runner that can pick build' do
before do
build.project.runners << runner
end
it 'ticks runner queue value' do
expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value }
end
......
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