From 45943ac4844205b4adb8d4ebdd795f9767f059be Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Thu, 9 Oct 2025 15:53:07 +0200 Subject: [PATCH 01/11] Remove stop_writing_builds_metadata feature flag Changelog: other --- app/models/ci/bridge.rb | 5 +- app/models/ci/build.rb | 12 +- app/models/concerns/ci/metadatable.rb | 42 -- app/services/ci/clone_job_service.rb | 8 - .../environments/create_for_job_service.rb | 4 - .../wip/stop_writing_builds_metadata.yml | 14 - ee/app/models/concerns/ee/ci/metadatable.rb | 4 - ee/app/models/ee/ci/build.rb | 9 - ee/spec/factories/ci/builds.rb | 16 +- .../pipeline_execution_policy_pipeline.rb | 6 - .../ee/gitlab/ci/pipeline/seed/build_spec.rb | 11 - ee/spec/models/ci/build_spec.rb | 6 - .../models/concerns/ee/ci/metadatable_spec.rb | 36 +- .../composite_identity_spec.rb | 18 - .../ee/ci/create_pipeline_service_spec.rb | 10 - lib/gitlab/ci/pipeline/seed/build.rb | 22 +- spec/factories/ci/bridge.rb | 10 +- spec/factories/ci/builds.rb | 10 +- spec/factories/ci/processable.rb | 18 - spec/finders/ci/auth_job_finder_spec.rb | 6 - .../security/security_jobs_finder_spec.rb | 21 - .../gitlab/ci/build/context/global_spec.rb | 8 - .../lib/gitlab/ci/pipeline/seed/build_spec.rb | 375 ------------------ spec/models/ci/bridge_spec.rb | 54 +-- spec/models/ci/build_metadata_spec.rb | 6 +- spec/models/ci/build_spec.rb | 88 +--- spec/models/ci/processable_spec.rb | 8 - spec/models/concerns/ci/metadatable_spec.rb | 280 ++----------- .../api/ci/runner/jobs_request_post_spec.rb | 59 --- spec/serializers/build_details_entity_spec.rb | 2 +- .../ci/cancel_pipeline_service_spec.rb | 3 - spec/services/ci/clone_job_service_spec.rb | 82 +--- .../job_definition_spec.rb | 12 - .../partitioning_spec.rb | 19 - .../ci/create_pipeline_service_spec.rb | 34 +- .../create_web_ide_terminal_service_spec.rb | 69 ---- .../ci/pipelines/add_job_service_spec.rb | 16 - .../ci/retry_pipeline_service_spec.rb | 10 - .../ci/deployable_shared_examples.rb | 5 +- .../create_for_job_shared_examples.rb | 55 +-- 40 files changed, 76 insertions(+), 1397 deletions(-) delete mode 100644 config/feature_flags/wip/stop_writing_builds_metadata.yml diff --git a/app/models/ci/bridge.rb b/app/models/ci/bridge.rb index 7ff18ded6c93b2..cfd3217b4e8260 100644 --- a/app/models/ci/bridge.rb +++ b/app/models/ci/bridge.rb @@ -97,9 +97,8 @@ def self.with_preloads end def self.clone_accessors - %i[pipeline project ref tag options name - allow_failure stage_idx - yaml_variables when environment description needs_attributes + %i[pipeline project ref tag name allow_failure stage_idx + when environment description needs_attributes scheduling_type ci_stage partition_id resource_group].freeze end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 14695b8714504d..30b1a4f5b1a1ca 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -275,13 +275,11 @@ def with_preloads end def clone_accessors - %i[pipeline project ref tag options name - allow_failure stage_idx - yaml_variables when environment coverage_regex - description tag_list protected needs_attributes - job_variables_attributes resource_group scheduling_type - timeout timeout_source debug_trace_enabled - ci_stage partition_id id_tokens interruptible execution_config_id inputs_attributes].freeze + %i[pipeline project ref tag name allow_failure stage_idx when + environment coverage_regex description tag_list protected + needs_attributes job_variables_attributes resource_group + scheduling_type timeout timeout_source debug_trace_enabled + ci_stage partition_id execution_config_id inputs_attributes].freeze end def supported_keyset_orderings diff --git a/app/models/concerns/ci/metadatable.rb b/app/models/concerns/ci/metadatable.rb index 7c59e012e11b53..4f637622352e58 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 - before_validation :ensure_metadata, on: :create, if: :can_write_metadata? - scope :with_project_and_metadata, -> do preload(:project, :metadata, :job_definition) end @@ -63,11 +61,6 @@ def has_exposed_artifacts? artifacts_exposed_as.present? end - # Remove this method with FF `stop_writing_builds_metadata` - def ensure_metadata - metadata || build_metadata(project: project) - end - def degenerated? self.options.blank? end @@ -90,14 +83,6 @@ def yaml_variables read_metadata_attribute(:yaml_variables, :config_variables, :yaml_variables, []) end - def options=(value) - write_metadata_attribute(:options, :config_options, value) - end - - def yaml_variables=(value) - write_metadata_attribute(:yaml_variables, :config_variables, value) - end - def interruptible return job_definition.interruptible if job_definition return temp_job_definition.interruptible if temp_job_definition @@ -105,10 +90,6 @@ def interruptible metadata&.read_attribute(:interruptible) end - def interruptible=(value) - ensure_metadata.interruptible = value if can_write_metadata? - end - def id_tokens read_metadata_attribute(nil, :id_tokens, :id_tokens, {}).deep_stringify_keys end @@ -117,10 +98,6 @@ def id_tokens? id_tokens.present? end - def id_tokens=(value) - ensure_metadata.id_tokens = value if can_write_metadata? - end - def debug_trace_enabled? return debug_trace_enabled unless debug_trace_enabled.nil? return true if degenerated? @@ -130,7 +107,6 @@ def debug_trace_enabled? def enable_debug_trace! update!(debug_trace_enabled: true) - ensure_metadata.enable_debug_trace! if can_write_metadata? end def timeout_human_readable_value @@ -147,11 +123,6 @@ def update_timeout_state timeout = ::Ci::Builds::TimeoutCalculator.new(self).applicable_timeout return unless timeout - if can_write_metadata? - success = ensure_metadata.update(timeout: timeout.value, timeout_source: timeout.source) - return false unless success - end - # We don't use update because we're already in a Ci::Build transaction write_attribute(:timeout, timeout.value) write_attribute(:timeout_source, timeout.source) @@ -190,7 +161,6 @@ def exit_code=(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 @@ -204,18 +174,6 @@ def read_metadata_attribute(legacy_key, metadata_key, job_definition_key, defaul metadata&.read_attribute(metadata_key) || default_value end - - def write_metadata_attribute(legacy_key, metadata_key, value) - return unless can_write_metadata? - - ensure_metadata.write_attribute(metadata_key, value) - write_attribute(legacy_key, nil) - end - - def can_write_metadata? - Feature.disabled?(:stop_writing_builds_metadata, project) - end - strong_memoize_attr :can_write_metadata? end end diff --git a/app/services/ci/clone_job_service.rb b/app/services/ci/clone_job_service.rb index 2386a2913b4fc9..c40bbe32630821 100644 --- a/app/services/ci/clone_job_service.rb +++ b/app/services/ci/clone_job_service.rb @@ -10,7 +10,6 @@ def initialize(job, current_user:) def execute(new_job_variables: []) new_attributes = build_base_attributes - add_environment_attributes!(new_attributes) if persisted_environment.present? add_job_variables_attributes!(new_attributes, new_job_variables) add_job_definition_attributes!(new_attributes) @@ -36,13 +35,6 @@ def build_base_attributes clone_accessors.index_with { |attribute| job.method(attribute).call } end - def add_environment_attributes!(attributes) - return if Feature.enabled?(:stop_writing_builds_metadata, job.project) - - attributes[:metadata_attributes] ||= {} - attributes[:metadata_attributes][:expanded_environment_name] = expanded_environment_name - end - def add_job_variables_attributes!(attributes, new_job_variables) return unless clone_accessors.include?(:job_variables_attributes) return unless job.action? && new_job_variables.any? diff --git a/app/services/environments/create_for_job_service.rb b/app/services/environments/create_for_job_service.rb index 72de078819c3ab..3861b9404dcfcd 100644 --- a/app/services/environments/create_for_job_service.rb +++ b/app/services/environments/create_for_job_service.rb @@ -17,10 +17,6 @@ def execute(job) job.persisted_environment = environment - if Feature.disabled?(:stop_writing_builds_metadata, job.project) - job.assign_attributes(metadata_attributes: { expanded_environment_name: environment.name }) - end - track_environment_usage(job, environment) else job.assign_attributes(status: :failed, failure_reason: :environment_creation_failure) diff --git a/config/feature_flags/wip/stop_writing_builds_metadata.yml b/config/feature_flags/wip/stop_writing_builds_metadata.yml deleted file mode 100644 index f2012c9ba181a2..00000000000000 --- a/config/feature_flags/wip/stop_writing_builds_metadata.yml +++ /dev/null @@ -1,14 +0,0 @@ ---- -name: stop_writing_builds_metadata -description: > - Controls when to stop writing to the legacy ci_builds_metadata table. - This feature flag is not fully reversible. Ensure the FF `read_from_new_ci_destinations` - is enabled for an extended period of time (1-2 weeks) to ensure there are no issues - before enabling this FF. -feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/552057 -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/202462 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/552065 -milestone: '18.4' -group: group::ci platform -type: wip -default_enabled: false diff --git a/ee/app/models/concerns/ee/ci/metadatable.rb b/ee/app/models/concerns/ee/ci/metadatable.rb index bc4eda3ceb5c71..c38bddb940d5c7 100644 --- a/ee/app/models/concerns/ee/ci/metadatable.rb +++ b/ee/app/models/concerns/ee/ci/metadatable.rb @@ -12,10 +12,6 @@ def secrets def secrets? secrets.present? end - - def secrets=(value) - ensure_metadata.secrets = value if can_write_metadata? - end end end end diff --git a/ee/app/models/ee/ci/build.rb b/ee/app/models/ee/ci/build.rb index 5a9271b8f0c850..85df270fcb7e19 100644 --- a/ee/app/models/ee/ci/build.rb +++ b/ee/app/models/ee/ci/build.rb @@ -94,15 +94,6 @@ module Build end end - class_methods do - extend ::Gitlab::Utils::Override - - override :clone_accessors - def clone_accessors - (super + %i[secrets]).freeze - end - end - override :variables def variables strong_memoize(:variables) do diff --git a/ee/spec/factories/ci/builds.rb b/ee/spec/factories/ci/builds.rb index fef1be0b0c759c..df8b26c33501ca 100644 --- a/ee/spec/factories/ci/builds.rb +++ b/ee/spec/factories/ci/builds.rb @@ -9,15 +9,7 @@ end after(:build) do |build, evaluator| - if evaluator.secrets - # TODO: Remove this when FF `stop_writing_builds_metadata` is removed. - # https://gitlab.com/gitlab-org/gitlab/-/issues/552065 - if Feature.disabled?(:stop_writing_builds_metadata, build.project) - build.metadata.write_attribute(:secrets, evaluator.secrets) - end - - Ci::JobFactoryHelpers.mutate_temp_job_definition(build, secrets: evaluator.secrets) - end + Ci::JobFactoryHelpers.mutate_temp_job_definition(build, secrets: evaluator.secrets) if evaluator.secrets end trait :protected_environment_failure do @@ -295,12 +287,6 @@ execution_policy_variables_override: { allowed: false, exceptions: evaluator.variables_override_exceptions } ) - # TODO: Remove this when FF `stop_writing_builds_metadata` is removed. - # https://gitlab.com/gitlab-org/gitlab/-/issues/552065 - if Feature.disabled?(:stop_writing_builds_metadata, build.project) - build.metadata.write_attribute(:config_options, updated_options) - end - Ci::JobFactoryHelpers.mutate_temp_job_definition(build, options: updated_options) end end diff --git a/ee/spec/factories/security/pipeline_execution_policy_pipeline.rb b/ee/spec/factories/security/pipeline_execution_policy_pipeline.rb index a694e6844f47a9..3fda9348ecf699 100644 --- a/ee/spec/factories/security/pipeline_execution_policy_pipeline.rb +++ b/ee/spec/factories/security/pipeline_execution_policy_pipeline.rb @@ -41,12 +41,6 @@ job = instance.pipeline.stages[0].statuses[0] updated_options = { script: evaluator.job_script } - # TODO: Remove this when FF `stop_writing_builds_metadata` is removed. - # https://gitlab.com/gitlab-org/gitlab/-/issues/552065 - if Feature.disabled?(:stop_writing_builds_metadata, job.project) - job.metadata.write_attribute(:config_options, updated_options) - end - Ci::JobFactoryHelpers.mutate_temp_job_definition(job, options: updated_options) end end diff --git a/ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb b/ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb index 6d794ad0dfc728..c0cb126f88a43b 100644 --- a/ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb +++ b/ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb @@ -45,17 +45,6 @@ it 'propagates the composite identity as `scoped_user_id`' do expect(seed_attributes[:scoped_user_id]).to be(scoped_user.id) end - - context 'with the feature flag disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'propagates the composite identity as `scoped_user_id` in the options and attributes' do - expect(seed_attributes[:options][:scoped_user_id]).to be(scoped_user.id) - expect(seed_attributes[:scoped_user_id]).to be(scoped_user.id) - end - end end end end diff --git a/ee/spec/models/ci/build_spec.rb b/ee/spec/models/ci/build_spec.rb index 3276c9b8d3e1ae..65d2d85b01487a 100644 --- a/ee/spec/models/ci/build_spec.rb +++ b/ee/spec/models/ci/build_spec.rb @@ -73,12 +73,6 @@ end end - describe 'clone_accessors' do - it 'includes the cloneable extra accessors' do - expect(::Ci::Build.clone_accessors).to include(:secrets) - end - end - describe 'associations' do it { is_expected.to have_many(:security_scans).class_name('Security::Scan').with_foreign_key(:build_id) } it { is_expected.to have_many(:security_report_artifacts).class_name('Ci::JobArtifact') } diff --git a/ee/spec/models/concerns/ee/ci/metadatable_spec.rb b/ee/spec/models/concerns/ee/ci/metadatable_spec.rb index 4254695b17ff0d..d86de279df3e58 100644 --- a/ee/spec/models/concerns/ee/ci/metadatable_spec.rb +++ b/ee/spec/models/concerns/ee/ci/metadatable_spec.rb @@ -5,11 +5,6 @@ RSpec.describe EE::Ci::Metadatable, feature_category: :continuous_integration do let_it_be_with_refind(:processable) { create(:ci_processable, options: { script: 'echo' }) } - before do - # Remove when FF `stop_writing_builds_metadata` is removed - processable.clear_memoization(:can_write_metadata?) - end - describe '#secrets' do let(:metadata_secrets) do { @@ -32,7 +27,7 @@ context 'when metadata secrets are present' do before do - processable.ensure_metadata.write_attribute(:secrets, metadata_secrets) + create(:ci_build_metadata, build: processable, secrets: metadata_secrets) end it 'returns metadata secrets' do @@ -53,33 +48,4 @@ end end end - - describe '#secrets=' do - let(:secrets) do - { - PASSWORD: { vault: { engine: { name: 'test', path: 'ops' }, path: 'test/db', field: 'test' } } - }.deep_stringify_keys - end - - subject(:set_secrets) { processable.secrets = secrets } - - it 'does not change metadata.secrets' do - processable.ensure_metadata - - expect { set_secrets } - .to not_change { processable.metadata.secrets } - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'sets the value into metadata.secrets' do - set_secrets - - expect(processable.metadata.secrets).to eq(secrets) - end - end - end end diff --git a/ee/spec/services/ee/ci/create_pipeline_service/composite_identity_spec.rb b/ee/spec/services/ee/ci/create_pipeline_service/composite_identity_spec.rb index 068de869205d56..c8f809640205e8 100644 --- a/ee/spec/services/ee/ci/create_pipeline_service/composite_identity_spec.rb +++ b/ee/spec/services/ee/ci/create_pipeline_service/composite_identity_spec.rb @@ -48,24 +48,6 @@ { trigger: { project: 'test-project' } } ]) end - - context 'with the feature flag disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'propagates the scoped user into each job without overriding `options`' do - expect(pipeline).to be_created_successfully - expect(pipeline.builds).to be_present - - options = pipeline.statuses.map(&:options) - expect(pipeline.statuses.map(&:scoped_user_id)).to match_array([scoped_user.id, scoped_user.id]) - expect(options).to match_array([ - { script: ['echo'], job_timeout: 1.hour.to_i, scoped_user_id: scoped_user.id }, - { trigger: { project: 'test-project' }, scoped_user_id: scoped_user.id } - ]) - end - end end end end diff --git a/ee/spec/services/ee/ci/create_pipeline_service_spec.rb b/ee/spec/services/ee/ci/create_pipeline_service_spec.rb index 737b0883bfc411..cba374ff128932 100644 --- a/ee/spec/services/ee/ci/create_pipeline_service_spec.rb +++ b/ee/spec/services/ee/ci/create_pipeline_service_spec.rb @@ -162,16 +162,6 @@ it 'does not write to ci_builds_metadata' do expect { create_pipeline! }.to not_change { Ci::BuildMetadata.count } end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'writes to ci_builds_metadata' do - expect { create_pipeline! }.to change { Ci::BuildMetadata.count }.by(1) - end - end end context 'with partition_id param' do diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 37c73699249b5e..47c74f145b2f39 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -67,7 +67,6 @@ def errors def attributes @seed_attributes .deep_merge(pipeline_attributes) - .deep_merge(metadata_attributes) .deep_merge(rules_attributes) .deep_merge(allow_failure_criteria_attributes) .deep_merge(@cache.cache_attributes) @@ -168,12 +167,6 @@ def pipeline_attributes } end - def metadata_attributes - return {} unless can_write_metadata? - - { metadata_attributes: { partition_id: @pipeline.partition_id } } - end - # Scoped user is present when the user creating the pipeline supports composite identity. # For example: a service account like GitLab Duo. The scoped user is used to further restrict # the permissions of the CI job token associated to the `job.user`. @@ -182,13 +175,7 @@ def scoped_user_id_attribute return {} unless user_identity&.composite? && user_identity.linked? - attrs = { scoped_user_id: user_identity.scoped_user.id } - - if Feature.enabled?(:stop_writing_builds_metadata, @pipeline.project) - attrs - else - attrs.merge(options: attrs) - end + { scoped_user_id: user_identity.scoped_user.id } end def rules_attributes @@ -267,15 +254,8 @@ def build_job_definition_config(attrs) end def remove_ci_builds_metadata_attributes(attrs) - return attrs if can_write_metadata? - attrs.except(*::Ci::JobDefinition::CONFIG_ATTRIBUTES_FROM_METADATA) end - - def can_write_metadata? - Feature.disabled?(:stop_writing_builds_metadata, @pipeline.project) - end - strong_memoize_attr :can_write_metadata? end end end diff --git a/spec/factories/ci/bridge.rb b/spec/factories/ci/bridge.rb index f1a5d6409a27b6..6caa51adf3c237 100644 --- a/spec/factories/ci/bridge.rb +++ b/spec/factories/ci/bridge.rb @@ -39,15 +39,7 @@ ) end - if updated_options - # TODO: Remove this when FF `stop_writing_builds_metadata` is removed. - # https://gitlab.com/gitlab-org/gitlab/-/issues/552065 - if Feature.disabled?(:stop_writing_builds_metadata, bridge.project) - bridge.metadata.write_attribute(:config_options, updated_options) - end - - Ci::JobFactoryHelpers.mutate_temp_job_definition(bridge, options: updated_options) - end + Ci::JobFactoryHelpers.mutate_temp_job_definition(bridge, options: updated_options) if updated_options end trait :retried do diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 08a6a510cd9a28..2550a5f327e3fc 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -45,15 +45,7 @@ create(:ci_runner_machine_build, build: build, runner_manager: evaluator.runner_manager) end - if evaluator.id_tokens - # TODO: Remove this when FF `stop_writing_builds_metadata` is removed. - # https://gitlab.com/gitlab-org/gitlab/-/issues/552065 - if Feature.disabled?(:stop_writing_builds_metadata, build.project) - build.metadata.write_attribute(:id_tokens, evaluator.id_tokens) - end - - Ci::JobFactoryHelpers.mutate_temp_job_definition(build, id_tokens: evaluator.id_tokens) - end + Ci::JobFactoryHelpers.mutate_temp_job_definition(build, id_tokens: evaluator.id_tokens) if evaluator.id_tokens end trait :with_token do diff --git a/spec/factories/ci/processable.rb b/spec/factories/ci/processable.rb index e9a81167379e8a..738afdf7d1d821 100644 --- a/spec/factories/ci/processable.rb +++ b/spec/factories/ci/processable.rb @@ -13,20 +13,6 @@ scheduling_type { 'stage' } partition_id { pipeline.partition_id } - # TODO: Remove metadata association when FF `stop_writing_builds_metadata` is removed. - # https://gitlab.com/gitlab-org/gitlab/-/issues/552065 - metadata do - if Feature.disabled?(:stop_writing_builds_metadata, project) - association( - :ci_build_metadata, - build: instance, - config_options: options, - config_variables: yaml_variables, - strategy: :build - ) - end - 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 blocks can be updated once all instances of `stage` are removed from the spec files: @@ -124,10 +110,6 @@ trait :interruptible do after(:build) do |processable| - if Feature.disabled?(:stop_writing_builds_metadata, processable.project) - processable.metadata.interruptible = true - end - Ci::JobFactoryHelpers.mutate_temp_job_definition(processable, interruptible: true) end end diff --git a/spec/finders/ci/auth_job_finder_spec.rb b/spec/finders/ci/auth_job_finder_spec.rb index 1e6d92926025b2..bd6cc6cd8df227 100644 --- a/spec/finders/ci/auth_job_finder_spec.rb +++ b/spec/finders/ci/auth_job_finder_spec.rb @@ -29,12 +29,6 @@ context 'when job has a `scoped_user_id` tracked' do let(:scoped_user) { create(:user) } - before do - # Remove stub with stop_writing_builds_metadata - stub_ci_job_definition(job, options: job.options.merge(scoped_user_id: scoped_user.id)) - job.update!(scoped_user_id: scoped_user.id) - end - context 'when job user does not support composite identity' do it 'does not link the scoped user as composite identity' do execute diff --git a/spec/finders/security/security_jobs_finder_spec.rb b/spec/finders/security/security_jobs_finder_spec.rb index 83966106e7eb67..2b468461d9e1ba 100644 --- a/spec/finders/security/security_jobs_finder_spec.rb +++ b/spec/finders/security/security_jobs_finder_spec.rb @@ -26,27 +26,6 @@ is_expected.not_to include(dast_build) end - - context 'with empty definition' do - before_all do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - let_it_be(:sast_build) { create(:ci_build, :without_job_definition, :sast, pipeline: pipeline) } - let_it_be(:container_scanning_build) { create(:ci_build, :without_job_definition, :container_scanning, pipeline: pipeline) } - let_it_be(:dast_build) { create(:ci_build, :without_job_definition, :dast, pipeline: pipeline) } - let_it_be(:secret_detection_build) { create(:ci_build, :without_job_definition, :secret_detection, pipeline: pipeline) } - - let(:finder) { described_class.new(pipeline: pipeline, job_types: [:sast, :container_scanning, :secret_detection]) } - - it 'returns only those requested' do - is_expected.to include(sast_build) - is_expected.to include(container_scanning_build) - is_expected.to include(secret_detection_build) - - is_expected.not_to include(dast_build) - end - end end context 'with combination of security jobs and license scanning jobs' do diff --git a/spec/lib/gitlab/ci/build/context/global_spec.rb b/spec/lib/gitlab/ci/build/context/global_spec.rb index 1fa1c727cd673c..8603f673cedbdc 100644 --- a/spec/lib/gitlab/ci/build/context/global_spec.rb +++ b/spec/lib/gitlab/ci/build/context/global_spec.rb @@ -22,14 +22,6 @@ end it_behaves_like 'with passed yaml variables' - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it_behaves_like 'with passed yaml variables' - end end describe '#variables' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index 4e6e53efd0032b..7f7345759036eb 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -32,23 +32,12 @@ it { is_expected.to include(when: 'delayed') } - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - it 'assigns config attributes to job definition' do expect(seed_attributes[:temp_job_definition]).to have_attributes( interruptible: false, config: { options: { start_in: '3 hours' }, yaml_variables: [] } ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it { is_expected.to include(when: 'delayed', options: { start_in: '3 hours' }) } - end end context 'with job:rules:[when:]' do @@ -111,22 +100,11 @@ it { is_expected.to include(when: 'delayed') } - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - it 'assigns options to job definition' do expect(seed_attributes[:temp_job_definition]).to have_attributes( config: a_hash_including(options: { start_in: '3 hours' }) ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it { is_expected.to include(when: 'delayed', options: { start_in: '3 hours' }) } - end end context 'is not matched' do @@ -189,9 +167,6 @@ } end - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - it 'assigns options to job definition' do expect(seed_attributes[:temp_job_definition].config[:yaml_variables]).to match_array( [{ key: 'VAR1', value: 'new var 1' }, @@ -199,18 +174,6 @@ { key: 'VAR2', value: 'var 2' }] ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it do - is_expected.to include(yaml_variables: [{ key: 'VAR1', value: 'new var 1' }, - { key: 'VAR3', value: 'var 3' }, - { key: 'VAR2', value: 'var 2' }]) - end - end end context 'with job:rules:[needs:]' do @@ -347,25 +310,12 @@ context 'when rule evaluates to true' do let(:rules) { [{ if: '$VAR == null', interruptible: true }] } - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:interruptible) } - it 'overrides the job interruptible value' do expect(seed_attributes[:temp_job_definition]).to have_attributes( config: a_hash_including(interruptible: true) ) end - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'overrides the job interruptible value' do - is_expected.to include(interruptible: true) - end - end - context 'when job does not have an interruptible value' do let(:attributes) do { @@ -380,19 +330,6 @@ config: a_hash_including(interruptible: true) ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:interruptible) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'adds interruptible value to the job' do - is_expected.to include(interruptible: true) - end - end end context 'when rules:interruptible is not specified' do @@ -403,19 +340,6 @@ config: a_hash_including(interruptible: false) ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:interruptible) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'does not change the job interruptible value' do - is_expected.to include(interruptible: false) - end - end end end @@ -427,19 +351,6 @@ config: a_hash_including(interruptible: false) ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:interruptible) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'does not change the job interruptible value' do - is_expected.to include(interruptible: false) - end - end end end @@ -455,9 +366,6 @@ it { is_expected.to include(tag_list: ['static-tag', 'value', '$NO_VARIABLE']) } - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - it { expect(subject[:temp_job_definition].config).to include({ tag_list: ['static-tag', 'value', '$NO_VARIABLE'] }) } it 'assigns yaml_variables to job definition' do @@ -465,14 +373,6 @@ [{ key: 'VARIABLE', value: 'value' }] ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it { is_expected.to include(yaml_variables: [{ key: 'VARIABLE', value: 'value' }]) } - end end context 'with cache:key' do @@ -486,23 +386,12 @@ } end - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - it 'assigns options to job definition' do expect(seed_attributes[:temp_job_definition]).to have_attributes( config: a_hash_including(options: { cache: [a_hash_including(key: 'a-value')] }) ) end - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it { is_expected.to include(options: { cache: [a_hash_including(key: 'a-value')] }) } - end - context 'with cache:key:files' do let(:attributes) do { @@ -523,25 +412,6 @@ }) ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'includes cache options' do - cache_options = { - options: { - cache: [a_hash_including(key: '0_VERSION-30be75a2f82c8279268f1a442c1c60913cd11739')] - } - } - - is_expected.to include(cache_options) - end - end end context 'with cache:key:prefix' do @@ -557,22 +427,11 @@ } end - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - it 'assigns cache options to job definition' do expect(seed_attributes[:temp_job_definition]).to have_attributes( config: a_hash_including(options: { cache: [a_hash_including(key: 'something-default')] }) ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it { is_expected.to include(options: { cache: [a_hash_including(key: 'something-default')] }) } - end end context 'with cache:key:files and prefix' do @@ -596,25 +455,6 @@ }) ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'includes cache options' do - cache_options = { - options: { - cache: [a_hash_including(key: 'something-30be75a2f82c8279268f1a442c1c60913cd11739')] - } - } - - is_expected.to include(cache_options) - end - end end end @@ -649,22 +489,11 @@ end context 'when rules does not override allow_failure' do - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - it 'assigns options to job definition' do expect(seed_attributes[:temp_job_definition]).to have_attributes( config: a_hash_including(options: options) ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it { is_expected.to match a_hash_including(options: options) } - end end context 'when rules set allow_failure to true' do @@ -672,22 +501,11 @@ [{ if: '$VAR == null', when: 'always', allow_failure: true }] end - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - it 'assigns options to job definition' do expect(seed_attributes[:temp_job_definition]).to have_attributes( config: a_hash_including(options: { allow_failure_criteria: nil }) ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it { is_expected.to match a_hash_including(options: { allow_failure_criteria: nil }) } - end end context 'when rules set allow_failure to false' do @@ -695,22 +513,11 @@ [{ if: '$VAR == null', when: 'always', allow_failure: false }] end - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:options) } - it 'assigns options to job definition' do expect(seed_attributes[:temp_job_definition]).to have_attributes( config: a_hash_including(options: { allow_failure_criteria: nil }) ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it { is_expected.to match a_hash_including(options: { allow_failure_criteria: nil }) } - end end end @@ -744,24 +551,6 @@ { key: 'VAR4', value: 'new var pipeline 4' }] ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'returns calculated yaml variables' do - expect(subject[:yaml_variables]).to match_array( - [{ key: 'VAR1', value: 'var overridden pipeline 1' }, - { key: 'VAR2', value: 'var 2' }, - { key: 'VAR3', value: 'var 3' }, - { key: 'VAR4', value: 'new var pipeline 4' }] - ) - end - end end context 'when root_variables_inheritance is false' do @@ -773,22 +562,6 @@ { key: 'VAR3', value: 'var 3' }] ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'returns job variables' do - expect(subject[:yaml_variables]).to match_array( - [{ key: 'VAR2', value: 'var 2' }, - { key: 'VAR3', value: 'var 3' }] - ) - end - end end context 'when root_variables_inheritance is an array' do @@ -801,23 +574,6 @@ { key: 'VAR3', value: 'var 3' }] ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'returns calculated yaml variables' do - expect(subject[:yaml_variables]).to match_array( - [{ key: 'VAR1', value: 'var overridden pipeline 1' }, - { key: 'VAR2', value: 'var 2' }, - { key: 'VAR3', value: 'var 3' }] - ) - end - end end end @@ -829,21 +585,6 @@ [{ key: 'VAR2', value: 'var 2' }, { key: 'VAR3', value: 'var 3' }]) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'returns seed yaml variables' do - expect(subject[:yaml_variables]).to match_array( - [{ key: 'VAR2', value: 'var 2' }, - { key: 'VAR3', value: 'var 3' }]) - end - end end end @@ -870,22 +611,6 @@ { key: 'VAR2', value: 'new var 2' }] ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'recalculates the variables' do - expect(subject[:yaml_variables]).to contain_exactly( - { key: 'VAR1', value: 'overridden var 1' }, - { key: 'VAR2', value: 'new var 2' } - ) - end - end end context 'when the rules use root variables' do @@ -904,22 +629,6 @@ ) end - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'recalculates the variables' do - expect(subject[:yaml_variables]).to contain_exactly( - { key: 'VAR1', value: 'overridden var 1' }, - { key: 'VAR2', value: 'overridden var 2' } - ) - end - end - context 'when the root_variables_inheritance is false' do let(:root_variables_inheritance) { false } @@ -928,19 +637,6 @@ [{ key: 'VAR1', value: 'var 1' }] ) end - - # Remove when the FF `stop_writing_builds_metadata` is removed. - it { is_expected.not_to have_key(:yaml_variables) } - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'does not recalculate the variables' do - expect(subject[:yaml_variables]).to contain_exactly({ key: 'VAR1', value: 'var 1' }) - end - end end end end @@ -973,18 +669,6 @@ expect(subject[:temp_job_definition].config[:interruptible]).to eq(attributes[:interruptible]) end - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'preserves original job attributes' do - expect(subject[:options]).to eq(attributes[:options]) - expect(subject[:yaml_variables]).to eq(attributes[:yaml_variables]) - expect(subject[:interruptible]).to eq(attributes[:interruptible]) - end - end - context 'with id_tokens' do let(:attributes) do { @@ -998,19 +682,6 @@ it 'includes id_tokens in temp_job_definition' do expect(subject[:temp_job_definition].config[:id_tokens]).to eq(attributes[:id_tokens]) - - # Remove when the FF `stop_writing_builds_metadata` is removed. - expect(subject).not_to have_key(:id_tokens) - end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'includes id_tokens in build attributes' do - expect(subject[:id_tokens]).to eq(attributes[:id_tokens]) - end end end @@ -1029,19 +700,6 @@ it 'includes secrets in temp_job_definition' do expect(subject[:temp_job_definition].config[:secrets]).to eq(attributes[:secrets]) - - # Remove when the FF `stop_writing_builds_metadata` is removed. - expect(subject).not_to have_key(:secrets) - end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'includes secrets in build attributes' do - expect(subject[:secrets]).to eq(attributes[:secrets]) - end end end @@ -1130,18 +788,6 @@ expect(checksum1).not_to eq(checksum2) end end - - context 'when stop_writing_builds_metadata feature flag is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'includes original job attributes' do - expect(subject[:options]).to eq(attributes[:options]) - expect(subject[:yaml_variables]).to eq(attributes[:yaml_variables]) - expect(subject[:interruptible]).to eq(attributes[:interruptible]) - end - end end describe 'propagating composite identity', :request_store do @@ -1159,17 +805,6 @@ expect(seed_attributes[:temp_job_definition].config[:options].key?(:scoped_user_id)).to be(false) expect(seed_attributes.key?(:scoped_user_id)).to be(false) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'does not propagate composite identity by default' do - expect(seed_attributes[:options].key?(:scoped_user_id)).to be(false) - expect(seed_attributes.key?(:scoped_user_id)).to be(false) - end - end end end @@ -1634,16 +1269,6 @@ config: a_hash_including(options: { start_in: '1 day' }) ) end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'correctly populates when:' do - expect(seed_build.attributes).to include(when: 'delayed', options: { start_in: '1 day' }) - end - end end end diff --git a/spec/models/ci/bridge_spec.rb b/spec/models/ci/bridge_spec.rb index d863d044341028..0145c938ccfb48 100644 --- a/spec/models/ci/bridge_spec.rb +++ b/spec/models/ci/bridge_spec.rb @@ -83,9 +83,8 @@ describe '.clone_accessors' do it 'returns the correct list of attributes to clone' do expected_accessors = %i[ - pipeline project ref tag options name - allow_failure stage_idx - yaml_variables when environment description needs_attributes + pipeline project ref tag name allow_failure stage_idx + when environment description needs_attributes scheduling_type ci_stage partition_id resource_group ] @@ -1091,26 +1090,6 @@ end end - describe 'metadata support' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'reads YAML variables correctly' do - expect(bridge.yaml_variables).not_to be_empty - expect(bridge.metadata).to be_a Ci::BuildMetadata - expect(bridge.read_attribute(:yaml_variables)).to be_nil - expect(bridge.metadata.config_variables).to eq bridge.yaml_variables - end - - it 'reads options correctly' do - expect(bridge.options).not_to be_empty - expect(bridge.metadata).to be_a Ci::BuildMetadata - expect(bridge.read_attribute(:options)).to be_nil - expect(bridge.metadata.config_options).to eq bridge.options - end - end - describe '#triggers_child_pipeline?' do subject { bridge.triggers_child_pipeline? } @@ -1369,35 +1348,6 @@ end end - describe 'metadata partitioning' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - let(:pipeline) do - create(:ci_pipeline, project: project, partition_id: ci_testing_partition_id) - end - - let(:ci_stage) { create(:ci_stage, pipeline: pipeline) } - - let(:bridge) do - build(:ci_bridge, pipeline: pipeline, ci_stage: ci_stage) - end - - it 'creates the metadata record and assigns its partition' do - # 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 - - expect(bridge.metadata).to be_present - expect(bridge.metadata).to be_valid - expect(bridge.metadata.partition_id).to eq(ci_testing_partition_id) - end - end - describe '#deployment_job?' do subject { bridge.deployment_job? } diff --git a/spec/models/ci/build_metadata_spec.rb b/spec/models/ci/build_metadata_spec.rb index 12b0d242e45953..5f1df11f17f6a7 100644 --- a/spec/models/ci/build_metadata_spec.rb +++ b/spec/models/ci/build_metadata_spec.rb @@ -19,11 +19,7 @@ let_it_be_with_reload(:runner) { create(:ci_runner) } let(:job) { create(:ci_build, pipeline: pipeline, runner: runner) } - let(:metadata) { job.metadata } - - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end + let(:metadata) { create(:ci_build_metadata, build: job) } it_behaves_like 'having unique enum values' diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 74604b11a37811..7427bca292fe9d 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -187,7 +187,7 @@ describe 'with_secure_reports_from_metadata_config_options' do let_it_be(:pipeline) { create(:ci_empty_pipeline) } let_it_be(:build) { create(:ci_build, pipeline: pipeline) } - let_it_be_with_refind(:build_metadata) { build.ensure_metadata.tap(&:save!) } + let_it_be_with_refind(:build_metadata) { create(:ci_build_metadata, build: build) } let(:job_types) { %w[sast secret_detection] } subject(:query) { described_class.with_secure_reports_from_metadata_config_options(job_types) } @@ -258,14 +258,6 @@ end it_behaves_like "when build receives #{action} event" - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it_behaves_like "when build receives #{action} event" - end end end @@ -306,14 +298,6 @@ end it_behaves_like "when build receives #{action} event" - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it_behaves_like "when build receives #{action} event" - end end end @@ -2330,18 +2314,6 @@ expect(build.options['image']).to be_nil end - context 'when allowed to write metadata' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - let(:build) { create(:ci_build, pipeline: pipeline, yaml_variables: []) } - - it 'persist data in build metadata' do - expect(build.metadata.read_attribute(:config_options)).to eq(options.symbolize_keys) - end - end - it 'does not persist data in build' do expect(build.read_attribute(:options)).to be_nil end @@ -4053,16 +4025,6 @@ def insert_expected_predefined_variables(variables, after:) it_behaves_like 'having consistent representation' - context 'when allowed to write to metadata' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'persist data in build metadata' do - expect(build.metadata.read_attribute(:config_variables)).not_to be_nil - end - end - it 'does not persist data in build' do expect(build.read_attribute(:yaml_variables)).to be_nil end @@ -4275,14 +4237,6 @@ def run_job_without_exception end it_behaves_like 'saves data on transition' - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it_behaves_like 'saves data on transition' - end end context "when runner timeout doesn't override project timeout" do @@ -4294,14 +4248,6 @@ def run_job_without_exception end it_behaves_like 'saves data on transition' - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it_behaves_like 'saves data on transition' - end end end @@ -5103,14 +5049,6 @@ def run_job_without_exception end end - it_behaves_like 'a degenerable job' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - subject(:job) { create(:ci_build, pipeline: pipeline) } - end - describe '#invalid_dependencies' do it 'returns invalid dependencies' do dependencies_double = instance_double(Ci::BuildDependencies) @@ -5913,30 +5851,6 @@ def run_job_without_exception end end - describe 'metadata partitioning' do - let(:pipeline) { create(:ci_pipeline, project: project, partition_id: ci_testing_partition_id) } - - let(:ci_stage) { create(:ci_stage, pipeline: pipeline) } - let(:build) { FactoryBot.build(:ci_build, pipeline: pipeline, ci_stage: ci_stage) } - - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'creates the metadata record and assigns its partition' do - # The record is initialized by the factory calling metadatable setters - build.metadata = nil - - expect(build.metadata).to be_nil - - expect(build.save!).to be_truthy - - expect(build.metadata).to be_present - expect(build.metadata).to be_valid - expect(build.metadata.partition_id).to eq(ci_testing_partition_id) - end - end - describe 'secrets management id_tokens usage data' do context 'when ID tokens are defined' do context 'on create' do diff --git a/spec/models/ci/processable_spec.rb b/spec/models/ci/processable_spec.rb index cf9e958d5f3b97..86dcb2aa00ee4a 100644 --- a/spec/models/ci/processable_spec.rb +++ b/spec/models/ci/processable_spec.rb @@ -140,14 +140,6 @@ end end - it_behaves_like 'a degenerable job' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - subject(:job) { create(:ci_bridge, pipeline: pipeline) } - end - describe '#archived?' do shared_examples 'an archivable job' do it { is_expected.not_to be_archived } diff --git a/spec/models/concerns/ci/metadatable_spec.rb b/spec/models/concerns/ci/metadatable_spec.rb index 5e99c8c7e6e59e..9426c0eb4e4fac 100644 --- a/spec/models/concerns/ci/metadatable_spec.rb +++ b/spec/models/concerns/ci/metadatable_spec.rb @@ -5,13 +5,8 @@ RSpec.describe Ci::Metadatable, feature_category: :continuous_integration do let_it_be_with_refind(:processable) { create(:ci_processable, options: { script: 'echo' }) } - before do - # Remove when FF `stop_writing_builds_metadata` is removed - processable.clear_memoization(:can_write_metadata?) - end - describe '#options' do - let(:job) { build(:ci_build, :without_job_definition, metadata: nil) } + let(:job) { build(:ci_build, :without_job_definition) } let(:legacy_job_options) { { script: 'legacy job' } } let(:job_definition_options) { { script: 'job_definition' } } let(:temp_job_definition_options) { { script: 'temp_job_definition' } } @@ -25,7 +20,7 @@ context 'when metadata options are present' do before do - job.ensure_metadata.write_attribute(:config_options, metadata_options) + create(:ci_build_metadata, build: job, config_options: metadata_options) end it { is_expected.to eq(metadata_options) } @@ -58,7 +53,7 @@ end describe '#yaml_variables' do - let(:job) { build(:ci_build, :without_job_definition, metadata: nil) } + let(:job) { build(:ci_build, :without_job_definition) } let(:legacy_job_variables) { [{ key: 'VAR', value: 'legacy job' }] } let(:job_definition_variables) { [{ key: 'VAR', value: 'job_definition' }] } let(:temp_job_definition_variables) { [{ key: 'VAR', value: 'temp_job_definition' }] } @@ -72,7 +67,7 @@ context 'when metadata variables are present' do before do - job.ensure_metadata.write_attribute(:config_variables, metadata_variables) + create(:ci_build_metadata, build: job, config_variables: metadata_variables) end it { is_expected.to eq(metadata_variables) } @@ -104,80 +99,6 @@ end end - describe '#options=' do - let(:options) { { script: 'echo test' } } - - subject(:set_options) { processable.options = options } - - before do - processable.ensure_metadata - processable.write_attribute(:options, { script: 'echo something' }) - end - - it 'does not change metadata.config_options' do - expect { set_options } - .to not_change { processable.metadata.config_options } - end - - it 'does not set legacy job options to nil' do - expect { set_options } - .to not_change { processable.read_attribute(:options) } - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'sets value into metadata.config_options' do - expect { set_options } - .to change { processable.metadata.config_options }.to(options) - end - - it 'sets legacy job options to nil' do - expect { set_options } - .to change { processable.read_attribute(:options) }.to(nil) - end - end - end - - describe '#yaml_variables=' do - let(:yaml_variables) { [{ key: 'VAR', value: 'test' }] } - - subject(:set_yaml_variables) { processable.yaml_variables = yaml_variables } - - before do - processable.ensure_metadata - processable.write_attribute(:yaml_variables, [{ key: 'VAR', value: 'something' }]) - end - - it 'does not change metadata.config_variables' do - expect { set_yaml_variables } - .to not_change { processable.metadata.config_variables } - end - - it 'does not set legacy job yaml_variables to nil' do - expect { set_yaml_variables } - .to not_change { processable.read_attribute(:yaml_variables) } - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'sets value into metadata.config_variables' do - expect { set_yaml_variables } - .to change { processable.metadata.config_variables }.to(yaml_variables) - end - - it 'sets legacy job yaml_variables to nil' do - expect { set_yaml_variables } - .to change { processable.read_attribute(:yaml_variables) }.to(nil) - end - end - end - describe '#timeout_human_readable_value' do let_it_be_with_refind(:job) { create(:ci_build) } @@ -187,7 +108,7 @@ context 'when metadata timeout is present' do before do - job.ensure_metadata.write_attribute(:timeout, 60) + create(:ci_build_metadata, build: job, timeout: 60) end it { is_expected.to eq('1m') } @@ -209,7 +130,7 @@ context 'when metadata timeout is present' do before do - processable.ensure_metadata.write_attribute(:timeout, 60) + create(:ci_build_metadata, build: processable, timeout: 60) end it { is_expected.to eq(60) } @@ -240,14 +161,10 @@ it { is_expected.to be_nil } - it 'does not change job timeout nor metadata timeout values' do - processable.ensure_metadata - + it 'does not change job timeout values' do expect { update_timeout_state } .to not_change { processable.read_attribute(:timeout) } .and not_change { processable.read_attribute(:timeout_source) } - .and not_change { processable.metadata.timeout } - .and not_change { processable.metadata.timeout_source } end end @@ -266,43 +183,6 @@ .and change { processable.read_attribute(:timeout_source) }.from(nil).to(4) end - it 'does not change metadata timeout values' do - processable.ensure_metadata - - expect { update_timeout_state } - .to not_change { processable.metadata.timeout } - .and not_change { processable.metadata.timeout_source } - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - processable.ensure_metadata - end - - it { is_expected.to be(true) } - - it 'updates metadata timeout values' do - expect { update_timeout_state } - .to change { processable.metadata.timeout }.from(nil).to(25) - .and change { processable.metadata.timeout_source }.from('unknown_timeout_source').to('job_timeout_source') - end - - context 'when metadata timeout values fail to save' do - before do - allow(processable.metadata).to receive(:update).and_return(false) - end - - it { is_expected.to be(false) } - - it 'does not change job timeout values' do - expect { update_timeout_state } - .to not_change { processable.read_attribute(:timeout) } - .and not_change { processable.read_attribute(:timeout_source) } - end - end - end - context 'when job timeout values are invalid' do before do allow(processable).to receive(:valid?).and_return(false) @@ -320,18 +200,13 @@ context 'when metadata exists' do before do - job.ensure_metadata.save! + create(:ci_build_metadata, build: job) end it { is_expected.to eq('unknown_timeout_source') } end context 'when metadata does not exist' do - before do - job.metadata&.delete - job.reload - end - it { is_expected.to eq('unknown_timeout_source') } end @@ -382,26 +257,6 @@ .to change { processable.read_attribute(:debug_trace_enabled) } .from(nil).to(true) end - - it 'does not change metadata.debug_trace_enabled' do - processable.ensure_metadata - - expect { enable_debug_trace! } - .to not_change { processable.metadata.debug_trace_enabled } - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - processable.ensure_metadata - end - - it 'sets metadata.debug_trace_enabled to true' do - expect { enable_debug_trace! } - .to change { processable.metadata.debug_trace_enabled } - .from(false).to(true) - end - end end describe '#debug_trace_enabled?' do @@ -410,10 +265,7 @@ shared_examples 'when job debug_trace_enabled is nil' do context 'when metadata.debug_trace_enabled is true' do before do - processable.ensure_metadata.update!( - debug_trace_enabled: true, - config_options: { image: 'image:1.0' } # job is not degenerated - ) + create(:ci_build_metadata, build: processable, debug_trace_enabled: true) end it { is_expected.to be(true) } @@ -421,10 +273,7 @@ context 'when metadata.debug_trace_enabled is false' do before do - processable.ensure_metadata.update!( - debug_trace_enabled: false, - config_options: { image: 'image:1.0' } # job is not degenerated - ) + create(:ci_build_metadata, build: processable, debug_trace_enabled: false) end it { is_expected.to be(false) } @@ -434,7 +283,6 @@ before do # Very old jobs populated this column instead of metadata processable.update_column(:options, '{}') - processable.metadata&.delete processable.reload end @@ -484,7 +332,7 @@ context 'when metadata id_tokens are present' do before do - processable.ensure_metadata.write_attribute(:id_tokens, metadata_id_tokens) + create(:ci_build_metadata, build: processable, id_tokens: metadata_id_tokens) end it 'returns metadata id_tokens' do @@ -515,33 +363,6 @@ end end - describe '#id_tokens=' do - let(:new_id_tokens) { { 'TEST_ID_TOKEN' => { 'aud' => 'https://client.test' } } } - - subject(:set_id_tokens) { processable.id_tokens = new_id_tokens } - - before do - processable.ensure_metadata.save! - end - - it 'does not change metadata.id_tokens' do - expect { set_id_tokens } - .to not_change { processable.metadata.id_tokens } - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'sets the value into metadata.id_tokens' do - 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 } @@ -549,7 +370,7 @@ context 'when metadata exit_code is present' do before do - processable.ensure_metadata.write_attribute(:exit_code, 1) + create(:ci_build_metadata, build: processable, exit_code: 1) end it { is_expected.to eq(1) } @@ -572,7 +393,7 @@ before do processable.write_attribute(:exit_code, existing_exit_code) - processable.ensure_metadata.write_attribute(:exit_code, existing_exit_code) + create(:ci_build_metadata, build: processable, exit_code: existing_exit_code) end it 'does not change job exit_code nor metadata exit_code value' do @@ -603,18 +424,6 @@ 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 @@ -626,58 +435,37 @@ before do processable.job_definition = nil processable.temp_job_definition = nil - processable.ensure_metadata.write_attribute(:interruptible, true) + + create(:ci_build_metadata, build: processable, interruptible: true) end it 'returns metadata interruptible' do expect(interruptible).to be(true) end - - context 'when temp job definition interruptible is present' do - before do - processable.ensure_metadata.write_attribute(:interruptible, nil) - temp_job_definition = Ci::JobDefinition.fabricate( - config: { interruptible: false }, - partition_id: processable.partition_id, - project_id: processable.project_id - ) - processable.temp_job_definition = temp_job_definition - end - - it 'returns temp job definition interruptible' do - expect(interruptible).to be(false) - end - - context 'when job definition interruptible is present' do - before do - processable.build_job_definition.write_attribute(:interruptible, true) - end - - it 'returns job definition interruptible' do - expect(interruptible).to be(true) - end - end - end - end - end - - describe '#interruptible=' do - it 'does not change metadata.interruptible' do - processable.ensure_metadata - - expect { processable.interruptible = true } - .to not_change { processable.metadata.interruptible } end - context 'when FF `stop_writing_builds_metadata` is disabled' do + context 'when temp job definition interruptible is present' do before do - stub_feature_flags(stop_writing_builds_metadata: false) - processable.ensure_metadata + temp_job_definition = Ci::JobDefinition.fabricate( + config: { interruptible: false }, + partition_id: processable.partition_id, + project_id: processable.project_id + ) + processable.temp_job_definition = temp_job_definition + end + + it 'returns temp job definition interruptible' do + expect(interruptible).to be(false) end - it 'sets the value into metadata.interruptible' do - expect { processable.interruptible = true } - .to change { processable.metadata.interruptible } + context 'when job definition interruptible is present' do + before do + processable.build_job_definition.write_attribute(:interruptible, true) + end + + it 'returns job definition interruptible' do + expect(interruptible).to be(true) + end end end end diff --git a/spec/requests/api/ci/runner/jobs_request_post_spec.rb b/spec/requests/api/ci/runner/jobs_request_post_spec.rb index 234b0a8295e521..3edcde1543658e 100644 --- a/spec/requests/api/ci/runner/jobs_request_post_spec.rb +++ b/spec/requests/api/ci/runner/jobs_request_post_spec.rb @@ -1420,65 +1420,6 @@ def request_job(token = runner.token, **params) end end - context 'for web-ide job' do - let_it_be(:user) { create(:user) } - let_it_be(:project) { create(:project, :repository) } - - let(:runner) { create(:ci_runner, :project, projects: [project]) } - let(:service) { ::Ci::CreateWebIdeTerminalService.new(project, user, ref: 'master').execute } - let(:pipeline) { service[:pipeline] } - let(:build) { pipeline.builds.first } - let(:job) { build_stubbed(:ci_build) } - let(:config_content) do - 'terminal: { image: ruby, services: [mysql], before_script: [ls], tags: [tag-1], variables: { KEY: value } }' - end - - before do - # WebIde Terminal features to be removed in https://gitlab.com/gitlab-org/gitlab/-/issues/568849. - # We stub this FF for now to pass the test. - stub_feature_flags(stop_writing_builds_metadata: false) - - stub_webide_config_file(config_content) - project.add_maintainer(user) - - pipeline - end - - context 'when runner has matching tag' do - before do - runner.update!(tag_list: ['tag-1']) - end - - it 'successfully picks job' do - request_job - - build.reload - - expect(build).to be_running - expect(build.runner).to eq(runner) - - expect(response).to have_gitlab_http_status(:created) - expect(json_response).to include( - "id" => build.id, - "variables" => include("key" => 'KEY', "value" => 'value', "public" => true, "masked" => false), - "image" => a_hash_including("name" => 'ruby'), - "services" => all(a_hash_including("name" => 'mysql')), - "job_info" => a_hash_including("name" => 'terminal', "stage" => 'terminal')) - end - end - - context 'when runner does not have matching tags' do - it 'does not pick a job' do - request_job - - build.reload - - expect(build).to be_pending - expect(response).to have_gitlab_http_status(:no_content) - end - end - end - def request_job(token = runner.token, **params) post api('/jobs/request'), params: params.merge(token: token) end diff --git a/spec/serializers/build_details_entity_spec.rb b/spec/serializers/build_details_entity_spec.rb index 400fad79c65c2e..044c6be03969e0 100644 --- a/spec/serializers/build_details_entity_spec.rb +++ b/spec/serializers/build_details_entity_spec.rb @@ -326,7 +326,7 @@ context 'when metadata exists' do before do - stub_feature_flags(stop_writing_builds_metadata: false) + create(:ci_build_metadata, build: build) end it 'returns default values' do diff --git a/spec/services/ci/cancel_pipeline_service_spec.rb b/spec/services/ci/cancel_pipeline_service_spec.rb index afbe71ae1737b7..7152f54be62314 100644 --- a/spec/services/ci/cancel_pipeline_service_spec.rb +++ b/spec/services/ci/cancel_pipeline_service_spec.rb @@ -21,7 +21,6 @@ let(:auto_canceled_by_pipeline) { nil } let(:execute_async) { true } let(:safe_cancellation) { false } - let(:stop_writing_builds_metadata_flag_value) { true } shared_examples 'force_execute' do context 'when pipeline is not cancelable' do @@ -33,8 +32,6 @@ context 'when pipeline is cancelable' do before do - stub_feature_flags(stop_writing_builds_metadata: stop_writing_builds_metadata_flag_value) - create(:ci_build, :running, pipeline: pipeline, name: 'build1') create(:ci_build, :created, pipeline: pipeline, name: 'build2') create(:ci_build, :success, pipeline: pipeline, name: 'build3') diff --git a/spec/services/ci/clone_job_service_spec.rb b/spec/services/ci/clone_job_service_spec.rb index 68816d5554ee1a..0584e9e9a572ba 100644 --- a/spec/services/ci/clone_job_service_spec.rb +++ b/spec/services/ci/clone_job_service_spec.rb @@ -169,7 +169,7 @@ context 'when the job definitions do not exit' do before do - job.ensure_metadata + create(:ci_build_metadata, build: job) Ci::JobDefinitionInstance.delete_all Ci::JobDefinition.delete_all end @@ -181,15 +181,24 @@ end context 'when a job definition for the metadata attributes already exits' do + let(:metadata) do + create(:ci_build_metadata, build: job, + config_options: job.options, + config_variables: job.yaml_variables, + id_tokens: job.id_tokens, + interruptible: job.interruptible + ) + end + let(:config) do { - options: job.metadata.config_options, - yaml_variables: job.metadata.config_variables, - id_tokens: job.metadata.id_tokens, - secrets: job.metadata.secrets, + options: metadata.config_options, + yaml_variables: metadata.config_variables, + id_tokens: metadata.id_tokens, + secrets: metadata.secrets, tag_list: job.tag_list.to_a, run_steps: job.try(:execution_config)&.run_steps || [], - interruptible: job.metadata.interruptible + interruptible: metadata.interruptible } end @@ -202,13 +211,6 @@ end before do - job.ensure_metadata.update!( - config_options: job.options, - config_variables: job.yaml_variables, - id_tokens: job.id_tokens, - interruptible: job.interruptible - ) - Ci::JobDefinitionInstance.delete_all Ci::JobDefinition.fabricate(**attributes).save! job.reload # clear the associated records @@ -284,45 +286,6 @@ it 'does not write to ci_builds_metadata' do expect { new_job }.to not_change { Ci::BuildMetadata.count } end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'persists the expanded environment name in metadata' do - expect { new_job }.to change { Ci::BuildMetadata.count }.by(1) - expect(new_job.metadata.expanded_environment_name).to eq('production') - end - end - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - # The responsibility of linking the new job to the existing persisted environment - # has been moved to Ci::RetryJobService using Ci::Deployable#link_to_environment. - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - context 'when it has a dynamic environment' do - let_it_be(:other_developer) { create(:user, developer_of: project) } - - let(:environment_name) { 'review/$CI_COMMIT_REF_SLUG-$GITLAB_USER_ID' } - - let!(:job) do - create(:ci_build, :with_deployment, - environment: environment_name, - options: { environment: { name: environment_name } }, - pipeline: pipeline, stage_id: stage.id, project: project, - user: other_developer) - end - - it 're-uses the previous persisted environment' do - expect(job.persisted_environment.name).to eq("review/#{job.ref}-#{other_developer.id}") - - expect(new_job.persisted_environment.name).to eq("review/#{job.ref}-#{other_developer.id}") - end - end end context 'when the job has job variables' do @@ -333,21 +296,6 @@ end end - # Remove with stop_writing_builds_metadata - context 'when writing to builds metadata' do - before do - job.clear_memoization(:can_write_metadata?) - job.ensure_metadata.update!(interruptible: true) - - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'clones the interruptible job attribute' do - expect(new_job.interruptible).not_to be_nil - expect(new_job.interruptible).to eq job.interruptible - end - end - context 'when build execution config is given' do let(:build_execution_config) { create(:ci_builds_execution_configs, pipeline: pipeline) } diff --git a/spec/services/ci/create_pipeline_service/job_definition_spec.rb b/spec/services/ci/create_pipeline_service/job_definition_spec.rb index 50fe830c0e11e9..11f39e9232f22e 100644 --- a/spec/services/ci/create_pipeline_service/job_definition_spec.rb +++ b/spec/services/ci/create_pipeline_service/job_definition_spec.rb @@ -75,18 +75,6 @@ expect(job.metadata).to be_nil end end - - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'also saves metadata for jobs' do - pipeline.processables.each do |job| - expect(job.metadata).not_to be_nil - end - end - end end context 'when there are jobs with run steps' do diff --git a/spec/services/ci/create_pipeline_service/partitioning_spec.rb b/spec/services/ci/create_pipeline_service/partitioning_spec.rb index 30aa902900942b..202cf95548d707 100644 --- a/spec/services/ci/create_pipeline_service/partitioning_spec.rb +++ b/spec/services/ci/create_pipeline_service/partitioning_spec.rb @@ -62,25 +62,6 @@ expect(processables_partition_ids).to eq([current_partition_id]) end - context 'when the FF stop_writing_builds_metadata is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'assigns partition_id to metadata' do - metadata_partition_ids = pipeline.processables.map { |job| job.metadata.partition_id }.uniq - - expect(metadata_partition_ids).to eq([current_partition_id]) - end - - it 'correctly assigns partition and environment' do - metadata = find_metadata('deploy') - - expect(metadata.partition_id).to eq(current_partition_id) - expect(metadata.expanded_environment_name).to eq('review/deploy') - end - end - context 'with pipeline variables' do let(:variables_attributes) do [ diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index a1691a2927e7cf..089ead93bb239a 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -264,7 +264,6 @@ def execute_service( context 'interruptible builds' do before do - stub_feature_flags(stop_writing_builds_metadata: false) stub_ci_pipeline_yaml_file(YAML.dump(config)) end @@ -301,21 +300,19 @@ def execute_service( } end - # Remove metadata interruptible with stop_writing_builds_metadata it 'properly configures interruptible status' do interruptible_status = pipeline_on_previous_commit .builds - .joins(:metadata) .joins(:job_definition) - .pluck(:name, "#{Ci::BuildMetadata.quoted_table_name}.interruptible", "#{Ci::JobDefinition.quoted_table_name}.interruptible") + .pluck(:name, "#{Ci::JobDefinition.quoted_table_name}.interruptible") expect(interruptible_status).to contain_exactly( - ['build_1_1', true, true], - ['build_1_2', true, true], - ['build_2_1', true, true], - ['build_3_1', false, false], - ['build_4_1', nil, false] + ['build_1_1', true], + ['build_1_2', true], + ['build_2_1', true], + ['build_3_1', false], + ['build_4_1', false] ) end @@ -882,15 +879,6 @@ def previous_commit_sha_from_ref(ref) it_behaves_like 'when resource group is defined for review app deployment' - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it_behaves_like 'with resource group' - it_behaves_like 'when resource group is defined for review app deployment' - end - context 'with timeout' do context 'when builds with custom timeouts are configured' do before do @@ -1819,16 +1807,6 @@ def previous_commit_sha_from_ref(ref) it 'does not write to ci_builds_metadata' do expect { execute_service }.to not_change { Ci::BuildMetadata.count } end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'writes to ci_builds_metadata' do - expect { execute_service }.to change { Ci::BuildMetadata.count }.by(1) - end - end end end diff --git a/spec/services/ci/create_web_ide_terminal_service_spec.rb b/spec/services/ci/create_web_ide_terminal_service_spec.rb index 94a74df32c6929..596881f2780751 100644 --- a/spec/services/ci/create_web_ide_terminal_service_spec.rb +++ b/spec/services/ci/create_web_ide_terminal_service_spec.rb @@ -37,75 +37,6 @@ subject end end - - before do - project.add_maintainer(user) - end - - context 'when web-ide has valid configuration' do - before do - # WebIde Terminal features to be removed in https://gitlab.com/gitlab-org/gitlab/-/issues/568849. - # We stub this FF for now to pass the test. - stub_feature_flags(stop_writing_builds_metadata: false) - - stub_webide_config_file(config_content) - end - - context 'for empty configuration' do - let(:config_content) do - 'terminal: {}' - end - - it_behaves_like 'be successful' - end - - context 'for configuration with container image' do - let(:config_content) do - 'terminal: { image: ruby }' - end - - it_behaves_like 'be successful' - end - - context 'for configuration with ports' do - let(:config_content) do - <<-EOS - terminal: - image: - name: image:1.0 - ports: - - 80 - script: rspec - services: - - name: test - alias: test - ports: - - 8080 - EOS - end - - it_behaves_like 'be successful' - end - - context 'for configuration with variables' do - let(:config_content) do - <<-EOS - terminal: - script: rspec - variables: - KEY1: VAL1 - EOS - end - - it_behaves_like 'be successful' - - it 'saves the variables' do - expect(subject[:pipeline].builds[0].variables).to include( - key: 'KEY1', value: 'VAL1', public: true, masked: false - ) - end - end - end end context 'error handling' do diff --git a/spec/services/ci/pipelines/add_job_service_spec.rb b/spec/services/ci/pipelines/add_job_service_spec.rb index 551b6066a43700..6010259c9a7181 100644 --- a/spec/services/ci/pipelines/add_job_service_spec.rb +++ b/spec/services/ci/pipelines/add_job_service_spec.rb @@ -26,22 +26,6 @@ end end - context 'when the metadata exists' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'assigns project to metadata' do - expect { execute }.to change { job.metadata.project }.to(pipeline.project) - end - - it 'assigns partition_id to job and metadata' do - expect { execute } - .to change(job, :partition_id).to(pipeline.partition_id) - .and change { job.metadata.partition_id }.to(pipeline.partition_id) - end - end - it 'assigns pipeline attributes to the job' do expect { execute } .to change { job.slice(:pipeline, :project, :ref) } diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index cf643fa23a2d70..96f62716bc2b37 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -27,16 +27,6 @@ it 'does not write to ci_builds_metadata' do expect { service.execute(pipeline) }.to not_change { Ci::BuildMetadata.count } end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'writes to ci_build_metadata' do - expect { service.execute(pipeline) }.to change { Ci::BuildMetadata.count }.by_at_least(1) - end - end end context 'when there are already retried jobs present' do diff --git a/spec/support/shared_examples/ci/deployable_shared_examples.rb b/spec/support/shared_examples/ci/deployable_shared_examples.rb index 17db7051ff3e23..208fb0f4fd2470 100644 --- a/spec/support/shared_examples/ci/deployable_shared_examples.rb +++ b/spec/support/shared_examples/ci/deployable_shared_examples.rb @@ -614,7 +614,8 @@ context 'when job metadata has already persisted the expanded environment name' do before do - job.ensure_metadata.expanded_environment_name = 'review/foo' + create(:ci_build_metadata, build: job, expanded_environment_name: 'review/foo') + job.reload_metadata end it 'returns a persisted expanded environment name without a list of variables' do @@ -672,7 +673,7 @@ context 'when the job metadata has the namespace persisted' do before do - job.ensure_metadata.expanded_environment_name = 'name-from-metadata' + create(:ci_build_metadata, build: job, expanded_environment_name: 'name-from-metadata') end it { is_expected.to eq('name-from-job-env') } diff --git a/spec/support/shared_examples/environments/create_for_job_shared_examples.rb b/spec/support/shared_examples/environments/create_for_job_shared_examples.rb index afdfdeeabcc7be..a9a6d2aafbda50 100644 --- a/spec/support/shared_examples/environments/create_for_job_shared_examples.rb +++ b/spec/support/shared_examples/environments/create_for_job_shared_examples.rb @@ -390,27 +390,6 @@ expect(environment.merge_request).to eq(merge_request) end end - - context 'when job metadata exists' do - before do - job.ensure_metadata - end - - it 'does not change metadata.expanded_environment_name' do - expect { subject }.to not_change { job.metadata.expanded_environment_name } - end - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - job.ensure_metadata - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'sets the expanded environment name in metadata' do - expect { subject }.to change { job.metadata.expanded_environment_name }.to('review/master') - end - end end context 'when an environment already exists' do @@ -435,27 +414,6 @@ expect(environment.merge_request).to be_nil end end - - context 'when job metadata exists' do - before do - job.ensure_metadata - end - - it 'does not change metadata.expanded_environment_name' do - expect { subject }.to not_change { job.metadata.expanded_environment_name } - end - end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - job.ensure_metadata - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'sets the expanded environment name in metadata' do - expect { subject }.to change { job.metadata.expanded_environment_name }.to('review/master') - end - end end context 'when an environment name contains an invalid character' do @@ -486,24 +444,13 @@ context 'when job metadata exists' do before do - job.ensure_metadata + job.metadata = build(:ci_build_metadata, project: project) end it 'does not change metadata.expanded_environment_name' do expect { subject }.to not_change { job.metadata.expanded_environment_name } end end - - context 'when FF `stop_writing_builds_metadata` is disabled' do - before do - job.ensure_metadata - stub_feature_flags(stop_writing_builds_metadata: false) - end - - it 'sets the expanded environment name in metadata' do - expect { subject }.to change { job.metadata.expanded_environment_name }.to('review/master') - end - end end context 'when a pipeline does not contain a deployment job' do -- GitLab From be52d9a2a0169e49dcba3d7859b5eeff8ce38f90 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Fri, 10 Oct 2025 11:39:00 +0200 Subject: [PATCH 02/11] Fix more tests --- spec/models/ci/pipeline_spec.rb | 2 +- spec/models/concerns/chronic_duration_attribute_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 32984d5139a43e..ad126de83fc73e 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -4292,7 +4292,7 @@ def create_pipeline(status, ref, sha) before do job.job_environment.destroy! - job.ensure_metadata.update!(expanded_environment_name: job.expanded_environment_name) + create(:ci_build_metadata, build: job, expanded_environment_name: job.expanded_environment_name) end it 'includes environments from both sources' do diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb index 61b86455840bbb..e5d3cb9cd97588 100644 --- a/spec/models/concerns/chronic_duration_attribute_spec.rb +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -121,7 +121,7 @@ let(:source_field) { :timeout } let(:virtual_field) { :timeout_human_readable } - subject { create(:ci_build).ensure_metadata } + subject { create(:ci_build_metadata) } it "doesn't contain dynamically created writer method" do expect(subject.class).not_to be_public_method_defined("#{virtual_field}=") -- GitLab From e02fdb6ad871862844e12748c7aa80692963f6d8 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Fri, 10 Oct 2025 11:46:49 +0200 Subject: [PATCH 03/11] Fix more tests --- .../shared_examples/ci/degenerable_job_shared_examples.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/shared_examples/ci/degenerable_job_shared_examples.rb b/spec/support/shared_examples/ci/degenerable_job_shared_examples.rb index 4e73317480d200..16478ce2d11bea 100644 --- a/spec/support/shared_examples/ci/degenerable_job_shared_examples.rb +++ b/spec/support/shared_examples/ci/degenerable_job_shared_examples.rb @@ -40,7 +40,7 @@ describe '#degenerate!' do before do - job.ensure_metadata + create(:ci_build_metadata, build: job) job.needs.create!(name: 'another-job') end -- GitLab From ffb17f79f2dbd15fb707cc8cecdd0441210cbce1 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Wed, 22 Oct 2025 11:51:21 +0200 Subject: [PATCH 04/11] Keep scoped_user_id update --- spec/finders/ci/auth_job_finder_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/finders/ci/auth_job_finder_spec.rb b/spec/finders/ci/auth_job_finder_spec.rb index bd6cc6cd8df227..f96297d3fb3175 100644 --- a/spec/finders/ci/auth_job_finder_spec.rb +++ b/spec/finders/ci/auth_job_finder_spec.rb @@ -29,6 +29,10 @@ context 'when job has a `scoped_user_id` tracked' do let(:scoped_user) { create(:user) } + before do + job.update!(scoped_user_id: scoped_user.id) + end + context 'when job user does not support composite identity' do it 'does not link the scoped user as composite identity' do execute -- GitLab From 976ed0146b75d82f220fa415081b1e7df6cab3b7 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Wed, 22 Oct 2025 11:53:47 +0200 Subject: [PATCH 05/11] Raise errors when updating metadatable options --- app/models/concerns/ci/metadatable.rb | 26 ++++++++++++++ spec/models/concerns/ci/metadatable_spec.rb | 38 +++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/app/models/concerns/ci/metadatable.rb b/app/models/concerns/ci/metadatable.rb index 4f637622352e58..32de31879fbd3a 100644 --- a/app/models/concerns/ci/metadatable.rb +++ b/app/models/concerns/ci/metadatable.rb @@ -163,6 +163,32 @@ def exit_code=(value) write_attribute(:exit_code, safe_value) end + # Should be removed when the column is dropped from p_ci_builds + def options=(value) + raise ActiveRecord::ReadonlyAttributeError, 'This data is read only' unless value.nil? + + super + end + + # Should be removed when the column is dropped from p_ci_builds + def yaml_variables=(value) + raise ActiveRecord::ReadonlyAttributeError, 'This data is read only' unless value.nil? + + super + end + + def interruptible=(_value) + raise ActiveRecord::ReadonlyAttributeError, 'This data is read only' + end + + def id_tokens=(_value) + raise ActiveRecord::ReadonlyAttributeError, 'This data is read only' + end + + def secrets=(_value) + raise ActiveRecord::ReadonlyAttributeError, 'This data is read only' + end + private def read_metadata_attribute(legacy_key, metadata_key, job_definition_key, default_value = nil) diff --git a/spec/models/concerns/ci/metadatable_spec.rb b/spec/models/concerns/ci/metadatable_spec.rb index 9426c0eb4e4fac..623588373ec891 100644 --- a/spec/models/concerns/ci/metadatable_spec.rb +++ b/spec/models/concerns/ci/metadatable_spec.rb @@ -469,4 +469,42 @@ end end end + + describe '#options=' do + it 'raises an error when overriding data' do + expect { processable.options = { a: :b } }.to raise_error ActiveRecord::ReadonlyAttributeError + end + + it 'allows nullifying data' do + expect { processable.options = nil }.not_to raise_error + end + end + + describe '#yaml_variables=' do + it 'raises an error when overriding data' do + expect { processable.yaml_variables = { a: :b } }.to raise_error ActiveRecord::ReadonlyAttributeError + end + + it 'allows nullifying data' do + expect { processable.yaml_variables = nil }.not_to raise_error + end + end + + describe '#interruptible=' do + it 'raises an error when overriding data' do + expect { processable.interruptible = true }.to raise_error ActiveRecord::ReadonlyAttributeError + end + end + + describe '#id_tokens=' do + it 'raises an error when overriding data' do + expect { processable.id_tokens = { a: :b } }.to raise_error ActiveRecord::ReadonlyAttributeError + end + end + + describe '#secrets=' do + it 'raises an error when overriding data' do + expect { processable.secrets = { a: :b } }.to raise_error ActiveRecord::ReadonlyAttributeError + end + end end -- GitLab From bc8051cb80b39de6968c8c99bf823a8f35cf0ee6 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Wed, 22 Oct 2025 12:03:05 +0200 Subject: [PATCH 06/11] Remove scoped user expectation --- ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb b/ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb index c0cb126f88a43b..504413320fb2c9 100644 --- a/ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb +++ b/ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb @@ -31,7 +31,6 @@ context 'when pipeline user supports composite identity' do it 'does not propagate composite identity if composite user is not linked' do - expect(seed_attributes[:temp_job_definition].config[:options].key?(:scoped_user_id)).to be(false) expect(seed_attributes.key?(:scoped_user_id)).to be(false) end -- GitLab From 0d66d806d22254e139ac0749bf0a56512cbab81a Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Wed, 22 Oct 2025 14:55:20 +0200 Subject: [PATCH 07/11] Fix fabricate method --- app/models/ci/processable.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/processable.rb b/app/models/ci/processable.rb index af3ab27c512122..72305f139a5a4f 100644 --- a/app/models/ci/processable.rb +++ b/app/models/ci/processable.rb @@ -150,7 +150,7 @@ class Processable < ::CommitStatus end def self.fabricate(attrs) - new(attrs).tap do |build| + new(attrs.except(*::Ci::JobDefinition::CONFIG_ATTRIBUTES)).tap do |build| job_definition = ::Ci::JobDefinition.fabricate( config: attrs, project_id: build.project_id, -- GitLab From 05fd7790cc23094c511cc50889ff1f40c9076d82 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Wed, 22 Oct 2025 18:02:15 +0200 Subject: [PATCH 08/11] Fix more tests --- spec/support/shared_examples/ci/deployable_shared_examples.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/shared_examples/ci/deployable_shared_examples.rb b/spec/support/shared_examples/ci/deployable_shared_examples.rb index 208fb0f4fd2470..b6bf2163452fbd 100644 --- a/spec/support/shared_examples/ci/deployable_shared_examples.rb +++ b/spec/support/shared_examples/ci/deployable_shared_examples.rb @@ -499,7 +499,7 @@ context 'when options does not include deployment_tier' do let(:options) { { environment: { name: 'production' } } } - let(:job) { described_class.new(options: options, environment: 'production', project: project) } + let(:job) { FactoryBot.build(factory_type, options: options, environment: 'production', project: project) } it 'uses tier from environment' do is_expected.to eq('development') -- GitLab From d7f75bf62da25999e1d7bbaa46a26bd8e2b23079 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Thu, 23 Oct 2025 13:26:38 +0200 Subject: [PATCH 09/11] Fix pipeline fixtures --- db/fixtures/development/14_pipelines.rb | 42 +++++++++++++++++++++---- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb index e02b55786ad4d3..58c7567a5e29f2 100644 --- a/db/fixtures/development/14_pipelines.rb +++ b/db/fixtures/development/14_pipelines.rb @@ -56,7 +56,7 @@ class Gitlab::Seeder::Pipelines # rubocop:disable Style/ClassAndModuleChildren position: 2, builds: [ { name: 'staging', environment: 'staging', status_event: :success, - options: { environment: { action: 'start', on_stop: 'stop staging' } }, + options: { environment: { action: 'start' } }, queued_at: 7.hours.ago, started_at: 6.hours.ago, finished_at: 4.hours.ago }, { name: 'stop staging', environment: 'staging', when: 'manual', status: :skipped }, { name: 'production', environment: 'production', when: 'manual', status: :skipped } @@ -195,26 +195,43 @@ def create_pipeline!(project, ref, commit) def build_create!(pipeline, stage, opts = {}) attributes = job_attributes(pipeline, stage, opts) - attributes[:options] ||= {} attributes[:options][:script] = 'build command' + environment_attrs = attributes[:options].extract!(:environment) + definition_attrs = attributes.extract!(*Ci::JobDefinition::CONFIG_ATTRIBUTES) + definition = find_or_create_job_definition(definition_attrs, pipeline) Ci::Build.create!(attributes).tap do |build| # We need to set build trace and artifacts after saving a build # (id required), that is why we need `#tap` method instead of passing # block directly to `Ci::Build#create!`. - + build.create_job_definition_instance!(job_definition: definition, project: pipeline.project) setup_artifacts(build) setup_test_reports(build) setup_build_log(build) - - build.project.environments - .find_or_create_by(name: build.expanded_environment_name) + setup_environment(build, environment_attrs) build.save! end end + def find_or_create_job_definition(attributes, pipeline) + new_definition = Ci::JobDefinition.fabricate( + config: attributes, + project_id: pipeline.project_id, + partition_id: pipeline.partition_id) + + existing_defintion = Ci::JobDefinition.find_by( + checksum: new_definition.checksum, + project_id: new_definition.project_id, + partition_id: new_definition.partition_id) + + return existing_defintion if existing_defintion + + new_definition.save! + new_definition + end + def setup_artifacts(build) return unless build.ci_stage.name == 'build' @@ -249,6 +266,19 @@ def setup_build_log(build) end end + def setup_environment(build, attrs) + return unless build.has_environment_keyword? + + environment = build.project.environments.find_or_create_by!(name: build.environment) + build.create_job_environment!( + environment: environment, + expanded_environment_name: environment.name, + project: build.project, + pipeline: build.pipeline, + options: attrs.fetch(:environment, {}) + ) + end + def generic_commit_status_create!(pipeline, stage, opts = {}) attributes = job_attributes(pipeline, stage, opts) -- GitLab From 355a64dba0c3b6deb055ea3c7e999d17944de938 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Thu, 23 Oct 2025 13:35:08 +0200 Subject: [PATCH 10/11] Apply reviewer feedback --- app/models/concerns/ci/metadatable.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/concerns/ci/metadatable.rb b/app/models/concerns/ci/metadatable.rb index 32de31879fbd3a..7fbf0671c985e0 100644 --- a/app/models/concerns/ci/metadatable.rb +++ b/app/models/concerns/ci/metadatable.rb @@ -164,6 +164,7 @@ def exit_code=(value) end # Should be removed when the column is dropped from p_ci_builds + # allows deleting data for `degenerate!` def options=(value) raise ActiveRecord::ReadonlyAttributeError, 'This data is read only' unless value.nil? @@ -171,6 +172,7 @@ def options=(value) end # Should be removed when the column is dropped from p_ci_builds + # allows deleting data for `degenerate!` def yaml_variables=(value) raise ActiveRecord::ReadonlyAttributeError, 'This data is read only' unless value.nil? -- GitLab From a39898e7d7a93a4ffe932888398ff16b87bbafa1 Mon Sep 17 00:00:00 2001 From: Marius Bobin Date: Thu, 23 Oct 2025 14:15:54 +0200 Subject: [PATCH 11/11] Save tag list to both destinations --- app/models/ci/processable.rb | 8 ++++++-- spec/models/ci/build_spec.rb | 27 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/app/models/ci/processable.rb b/app/models/ci/processable.rb index 72305f139a5a4f..2148b66f5a4034 100644 --- a/app/models/ci/processable.rb +++ b/app/models/ci/processable.rb @@ -150,9 +150,13 @@ class Processable < ::CommitStatus end def self.fabricate(attrs) - new(attrs.except(*::Ci::JobDefinition::CONFIG_ATTRIBUTES)).tap do |build| + attrs = attrs.dup + definition_attrs = attrs.extract!(*Ci::JobDefinition::CONFIG_ATTRIBUTES) + attrs[:tag_list] = definition_attrs[:tag_list] if definition_attrs.key?(:tag_list) + + new(attrs).tap do |build| job_definition = ::Ci::JobDefinition.fabricate( - config: attrs, + config: definition_attrs, project_id: build.project_id, partition_id: build.partition_id ) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 7427bca292fe9d..0f29ade8b7adce 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -6033,4 +6033,31 @@ def run_job_without_exception it { is_expected.to eq(expected_token) } end end + + describe '.fabricate' do + let(:tag_list) { %w[ruby docker postgres] } + let(:build_attributes) do + { + options: { script: ['echo'] }, + tag_list: tag_list, + project_id: 1, + partition_id: 99 + } + end + + subject(:fabricate) { described_class.fabricate(build_attributes) } + + it 'initializes with temp_job_definition' do + expect(fabricate).to have_attributes( + temp_job_definition: instance_of(Ci::JobDefinition), + job_definition: nil, + tag_list: tag_list + ) + definition = fabricate.temp_job_definition + + expect(definition.config).to eq({ options: build_attributes[:options], tag_list: tag_list }) + expect(definition.project_id).to eq(build_attributes[:project_id]) + expect(definition.partition_id).to eq(build_attributes[:partition_id]) + end + end end -- GitLab