diff --git a/.rubocop_todo/rspec/feature_category.yml b/.rubocop_todo/rspec/feature_category.yml index 346e259b4ddd5db402dd5eefc370b60bbf9b3636..6637db097c5f791fd9dca2a83f13268af7b5dda6 100644 --- a/.rubocop_todo/rspec/feature_category.yml +++ b/.rubocop_todo/rspec/feature_category.yml @@ -2909,7 +2909,6 @@ RSpec/FeatureCategory: - 'spec/lib/gitlab/ci/config/edge_stages_injector_spec.rb' - 'spec/lib/gitlab/ci/config/entry/allow_failure_spec.rb' - 'spec/lib/gitlab/ci/config/entry/artifacts_spec.rb' - - 'spec/lib/gitlab/ci/config/entry/bridge_spec.rb' - 'spec/lib/gitlab/ci/config/entry/cache_spec.rb' - 'spec/lib/gitlab/ci/config/entry/caches_spec.rb' - 'spec/lib/gitlab/ci/config/entry/commands_spec.rb' diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index dbc56079a0680e41989702eaaff2e4e5b60a1d87..69584f2dd382bde83c6f60e5b67150c9498bab64 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -111,11 +111,6 @@ class Build < Ci::Processable validates :coverage, numericality: true, allow_blank: true validates :ref, presence: true - scope :not_interruptible, -> do - joins(:metadata) - .where.not(Ci::BuildMetadata.table_name => { id: Ci::BuildMetadata.scoped_build.with_interruptible.select(:id) }) - end - scope :unstarted, -> { where(runner_id: nil) } scope :with_any_artifacts, -> do diff --git a/app/models/ci/processable.rb b/app/models/ci/processable.rb index 132f706d265db048e9dd710b971aed8838e37a39..414d36da7c317189e20b80d7b87874d4cd034b45 100644 --- a/app/models/ci/processable.rb +++ b/app/models/ci/processable.rb @@ -33,6 +33,12 @@ class Processable < ::CommitStatus where('NOT EXISTS (?)', needs) end + scope :not_interruptible, -> do + joins(:metadata).where.not( + Ci::BuildMetadata.table_name => { id: Ci::BuildMetadata.scoped_build.with_interruptible.select(:id) } + ) + end + state_machine :status do event :enqueue do transition [:created, :skipped, :manual, :scheduled] => :waiting_for_resource, if: :with_resource_group? diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb index 5fcafcba829b51c21f13b2ebf57b199abbe31cab..7ea4b460640e18e664b17a58719478e6bec69a9d 100644 --- a/lib/gitlab/ci/config/entry/job.rb +++ b/lib/gitlab/ci/config/entry/job.rb @@ -13,7 +13,7 @@ class Job < ::Gitlab::Config::Entry::Node ALLOWED_WHEN = %w[on_success on_failure always manual delayed].freeze ALLOWED_KEYS = %i[tags script image services start_in artifacts cache dependencies before_script after_script hooks - coverage retry parallel interruptible timeout + coverage retry parallel timeout release id_tokens publish pages].freeze validations do @@ -83,10 +83,6 @@ class Job < ::Gitlab::Config::Entry::Node description: 'Services that will be used to execute this job.', inherit: true - entry :interruptible, ::Gitlab::Config::Entry::Boolean, - description: 'Set jobs interruptible value.', - inherit: true - entry :timeout, Entry::Timeout, description: 'Timeout duration of this job.', inherit: true @@ -139,7 +135,7 @@ class Job < ::Gitlab::Config::Entry::Node attributes :script, :tags, :when, :dependencies, :needs, :retry, :parallel, :start_in, - :interruptible, :timeout, :release, + :timeout, :release, :allow_failure, :publish, :pages def self.matching?(name, config) @@ -169,7 +165,6 @@ def value coverage: coverage_defined? ? coverage_value : nil, retry: retry_defined? ? retry_value : nil, parallel: has_parallel? ? parallel_value : nil, - interruptible: interruptible_defined? ? interruptible_value : nil, timeout: parsed_timeout, artifacts: artifacts_value, release: release_value, diff --git a/lib/gitlab/ci/config/entry/processable.rb b/lib/gitlab/ci/config/entry/processable.rb index d0e9a9afc51b171882518a3b58d6367a33310810..0b322fd433cae15bec375a21383ff040c5d58583 100644 --- a/lib/gitlab/ci/config/entry/processable.rb +++ b/lib/gitlab/ci/config/entry/processable.rb @@ -15,7 +15,8 @@ module Processable include ::Gitlab::Config::Entry::Inheritable PROCESSABLE_ALLOWED_KEYS = %i[extends stage only except rules variables - inherit allow_failure when needs resource_group environment].freeze + inherit allow_failure when needs resource_group environment + interruptible].freeze MAX_NESTING_LEVEL = 10 included do @@ -74,6 +75,10 @@ module Processable description: 'Environment configuration for this job.', inherit: false + entry :interruptible, ::Gitlab::Config::Entry::Boolean, + description: 'Set jobs interruptible value.', + inherit: true + attributes :extends, :rules, :resource_group end @@ -133,7 +138,8 @@ def value except: except_value, environment: environment_defined? ? environment_value : nil, environment_name: environment_defined? ? environment_value[:name] : nil, - resource_group: resource_group }.compact + resource_group: resource_group, + interruptible: interruptible_defined? ? interruptible_value : nil }.compact end def root_variables_inheritance diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 18415a6079fb5b81617707526a65d01945863e6c..6662e83b56474226b01fb611fba24d9ef090ad93 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -637,11 +637,5 @@ build.build_runner_session(url: 'https://gitlab.example.com') end end - - trait :interruptible do - after(:build) do |build| - build.metadata.interruptible = true - end - end end end diff --git a/spec/factories/ci/processable.rb b/spec/factories/ci/processable.rb index 497564337131a83b717f6f7cf77480222a5ad74a..c84a2d5d93d00d2172205458193e1dbaa168026f 100644 --- a/spec/factories/ci/processable.rb +++ b/spec/factories/ci/processable.rb @@ -11,6 +11,10 @@ scheduling_type { 'stage' } partition_id { pipeline.partition_id } + options do + {} + end + # This factory was updated to help with the efforts of the removal of `ci_builds.stage`: # https://gitlab.com/gitlab-org/gitlab/-/issues/364377 # These additions can be removed once the specs that use the stage attribute have been updated @@ -52,5 +56,11 @@ processable.resource_group = create(:ci_resource_group, project: processable.project) end end + + trait :interruptible do + after(:build) do |processable| + processable.metadata.interruptible = true + end + end end end diff --git a/spec/lib/gitlab/ci/config/entry/bridge_spec.rb b/spec/lib/gitlab/ci/config/entry/bridge_spec.rb index 6e6b9d949c5d4f41592d1242a44491156cbe17d4..35f2a99ee872fb8b8e68c5d32f6dff92e4c02b7e 100644 --- a/spec/lib/gitlab/ci/config/entry/bridge_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/bridge_spec.rb @@ -2,10 +2,11 @@ require 'spec_helper' -RSpec.describe Gitlab::Ci::Config::Entry::Bridge do +RSpec.describe Gitlab::Ci::Config::Entry::Bridge, feature_category: :continuous_integration do subject(:entry) { described_class.new(config, name: :my_bridge) } it_behaves_like 'with inheritable CI config' do + let(:config) { { trigger: 'some/project' } } let(:inheritable_key) { 'default' } let(:inheritable_class) { Gitlab::Ci::Config::Entry::Default } @@ -13,9 +14,13 @@ # that we know that we don't want to inherit # as they do not have sense in context of Bridge let(:ignored_inheritable_columns) do - %i[before_script after_script hooks image services cache interruptible timeout + %i[before_script after_script hooks image services cache timeout retry tags artifacts id_tokens] end + + before do + allow(entry).to receive_message_chain(:inherit_entry, :default_entry, :inherit?).and_return(true) + end end describe '.matching?' do diff --git a/spec/lib/gitlab/ci/config/entry/processable_spec.rb b/spec/lib/gitlab/ci/config/entry/processable_spec.rb index 44e2fdbac37599e2393558172fccbd9def8ba279..84a8fd827cbc95828e7c0c3568d0d9650aa2f90b 100644 --- a/spec/lib/gitlab/ci/config/entry/processable_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/processable_spec.rb @@ -217,6 +217,15 @@ def self.name end end end + + context 'when interruptible is not a boolean' do + let(:config) { { interruptible: 123 } } + + it 'returns error about wrong value type' do + expect(entry).not_to be_valid + expect(entry.errors).to include "interruptible config should be a boolean value" + end + end end describe '#relevant?' do @@ -462,6 +471,28 @@ def self.name end end end + + context 'with interruptible' do + context 'when interruptible is not defined' do + let(:config) { { script: 'ls' } } + + it 'sets interruptible to nil' do + entry.compose!(deps) + + expect(entry.value[:interruptible]).to be_nil + end + end + + context 'when interruptible is defined' do + let(:config) { { script: 'ls', interruptible: true } } + + it 'sets interruptible to the value' do + entry.compose!(deps) + + expect(entry.value[:interruptible]).to eq(true) + end + end + end end context 'when composed' do diff --git a/spec/lib/gitlab/ci/yaml_processor/test_cases/interruptible_spec.rb b/spec/lib/gitlab/ci/yaml_processor/test_cases/interruptible_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..03ff7077969fc10453b39ed8b10a82dbb804b56b --- /dev/null +++ b/spec/lib/gitlab/ci/yaml_processor/test_cases/interruptible_spec.rb @@ -0,0 +1,96 @@ +# frozen_string_literal: true + +require 'spec_helper' + +module Gitlab + module Ci + RSpec.describe YamlProcessor, feature_category: :pipeline_composition do + subject(:processor) { described_class.new(config, user: nil).execute } + + let(:builds) { processor.builds } + + context 'with interruptible' do + let(:default_config) { nil } + + let(:config) do + <<~YAML + #{default_config} + + build1: + script: rspec + interruptible: true + + build2: + script: rspec + interruptible: false + + build3: + script: rspec + + bridge1: + trigger: some/project + interruptible: true + + bridge2: + trigger: some/project + interruptible: false + + bridge3: + trigger: some/project + YAML + end + + it 'returns jobs with their interruptible value' do + expect(builds).to contain_exactly( + a_hash_including(name: 'build1', interruptible: true), + a_hash_including(name: 'build2', interruptible: false), + a_hash_including(name: 'build3').and(exclude(:interruptible)), + a_hash_including(name: 'bridge1', interruptible: true), + a_hash_including(name: 'bridge2', interruptible: false), + a_hash_including(name: 'bridge3').and(exclude(:interruptible)) + ) + end + + context 'when default:interruptible is true' do + let(:default_config) do + <<~YAML + default: + interruptible: true + YAML + end + + it 'returns jobs with their interruptible value' do + expect(builds).to contain_exactly( + a_hash_including(name: 'build1', interruptible: true), + a_hash_including(name: 'build2', interruptible: false), + a_hash_including(name: 'build3', interruptible: true), + a_hash_including(name: 'bridge1', interruptible: true), + a_hash_including(name: 'bridge2', interruptible: false), + a_hash_including(name: 'bridge3', interruptible: true) + ) + end + end + + context 'when default:interruptible is false' do + let(:default_config) do + <<~YAML + default: + interruptible: false + YAML + end + + it 'returns jobs with their interruptible value' do + expect(builds).to contain_exactly( + a_hash_including(name: 'build1', interruptible: true), + a_hash_including(name: 'build2', interruptible: false), + a_hash_including(name: 'build3', interruptible: false), + a_hash_including(name: 'bridge1', interruptible: true), + a_hash_including(name: 'bridge2', interruptible: false), + a_hash_including(name: 'bridge3', interruptible: false) + ) + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index 511fd4b14f8e33ba39bac0f2fbc68f16eeaed426..3ba62888b7cfe239bf75515c94110fac1589f20f 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -123,55 +123,6 @@ module Ci end end - describe 'interruptible entry' do - describe 'interruptible job' do - let(:config) do - YAML.dump(rspec: { script: 'rspec', interruptible: true }) - end - - it { expect(rspec_build[:interruptible]).to be_truthy } - end - - describe 'interruptible job with default value' do - let(:config) do - YAML.dump(rspec: { script: 'rspec' }) - end - - it { expect(rspec_build).not_to have_key(:interruptible) } - end - - describe 'uninterruptible job' do - let(:config) do - YAML.dump(rspec: { script: 'rspec', interruptible: false }) - end - - it { expect(rspec_build[:interruptible]).to be_falsy } - end - - it "returns interruptible when overridden for job" do - config = YAML.dump({ default: { interruptible: true }, - rspec: { script: "rspec" } }) - - config_processor = described_class.new(config).execute - builds = config_processor.builds.select { |b| b[:stage] == "test" } - - expect(builds.size).to eq(1) - expect(builds.first).to eq({ - stage: "test", - stage_idx: 2, - name: "rspec", - only: { refs: %w[branches tags] }, - options: { script: ["rspec"] }, - interruptible: true, - allow_failure: false, - when: "on_success", - job_variables: [], - root_variables_inheritance: true, - scheduling_type: :stage - }) - end - end - describe 'retry entry' do context 'when retry count is specified' do let(:config) do diff --git a/spec/models/ci/bridge_spec.rb b/spec/models/ci/bridge_spec.rb index 53ad3a3a698b76346e3ebb1d7332f0642fdb2b18..ae8c5aea8580bf41ea469e0df5a081be284b0226 100644 --- a/spec/models/ci/bridge_spec.rb +++ b/spec/models/ci/bridge_spec.rb @@ -1039,8 +1039,9 @@ end it 'creates the metadata record and assigns its partition' do - # the factory doesn't use any metadatable setters by default - # so the record will be initialized by the before_validation callback + # The record is initialized by the factory calling metadatable setters + bridge.metadata = nil + expect(bridge.metadata).to be_nil expect(bridge.save!).to be_truthy diff --git a/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb b/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb index a5dda1d13aa400720a9d54209cc44c5e786c9a15..0d83187f9e4ba374cd1b9c2aabe08c2f12de20e5 100644 --- a/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb +++ b/spec/services/ci/pipeline_creation/cancel_redundant_pipelines_service_spec.rb @@ -207,12 +207,12 @@ it 'does not cancel any builds' do expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') - expect(build_statuses(parent_pipeline)).to contain_exactly('running', 'running') + expect(build_statuses(parent_pipeline)).to contain_exactly('created', 'running', 'running') execute expect(build_statuses(prev_pipeline)).to contain_exactly('running', 'success', 'created') - expect(build_statuses(parent_pipeline)).to contain_exactly('running', 'running') + expect(build_statuses(parent_pipeline)).to contain_exactly('created', 'running', 'running') end end @@ -227,6 +227,25 @@ end end + context 'when there are trigger jobs' do + before do + create(:ci_bridge, :created, pipeline: prev_pipeline) + create(:ci_bridge, :running, pipeline: prev_pipeline) + create(:ci_bridge, :success, pipeline: prev_pipeline) + create(:ci_bridge, :interruptible, :created, pipeline: prev_pipeline) + create(:ci_bridge, :interruptible, :running, pipeline: prev_pipeline) + create(:ci_bridge, :interruptible, :success, pipeline: prev_pipeline) + end + + it 'still cancels the pipeline because auto-cancel is not affected by non-interruptible started triggers' do + execute + + expect(job_statuses(prev_pipeline)).to contain_exactly( + 'canceled', 'success', 'canceled', 'canceled', 'canceled', 'success', 'canceled', 'canceled', 'success') + expect(job_statuses(pipeline)).to contain_exactly('pending') + end + end + it 'does not cancel future pipelines' do expect(prev_pipeline.id).to be < pipeline.id expect(build_statuses(pipeline)).to contain_exactly('pending') @@ -269,7 +288,8 @@ private - def build_statuses(pipeline) - pipeline.builds.pluck(:status) + def job_statuses(pipeline) + pipeline.statuses.pluck(:status) end + alias_method :build_statuses, :job_statuses end