diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 2b72a4c4c89c458d4dfa497cf8be2f89f4b21f34..87adedf936e05a0e868f1704cb4158282f2c3b6f 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -194,7 +194,7 @@ def persisted_environment=(environment)
after_save :stick_build_if_status_changed
after_create unless: :importing? do |build|
- run_after_commit { BuildHooksWorker.perform_async(build) }
+ run_after_commit { build.feature_flagged_execute_hooks }
end
class << self
@@ -285,7 +285,7 @@ def clone_accessors
build.run_after_commit do
BuildQueueWorker.perform_async(id)
- BuildHooksWorker.perform_async(build)
+ build.feature_flagged_execute_hooks
end
end
@@ -313,7 +313,7 @@ def clone_accessors
build.run_after_commit do
build.ensure_persistent_ref
- BuildHooksWorker.perform_async(build)
+ build.feature_flagged_execute_hooks
end
end
@@ -780,10 +780,20 @@ def stuck?
pending? && !any_runners_online?
end
+ def feature_flagged_execute_hooks
+ if Feature.enabled?(:execute_build_hooks_inline, project)
+ execute_hooks
+ else
+ BuildHooksWorker.perform_async(self)
+ end
+ end
+
def execute_hooks
return unless project
return if user&.blocked?
+ ActiveRecord::Associations::Preloader.new.preload([self], { runner: :tags })
+
project.execute_hooks(build_data.dup, :job_hooks) if project.has_active_hooks?(:job_hooks)
project.execute_integrations(build_data.dup, :job_hooks) if project.has_active_integrations?(:job_hooks)
end
diff --git a/app/models/project.rb b/app/models/project.rb
index 28dd51fc67d61ce560e415b3507067b856669cc8..b8a2fb13f9aa22a3a20877e5e7b20ccfeae600b1 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1682,7 +1682,13 @@ def execute_integrations(data, hooks_scope = :push_hooks)
end
def has_active_hooks?(hooks_scope = :push_hooks)
- hooks.hooks_for(hooks_scope).any? || SystemHook.hooks_for(hooks_scope).any? || Gitlab::FileHook.any?
+ @has_active_hooks ||= {} # rubocop: disable Gitlab/PredicateMemoization
+
+ return @has_active_hooks[hooks_scope] if @has_active_hooks.key?(hooks_scope)
+
+ @has_active_hooks[hooks_scope] = hooks.hooks_for(hooks_scope).any? ||
+ SystemHook.hooks_for(hooks_scope).any? ||
+ Gitlab::FileHook.any?
end
def has_active_integrations?(hooks_scope = :push_hooks)
diff --git a/app/workers/build_hooks_worker.rb b/app/workers/build_hooks_worker.rb
index 5c08344bfe3ece6b86bdb4cd65fd71c698f17c15..2c62aed72d6f36f121a071a650eb074e98969fff 100644
--- a/app/workers/build_hooks_worker.rb
+++ b/app/workers/build_hooks_worker.rb
@@ -13,9 +13,9 @@ class BuildHooksWorker # rubocop:disable Scalability/IdempotentWorker
# rubocop: disable CodeReuse/ActiveRecord
def perform(build_id)
- Ci::Build.includes({ runner: :tags })
- .find_by_id(build_id)
- .try(:execute_hooks)
+ build = Ci::Build.find_by_id(build_id)
+
+ build.execute_hooks if build
end
# rubocop: enable CodeReuse/ActiveRecord
diff --git a/app/workers/ci/build_finished_worker.rb b/app/workers/ci/build_finished_worker.rb
index 25c7637a79f146f8cc659e95259743d35c972705..36a50735fed2b2ab9933a4210663324ed236a176 100644
--- a/app/workers/ci/build_finished_worker.rb
+++ b/app/workers/ci/build_finished_worker.rb
@@ -36,8 +36,7 @@ def process_build(build)
build.update_coverage
Ci::BuildReportResultService.new.execute(build)
- # We execute these async as these are independent operations.
- BuildHooksWorker.perform_async(build)
+ build.feature_flagged_execute_hooks
ChatNotificationWorker.perform_async(build.id) if build.pipeline.chat?
build.track_deployment_usage
build.track_verify_usage
diff --git a/config/feature_flags/development/execute_build_hooks_inline.yml b/config/feature_flags/development/execute_build_hooks_inline.yml
new file mode 100644
index 0000000000000000000000000000000000000000..0389fca3bb1b556a69e2a1ebf888fdaece9e7df0
--- /dev/null
+++ b/config/feature_flags/development/execute_build_hooks_inline.yml
@@ -0,0 +1,8 @@
+---
+name: execute_build_hooks_inline
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/93665
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/370387
+milestone: '15.3'
+type: development
+group: group::integrations
+default_enabled: false
diff --git a/ee/spec/workers/ee/ci/build_finished_worker_spec.rb b/ee/spec/workers/ee/ci/build_finished_worker_spec.rb
index dffe4f85ca86fd2327b4c3631ea757244ec8a969..54f1a1afe2d905cab08ec9083a98e9a77a8b7111 100644
--- a/ee/spec/workers/ee/ci/build_finished_worker_spec.rb
+++ b/ee/spec/workers/ee/ci/build_finished_worker_spec.rb
@@ -4,7 +4,7 @@
RSpec.describe Ci::BuildFinishedWorker do
let_it_be(:ci_runner) { create(:ci_runner) }
- let_it_be(:build) { create(:ee_ci_build, :sast, :success, runner: ci_runner) }
+ let_it_be_with_reload(:build) { create(:ee_ci_build, :sast, :success, runner: ci_runner) }
let_it_be(:project) { build.project }
let_it_be(:namespace) { project.shared_runners_limit_namespace }
@@ -34,13 +34,10 @@ def project_stats
end
context 'when exception is raised in `super`' do
- before do
- allow(::BuildHooksWorker)
- .to receive(:perform_async)
- .and_raise(ArgumentError)
- end
-
it 'does not enqueue the worker in EE' do
+ allow(Ci::Build).to receive(:find_by_id).with(build.id).and_return(build)
+ allow(build).to receive(:execute_hooks).and_raise(ArgumentError)
+
expect { subject }.to raise_error(ArgumentError)
expect(::Security::TrackSecureScansWorker).not_to receive(:perform_async)
@@ -105,14 +102,6 @@ def project_stats
project.add_reporter(user)
end
- shared_examples 'schedules processing of requirement reports' do
- it do
- expect(RequirementsManagement::ProcessRequirementsReportsWorker).to receive(:perform_async)
-
- subject
- end
- end
-
shared_examples 'does not schedule processing of requirement reports' do
it do
expect(RequirementsManagement::ProcessRequirementsReportsWorker).not_to receive(:perform_async)
@@ -126,7 +115,11 @@ def project_stats
stub_licensed_features(requirements: true)
end
- it_behaves_like 'schedules processing of requirement reports'
+ it 'schedules processing of requirement reports' do
+ expect(RequirementsManagement::ProcessRequirementsReportsWorker).to receive(:perform_async)
+
+ subject
+ end
context 'when user has insufficient permissions to create test reports' do
before do
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 546cfd80fcf519f36c2965f121d1b3bfea0eb6d3..8b1d27687542d853a5270347a5beef40f21fa5c4 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -4,6 +4,8 @@
RSpec.describe Ci::Build do
include Ci::TemplateHelpers
+ include AfterNextHelpers
+
let_it_be(:user) { create(:user) }
let_it_be(:group, reload: true) { create(:group) }
let_it_be(:project, reload: true) { create(:project, :repository, group: group) }
@@ -60,11 +62,36 @@
describe 'callbacks' do
context 'when running after_create callback' do
- it 'triggers asynchronous build hooks worker' do
- expect(BuildHooksWorker).to receive(:perform_async)
+ it 'executes hooks' do
+ expect_next(described_class).to receive(:execute_hooks)
create(:ci_build)
end
+
+ context 'when the execute_build_hooks_inline flag is disabled' do
+ before do
+ stub_feature_flags(execute_build_hooks_inline: false)
+ end
+
+ it 'uses the old job hooks worker' do
+ expect(::BuildHooksWorker).to receive(:perform_async).with(Ci::Build)
+
+ create(:ci_build)
+ end
+ end
+
+ context 'when the execute_build_hooks_inline flag is enabled for a project' do
+ before do
+ stub_feature_flags(execute_build_hooks_inline: project)
+ end
+
+ it 'executes hooks inline' do
+ expect(::BuildHooksWorker).not_to receive(:perform_async)
+ expect_next(described_class).to receive(:execute_hooks)
+
+ create(:ci_build, project: project)
+ end
+ end
end
end
@@ -1663,20 +1690,18 @@
end
it 'returns an expanded environment name with a list of variables' do
- expect(build).to receive(:simple_variables).once.and_call_original
-
is_expected.to eq('review/host')
end
context 'when build metadata has already persisted the expanded environment name' do
before do
- build.metadata.expanded_environment_name = 'review/host'
+ build.metadata.expanded_environment_name = 'review/foo'
end
it 'returns a persisted expanded environment name without a list of variables' do
expect(build).not_to receive(:simple_variables)
- is_expected.to eq('review/host')
+ is_expected.to eq('review/foo')
end
end
end
@@ -3872,8 +3897,20 @@
build.enqueue
end
- it 'queues BuildHooksWorker' do
- expect(BuildHooksWorker).to receive(:perform_async).with(build)
+ context 'when the execute_build_hooks_inline flag is disabled' do
+ before do
+ stub_feature_flags(execute_build_hooks_inline: false)
+ end
+
+ it 'queues BuildHooksWorker' do
+ expect(BuildHooksWorker).to receive(:perform_async).with(build)
+
+ build.enqueue
+ end
+ end
+
+ it 'executes hooks' do
+ expect(build).to receive(:execute_hooks)
build.enqueue
end
@@ -4624,6 +4661,7 @@ def run_job_without_exception
end
before do
+ allow(build).to receive(:execute_hooks)
stub_artifacts_object_storage
end
@@ -5564,7 +5602,7 @@ def run_job_without_exception
build.cancel_gracefully?
end
- let_it_be(:build) { create(:ci_build, pipeline: pipeline) }
+ let(:build) { create(:ci_build, pipeline: pipeline) }
it 'cannot cancel gracefully' do
expect(subject).to be false
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 2920278679ff0ec8f81595f6fe81af11963ae7d7..0d11586c487ed819ddd43a40a9672664db2dbac4 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -5889,7 +5889,7 @@ def has_external_wiki
end
describe '#has_active_hooks?' do
- let_it_be(:project) { create(:project) }
+ let_it_be_with_refind(:project) { create(:project) }
it { expect(project.has_active_hooks?).to be_falsey }
diff --git a/spec/workers/build_hooks_worker_spec.rb b/spec/workers/build_hooks_worker_spec.rb
index 426eb03638c22689a127e6442528cb908d63df81..80dc36d268faa79b1b5e9c41dd4071df7770b164 100644
--- a/spec/workers/build_hooks_worker_spec.rb
+++ b/spec/workers/build_hooks_worker_spec.rb
@@ -23,8 +23,8 @@
end
end
- describe '.perform_async' do
- it 'sends a message to the application logger, before performing', :sidekiq_inline do
+ describe '.perform_async', :sidekiq_inline do
+ it 'sends a message to the application logger, before performing' do
build = create(:ci_build)
expect(Gitlab::AppLogger).to receive(:info).with(
diff --git a/spec/workers/ci/build_finished_worker_spec.rb b/spec/workers/ci/build_finished_worker_spec.rb
index 201182636e7ee06c2cd8ad7716927892e5438036..5ddaabc3938e2b45f1650a00e97706ac7667f0d8 100644
--- a/spec/workers/ci/build_finished_worker_spec.rb
+++ b/spec/workers/ci/build_finished_worker_spec.rb
@@ -3,6 +3,8 @@
require 'spec_helper'
RSpec.describe Ci::BuildFinishedWorker do
+ include AfterNextHelpers
+
subject { described_class.new.perform(build.id) }
describe '#perform' do
@@ -16,17 +18,28 @@
it 'calculates coverage and calls hooks', :aggregate_failures do
expect(build).to receive(:update_coverage).ordered
- expect_next_instance_of(Ci::BuildReportResultService) do |build_report_result_service|
- expect(build_report_result_service).to receive(:execute).with(build)
- end
+ expect_next(Ci::BuildReportResultService).to receive(:execute).with(build)
- expect(BuildHooksWorker).to receive(:perform_async)
+ expect(build).to receive(:execute_hooks)
expect(ChatNotificationWorker).not_to receive(:perform_async)
expect(Ci::ArchiveTraceWorker).to receive(:perform_in)
subject
end
+ context 'when the execute_build_hooks_inline feature flag is disabled' do
+ before do
+ stub_feature_flags(execute_build_hooks_inline: false)
+ end
+
+ it 'uses the BuildHooksWorker' do
+ expect(build).not_to receive(:execute_hooks)
+ expect(BuildHooksWorker).to receive(:perform_async).with(build)
+
+ subject
+ end
+ end
+
context 'when build is failed' do
before do
build.update!(status: :failed)