Merge branch '56087-danger-roulette' into 'master'

Reviewer roulette via Danger

Closes #56087

See merge request gitlab-org/gitlab-ce!24938

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')

danger/changelog/Dangerfile

@@ -16,16 +16,12 @@ consider adding any of the %<labels>s labels.
 #{SEE_DOC}
 MSG
 
-def ee?
-  ENV['CI_PROJECT_NAME'] == 'gitlab-ee' || File.exist?('../../CHANGELOG-EE.md')
-end
-
 def ee_changelog?(changelog_path)
   changelog_path =~ /unreleased-ee/
 end
 
 def ce_port_changelog?(changelog_path)
-  ee? && !ee_changelog?(changelog_path)
+  helper.ee? && !ee_changelog?(changelog_path)
 end
 
 def check_changelog(path)

danger/documentation/Dangerfile

 # frozen_string_literal: true
 
-# All the files/directories that should be reviewed by the Docs team.
-DOCS_FILES = [
-  'doc/'
-].freeze
-
-def docs_paths_requiring_review(files)
-  files.select do |file|
-    DOCS_FILES.any? { |pattern| file.start_with?(pattern) }
-  end
-end
-
-docs_paths_to_review = docs_paths_requiring_review(helper.all_changed_files)
+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 ' \

danger/plugins/helper.rb

 # 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
+    # Put the helper code somewhere it can be tested
+    include Gitlab::Danger::Helper
   end
 end

danger/roulette/Dangerfile

+# 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

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):
 

lib/gitlab/danger/helper.rb

+# 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

lib/gitlab/danger/teammate.rb

+# 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

spec/lib/gitlab/danger/helper_spec.rb

+# 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