Commit 62478864 authored by Sean McGivern's avatar Sean McGivern

Don't create build failed todo when build is retried

When a build is retried automatically, we close any open todos. However, we do
that _before_ creating a new build failed todo.

To solve this, we check if the build is retried before creating the todo. We
also ensure that the build _instance_ has the correct attribute set, without
needing to reload it from the database.
parent 5e0fca33
...@@ -28,6 +28,8 @@ module Ci ...@@ -28,6 +28,8 @@ module Ci
attributes.push([:user, current_user]) attributes.push([:user, current_user])
build.retried = true
Ci::Build.transaction do Ci::Build.transaction do
# mark all other builds of that name as retried # mark all other builds of that name as retried
build.pipeline.builds.latest build.pipeline.builds.latest
......
...@@ -3,7 +3,7 @@ module MergeRequests ...@@ -3,7 +3,7 @@ module MergeRequests
# Adds a todo to the parent merge_request when a CI build fails # Adds a todo to the parent merge_request when a CI build fails
# #
def execute(commit_status) def execute(commit_status)
return if commit_status.allow_failure? return if commit_status.allow_failure? || commit_status.retried?
commit_status_merge_requests(commit_status) do |merge_request| commit_status_merge_requests(commit_status) do |merge_request|
todo_service.merge_request_build_failed(merge_request) todo_service.merge_request_build_failed(merge_request)
......
---
title: Don't create build failed todos when the job is automatically retried
merge_request:
author:
type: fixed
...@@ -1743,19 +1743,34 @@ describe Ci::Build do ...@@ -1743,19 +1743,34 @@ describe Ci::Build do
end end
describe 'state transition when build fails' do describe 'state transition when build fails' do
let(:service) { MergeRequests::AddTodoWhenBuildFailsService.new(project, user) }
before do
allow(MergeRequests::AddTodoWhenBuildFailsService).to receive(:new).and_return(service)
allow(service).to receive(:close)
end
context 'when build is configured to be retried' do context 'when build is configured to be retried' do
subject { create(:ci_build, :running, options: { retry: 3 }) } subject { create(:ci_build, :running, options: { retry: 3 }, project: project, user: user) }
it 'retries builds and assigns a same user to it' do it 'retries build and assigns the same user to it' do
expect(described_class).to receive(:retry) expect(described_class).to receive(:retry)
.with(subject, subject.user) .with(subject, user)
subject.drop!
end
it 'does not try to create a todo' do
project.add_developer(user)
expect(service).not_to receive(:commit_status_merge_requests)
subject.drop! subject.drop!
end end
end end
context 'when build is not configured to be retried' do context 'when build is not configured to be retried' do
subject { create(:ci_build, :running) } subject { create(:ci_build, :running, project: project, user: user) }
it 'does not retry build' do it 'does not retry build' do
expect(described_class).not_to receive(:retry) expect(described_class).not_to receive(:retry)
...@@ -1770,6 +1785,14 @@ describe Ci::Build do ...@@ -1770,6 +1785,14 @@ describe Ci::Build do
subject.drop! subject.drop!
end end
it 'creates a todo' do
project.add_developer(user)
expect(service).to receive(:commit_status_merge_requests)
subject.drop!
end
end end
end end
end end
...@@ -160,8 +160,9 @@ describe Ci::RetryBuildService do ...@@ -160,8 +160,9 @@ describe Ci::RetryBuildService do
expect(new_build).to be_created expect(new_build).to be_created
end end
it 'does mark old build as retried' do it 'does mark old build as retried in the database and on the instance' do
expect(new_build).to be_latest expect(new_build).to be_latest
expect(build).to be_retried
expect(build.reload).to be_retried expect(build.reload).to be_retried
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment