From 77b2ecd2b1de3f66c3d9ebd5e58fedafb17ea606 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 5 Feb 2019 17:16:18 +0000 Subject: 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. --- Dangerfile | 1 + danger/documentation/Dangerfile | 2 +- danger/plugins/helper.rb | 59 ++----- danger/roulette/Dangerfile | 78 +++++++++ doc/development/code_review.md | 5 + lib/gitlab/danger/helper.rb | 131 +++++++++++++++ lib/gitlab/danger/teammate.rb | 42 +++++ spec/lib/gitlab/danger/helper_spec.rb | 294 ++++++++++++++++++++++++++++++++++ 8 files changed, 561 insertions(+), 51 deletions(-) create mode 100644 danger/roulette/Dangerfile create mode 100644 lib/gitlab/danger/helper.rb create mode 100644 lib/gitlab/danger/teammate.rb create mode 100644 spec/lib/gitlab/danger/helper_spec.rb 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] - 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>] - 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 = < 10 + "\n
\n\n#{list}\n\n
" + 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] + 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] + 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] + def project_team + team.select { |member| member.in_project?(project_name) } + end + + # @return [Hash>] + 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 -- cgit v1.2.1