summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJacopo <beschi.jacopo@gmail.com>2019-05-30 12:50:40 +0200
committerJacopo <beschi.jacopo@gmail.com>2019-06-12 21:32:35 +0200
commitcef127e10778a21756c00c4226592f32f15a6c1f (patch)
treea3d1500d26ec126dd5c6921475727e2972bb8eb4
parent8ade1f8758efa687e67c0c29e8d67217c31c9e0f (diff)
downloadgitlab-ce-cef127e10778a21756c00c4226592f32f15a6c1f.tar.gz
Excludes MR author from gitlab_ui and single_codebase Review roulette results.
-rw-r--r--changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml5
-rw-r--r--danger/gitlab_ui_wg/Dangerfile29
-rw-r--r--danger/roulette/Dangerfile8
-rw-r--r--danger/single_codebase/Dangerfile14
-rw-r--r--lib/gitlab/danger/helper.rb4
-rw-r--r--lib/gitlab/danger/roulette.rb20
-rw-r--r--spec/lib/gitlab/danger/helper_spec.rb10
-rw-r--r--spec/lib/gitlab/danger/roulette_spec.rb43
8 files changed, 100 insertions, 33 deletions
diff --git a/changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml b/changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml
new file mode 100644
index 00000000000..8d1a38b3db5
--- /dev/null
+++ b/changelogs/unreleased/61157-reviewer-roulette-shouldn-t-include-the-author-as-a-possibility.yml
@@ -0,0 +1,5 @@
+---
+title: Excludes MR author from Review roulette
+merge_request: 28886
+author: Jacopo Beschi @jacopo-beschi
+type: fixed
diff --git a/danger/gitlab_ui_wg/Dangerfile b/danger/gitlab_ui_wg/Dangerfile
index 02d94fa5ab7..672b1deecb3 100644
--- a/danger/gitlab_ui_wg/Dangerfile
+++ b/danger/gitlab_ui_wg/Dangerfile
@@ -1,29 +1,36 @@
+FRONTEND_MAINTAINERS = %w[filipa iamphill psimyn sarahghp mishunov].freeze
+UX_MAINTAINERS = %w[tauriedavis rverissimo].freeze
+NO_REVIEWER = 'No reviewer available'.freeze
+
def mention_single_codebase_approvers
- frontend_maintainers = %w(@filipa @iamphill @psimyn @sarahghp @mishunov)
- ux_maintainers = %w(@tauriedavis @rverissimo)
+ canonical_branch_name =
+ roulette.canonical_branch_name(gitlab.mr_json['source_branch'])
+
+ random = roulette.new_random(canonical_branch_name)
+
+ frontend_maintainers = helper.new_teammates(FRONTEND_MAINTAINERS)
+ ux_maintainers = helper.new_teammates(UX_MAINTAINERS)
rows = []
- users = []
if gitlab.mr_labels.include?('frontend')
- frontend_maintainer = frontend_maintainers.sample
+ frontend_maintainer =
+ roulette.spin_for_person(frontend_maintainers, random: random)
- rows << "| ~frontend | `#{frontend_maintainer}`"
- users << frontend_maintainer
+ rows << "| ~frontend | #{frontend_maintainer&.markdown_name || NO_REVIEWER}"
end
if gitlab.mr_labels.include?('UX')
- ux_maintainers = ux_maintainers.sample
+ ux_maintainers =
+ roulette.spin_for_person(ux_maintainers, random: random)
- rows << "| ~UX | `#{ux_maintainers}`"
- users << ux_maintainers
+ rows << "| ~UX | #{ux_maintainers&.markdown_name || NO_REVIEWER}"
end
if rows.empty?
backup_maintainer = frontend_maintainers.sample
- rows << "| ~frontend / ~UX | `#{backup_maintainer}`"
- users << backup_maintainer
+ rows << "| ~frontend / ~UX | #{backup_maintainer.markdown_name}"
end
markdown(<<~MARKDOWN.strip)
diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile
index a0f1447e76a..6718e218233 100644
--- a/danger/roulette/Dangerfile
+++ b/danger/roulette/Dangerfile
@@ -31,6 +31,9 @@ Please consider creating a merge request to
for them.
MARKDOWN
+NO_REVIEWER = 'No reviewer available'.freeze
+NO_MAINTAINER = 'No maintainer available'.freeze
+
def spin_for_category(team, project, category, branch_name)
random = roulette.new_random(branch_name)
labels = gitlab.mr_labels
@@ -49,7 +52,7 @@ def spin_for_category(team, project, category, branch_name)
reviewer = roulette.spin_for_person(reviewers + traintainers + traintainers, random: random)
maintainer = roulette.spin_for_person(maintainers, random: random)
- "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name} | #{maintainer&.markdown_name} |"
+ "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name || NO_REVIEWER} | #{maintainer&.markdown_name || NO_MAINTAINER} |"
end
def build_list(items)
@@ -85,9 +88,6 @@ if changes.any? && !gitlab.mr_labels.include?('single codebase') && !gitlab.mr_l
[]
end
- # Exclude the MR author from the team for selection purposes
- team.delete_if { |teammate| teammate.username == gitlab.mr_author }
-
project = helper.project_name
unknown = changes.fetch(:unknown, [])
diff --git a/danger/single_codebase/Dangerfile b/danger/single_codebase/Dangerfile
index d1f538bec7f..f371a42e9b1 100644
--- a/danger/single_codebase/Dangerfile
+++ b/danger/single_codebase/Dangerfile
@@ -1,6 +1,6 @@
-def new_teammates(usernames)
- usernames.map { |u| ::Gitlab::Danger::Teammate.new('username' => u) }
-end
+FRONTEND_MAINTAINERS = %w[filipa iamphill].freeze
+BACKEND_MAINTAINERS = %w[rspeicher rymai yorickpeterse godfat].freeze
+NO_REVIEWER = 'No reviewer available'.freeze
def mention_single_codebase_approvers
canonical_branch_name =
@@ -8,8 +8,8 @@ def mention_single_codebase_approvers
random = roulette.new_random(canonical_branch_name)
- frontend_maintainers = new_teammates(%w[filipa iamphill])
- backend_maintainers = new_teammates(%w[rspeicher rymai yorickpeterse godfat])
+ frontend_maintainers = helper.new_teammates(FRONTEND_MAINTAINERS)
+ backend_maintainers = helper.new_teammates(BACKEND_MAINTAINERS)
rows = []
@@ -17,14 +17,14 @@ def mention_single_codebase_approvers
frontend_maintainer =
roulette.spin_for_person(frontend_maintainers, random: random)
- rows << "| ~frontend | #{frontend_maintainer.markdown_name}"
+ rows << "| ~frontend | #{frontend_maintainer&.markdown_name || NO_REVIEWER}"
end
if gitlab.mr_labels.include?('backend')
backend_maintainer =
roulette.spin_for_person(backend_maintainers, random: random)
- rows << "| ~backend | #{backend_maintainer.markdown_name}"
+ rows << "| ~backend | #{backend_maintainer&.markdown_name || NO_REVIEWER}"
end
if rows.empty?
diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb
index 7a0fb419f8e..1ecf4a12db7 100644
--- a/lib/gitlab/danger/helper.rb
+++ b/lib/gitlab/danger/helper.rb
@@ -124,6 +124,10 @@ module Gitlab
%r{\.(md|txt)\z} => :none, # To reinstate roulette for documentation, set to `:docs`.
%r{\.js\z} => :frontend
}.freeze
+
+ def new_teammates(usernames)
+ usernames.map { |u| Gitlab::Danger::Teammate.new('username' => u) }
+ end
end
end
end
diff --git a/lib/gitlab/danger/roulette.rb b/lib/gitlab/danger/roulette.rb
index 062eda38ee4..25de0a87c9d 100644
--- a/lib/gitlab/danger/roulette.rb
+++ b/lib/gitlab/danger/roulette.rb
@@ -45,21 +45,19 @@ module Gitlab
# Known issue: If someone is rejected due to OOO, and then becomes not OOO, the
# selection will change on next spin
def spin_for_person(people, random:)
- person = nil
- people = people.dup
-
- people.size.times do
- person = people.sample(random: random)
-
- break person unless out_of_office?(person)
+ people.shuffle(random: random)
+ .find(&method(:valid_person?))
+ end
- people -= [person]
- end
+ private
- person
+ def valid_person?(person)
+ !mr_author?(person) && !out_of_office?(person)
end
- private
+ def mr_author?(person)
+ person.username == gitlab.mr_author
+ end
def out_of_office?(person)
username = CGI.escape(person.username)
diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb
index f7642182a17..22e52901758 100644
--- a/spec/lib/gitlab/danger/helper_spec.rb
+++ b/spec/lib/gitlab/danger/helper_spec.rb
@@ -202,4 +202,14 @@ describe Gitlab::Danger::Helper do
it { is_expected.to eq(expected_label) }
end
end
+
+ describe '#new_teammates' do
+ it 'returns an array of Teammate' do
+ usernames = %w[filipa iamphil]
+
+ teammates = helper.new_teammates(usernames)
+
+ expect(teammates.map(&:username)).to eq(usernames)
+ end
+ end
end
diff --git a/spec/lib/gitlab/danger/roulette_spec.rb b/spec/lib/gitlab/danger/roulette_spec.rb
index 40dce0c5378..121c5d8ecd9 100644
--- a/spec/lib/gitlab/danger/roulette_spec.rb
+++ b/spec/lib/gitlab/danger/roulette_spec.rb
@@ -98,4 +98,47 @@ describe Gitlab::Danger::Roulette do
is_expected.to contain_exactly(ce_teammate_matcher)
end
end
+
+ describe '#spin_for_person' do
+ let(:person1) { Gitlab::Danger::Teammate.new('username' => 'rymai') }
+ let(:person2) { Gitlab::Danger::Teammate.new('username' => 'godfat') }
+ let(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa') }
+ let(:ooo) { Gitlab::Danger::Teammate.new('username' => 'jacopo-beschi') }
+
+ before do
+ stub_person_message(person1, 'making GitLab magic')
+ stub_person_message(person2, 'making GitLab magic')
+ stub_person_message(ooo, 'OOO till 15th')
+ # we don't stub Filipa, as she is the author and
+ # we should not fire request checking for her
+
+ allow(subject).to receive_message_chain(:gitlab, :mr_author).and_return(author.username)
+ end
+
+ it 'returns a random person' do
+ persons = [person1, person2]
+
+ selected = subject.spin_for_person(persons, random: Random.new)
+
+ expect(selected.username).to be_in(persons.map(&:username))
+ end
+
+ it 'excludes OOO persons' do
+ expect(subject.spin_for_person([ooo], random: Random.new)).to be_nil
+ end
+
+ it 'excludes mr.author' do
+ expect(subject.spin_for_person([author], random: Random.new)).to be_nil
+ end
+
+ private
+
+ def stub_person_message(person, message)
+ body = { message: message }.to_json
+
+ WebMock
+ .stub_request(:get, "https://gitlab.com/api/v4/users/#{person.username}/status")
+ .to_return(body: body)
+ end
+ end
end