From c90ba127bf8cdd4ccac9692b6c96fa746314cd55 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 29 May 2019 22:38:26 +0800
Subject: [PATCH] Extract roulette to its own module
So it's more modular and extensible
---
Dangerfile | 1 +
danger/plugins/helper.rb | 3 -
danger/plugins/roulette.rb | 10 +++
danger/roulette/Dangerfile | 45 ++---------
lib/gitlab/danger/helper.rb | 29 -------
lib/gitlab/danger/roulette.rb | 84 ++++++++++++++++++++
spec/lib/gitlab/danger/helper_spec.rb | 97 -----------------------
spec/lib/gitlab/danger/roulette_spec.rb | 101 ++++++++++++++++++++++++
spec/lib/gitlab/danger/teammate_spec.rb | 16 ++--
9 files changed, 212 insertions(+), 174 deletions(-)
create mode 100644 danger/plugins/roulette.rb
create mode 100644 lib/gitlab/danger/roulette.rb
create mode 100644 spec/lib/gitlab/danger/roulette_spec.rb
diff --git a/Dangerfile b/Dangerfile
index 9e3a08949b011..d0a605f8d8e12 100644
--- a/Dangerfile
+++ b/Dangerfile
@@ -1,5 +1,6 @@
# frozen_string_literal: true
danger.import_plugin('danger/plugins/helper.rb')
+danger.import_plugin('danger/plugins/roulette.rb')
unless helper.release_automation?
danger.import_dangerfile(path: 'danger/metadata')
diff --git a/danger/plugins/helper.rb b/danger/plugins/helper.rb
index 581c0720083d0..2d7a933e801d2 100644
--- a/danger/plugins/helper.rb
+++ b/danger/plugins/helper.rb
@@ -1,8 +1,5 @@
# frozen_string_literal: true
-require 'net/http'
-require 'yaml'
-
require_relative '../../lib/gitlab/danger/helper'
module Danger
diff --git a/danger/plugins/roulette.rb b/danger/plugins/roulette.rb
new file mode 100644
index 0000000000000..7c62cff0c9203
--- /dev/null
+++ b/danger/plugins/roulette.rb
@@ -0,0 +1,10 @@
+# frozen_string_literal: true
+
+require_relative '../../lib/gitlab/danger/roulette'
+
+module Danger
+ class Roulette < Plugin
+ # Put the helper code somewhere it can be tested
+ include Gitlab::Danger::Roulette
+ end
+end
diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile
index 62e5526c02b52..de314c5b39ff1 100644
--- a/danger/roulette/Dangerfile
+++ b/danger/roulette/Dangerfile
@@ -32,7 +32,7 @@ for them.
MARKDOWN
def spin_for_category(team, project, category, branch_name)
- rng = Random.new(Digest::MD5.hexdigest(branch_name).to_i(16))
+ random = roulette.new_random(branch_name)
reviewers = team.select { |member| member.reviewer?(project, category) }
traintainers = team.select { |member| member.traintainer?(project, category) }
@@ -42,43 +42,12 @@ def spin_for_category(team, project, category, branch_name)
# https://gitlab.com/gitlab-org/gitlab-ce/issues/57653
# Make traintainers have triple the chance to be picked as a reviewer
- reviewer = spin_for_person(reviewers + traintainers + traintainers, random: rng)
- maintainer = spin_for_person(maintainers, random: rng)
+ 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} |"
end
-# 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 -= [person]
- end
-
- person
-end
-
-def out_of_office?(person)
- username = CGI.escape(person.username)
- api_endpoint = "https://gitlab.com/api/v4/users/#{username}/status"
- response = HTTParty.get(api_endpoint) # rubocop:disable Gitlab/HTTParty
-
- if response.code == 200
- response["message"]&.match(/OOO/i)
- else
- false # this is no worse than not checking for OOO
- end
-rescue
- false
-end
-
def build_list(items)
list = items.map { |filename| "* `#{filename}`" }.join("\n")
@@ -101,14 +70,12 @@ categories = changes.keys - [:unknown]
# disable the review roulette for such MRs.
if changes.any? && !gitlab.mr_labels.include?('single codebase') && !gitlab.mr_labels.include?('CSS cleanup')
# Strip leading and trailing CE/EE markers
- canonical_branch_name = gitlab
- .mr_json['source_branch']
- .gsub(/^[ce]e-/, '')
- .gsub(/-[ce]e$/, '')
+ canonical_branch_name =
+ roulette.canonical_branch_name(gitlab.mr_json['source_branch'])
team =
begin
- helper.project_team
+ roulette.project_team(helper.project_name)
rescue => err
warn("Reviewer roulette failed to load team data: #{err.message}")
[]
diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb
index f0ca397609dfa..7effb8026787d 100644
--- a/lib/gitlab/danger/helper.rb
+++ b/lib/gitlab/danger/helper.rb
@@ -1,6 +1,4 @@
# frozen_string_literal: true
-require 'net/http'
-require 'json'
require_relative 'teammate'
@@ -8,7 +6,6 @@ module Gitlab
module Danger
module Helper
RELEASE_TOOLS_BOT = 'gitlab-release-tools-bot'
- ROULETTE_DATA_URL = URI.parse('https://about.gitlab.com/roulette.json').freeze
# Returns a list of all files that have been added, modified or renamed.
# `git.modified_files` might contain paths that already have been renamed,
@@ -49,32 +46,6 @@ def project_name
ee? ? 'gitlab-ee' : 'gitlab-ce'
end
- # Looks up the current list of GitLab team members and parses it into a
- # useful form
- #
- # @return [Array<Teammate>]
- def team
- @team ||=
- begin
- rsp = Net::HTTP.get_response(ROULETTE_DATA_URL)
- raise "Failed to read #{ROULETTE_DATA_URL}: #{rsp.code} #{rsp.message}" unless
- rsp.is_a?(Net::HTTPSuccess)
-
- data = JSON.parse(rsp.body)
- data.map { |hash| ::Gitlab::Danger::Teammate.new(hash) }
- rescue JSON::ParserError
- raise "Failed to parse JSON response from #{ROULETTE_DATA_URL}"
- end
- end
-
- # Like +team+, but only returns teammates in the current project, based on
- # project_name.
- #
- # @return [Array<Teammate>]
- def project_team
- team.select { |member| member.in_project?(project_name) }
- end
-
# @return [Hash<String,Array<String>>]
def changes_by_category
all_changed_files.each_with_object(Hash.new { |h, k| h[k] = [] }) do |file, hash|
diff --git a/lib/gitlab/danger/roulette.rb b/lib/gitlab/danger/roulette.rb
new file mode 100644
index 0000000000000..062eda38ee4fe
--- /dev/null
+++ b/lib/gitlab/danger/roulette.rb
@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+
+require 'net/http'
+require 'json'
+require 'cgi'
+
+require_relative 'teammate'
+
+module Gitlab
+ module Danger
+ module Roulette
+ ROULETTE_DATA_URL = 'https://about.gitlab.com/roulette.json'
+ HTTPError = Class.new(RuntimeError)
+
+ # Looks up the current list of GitLab team members and parses it into a
+ # useful form
+ #
+ # @return [Array<Teammate>]
+ def team
+ @team ||=
+ begin
+ data = http_get_json(ROULETTE_DATA_URL)
+ data.map { |hash| ::Gitlab::Danger::Teammate.new(hash) }
+ rescue JSON::ParserError
+ raise "Failed to parse JSON response from #{ROULETTE_DATA_URL}"
+ end
+ end
+
+ # Like +team+, but only returns teammates in the current project, based on
+ # project_name.
+ #
+ # @return [Array<Teammate>]
+ def project_team(project_name)
+ team.select { |member| member.in_project?(project_name) }
+ end
+
+ def canonical_branch_name(branch_name)
+ branch_name.gsub(/^[ce]e-|-[ce]e$/, '')
+ end
+
+ def new_random(seed)
+ Random.new(Digest::MD5.hexdigest(seed).to_i(16))
+ end
+
+ # 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 -= [person]
+ end
+
+ person
+ end
+
+ private
+
+ def out_of_office?(person)
+ username = CGI.escape(person.username)
+ api_endpoint = "https://gitlab.com/api/v4/users/#{username}/status"
+ response = http_get_json(api_endpoint)
+ response["message"]&.match?(/OOO/i)
+ rescue HTTPError, JSON::ParserError
+ false # this is no worse than not checking for OOO
+ end
+
+ def http_get_json(url)
+ rsp = Net::HTTP.get_response(URI.parse(url))
+
+ unless rsp.is_a?(Net::HTTPSuccess)
+ raise HTTPError, "Failed to read #{url}: #{rsp.code} #{rsp.message}"
+ end
+
+ JSON.parse(rsp.body)
+ end
+ end
+ end
+end
diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb
index 32b90041c646d..f7642182a173b 100644
--- a/spec/lib/gitlab/danger/helper_spec.rb
+++ b/spec/lib/gitlab/danger/helper_spec.rb
@@ -2,7 +2,6 @@
require 'fast_spec_helper'
require 'rspec-parameterized'
-require 'webmock/rspec'
require 'gitlab/danger/helper'
@@ -19,39 +18,6 @@ def initialize(git:)
end
end
- let(:teammate_json) do
- <<~JSON
- [
- {
- "username": "in-gitlab-ce",
- "name": "CE maintainer",
- "projects":{ "gitlab-ce": "maintainer backend" }
- },
- {
- "username": "in-gitlab-ee",
- "name": "EE reviewer",
- "projects":{ "gitlab-ee": "reviewer frontend" }
- }
- ]
- JSON
- end
-
- let(:ce_teammate_matcher) do
- satisfy do |teammate|
- teammate.username == 'in-gitlab-ce' &&
- teammate.name == 'CE maintainer' &&
- teammate.projects == { 'gitlab-ce' => 'maintainer backend' }
- end
- end
-
- let(:ee_teammate_matcher) do
- satisfy do |teammate|
- teammate.username == 'in-gitlab-ee' &&
- teammate.name == 'EE reviewer' &&
- teammate.projects == { 'gitlab-ee' => 'reviewer frontend' }
- end
- end
-
let(:fake_git) { double('fake-git') }
subject(:helper) { FakeDanger.new(git: fake_git) }
@@ -119,69 +85,6 @@ def initialize(git:)
end
end
- describe '#team' do
- subject(:team) { helper.team }
-
- context 'HTTP failure' do
- before do
- WebMock
- .stub_request(:get, 'https://about.gitlab.com/roulette.json')
- .to_return(status: 404)
- end
-
- it 'raises a pretty error' do
- expect { team }.to raise_error(/Failed to read/)
- end
- end
-
- context 'JSON failure' do
- before do
- WebMock
- .stub_request(:get, 'https://about.gitlab.com/roulette.json')
- .to_return(body: 'INVALID JSON')
- end
-
- it 'raises a pretty error' do
- expect { team }.to raise_error(/Failed to parse/)
- end
- end
-
- context 'success' do
- before do
- WebMock
- .stub_request(:get, 'https://about.gitlab.com/roulette.json')
- .to_return(body: teammate_json)
- end
-
- it 'returns an array of teammates' do
- is_expected.to contain_exactly(ce_teammate_matcher, ee_teammate_matcher)
- end
-
- it 'memoizes the result' do
- expect(team.object_id).to eq(helper.team.object_id)
- end
- end
- end
-
- describe '#project_team' do
- subject { helper.project_team }
-
- before do
- WebMock
- .stub_request(:get, 'https://about.gitlab.com/roulette.json')
- .to_return(body: teammate_json)
- end
-
- it 'filters team by project_name' do
- expect(helper)
- .to receive(:project_name)
- .at_least(:once)
- .and_return('gitlab-ce')
-
- is_expected.to contain_exactly(ce_teammate_matcher)
- end
- end
-
describe '#changes_by_category' do
it 'categorizes changed files' do
expect(fake_git).to receive(:added_files) { %w[foo foo.md foo.rb foo.js db/foo qa/foo ee/changelogs/foo.yml] }
diff --git a/spec/lib/gitlab/danger/roulette_spec.rb b/spec/lib/gitlab/danger/roulette_spec.rb
new file mode 100644
index 0000000000000..40dce0c537892
--- /dev/null
+++ b/spec/lib/gitlab/danger/roulette_spec.rb
@@ -0,0 +1,101 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'webmock/rspec'
+
+require 'gitlab/danger/roulette'
+
+describe Gitlab::Danger::Roulette do
+ let(:teammate_json) do
+ <<~JSON
+ [
+ {
+ "username": "in-gitlab-ce",
+ "name": "CE maintainer",
+ "projects":{ "gitlab-ce": "maintainer backend" }
+ },
+ {
+ "username": "in-gitlab-ee",
+ "name": "EE reviewer",
+ "projects":{ "gitlab-ee": "reviewer frontend" }
+ }
+ ]
+ JSON
+ end
+
+ let(:ce_teammate_matcher) do
+ satisfy do |teammate|
+ teammate.username == 'in-gitlab-ce' &&
+ teammate.name == 'CE maintainer' &&
+ teammate.projects == { 'gitlab-ce' => 'maintainer backend' }
+ end
+ end
+
+ let(:ee_teammate_matcher) do
+ satisfy do |teammate|
+ teammate.username == 'in-gitlab-ee' &&
+ teammate.name == 'EE reviewer' &&
+ teammate.projects == { 'gitlab-ee' => 'reviewer frontend' }
+ end
+ end
+
+ subject(:roulette) { Object.new.extend(described_class) }
+
+ describe '#team' do
+ subject(:team) { roulette.team }
+
+ context 'HTTP failure' do
+ before do
+ WebMock
+ .stub_request(:get, described_class::ROULETTE_DATA_URL)
+ .to_return(status: 404)
+ end
+
+ it 'raises a pretty error' do
+ expect { team }.to raise_error(/Failed to read/)
+ end
+ end
+
+ context 'JSON failure' do
+ before do
+ WebMock
+ .stub_request(:get, described_class::ROULETTE_DATA_URL)
+ .to_return(body: 'INVALID JSON')
+ end
+
+ it 'raises a pretty error' do
+ expect { team }.to raise_error(/Failed to parse/)
+ end
+ end
+
+ context 'success' do
+ before do
+ WebMock
+ .stub_request(:get, described_class::ROULETTE_DATA_URL)
+ .to_return(body: teammate_json)
+ end
+
+ it 'returns an array of teammates' do
+ is_expected.to contain_exactly(ce_teammate_matcher, ee_teammate_matcher)
+ end
+
+ it 'memoizes the result' do
+ expect(team.object_id).to eq(roulette.team.object_id)
+ end
+ end
+ end
+
+ describe '#project_team' do
+ subject { roulette.project_team('gitlab-ce') }
+
+ before do
+ WebMock
+ .stub_request(:get, described_class::ROULETTE_DATA_URL)
+ .to_return(body: teammate_json)
+ end
+
+ it 'filters team by project_name' do
+ is_expected.to contain_exactly(ce_teammate_matcher)
+ end
+ end
+end
diff --git a/spec/lib/gitlab/danger/teammate_spec.rb b/spec/lib/gitlab/danger/teammate_spec.rb
index 4bc0a4c1398fe..753c74ff814dc 100644
--- a/spec/lib/gitlab/danger/teammate_spec.rb
+++ b/spec/lib/gitlab/danger/teammate_spec.rb
@@ -1,5 +1,9 @@
# frozen_string_literal: true
+require 'fast_spec_helper'
+
+require 'gitlab/danger/teammate'
+
describe Gitlab::Danger::Teammate do
subject { described_class.new({ 'projects' => projects }) }
let(:projects) { { project => capabilities } }
@@ -9,15 +13,15 @@
let(:capabilities) { ['reviewer backend', 'maintainer frontend', 'trainee_maintainer database'] }
it '#reviewer? supports multiple roles per project' do
- expect(subject.reviewer?(project, 'backend')).to be_truthy
+ expect(subject.reviewer?(project, :backend)).to be_truthy
end
it '#traintainer? supports multiple roles per project' do
- expect(subject.traintainer?(project, 'database')).to be_truthy
+ expect(subject.traintainer?(project, :database)).to be_truthy
end
it '#maintainer? supports multiple roles per project' do
- expect(subject.maintainer?(project, 'frontend')).to be_truthy
+ expect(subject.maintainer?(project, :frontend)).to be_truthy
end
end
@@ -25,15 +29,15 @@
let(:capabilities) { 'reviewer backend' }
it '#reviewer? supports one role per project' do
- expect(subject.reviewer?(project, 'backend')).to be_truthy
+ expect(subject.reviewer?(project, :backend)).to be_truthy
end
it '#traintainer? supports one role per project' do
- expect(subject.traintainer?(project, 'database')).to be_falsey
+ expect(subject.traintainer?(project, :database)).to be_falsey
end
it '#maintainer? supports one role per project' do
- expect(subject.maintainer?(project, 'frontend')).to be_falsey
+ expect(subject.maintainer?(project, :frontend)).to be_falsey
end
end
end
--
GitLab