Merge branch 'suppress-allow-failure-builds' into 'master'

Suppress e-mails on failed builds if allow_failure is set

Every time I push to GitLab, I get > 2 emails saying a spec failed when I don't care about the benchmarks and others that have `allow_failure` set to `true`.

@ayufan mentioned creating a summary e-mail to prevent getting one e-mail per build, but the latter might actually be desirable. For example, I do want to know if Rubocop errors fail right away.

See merge request !2178

5 files changed, 31 insertions, 1 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 52cc8489d4f..10092243e4e 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -32,6 +32,7 @@ v 8.4.0 (unreleased)
 
 v 8.3.3 (unreleased)
   - Get "Merge when build succeeds" to work when commits were pushed to MR target branch while builds were running
+  - Suppress e-mails on failed builds if allow_failure is set (Stan Hu)
   - Fix project transfer e-mail sending incorrect paths in e-mail notification (Stan Hu)
   - Enable "Add key" button when user fills in a proper key (Stan Hu)
 
diff --git a/app/models/project_services/builds_email_service.rb b/app/models/project_services/builds_email_service.rb
index 92c9b13c9b9..f6313255cbb 100644
--- a/app/models/project_services/builds_email_service.rb
+++ b/app/models/project_services/builds_email_service.rb
@@ -73,12 +73,16 @@ class BuildsEmailService < Service
     when 'success'
       !notify_only_broken_builds?
     when 'failed'
-      true
+      !allow_failure?(data)
     else
       false
     end
   end
 
+  def allow_failure?(data)
+    data[:build_allow_failure] == true
+  end
+
   def all_recipients(data)
     all_recipients = recipients.split(',')
 
diff --git a/lib/gitlab/build_data_builder.rb b/lib/gitlab/build_data_builder.rb
index 86bfa0a4378..34e949130da 100644
--- a/lib/gitlab/build_data_builder.rb
+++ b/lib/gitlab/build_data_builder.rb
@@ -23,6 +23,7 @@ module Gitlab
           build_started_at: build.started_at,
           build_finished_at: build.finished_at,
           build_duration: build.duration,
+          build_allow_failure: build.allow_failure,
 
           # TODO: do we still need it?
           project_id: project.id,
diff --git a/spec/lib/gitlab/build_data_builder_spec.rb b/spec/lib/gitlab/build_data_builder_spec.rb
index 839b30f1ff4..38be9448794 100644
--- a/spec/lib/gitlab/build_data_builder_spec.rb
+++ b/spec/lib/gitlab/build_data_builder_spec.rb
@@ -14,6 +14,7 @@ describe 'Gitlab::BuildDataBuilder' do
     it { expect(data[:tag]).to eq(build.tag) }
     it { expect(data[:build_id]).to eq(build.id) }
     it { expect(data[:build_status]).to eq(build.status) }
+    it { expect(data[:build_allow_failure]).to eq(false) }
     it { expect(data[:project_id]).to eq(build.project.id) }
     it { expect(data[:project_name]).to eq(build.project.name_with_namespace) }
   end
diff --git a/spec/models/project_services/builds_email_service_spec.rb b/spec/models/project_services/builds_email_service_spec.rb
new file mode 100644
index 00000000000..905379a64e3
--- /dev/null
+++ b/spec/models/project_services/builds_email_service_spec.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+describe BuildsEmailService do
+  let(:build) { create(:ci_build) }
+  let(:data) { Gitlab::BuildDataBuilder.build(build) }
+  let(:service) { BuildsEmailService.new }
+
+  describe :execute do
+    it "sends email" do
+      service.recipients = 'test@gitlab.com'
+      data[:build_status] = 'failed'
+      expect(BuildEmailWorker).to receive(:perform_async)
+      service.execute(data)
+    end
+
+    it "does not sends email with failed build and allowed_failure on" do
+      data[:build_status] = 'failed'
+      data[:build_allow_failure] = true
+      expect(BuildEmailWorker).not_to receive(:perform_async)
+      service.execute(data)
+    end
+  end
+end