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

Make single codebase roulette respect OOO status

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

Dangerfile

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

danger/plugins/helper.rb

 # frozen_string_literal: true
 
-require 'net/http'
-require 'yaml'
-
 require_relative '../../lib/gitlab/danger/helper'
 
 module Danger

danger/plugins/roulette.rb

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

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}")
       []

danger/single_codebase/Dangerfile

+def new_teammates(usernames)
+  usernames.map { |u| ::Gitlab::Danger::Teammate.new('username' => u) }
+end
+
 def mention_single_codebase_approvers
-  frontend_maintainers = %w(@filipa @iamphill)
-  backend_maintainers = %w(@rspeicher @rymai @yorickpeterse @godfat)
+  canonical_branch_name =
+    roulette.canonical_branch_name(gitlab.mr_json['source_branch'])
+
+  random = roulette.new_random(canonical_branch_name)
+
+  frontend_maintainers = new_teammates(%w[filipa iamphill])
+  backend_maintainers = new_teammates(%w[rspeicher rymai yorickpeterse godfat])
 
   rows = []
-  users = []
 
   if gitlab.mr_labels.include?('frontend')
-    frontend_maintainer = frontend_maintainers.sample
+    frontend_maintainer =
+      roulette.spin_for_person(frontend_maintainers, random: random)
 
-    rows << "| ~frontend | `#{frontend_maintainer}`"
-    users << frontend_maintainer
+    rows << "| ~frontend | #{frontend_maintainer.markdown_name}"
   end
 
   if gitlab.mr_labels.include?('backend')
-    backend_maintainer = backend_maintainers.sample
+    backend_maintainer =
+      roulette.spin_for_person(backend_maintainers, random: random)
 
-    rows << "| ~backend | `#{backend_maintainer}`"
-    users << backend_maintainer
+    rows << "| ~backend | #{backend_maintainer.markdown_name}"
   end
 
   if rows.empty?
     backup_maintainer = backend_maintainers.sample
 
-    rows << "| ~frontend / ~backend | `#{backup_maintainer}`"
-    users << backup_maintainer
+    rows << "| ~frontend / ~backend | #{backup_maintainer.markdown_name}"
   end
 
   markdown(<<~MARKDOWN.strip)

lib/gitlab/danger/helper.rb

 # 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 @@ module Gitlab
         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|

lib/gitlab/danger/roulette.rb

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

lib/gitlab/danger/teammate.rb

@@ -6,8 +6,8 @@ module Gitlab
       attr_reader :name, :username, :projects
 
       def initialize(options = {})
-        @name = options['name']
         @username = options['username']
+        @name = options['name'] || @username
         @projects = options['projects']
       end
 

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 @@ describe Gitlab::Danger::Helper do
     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 @@ describe Gitlab::Danger::Helper do
     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] }

spec/lib/gitlab/danger/roulette_spec.rb

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

spec/lib/gitlab/danger/teammate_spec.rb

 # 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 @@ describe Gitlab::Danger::Teammate do
     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 @@ describe Gitlab::Danger::Teammate do
     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