Reviewer roulette via Danger

Make danger pick reviewers and maintainers at random, for feontend,
backend, database, etc, changes, whenever files belonging to those
teams get changed.
This commit is contained in:
Nick Thomas 2019-02-05 17:16:18 +00:00
parent 235fe67bdb
commit 77b2ecd2b1
No known key found for this signature in database
GPG Key ID: 2A313A47AFADACE9
8 changed files with 561 additions and 51 deletions

View File

@ -11,3 +11,4 @@ danger.import_dangerfile(path: 'danger/commit_messages')
danger.import_dangerfile(path: 'danger/duplicate_yarn_dependencies')
danger.import_dangerfile(path: 'danger/prettier')
danger.import_dangerfile(path: 'danger/eslint')
danger.import_dangerfile(path: 'danger/roulette')

View File

@ -1,6 +1,6 @@
# frozen_string_literal: true
docs_paths_to_review = helper.changes_by_category[:documentation]
docs_paths_to_review = helper.changes_by_category[:documentation]
unless docs_paths_to_review.empty?
message 'This merge request adds or changes files that require a ' \

View File

@ -1,56 +1,15 @@
# frozen_string_literal: true
require 'net/http'
require 'yaml'
require_relative '../../lib/gitlab/danger/helper'
module Danger
# Common helper functions for our danger scripts
# If we find ourselves repeating code in our danger files, we might as well put them in here.
# Common helper functions for our danger scripts. See Gitlab::Danger::Helper
# for more details
class Helper < Plugin
# Returns a list of all files that have been added, modified or renamed.
# `git.modified_files` might contain paths that already have been renamed,
# so we need to remove them from the list.
#
# Considering these changes:
#
# - A new_file.rb
# - D deleted_file.rb
# - M modified_file.rb
# - R renamed_file_before.rb -> renamed_file_after.rb
#
# it will return
# ```
# [ 'new_file.rb', 'modified_file.rb', 'renamed_file_after.rb' ]
# ```
#
# @return [Array<String>]
def all_changed_files
Set.new
.merge(git.added_files.to_a)
.merge(git.modified_files.to_a)
.merge(git.renamed_files.map { |x| x[:after] })
.subtract(git.renamed_files.map { |x| x[:before] })
.to_a
.sort
end
# @return [Boolean]
def ee?
ENV['CI_PROJECT_NAME'] == 'gitlab-ee' || File.exist?('../../CHANGELOG-EE.md')
end
# @return [Hash<String,Array<String>>]
def changes_by_category
all_changed_files.inject(Hash.new { |h, k| h[k] = [] }) do |hsh, file|
hsh[category_for_file(file)] << file
end
end
def category_for_file(file)
_, category = CATEGORIES.find { |regexp, _| regexp.match?(file) }
category || :unknown
end
CATEGORIES = {
%r{\Adoc/} => :documentation
}
# Put the helper code somewhere it can be tested
include Gitlab::Danger::Helper
end
end

View File

@ -0,0 +1,78 @@
# frozen_string_literal: true
MESSAGE = <<MARKDOWN
## Reviewer roulette
Changes that require review have been detected! A merge request is normally
reviewed by both a reviewer and a maintainer in its primary category (e.g.
~frontend or ~backend), and by a maintainer in all other categories.
MARKDOWN
CATEGORY_TABLE_HEADER = <<MARKDOWN
To spread load more evenly across eligible reviewers, Danger has randomly picked
a candidate for each review slot. Feel free to override this selection if you
think someone else would be better-suited, or the chosen person is unavailable.
Once you've decided who will review this merge request, mention them as you
normally would! Danger does not (yet?) automatically notify them for you.
| Category | Reviewer | Maintainer |
| -------- | -------- | ---------- |
MARKDOWN
UNKNOWN_FILES_MESSAGE = <<MARKDOWN
These files couldn't be categorised, so Danger was unable to suggest a reviewer.
Please consider creating a merge request to
[add support](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/danger/helper.rb)
for them.
MARKDOWN
def spin(team, project, category)
reviewers = team.select { |member| member.reviewer?(project, category) }
maintainers = team.select { |member| member.maintainer?(project, category) }
# TODO: filter out people who are currently not in the office
# TODO: take CODEOWNERS into account?
reviewer = reviewers[rand(reviewers.size)]
maintainer = maintainers[rand(maintainers.size)]
"| #{helper.label_for_category(category)} | #{reviewer&.markdown_name} | #{maintainer&.markdown_name} |"
end
def build_list(items)
list = items.map { |filename| "* `#{filename}`" }.join("\n")
if items.size > 10
"\n<details>\n\n#{list}\n\n</details>"
else
list
end
end
changes = helper.changes_by_category
categories = changes.keys - [:unknown]
unless changes.empty?
team =
begin
helper.project_team
rescue => err
warn("Reviewer roulette failed to load team data: #{err.message}")
[]
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, [])
rows = categories.map { |category| spin(team, project, category) }
markdown(MESSAGE)
markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty?
markdown(UNKNOWN_FILES_MESSAGE + build_list(unknown)) unless unknown.empty?
end

