From fed319b4ab763790865835eca35d179294883dd2 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Wed, 22 Mar 2017 20:54:17 +0100 Subject: [PATCH 1/2] Allow chat notifications only for the default branch Right now, it once a chat notifacation service has been enabled, there is no way to limit the branches which trigger a notification. Instead of allowing the user to specify the list, I opted to let the user check the box if they'd only want to be notified of the default branch. Tags are uneffected by this change. --- .../chat_notification_service.rb | 30 ++++++++++++++++++- .../project_services/mattermost_service.rb | 16 +++------- app/models/project_services/slack_service.rb | 16 +++------- .../zj-chat-notification-default-branch.yml | 4 +++ 4 files changed, 41 insertions(+), 25 deletions(-) create mode 100644 changelogs/unreleased/zj-chat-notification-default-branch.yml diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb index 200be99f36b..1a26d97283e 100644 --- a/app/models/project_services/chat_notification_service.rb +++ b/app/models/project_services/chat_notification_service.rb @@ -6,7 +6,7 @@ class ChatNotificationService < Service default_value_for :category, 'chat' prop_accessor :webhook, :username, :channel - boolean_accessor :notify_only_broken_pipelines + boolean_accessor :notify_only_broken_pipelines, :notify_only_default_branch validates :webhook, presence: true, url: true, if: :activated? @@ -17,6 +17,7 @@ class ChatNotificationService < Service if properties.nil? self.properties = {} self.notify_only_broken_pipelines = true + self.notify_only_default_branch = false end end @@ -29,6 +30,19 @@ class ChatNotificationService < Service pipeline wiki_page] end + def fields + default_fields + build_event_channels + end + + def default_fields + [ + { type: 'text', name: 'webhook', placeholder: "e.g. #{webhook_placeholder}" }, + { type: 'text', name: 'username', placeholder: 'e.g. GitLab' }, + { type: 'checkbox', name: 'notify_only_broken_pipelines' }, + { type: 'checkbox', name: 'notify_only_default_branch' }, + ] + end + def execute(data) return unless supported_events.include?(data[:object_kind]) return unless webhook.present? @@ -123,6 +137,20 @@ class ChatNotificationService < Service end def should_pipeline_be_notified?(data) + notify_for_branch(data) && notify_for_pipeline(data) + end + + def notify_for_branch(data) + ref_type = data[:object_attributes][:tag] ? 'tag' : 'branch' + + if ref_type == 'branch' && notify_only_default_branch + data[:object_attributes][:ref] == project.default_branch + else + true + end + end + + def notify_for_pipeline(data) case data[:object_attributes][:status] when 'success' !notify_only_broken_pipelines? diff --git a/app/models/project_services/mattermost_service.rb b/app/models/project_services/mattermost_service.rb index 1156d050622..0362ed172c7 100644 --- a/app/models/project_services/mattermost_service.rb +++ b/app/models/project_services/mattermost_service.rb @@ -22,19 +22,11 @@ class MattermostService < ChatNotificationService ' end - def fields - default_fields + build_event_channels - end - - def default_fields - [ - { type: 'text', name: 'webhook', placeholder: 'e.g. http://mattermost_host/hooks/…' }, - { type: 'text', name: 'username', placeholder: 'e.g. GitLab' }, - { type: 'checkbox', name: 'notify_only_broken_pipelines' }, - ] - end - def default_channel_placeholder "Channel handle (e.g. town-square)" end + + def webhook_placeholder + 'http://mattermost.example.com/hooks/…' + end end diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index b657db6f9ee..71da0af75f6 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -21,19 +21,11 @@ class SlackService < ChatNotificationService ' end - def fields - default_fields + build_event_channels - end - - def default_fields - [ - { type: 'text', name: 'webhook', placeholder: 'e.g. https://hooks.slack.com/services/…' }, - { type: 'text', name: 'username', placeholder: 'e.g. GitLab' }, - { type: 'checkbox', name: 'notify_only_broken_pipelines' }, - ] - end - def default_channel_placeholder "Channel name (e.g. general)" end + + def webhook_placeholder + 'https://hooks.slack.com/services/…' + end end diff --git a/changelogs/unreleased/zj-chat-notification-default-branch.yml b/changelogs/unreleased/zj-chat-notification-default-branch.yml new file mode 100644 index 00000000000..fa0052d5034 --- /dev/null +++ b/changelogs/unreleased/zj-chat-notification-default-branch.yml @@ -0,0 +1,4 @@ +--- +title: Only send chat notifications for the default branch +merge_request: +author: From 091c2608129445c6fe2dbe9a59e5fd3c78103d94 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Thu, 23 Mar 2017 08:16:48 +0100 Subject: [PATCH 2/2] Improve readability and add test --- .../chat_notification_service.rb | 17 +++++++---------- spec/features/admin/admin_settings_spec.rb | 1 + ...attermost_notifications_shared_examples.rb | 19 +++++++++++++++++++ 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb index 1a26d97283e..75834103db5 100644 --- a/app/models/project_services/chat_notification_service.rb +++ b/app/models/project_services/chat_notification_service.rb @@ -17,7 +17,7 @@ class ChatNotificationService < Service if properties.nil? self.properties = {} self.notify_only_broken_pipelines = true - self.notify_only_default_branch = false + self.notify_only_default_branch = true end end @@ -137,20 +137,17 @@ class ChatNotificationService < Service end def should_pipeline_be_notified?(data) - notify_for_branch(data) && notify_for_pipeline(data) + notify_for_ref?(data) && notify_for_pipeline?(data) end - def notify_for_branch(data) - ref_type = data[:object_attributes][:tag] ? 'tag' : 'branch' + def notify_for_ref?(data) + return true if data[:object_attributes][:tag] + return true unless notify_only_default_branch - if ref_type == 'branch' && notify_only_default_branch - data[:object_attributes][:ref] == project.default_branch - else - true - end + data[:object_attributes][:ref] == project.default_branch end - def notify_for_pipeline(data) + def notify_for_pipeline?(data) case data[:object_attributes][:status] when 'success' !notify_only_broken_pipelines? diff --git a/spec/features/admin/admin_settings_spec.rb b/spec/features/admin/admin_settings_spec.rb index 03daab12c8f..5099441dce2 100644 --- a/spec/features/admin/admin_settings_spec.rb +++ b/spec/features/admin/admin_settings_spec.rb @@ -34,6 +34,7 @@ feature 'Admin updates settings', feature: true do fill_in 'Username', with: 'test_user' fill_in 'service_push_channel', with: '#test_channel' page.check('Notify only broken pipelines') + page.check('Notify only default branch') check_all_events click_on 'Save' diff --git a/spec/support/slack_mattermost_notifications_shared_examples.rb b/spec/support/slack_mattermost_notifications_shared_examples.rb index 704922b6cf4..b902fe90707 100644 --- a/spec/support/slack_mattermost_notifications_shared_examples.rb +++ b/spec/support/slack_mattermost_notifications_shared_examples.rb @@ -324,5 +324,24 @@ RSpec.shared_examples 'slack or mattermost notifications' do it_behaves_like 'call Slack/Mattermost API' end end + + context 'only notify for the default branch' do + context 'when enabled' do + let(:pipeline) do + create(:ci_pipeline, project: project, status: 'failed', ref: 'not-the-default-branch') + end + + before do + chat_service.notify_only_default_branch = true + end + + it 'does not call the Slack/Mattermost API for pipeline events' do + data = Gitlab::DataBuilder::Pipeline.build(pipeline) + result = chat_service.execute(data) + + expect(result).to be_falsy + end + end + end end end