summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNick Thomas <nick@gitlab.com>2019-02-05 17:16:18 +0000
committerNick Thomas <nick@gitlab.com>2019-02-13 16:41:28 +0000
commit77b2ecd2b1de3f66c3d9ebd5e58fedafb17ea606 (patch)
treeeb360eb0264b50370ff3ad48910ad437183bc3b6
parent235fe67bdbc7f177a6f4b213bb1780f043944246 (diff)
downloadgitlab-ce-77b2ecd2b1de3f66c3d9ebd5e58fedafb17ea606.tar.gz
Reviewer roulette via Danger56087-danger-roulette
Make danger pick reviewers and maintainers at random, for feontend, backend, database, etc, changes, whenever files belonging to those teams get changed.
-rw-r--r--Dangerfile1
-rw-r--r--danger/documentation/Dangerfile2
-rw-r--r--danger/plugins/helper.rb59
-rw-r--r--danger/roulette/Dangerfile78
-rw-r--r--doc/development/code_review.md5
-rw-r--r--lib/gitlab/danger/helper.rb131
-rw-r--r--lib/gitlab/danger/teammate.rb42
-rw-r--r--spec/lib/gitlab/danger/helper_spec.rb294
8 files changed, 561 insertions, 51 deletions
diff --git a/Dangerfile b/Dangerfile
index 6a2c5cf2773..715a2bcbbae 100644
--- a/Dangerfile
+++ b/Dangerfile
@@ -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')
diff --git a/danger/documentation/Dangerfile b/danger/documentation/Dangerfile
index 76e81f5f62f..dd0e3f6deb6 100644
--- a/danger/documentation/Dangerfile
+++ b/danger/documentation/Dangerfile
@@ -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 ' \
diff --git a/danger/plugins/helper.rb b/danger/plugins/helper.rb
index 09ee50a2ef0..581c0720083 100644
--- a/danger/plugins/helper.rb
+++ b/danger/plugins/helper.rb
@@ -1,56 +1,15 @@
# frozen_string_literal: true
-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.
- 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
+require 'net/http'
+require 'yaml'
- def category_for_file(file)
- _, category = CATEGORIES.find { |regexp, _| regexp.match?(file) }
+require_relative '../../lib/gitlab/danger/helper'
- category || :unknown
- end
-
- CATEGORIES = {
- %r{\Adoc/} => :documentation
- }
+module Danger
+ # Common helper functions for our danger scripts. See Gitlab::Danger::Helper
+ # for more details
+ class Helper < Plugin
+ # Put the helper code somewhere it can be tested
+ include Gitlab::Danger::Helper
end
end
diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile
new file mode 100644
index 00000000000..5c3d7a4ca49
--- /dev/null
+++ b/danger/roulette/Dangerfile
@@ -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
diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 25ea2211b64..4e5fb47e331 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -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):
diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb
new file mode 100644
index 00000000000..80760e41c7d
--- /dev/null
+++ b/lib/gitlab/danger/helper.rb
@@ -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
diff --git a/lib/gitlab/danger/teammate.rb b/lib/gitlab/danger/teammate.rb
new file mode 100644
index 00000000000..4b822aa86c5
--- /dev/null
+++ b/lib/gitlab/danger/teammate.rb
@@ -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
diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb
new file mode 100644
index 00000000000..75080aacd96
--- /dev/null
+++ b/spec/lib/gitlab/danger/helper_spec.rb
@@ -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