View File

@ -23,6 +23,11 @@ one of the [Merge request coaches][team].
If you need assistance with security scans or comments, feel free to include the
Security Team (`@gitlab-com/gl-security`) in the review.
The `danger-review` CI job will randomly pick a reviewer and a maintainer for
each area of the codebase that your merge request seems to touch. It only makes
recommendations - feel free to override it if you think someone else is a better
fit!
Depending on the areas your merge request touches, it must be **approved** by one
or more [maintainers](https://about.gitlab.com/handbook/engineering/workflow/code-review/#maintainer):

131
lib/gitlab/danger/helper.rb Normal file
View File

@ -0,0 +1,131 @@
# frozen_string_literal: true
require 'net/http'
require 'json'
require_relative 'teammate'
module Gitlab
module Danger
module Helper
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,
# so we need to remove them from the list.
#
# Considering these changes:
#
# - A new_file.rb
# - D deleted_file.rb
# - M modified_file.rb
# - R renamed_file_before.rb -> renamed_file_after.rb
#
# it will return
# ```
# [ 'new_file.rb', 'modified_file.rb', 'renamed_file_after.rb' ]
# ```
#
# @return [Array<String>]
def all_changed_files
Set.new
.merge(git.added_files.to_a)
.merge(git.modified_files.to_a)
.merge(git.renamed_files.map { |x| x[:after] })
.subtract(git.renamed_files.map { |x| x[:before] })
.to_a
.sort
end
def ee?
ENV['CI_PROJECT_NAME'] == 'gitlab-ee' || File.exist?('../../CHANGELOG-EE.md')
end
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|
hash[category_for_file(file)] << file
end
end
# Determines the category a file is in, e.g., `:frontend` or `:backend`
# @return[Symbol]
def category_for_file(file)
_, category = CATEGORIES.find { |regexp, _| regexp.match?(file) }
category || :unknown
end
# Returns the GFM for a category label, making its best guess if it's not
# a category we know about.
#
# @return[String]
def label_for_category(category)
CATEGORY_LABELS.fetch(category, "~#{category}")
end
CATEGORY_LABELS = {
docs: "~Documentation",
qa: "~QA"
}.freeze
# rubocop:disable Style/RegexpLiteral
CATEGORIES = {
%r{\Adoc/} => :docs,
%r{\A(CONTRIBUTING|LICENSE|MAINTENANCE|PHILOSOPHY|PROCESS|README)(\.md)?\z} => :docs,
%r{\A(ee/)?app/(assets|views)/} => :frontend,
%r{\A(ee/)?public/} => :frontend,
%r{\A(ee/)?spec/javascripts/} => :frontend,
%r{\A(ee/)?vendor/assets/} => :frontend,
%r{\A(jest\.config\.js|package\.json|yarn\.lock)\z} => :frontend,
%r{\A(ee/)?app/(?!assets|views)[^/]+} => :backend,
%r{\A(ee/)?(bin|config|danger|generator_templates|lib|rubocop|scripts)/} => :backend,
%r{\A(ee/)?spec/(?!javascripts)[^/]+} => :backend,
%r{\A(ee/)?vendor/(?!assets)[^/]+} => :backend,
%r{\A(ee/)?vendor/(languages\.yml|licenses\.csv)\z} => :backend,
%r{\A(Dangerfile|Gemfile|Gemfile.lock|Procfile|Rakefile)\z} => :backend,
%r{\A[A-Z_]+_VERSION\z} => :backend,
%r{\A(ee/)?db/} => :database,
%r{\A(ee/)?qa/} => :qa,
# Fallbacks in case the above patterns miss anything
%r{\.rb\z} => :backend,
%r{\.(md|txt)\z} => :docs,
%r{\.js\z} => :frontend
}.freeze
# rubocop:enable Style/RegexpLiteral
end
end
end

View File

@ -0,0 +1,42 @@
# frozen_string_literal: true
module Gitlab
module Danger
class Teammate
attr_reader :name, :username, :projects
def initialize(options = {})
@name = options['name']
@username = options['username']
@projects = options['projects']
end
def markdown_name
"[#{name}](https://gitlab.com/#{username}) (`@#{username}`)"
end
def in_project?(name)
projects&.has_key?(name)
end
# Traintainers also count as reviewers
def reviewer?(project, category)
capabilities(project) == "reviewer #{category}" || traintainer?(project, category)
end
def traintainer?(project, category)
capabilities(project) == "trainee_maintainer #{category}"
end
def maintainer?(project, category)
capabilities(project) == "maintainer #{category}"
end
private
def capabilities(project)
projects.fetch(project, '')
end
end
end
end

View File

@ -0,0 +1,294 @@
# frozen_string_literal: true
require 'fast_spec_helper'
require 'rspec-parameterized'
require 'webmock/rspec'
require 'gitlab/danger/helper'
describe Gitlab::Danger::Helper do
using RSpec::Parameterized::TableSyntax
class FakeDanger
include Gitlab::Danger::Helper
attr_reader :git
def initialize(git:)
@git = 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) }
describe '#all_changed_files' do
subject { helper.all_changed_files }
it 'interprets a list of changes from the danger git plugin' do
expect(fake_git).to receive(:added_files) { %w[a b c.old] }
expect(fake_git).to receive(:modified_files) { %w[d e] }
expect(fake_git)
.to receive(:renamed_files)
.at_least(:once)
.and_return([{ before: 'c.old', after: 'c.new' }])
is_expected.to contain_exactly('a', 'b', 'c.new', 'd', 'e')
end
end
describe '#ee?' do
subject { helper.ee? }
it 'returns true if CI_PROJECT_NAME if set to gitlab-ee' do
stub_env('CI_PROJECT_NAME', 'gitlab-ee')
expect(File).not_to receive(:exist?)
is_expected.to be_truthy
end
it 'delegates to CHANGELOG-EE.md existence if CI_PROJECT_NAME is set to something else' do
stub_env('CI_PROJECT_NAME', 'something else')
expect(File).to receive(:exist?).with('../../CHANGELOG-EE.md') { true }
is_expected.to be_truthy
end
it 'returns true if CHANGELOG-EE.md exists' do
stub_env('CI_PROJECT_NAME', nil)
expect(File).to receive(:exist?).with('../../CHANGELOG-EE.md') { true }
is_expected.to be_truthy
end
it "returns false if CHANGELOG-EE.md doesn't exist" do
stub_env('CI_PROJECT_NAME', nil)
expect(File).to receive(:exist?).with('../../CHANGELOG-EE.md') { false }
is_expected.to be_falsy
end
end
describe '#project_name' do
subject { helper.project_name }
it 'returns gitlab-ee if ee? returns true' do
expect(helper).to receive(:ee?) { true }
is_expected.to eq('gitlab-ee')
end
it 'returns gitlab-ce if ee? returns false' do
expect(helper).to receive(:ee?) { false }
is_expected.to eq('gitlab-ce')
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] }
allow(fake_git).to receive(:modified_files) { [] }
allow(fake_git).to receive(:renamed_files) { [] }
expect(helper.changes_by_category).to eq(
backend: %w[foo.rb],
database: %w[db/foo],
docs: %w[foo.md],
frontend: %w[foo.js],
qa: %w[qa/foo],
unknown: %w[foo]
)
end
end
describe '#category_for_file' do
where(:path, :expected_category) do
'doc/foo' | :docs
'CONTRIBUTING.md' | :docs
'LICENSE' | :docs
'MAINTENANCE.md' | :docs
'PHILOSOPHY.md' | :docs
'PROCESS.md' | :docs
'README.md' | :docs
'ee/doc/foo' | :unknown
'ee/README' | :unknown
'app/assets/foo' | :frontend
'app/views/foo' | :frontend
'public/foo' | :frontend
'spec/javascripts/foo' | :frontend
'vendor/assets/foo' | :frontend
'jest.config.js' | :frontend
'package.json' | :frontend
'yarn.lock' | :frontend
'ee/app/assets/foo' | :frontend
'ee/app/views/foo' | :frontend
'ee/spec/javascripts/foo' | :frontend
'app/models/foo' | :backend
'bin/foo' | :backend
'config/foo' | :backend
'danger/foo' | :backend
'lib/foo' | :backend
'rubocop/foo' | :backend
'scripts/foo' | :backend
'spec/foo' | :backend
'spec/foo/bar' | :backend
'ee/app/foo' | :backend
'ee/bin/foo' | :backend
'ee/spec/foo' | :backend
'ee/spec/foo/bar' | :backend
'generator_templates/foo' | :backend
'vendor/languages.yml' | :backend
'vendor/licenses.csv' | :backend
'Dangerfile' | :backend
'Gemfile' | :backend
'Gemfile.lock' | :backend
'Procfile' | :backend
'Rakefile' | :backend
'FOO_VERSION' | :backend
'ee/FOO_VERSION' | :unknown
'db/foo' | :database
'qa/foo' | :qa
'ee/db/foo' | :database
'ee/qa/foo' | :qa
'FOO' | :unknown
'foo' | :unknown
'foo/bar.rb' | :backend
'foo/bar.js' | :frontend
'foo/bar.txt' | :docs
'foo/bar.md' | :docs
end
with_them do
subject { helper.category_for_file(path) }
it { is_expected.to eq(expected_category) }
end
end
describe '#label_for_category' do
where(:category, :expected_label) do
:backend | '~backend'
:database | '~database'
:docs | '~Documentation'
:foo | '~foo'
:frontend | '~frontend'
:qa | '~QA'
end
with_them do
subject { helper.label_for_category(category) }
it { is_expected.to eq(expected_label) }
end
end
end