From 92d0434a4653e011bf46b4c415149f086e27282f Mon Sep 17 00:00:00 2001 From: lma-git Date: Tue, 2 Sep 2025 15:28:40 -0700 Subject: [PATCH] Update job exit_code getter/setter Updates the getter/setter methods for the CI job exit_code so that it uses its new destination in p_ci_builds. --- app/models/ci/build.rb | 6 -- app/models/concerns/ci/metadatable.rb | 15 +++- spec/models/ci/build_spec.rb | 4 +- spec/models/concerns/ci/metadatable_spec.rb | 96 +++++++++++++++++++-- 4 files changed, 106 insertions(+), 15 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 09e527a42b4a63..a29cb2cf4f4404 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -481,12 +481,6 @@ def auto_retry_allowed? auto_retry.allowed? end - def exit_code=(value) - return unless value - - ensure_metadata.exit_code = value.to_i.clamp(0, Gitlab::Database::MAX_SMALLINT_VALUE) - end - def auto_retry_expected? failed? && auto_retry_allowed? end diff --git a/app/models/concerns/ci/metadatable.rb b/app/models/concerns/ci/metadatable.rb index c10abde4d1b284..96123c2c79ccb5 100644 --- a/app/models/concerns/ci/metadatable.rb +++ b/app/models/concerns/ci/metadatable.rb @@ -20,8 +20,6 @@ module Metadatable accepts_nested_attributes_for :metadata - delegate :exit_code, to: :metadata, allow_nil: true - before_validation :ensure_metadata, on: :create scope :with_project_and_metadata, -> do @@ -179,6 +177,19 @@ def scoped_user_id (read_from_new_destination? && read_attribute(:scoped_user_id)) || options[:scoped_user_id] end + def exit_code + (read_from_new_destination? && read_attribute(:exit_code)) || metadata&.exit_code + end + + def exit_code=(value) + return unless value + + safe_value = value.to_i.clamp(0, Gitlab::Database::MAX_SMALLINT_VALUE) + + write_attribute(:exit_code, safe_value) + ensure_metadata.exit_code = safe_value if can_write_metadata? + end + private def read_metadata_attribute(legacy_key, metadata_key, job_definition_key, default_value = nil) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index ca17cedec69b0a..43c49310b5085d 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -5339,7 +5339,7 @@ def run_job_without_exception it 'correctly sets the exit code' do expect { drop_with_exit_code } - .to change { build.reload.metadata&.exit_code }.from(nil).to(1) + .to change { build.reload.exit_code }.from(nil).to(1) end shared_examples 'drops the build without changing allow_failure' do @@ -5435,7 +5435,7 @@ def run_job_without_exception it 'wraps around to max size of a signed smallint' do expect { drop_with_exit_code } - .to change { build.reload.metadata&.exit_code }.from(nil).to(32767) + .to change { build.reload.exit_code }.from(nil).to(32767) end end end diff --git a/spec/models/concerns/ci/metadatable_spec.rb b/spec/models/concerns/ci/metadatable_spec.rb index 4ba6fb5fa1d43a..c97a3f7cd52662 100644 --- a/spec/models/concerns/ci/metadatable_spec.rb +++ b/spec/models/concerns/ci/metadatable_spec.rb @@ -167,7 +167,8 @@ context 'when metadata does not exist' do before do - job.delete + job.metadata.delete + job.reload end it { is_expected.to be_nil } @@ -378,9 +379,9 @@ end describe '#id_tokens=' do - let(:id_tokens) { { 'TEST_ID_TOKEN' => { 'aud' => 'https://client.test' } } } + let(:new_id_tokens) { { 'TEST_ID_TOKEN' => { 'aud' => 'https://client.test' } } } - subject(:set_id_tokens) { processable.id_tokens = id_tokens } + subject(:set_id_tokens) { processable.id_tokens = new_id_tokens } it 'does not change metadata.id_tokens' do expect { set_id_tokens } @@ -393,9 +394,94 @@ end it 'sets the value into metadata.id_tokens' do - set_id_tokens + expect { set_id_tokens } + .to change { processable.metadata.id_tokens } + .from({}).to(new_id_tokens) + end + end + end + + describe '#exit_code' do + subject(:exit_code) { processable.exit_code } + + it { is_expected.to be_nil } + + context 'when metadata exit_code is present' do + before do + processable.metadata.write_attribute(:exit_code, 1) + end + + it { is_expected.to eq(1) } + + context 'when job exit_code is present' do + before do + processable.write_attribute(:exit_code, 2) + end + + it { is_expected.to eq(2) } - expect(processable.metadata.id_tokens).to eq(id_tokens) + context 'when FF `read_from_new_ci_destinations` is disabled' do + before do + stub_feature_flags(read_from_new_ci_destinations: false) + end + + it { is_expected.to eq(1) } + end + end + end + end + + describe '#exit_code=' do + let(:existing_exit_code) { 1 } + let(:new_exit_code) { nil } + + subject(:set_exit_code) { processable.exit_code = new_exit_code } + + before do + processable.write_attribute(:exit_code, existing_exit_code) + processable.metadata.write_attribute(:exit_code, existing_exit_code) + end + + it 'does not change job exit_code nor metadata exit_code value' do + expect { set_exit_code } + .to not_change { processable.read_attribute(:exit_code) } + .and not_change { processable.metadata.exit_code } + end + + context 'when the new exit_code is not nil' do + using RSpec::Parameterized::TableSyntax + + where(:new_exit_code, :expected_exit_code) do + 2 | 2 + -3 | 0 + 0 | 0 + 500 | 500 + 40000 | 32767 + end + + with_them do + it 'updates job exit_code with the expected value' do + expect { set_exit_code } + .to change { processable.read_attribute(:exit_code) } + .from(existing_exit_code).to(expected_exit_code) + end + + it 'does not change metadata exit_code value' do + expect { set_exit_code } + .to not_change { processable.metadata.exit_code } + end + + context 'when FF `stop_writing_builds_metadata` is disabled' do + before do + stub_feature_flags(stop_writing_builds_metadata: false) + end + + it 'updates metadata exit_code with the expected value' do + expect { set_exit_code } + .to change { processable.metadata.exit_code } + .from(existing_exit_code).to(expected_exit_code) + end + end end end end -- GitLab