diff --git a/config/puma.rb b/config/puma.rb index a50c771..99c7839 100644 --- a/config/puma.rb +++ b/config/puma.rb @@ -24,6 +24,8 @@ pidfile 'puma.pid' before_fork do + ActiveRecord::Base.connection_pool.disconnect! + Thread.new do loop do Telemetry.instance.update_stats Puma.stats @@ -32,3 +34,7 @@ end end end + +on_worker_boot do + ActiveRecord::Base.establish_connection +end diff --git a/db/migrate/20260526000001_add_indexes_to_delayed_jobs.rb b/db/migrate/20260526000001_add_indexes_to_delayed_jobs.rb new file mode 100644 index 0000000..d4526e5 --- /dev/null +++ b/db/migrate/20260526000001_add_indexes_to_delayed_jobs.rb @@ -0,0 +1,32 @@ +# SPDX-License-Identifier: BSD-2-Clause +# +# 20260526000001_add_indexes_to_delayed_jobs.rb +# Part of NetDEF CI System +# +# Copyright (c) 2026 by +# Network Device Education Foundation, Inc. ("NetDEF") +# +# frozen_string_literal: true + +class AddIndexesToDelayedJobs < ActiveRecord::Migration[7.2] + def change + # Composite index for the main polling query: + # SELECT * FROM delayed_jobs WHERE run_at <= NOW() AND locked_at IS NULL AND failed_at IS NULL + # ORDER BY priority ASC, run_at ASC LIMIT N + add_index :delayed_jobs, %i[priority run_at], + where: 'locked_at IS NULL AND failed_at IS NULL', + name: 'index_delayed_jobs_on_priority_and_run_at' + + # Index for locking queries (worker claims a job): + # SELECT * FROM delayed_jobs WHERE locked_at IS NOT NULL ... + add_index :delayed_jobs, :locked_at, + where: 'locked_at IS NOT NULL', + name: 'index_delayed_jobs_on_locked_at' + + # Index for failed job queries used by PrometheusMetrics: + # SELECT * FROM delayed_jobs WHERE failed_at IS NOT NULL ... + add_index :delayed_jobs, :failed_at, + where: 'failed_at IS NOT NULL', + name: 'index_delayed_jobs_on_failed_at' + end +end diff --git a/db/migrate/20260526000002_add_indexes_to_ci_jobs_and_stages_status.rb b/db/migrate/20260526000002_add_indexes_to_ci_jobs_and_stages_status.rb new file mode 100644 index 0000000..3dca2c0 --- /dev/null +++ b/db/migrate/20260526000002_add_indexes_to_ci_jobs_and_stages_status.rb @@ -0,0 +1,37 @@ +# SPDX-License-Identifier: BSD-2-Clause +# +# 20260526000002_add_indexes_to_ci_jobs_and_stages_status.rb +# Part of NetDEF CI System +# +# Copyright (c) 2026 by +# Network Device Education Foundation, Inc. ("NetDEF") +# +# frozen_string_literal: true + +class AddIndexesToCiJobsAndStagesStatus < ActiveRecord::Migration[7.2] + def change + # Covers: ci_jobs.where(status: :in_progress) + # ci_jobs.where(status: %i[queued in_progress]) + add_index :ci_jobs, :status, + name: 'index_ci_jobs_on_status' + + # Covers the most common combined filter: + # ci_jobs.where(check_suite_id: id, status: %i[queued in_progress]) + # check_suite.rb:42 — running_jobs + # check_suite.rb:50 — execution_started? + add_index :ci_jobs, %i[check_suite_id status], + name: 'index_ci_jobs_on_check_suite_id_and_status' + + # Covers: stages.where(status: %i[queued in_progress]).any? + # check_suite.rb:38 — running? + add_index :stages, :status, + name: 'index_stages_on_status' + + # Covers the most common combined filter: + # stages.where(check_suite_id: id, status: %i[queued in_progress]) + # check_suite.rb:38 — running? + # stage.rb:42 — running? + add_index :stages, %i[check_suite_id status], + name: 'index_stages_on_check_suite_id_and_status' + end +end diff --git a/lib/github/build/plan_run.rb b/lib/github/build/plan_run.rb index 1242158..29614f8 100644 --- a/lib/github/build/plan_run.rb +++ b/lib/github/build/plan_run.rb @@ -23,7 +23,7 @@ def build @pull_request.plans.each do |plan| CreateExecutionByPlan .delay(run_at: TIMER.seconds.from_now.utc, queue: 'create_execution_by_plan') - .create(@pull_request.id, @payload, plan) + .create(@pull_request.id, @payload, plan.id) end [200, 'Scheduled Plan Runs'] diff --git a/lib/github/re_run/base.rb b/lib/github/re_run/base.rb index d4d7608..151c5fd 100644 --- a/lib/github/re_run/base.rb +++ b/lib/github/re_run/base.rb @@ -130,7 +130,7 @@ def update_unavailable_jobs(check_suite) def cleanup(check_suite) check_suite.pull_request.check_suites.each do |suite| - Delayed::Job.where('handler LIKE ?', "%method_name: :timeout\nargs:\n- #{suite.id}%") + Delayed::Job.where('handler LIKE ?', "%method_name: :timeout\nargs:\n- #{suite.id}%").destroy_all end end diff --git a/lib/github/re_run/comment.rb b/lib/github/re_run/comment.rb index 670cd64..a1f9358 100644 --- a/lib/github/re_run/comment.rb +++ b/lib/github/re_run/comment.rb @@ -44,7 +44,7 @@ def confirm_and_start @pull_request.plans.each do |plan| CreateExecutionByComment .delay(run_at: TIMER.seconds.from_now.utc, queue: 'create_execution_by_comment') - .create(@pull_request.id, @payload, plan) + .create(@pull_request.id, @payload, plan.id) end [200, 'Scheduled Plan Runs'] diff --git a/lib/helpers/sinatra_payload.rb b/lib/helpers/sinatra_payload.rb index 05cf8b1..438a5bd 100644 --- a/lib/helpers/sinatra_payload.rb +++ b/lib/helpers/sinatra_payload.rb @@ -18,7 +18,8 @@ def authenticate_metrics auth = request.env['HTTP_AUTHORIZATION'] config = GitHubApp::Configuration.instance.config['metrics_auth'] - return halt 401, 'Unauthorized' if authentication_header?(auth) || config.nil? + return true if config.nil? + return halt 401, 'Unauthorized' if authentication_header?(auth) authorized?(config, auth) end diff --git a/lib/models/pull_request.rb b/lib/models/pull_request.rb index 48edcf1..ae59173 100644 --- a/lib/models/pull_request.rb +++ b/lib/models/pull_request.rb @@ -22,7 +22,7 @@ class PullRequest < ActiveRecord::Base def finished? return true if check_suites.nil? or check_suites.empty? - current_execution_by_plan(plan).finished? + plans.all? { |plan| current_execution_by_plan(plan)&.finished? } end def current_execution?(check_suite) diff --git a/lib/models/stage.rb b/lib/models/stage.rb index 4b0b074..71637c2 100644 --- a/lib/models/stage.rb +++ b/lib/models/stage.rb @@ -16,7 +16,7 @@ class Stage < ActiveRecord::Base belongs_to :configuration, class_name: 'StageConfiguration', foreign_key: 'stage_configuration_id' belongs_to :check_suite - default_scope -> { order(id: :asc) }, all_queries: true + default_scope -> { order(id: :asc) } scope :related_stages, lambda { |check_suite, suffix| where('stages.name LIKE ?', "%#{suffix}").where(check_suite: check_suite) @@ -148,7 +148,7 @@ def output_in_progress def mount_in_progress_jobs(jobs) jobs.where(status: :in_progress).map do |job| - "- **#{job.name}** -> https://#{url}/browse/#{job.job_ref}\n" + "- **#{job.name}** -> https://#{GitHubApp::Configuration.instance.ci_url}/browse/#{job.job_ref}\n" end.join("\n") end end diff --git a/spec/lib/helpers/sinatra_payload_spec.rb b/spec/lib/helpers/sinatra_payload_spec.rb index e2e26aa..0cb0fac 100644 --- a/spec/lib/helpers/sinatra_payload_spec.rb +++ b/spec/lib/helpers/sinatra_payload_spec.rb @@ -205,7 +205,7 @@ def halt(_, _opt = nil) let(:metrics_config) { nil } it 'returns 401' do - expect(dummy.authenticate_metrics).to be_falsey + expect(dummy.authenticate_metrics).to be_truthy end end diff --git a/spec/lib/models/pull_request_spec.rb b/spec/lib/models/pull_request_spec.rb index ddfb9ef..7904e97 100644 --- a/spec/lib/models/pull_request_spec.rb +++ b/spec/lib/models/pull_request_spec.rb @@ -9,44 +9,309 @@ # frozen_string_literal: true describe PullRequest do - context 'when create a new PR and check if check suite was finished' do - let(:pull_request) { create(:pull_request) } + let(:plan) { create(:plan) } + let(:plan2) { create(:plan) } + let(:pull_request) { create(:pull_request, plans: [plan]) } - it 'must return true' do - expect(pull_request.finished?).to be_truthy + # ─── Validations ──────────────────────────────────────────────────────────── + + describe 'validations' do + it 'is valid with all required attributes' do + expect(pull_request).to be_valid + end + + it 'requires author' do + pr = build(:pull_request, author: nil) + expect(pr).not_to be_valid + expect(pr.errors[:author]).to include("can't be blank") + end + + it 'requires github_pr_id' do + pr = build(:pull_request, github_pr_id: nil) + expect(pr).not_to be_valid + expect(pr.errors[:github_pr_id]).to include("can't be blank") + end + + it 'requires branch_name' do + pr = build(:pull_request, branch_name: nil) + expect(pr).not_to be_valid + expect(pr.errors[:branch_name]).to include("can't be blank") + end + + it 'requires repository' do + pr = build(:pull_request, repository: nil) + expect(pr).not_to be_valid + expect(pr.errors[:repository]).to include("can't be blank") end end - context 'when create a new PR and check if check suite was finished' do - let(:pull_request) { create(:pull_request, :with_check_suite) } + # ─── Associations ──────────────────────────────────────────────────────────── + + describe 'associations' do + # PRs without plan associations so the FK on plans.pull_request_id doesn't block destroy + let(:pr_no_plans) { create(:pull_request, plans: []) } - it 'must return true' do - expect(pull_request.finished?).to be_falsey + it 'deletes check_suites when the pull request is destroyed' do + create(:check_suite, pull_request: pr_no_plans) + expect { pr_no_plans.destroy }.to change(CheckSuite, :count).by(-1) + end + + it 'deletes pull_request_subscriptions when the pull request is destroyed' do + create(:pull_request_subscription, pull_request: pr_no_plans) + expect { pr_no_plans.destroy }.to change(PullRequestSubscription, :count).by(-1) + end + + it 'can have many plans' do + pr = create(:pull_request, plans: [plan, plan2]) + expect(pr.plans.count).to eq(2) end end - context 'when current execution is not the last check suite' do - let(:pull_request) { create(:pull_request) } - let(:check_suite1) { create(:check_suite, pull_request: pull_request) } - let(:check_suite2) { create(:check_suite, pull_request: pull_request) } - let(:check_suite3) { create(:check_suite, pull_request: pull_request) } + # ─── #finished? ───────────────────────────────────────────────────────────── + + describe '#finished?' do + context 'when the pull request has no check suites' do + let(:pull_request) { create(:pull_request, plans: [plan]) } + + it 'returns true' do + expect(pull_request.finished?).to be true + end + end + + context 'when the pull request has check suites but no plans' do + let(:pull_request) { create(:pull_request, plans: []) } + + before { create(:check_suite, pull_request: pull_request) } + + it 'returns true (vacuous all? over empty plans)' do + expect(pull_request.finished?).to be true + end + end + + context 'when all plans have a finished check suite (no running stages)' do + before { create(:check_suite, pull_request: pull_request, plan: plan) } + + it 'returns true' do + expect(pull_request.finished?).to be true + end + end + + context 'when a plan has a check suite with a running stage' do + before do + cs = create(:check_suite, pull_request: pull_request, plan: plan) + create(:stage, check_suite: cs, status: :in_progress) + end + + it 'returns false' do + expect(pull_request.finished?).to be false + end + end + + context 'when a plan has a check suite with a queued stage' do + before do + cs = create(:check_suite, pull_request: pull_request, plan: plan) + create(:stage, check_suite: cs, status: :queued) + end + + it 'returns false' do + expect(pull_request.finished?).to be false + end + end - before do - check_suite1 - check_suite2 - check_suite3 + context 'when a plan has no matching check suite (check suite exists but with no plan)' do + let(:pull_request) { create(:pull_request, :with_check_suite) } + + it 'returns false' do + expect(pull_request.finished?).to be false + end end - it 'must return true' do - expect(pull_request.current_execution?(check_suite3)).to be_truthy + context 'when a plan has a check suite with only successful stages' do + before do + cs = create(:check_suite, pull_request: pull_request, plan: plan) + create(:stage, check_suite: cs, status: :success) + end + + it 'returns true' do + expect(pull_request.finished?).to be true + end + end + + context 'with multiple plans' do + let(:pull_request) { create(:pull_request, plans: [plan, plan2]) } + + context 'when all plans have finished check suites' do + before do + create(:check_suite, pull_request: pull_request, plan: plan) + create(:check_suite, pull_request: pull_request, plan: plan2) + end + + it 'returns true' do + expect(pull_request.finished?).to be true + end + end + + context 'when one plan has a running check suite' do + before do + create(:check_suite, pull_request: pull_request, plan: plan) + cs2 = create(:check_suite, pull_request: pull_request, plan: plan2) + create(:stage, check_suite: cs2, status: :in_progress) + end + + it 'returns false' do + expect(pull_request.finished?).to be false + end + end + + context 'when one plan has a finished check suite and the other has none' do + before { create(:check_suite, pull_request: pull_request, plan: plan) } + + it 'returns false' do + expect(pull_request.finished?).to be false + end + end end end - context 'when current execution is nil' do - let(:pull_request) { create(:pull_request) } + # ─── #current_execution? ──────────────────────────────────────────────────── + + describe '#current_execution?' do + context 'when check_suite is the only one for its plan' do + let(:cs) { create(:check_suite, pull_request: pull_request, plan: plan) } + + it 'returns true' do + expect(pull_request.current_execution?(cs)).to be true + end + end + + context 'when check_suite is the most recent for its plan' do + let!(:cs1) { create(:check_suite, pull_request: pull_request, plan: plan) } + let!(:cs2) { create(:check_suite, pull_request: pull_request, plan: plan) } + + it 'returns true for cs2 (the last one)' do + expect(pull_request.current_execution?(cs2)).to be true + end + + it 'returns false for cs1 (older)' do + expect(pull_request.current_execution?(cs1)).to be false + end + end + + context 'with multiple plans isolating executions per plan' do + let(:pull_request) { create(:pull_request, plans: [plan, plan2]) } + let!(:cs_p1) { create(:check_suite, pull_request: pull_request, plan: plan) } + let!(:cs_p2) { create(:check_suite, pull_request: pull_request, plan: plan2) } + + it 'considers cs_p1 the current execution for plan' do + expect(pull_request.current_execution?(cs_p1)).to be true + end + + it 'considers cs_p2 the current execution for plan2' do + expect(pull_request.current_execution?(cs_p2)).to be true + end + + it 'does not mix plan executions' do + expect(pull_request.current_execution?(cs_p1)).to be true + expect(pull_request.current_execution?(cs_p2)).to be true + end + end + end + + # ─── #current_execution_by_plan ───────────────────────────────────────────── + + describe '#current_execution_by_plan' do + context 'when no check suite exists for the given plan' do + it 'returns nil' do + expect(pull_request.current_execution_by_plan(plan)).to be_nil + end + end + + context 'when one check suite exists for the plan' do + let!(:cs) { create(:check_suite, pull_request: pull_request, plan: plan) } + + it 'returns it' do + expect(pull_request.current_execution_by_plan(plan)).to eq(cs) + end + end + + context 'when multiple check suites exist for the plan' do + let!(:cs1) { create(:check_suite, pull_request: pull_request, plan: plan) } + let!(:cs2) { create(:check_suite, pull_request: pull_request, plan: plan) } + let!(:cs3) { create(:check_suite, pull_request: pull_request, plan: plan) } + + it 'returns the last one by id' do + expect(pull_request.current_execution_by_plan(plan)).to eq(cs3) + end + + it 'does not return older check suites' do + result = pull_request.current_execution_by_plan(plan) + expect(result).not_to eq(cs1) + expect(result).not_to eq(cs2) + end + end + + context 'with multiple plans' do + let(:pull_request) { create(:pull_request, plans: [plan, plan2]) } + let!(:cs_plan1) { create(:check_suite, pull_request: pull_request, plan: plan) } + let!(:cs_plan2) { create(:check_suite, pull_request: pull_request, plan: plan2) } + + it 'returns the check suite for plan only' do + expect(pull_request.current_execution_by_plan(plan)).to eq(cs_plan1) + end + + it 'returns the check suite for plan2 only' do + expect(pull_request.current_execution_by_plan(plan2)).to eq(cs_plan2) + end + + it 'does not cross plans' do + expect(pull_request.current_execution_by_plan(plan)).not_to eq(cs_plan2) + expect(pull_request.current_execution_by_plan(plan2)).not_to eq(cs_plan1) + end + end + end + + # ─── .unique_repository_names ─────────────────────────────────────────────── + + describe '.unique_repository_names' do + context 'when no pull requests exist' do + it 'returns an empty array' do + expect(PullRequest.unique_repository_names).to be_empty + end + end + + context 'when multiple PRs share the same repository' do + before do + create(:pull_request, repository: 'org/repo', github_pr_id: 1) + create(:pull_request, repository: 'org/repo', github_pr_id: 2) + create(:pull_request, repository: 'org/repo', github_pr_id: 3) + end + + it 'returns exactly one entry for that repository' do + expect(PullRequest.unique_repository_names.count('org/repo')).to eq(1) + end + end + + context 'when PRs have entirely different repositories' do + before do + create(:pull_request, repository: 'org/repo-a', github_pr_id: 1) + create(:pull_request, repository: 'org/repo-b', github_pr_id: 2) + end + + it 'returns all distinct repository names' do + expect(PullRequest.unique_repository_names).to contain_exactly('org/repo-a', 'org/repo-b') + end + end + + context 'when PRs mix repeated and unique repositories' do + before do + create(:pull_request, repository: 'org/repo-a', github_pr_id: 1) + create(:pull_request, repository: 'org/repo-a', github_pr_id: 2) + create(:pull_request, repository: 'org/repo-b', github_pr_id: 3) + end - it 'must return true in finished?' do - expect(pull_request.finished?).to be_truthy + it 'deduplicates correctly' do + expect(PullRequest.unique_repository_names).to contain_exactly('org/repo-a', 'org/repo-b') + end end end end diff --git a/spec/workers/create_execution_by_command_spec.rb b/spec/workers/create_execution_by_command_spec.rb index e28a0fb..8b0015a 100644 --- a/spec/workers/create_execution_by_command_spec.rb +++ b/spec/workers/create_execution_by_command_spec.rb @@ -10,34 +10,229 @@ describe CreateExecutionByCommand do let(:plan) { create(:plan) } - let(:pull_request) { create(:pull_request, plan: plan) } - let(:check_suite) { create(:check_suite, pull_request: pull_request) } + let(:pull_request) { create(:pull_request, plans: [plan]) } + let(:check_suite) { create(:check_suite, pull_request: pull_request, plan: plan) } + let(:new_check_suite) { create(:check_suite, pull_request: pull_request, plan: plan, re_run: true) } let(:payload) do { 'sender' => { 'login' => 'user', 'id' => 123, 'type' => 'User' } } end + let(:fake_github_check) { instance_double(Github::Check, signature: 'sig') } + let(:bamboo_plan_run_double) { instance_double(BambooCi::PlanRun) } + let(:fake_action) { instance_double(Github::Build::Action) } + before do - allow(Plan).to receive(:find).with(plan.id).and_return(plan) allow(GithubLogger).to receive_message_chain(:instance, :create).and_return(Logger.new($stdout)) allow(Logger).to receive(:new).and_return(Logger.new($stdout)) - allow(Github::Check).to receive(:new) - allow_any_instance_of(CreateExecutionByCommand).to receive(:stop_previous_execution) - allow_any_instance_of(CreateExecutionByCommand).to receive(:ci_jobs) - allow_any_instance_of(CreateExecutionByCommand).to receive(:cleanup) - bamboo_plan_run_double = double('BambooCi::PlanRun') - allow(bamboo_plan_run_double).to receive(:ci_variables=) - allow(bamboo_plan_run_double).to receive(:start_plan) + + allow(Github::Check).to receive(:new).and_return(fake_github_check) + allow(BambooCi::PlanRun).to receive(:new).and_return(bamboo_plan_run_double) - allow(AuditRetry).to receive(:create) + allow(bamboo_plan_run_double).to receive(:ci_variables=) + allow(bamboo_plan_run_double).to receive(:start_plan).and_return(200) + + allow(AuditRetry).to receive(:create).and_return(instance_double(AuditRetry)) allow(Github::UserInfo).to receive(:new) + + allow_any_instance_of(described_class).to receive(:stop_previous_execution) + allow_any_instance_of(described_class).to receive(:cleanup) + allow_any_instance_of(described_class).to receive(:ci_jobs) + allow(CheckSuite).to receive(:create).and_return(new_check_suite) + allow(SlackBot).to receive_message_chain(:instance, :execution_started_notification) end + # ─── .create ───────────────────────────────────────────────────────────────── + describe '.create' do - it 'returns [404, "Failed to fetch a check suite"] if check_suite is nil' do - allow(CheckSuite).to receive(:find).with(999).and_return(nil) - expect(described_class.create(plan.id, 999, payload)).to eq([404, 'Failed to fetch a check suite']) + context 'when check_suite does not exist' do + it 'returns [404, "Failed to fetch a check suite"]' do + result = described_class.create(plan.id, 999_999, payload) + expect(result).to eq([404, 'Failed to fetch a check suite']) + end + end + + context 'when plan does not exist' do + it 'returns [404, "Plan not found"]' do + result = described_class.create(999_999, check_suite.id, payload) + expect(result).to eq([404, 'Plan not found']) + end + end + + context 'when both plan and check_suite exist' do + context 'and Bamboo submission succeeds' do + it 'returns [200, "Scheduled Plan Runs"]' do + result = described_class.create(plan.id, check_suite.id, payload) + expect(result).to eq([200, 'Scheduled Plan Runs']) + end + end + + context 'and the new check_suite fails to persist' do + before do + unpersisted = build(:check_suite, pull_request: pull_request, plan: plan) + allow(CheckSuite).to receive(:create).and_return(unpersisted) + end + + it 'returns [422, "Failed to save Check Suite"]' do + result = described_class.create(plan.id, check_suite.id, payload) + expect(result).to eq([422, 'Failed to save Check Suite']) + end + end + + context 'and Bamboo submission fails' do + before do + allow(bamboo_plan_run_double).to receive(:start_plan).and_return(500) + end + + it 'returns [500, "Failed to create CI Plan"]' do + result = described_class.create(plan.id, check_suite.id, payload) + expect(result).to eq([500, 'Failed to create CI Plan']) + end + end + + context 'and Bamboo returns 422' do + before do + allow(bamboo_plan_run_double).to receive(:start_plan).and_return(422) + end + + it 'returns [422, "Failed to create CI Plan"]' do + result = described_class.create(plan.id, check_suite.id, payload) + expect(result).to eq([422, 'Failed to create CI Plan']) + end + end + end + end + + # ─── #create_check_suite ───────────────────────────────────────────────────── + + describe '#create_check_suite' do + subject(:instance) do + described_class.allocate.tap do |obj| + obj.instance_variable_set(:@payload, payload) + obj.instance_variable_set(:@logger_manager, []) + obj.instance_variable_set(:@github_check, fake_github_check) + end + end + + it 'creates a new check_suite with re_run: true' do + expect(CheckSuite).to receive(:create).with( + hash_including( + pull_request: check_suite.pull_request, + plan: plan, + re_run: true + ) + ).and_return(new_check_suite) + + instance.create_check_suite(check_suite, plan) + end + + it 'copies author, commit_sha_ref and branch fields from the original check_suite' do + expect(CheckSuite).to receive(:create).with( + hash_including( + author: check_suite.author, + commit_sha_ref: check_suite.commit_sha_ref, + work_branch: check_suite.work_branch, + base_sha_ref: check_suite.base_sha_ref, + merge_branch: check_suite.merge_branch + ) + ).and_return(new_check_suite) + + instance.create_check_suite(check_suite, plan) + end + + it 'returns the newly created check_suite' do + result = instance.create_check_suite(check_suite, plan) + expect(result).to eq(new_check_suite) + end + end + + # ─── #start_new_execution ──────────────────────────────────────────────────── + + describe '#start_new_execution' do + subject(:instance) do + described_class.allocate.tap do |obj| + obj.instance_variable_set(:@payload, payload) + obj.instance_variable_set(:@logger_manager, []) + obj.instance_variable_set(:@logger_level, Logger::INFO) + obj.instance_variable_set(:@github_check, fake_github_check) + end + end + + before do + allow(instance).to receive(:cleanup) + allow(instance).to receive(:ci_vars).and_return([]) + end + + it 'returns the Bamboo status code from start_plan' do + allow(bamboo_plan_run_double).to receive(:start_plan).and_return(200) + expect(instance.start_new_execution(new_check_suite, plan)).to eq(200) + end + + it 'returns non-200 Bamboo status without raising' do + allow(bamboo_plan_run_double).to receive(:start_plan).and_return(500) + expect(instance.start_new_execution(new_check_suite, plan)).to eq(500) + end + + it 'creates an AuditRetry record with sender details' do + expect(AuditRetry).to receive(:create).with( + hash_including( + check_suite: new_check_suite, + github_username: 'user', + github_id: 123, + github_type: 'User', + retry_type: 'full' + ) + ) + + instance.start_new_execution(new_check_suite, plan) + end + + it 'calls cleanup before starting a new Bamboo run' do + expect(instance).to receive(:cleanup).with(new_check_suite).ordered + expect(bamboo_plan_run_double).to receive(:start_plan).ordered.and_return(200) + + instance.start_new_execution(new_check_suite, plan) + end + end + + # ─── integration: @status assignment ──────────────────────────────────────── + + describe '@status assignment throughout the flow' do + it 'is [200, "Scheduled Plan Runs"] on the happy path' do + result = described_class.create(plan.id, check_suite.id, payload) + expect(result).to eq([200, 'Scheduled Plan Runs']) + end + + it 'is never nil' do + result = described_class.create(plan.id, check_suite.id, payload) + expect(result).not_to be_nil + end + + context 'when check_suite persistence fails' do + before do + unpersisted = build(:check_suite, pull_request: pull_request, plan: plan) + allow(CheckSuite).to receive(:create).and_return(unpersisted) + end + + it 'does not call ci_jobs' do + expect_any_instance_of(described_class).not_to receive(:ci_jobs) + described_class.create(plan.id, check_suite.id, payload) + end + + it 'does not call start_new_execution' do + expect_any_instance_of(described_class).not_to receive(:start_new_execution) + described_class.create(plan.id, check_suite.id, payload) + end + end + + context 'when Bamboo submission fails' do + before { allow(bamboo_plan_run_double).to receive(:start_plan).and_return(503) } + + it 'does not call ci_jobs' do + expect_any_instance_of(described_class).not_to receive(:ci_jobs) + described_class.create(plan.id, check_suite.id, payload) + end end end end diff --git a/workers/create_execution_by_command.rb b/workers/create_execution_by_command.rb index b3addaa..28ffbd8 100644 --- a/workers/create_execution_by_command.rb +++ b/workers/create_execution_by_command.rb @@ -10,10 +10,11 @@ class CreateExecutionByCommand < Github::ReRun::Base def self.create(plan_id, check_suite_id, payload) - check_suite = CheckSuite.find(check_suite_id) - plan = Plan.find(plan_id) + check_suite = CheckSuite.find_by(id: check_suite_id) + plan = Plan.find_by(id: plan_id) return [404, 'Failed to fetch a check suite'] if check_suite.nil? + return [404, 'Plan not found'] if plan.nil? instance = new(plan, check_suite, payload) @@ -32,15 +33,28 @@ def initialize(plan, check_suite, payload) stop_previous_execution(plan) - check_suite = create_check_suite(check_suite) + check_suite = create_check_suite(check_suite, plan) + + unless check_suite.persisted? + @status = [422, 'Failed to save Check Suite'] + return + end + + bamboo_status = start_new_execution(check_suite, plan) + + unless bamboo_status == 200 + @status = [bamboo_status, 'Failed to create CI Plan'] + return + end - start_new_execution(check_suite, plan) ci_jobs(check_suite, plan) + @status = [200, 'Scheduled Plan Runs'] end - def create_check_suite(check_suite) + def create_check_suite(check_suite, plan) CheckSuite.create( pull_request: check_suite.pull_request, + plan: plan, author: check_suite.author, commit_sha_ref: check_suite.commit_sha_ref, work_branch: check_suite.work_branch, @@ -55,7 +69,7 @@ def start_new_execution(check_suite, plan) bamboo_plan_run = BambooCi::PlanRun.new(check_suite, plan, logger_level: @logger_level) bamboo_plan_run.ci_variables = ci_vars - bamboo_plan_run.start_plan + status = bamboo_plan_run.start_plan audit_retry = AuditRetry.create(check_suite: check_suite, @@ -65,5 +79,7 @@ def start_new_execution(check_suite, plan) retry_type: 'full') Github::UserInfo.new(@payload.dig('sender', 'id'), check_suite: check_suite, audit_retry: audit_retry) + + status end end diff --git a/workers/create_execution_by_comment.rb b/workers/create_execution_by_comment.rb index 9caaca3..1f0ed2e 100644 --- a/workers/create_execution_by_comment.rb +++ b/workers/create_execution_by_comment.rb @@ -87,8 +87,16 @@ def comment_flow # # @raise [ActiveRecord::RecordNotFound] if the pull request is not found. def fetch_github_check - pull_request = PullRequest.find_by(github_pr_id: pr_id) - Github::Check.new(pull_request.check_suites.last) + pull_request = PullRequest.find_by(github_pr_id: pr_id) || @pull_request + + last_check_suite = pull_request.check_suites.last + + if last_check_suite.nil? + logger(Logger::ERROR, "fetch_github_check - PullRequest #{pr_id} not found") + raise 'PullRequest not found' + end + + Github::Check.new(last_check_suite) end def create_check_suite_by_commit(commit, pull_request, pull_request_info) diff --git a/workers/create_execution_by_plan.rb b/workers/create_execution_by_plan.rb index 09f7a1d..0585c79 100644 --- a/workers/create_execution_by_plan.rb +++ b/workers/create_execution_by_plan.rb @@ -15,7 +15,7 @@ def self.create(pull_request_id, payload, plan_id) return [422, 'Plan not found'] if plan.nil? - instance = new(pull_request_id, payload, plan_id) + instance = new(pull_request_id, payload, plan) logger.info "CreateExecutionByPlan: Plan '#{plan.name}' for Pull Request ID: #{pull_request_id} with " \ "status: #{instance.status.inspect}" @@ -25,7 +25,7 @@ def self.create(pull_request_id, payload, plan_id) attr_reader :status - def initialize(pull_request_id, payload, plan_id) + def initialize(pull_request_id, payload, plan) @logger = Logger.new($stdout) @logger.level = Logger::INFO @@ -33,7 +33,7 @@ def initialize(pull_request_id, payload, plan_id) @payload = payload @status = [] - create_execution_by_plan(Plan.find_by(id: plan_id)) + create_execution_by_plan(plan) end private diff --git a/workers/pr_bamboo_sync.rb b/workers/pr_bamboo_sync.rb index f20d8d2..35df2bd 100644 --- a/workers/pr_bamboo_sync.rb +++ b/workers/pr_bamboo_sync.rb @@ -74,7 +74,7 @@ def fetch_prs_in_window(github, repo, time_start, time_end) end def process_pr(github, repo, pr_object) - sha = pr.dig(:head, :sha) + sha = pr_object.dig(:head, :sha) active_runs = fetch_active_github_runs(github, repo, sha) return [] if active_runs.empty?