From 02ee393eefb8d60f24b865552aad1f791195874d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Wed, 3 Jun 2020 17:43:04 +0200
Subject: [PATCH] Use the JSON from the gitlab-roulette project for Danger
roulette
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This removes the OOO/at capacity parts of the roulette logic since it's
now handled by the https://gitlab.com/gitlab-org/gitlab-roulette project
which updates and publishes roulette data every hour.
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
danger/ce_ee_vue_templates/Dangerfile | 4 +-
lib/gitlab/danger/roulette.rb | 4 +-
lib/gitlab/danger/teammate.rb | 55 ++---------
spec/lib/gitlab/danger/roulette_spec.rb | 126 +++++++++++++-----------
spec/lib/gitlab/danger/teammate_spec.rb | 75 --------------
5 files changed, 77 insertions(+), 187 deletions(-)
diff --git a/danger/ce_ee_vue_templates/Dangerfile b/danger/ce_ee_vue_templates/Dangerfile
index f7715eb2a89f0..727bf75fe9985 100644
--- a/danger/ce_ee_vue_templates/Dangerfile
+++ b/danger/ce_ee_vue_templates/Dangerfile
@@ -10,9 +10,7 @@ def get_vue_files_with_ce_and_ee_versions(files)
"ee/#{file}"
end
- escaped_path = CGI.escape(counterpart_path)
- api_endpoint = "https://gitlab.com/api/v4/projects/gitlab-org%2Fgitlab-ee/repository/files/#{escaped_path}?ref=master"
- response = HTTParty.get(api_endpoint) # rubocop:disable Gitlab/HTTParty
+ response = gitlab.api.get_file(gitlab.mr_json['project_id'], counterpart_path, 'master')
response.code != 404
else
false
diff --git a/lib/gitlab/danger/roulette.rb b/lib/gitlab/danger/roulette.rb
index 9f7980dc20aec..9783e517bbba4 100644
--- a/lib/gitlab/danger/roulette.rb
+++ b/lib/gitlab/danger/roulette.rb
@@ -5,7 +5,7 @@
module Gitlab
module Danger
module Roulette
- ROULETTE_DATA_URL = 'https://about.gitlab.com/roulette.json'
+ ROULETTE_DATA_URL = 'https://gitlab-org.gitlab.io/gitlab-roulette/roulette.json'
OPTIONAL_CATEGORIES = [:qa, :test].freeze
Spin = Struct.new(:category, :reviewer, :maintainer, :optional_role)
@@ -90,7 +90,7 @@ def spin_for_person(people, random:)
# @param [Teammate] person
# @return [Boolean]
def valid_person?(person)
- !mr_author?(person) && person.available?
+ !mr_author?(person) && person.available && person.has_capacity
end
# @param [Teammate] person
diff --git a/lib/gitlab/danger/teammate.rb b/lib/gitlab/danger/teammate.rb
index 651b002d2bf92..c732b19a53540 100644
--- a/lib/gitlab/danger/teammate.rb
+++ b/lib/gitlab/danger/teammate.rb
@@ -1,28 +1,19 @@
# frozen_string_literal: true
-require 'cgi'
-require 'set'
-
module Gitlab
module Danger
class Teammate
- attr_reader :name, :username, :role, :projects
-
- AT_CAPACITY_EMOJI = Set.new(%w[red_circle]).freeze
- OOO_EMOJI = Set.new(%w[
- palm_tree
- beach beach_umbrella beach_with_umbrella
- ]).freeze
+ attr_reader :username, :name, :markdown_name, :role, :projects, :available, :has_capacity
+ # The options data are produced by https://gitlab.com/gitlab-org/gitlab-roulette/-/blob/master/lib/team_member.rb
def initialize(options = {})
@username = options['username']
- @name = options['name'] || @username
+ @name = options['name']
+ @markdown_name = options['markdown_name']
@role = options['role']
@projects = options['projects']
- end
-
- def markdown_name
- "[#{name}](https://gitlab.com/#{username}) (`@#{username}`)"
+ @available = options['available']
+ @has_capacity = options['has_capacity']
end
def in_project?(name)
@@ -43,42 +34,8 @@ def maintainer?(project, category, labels)
has_capability?(project, category, :maintainer, labels)
end
- def status
- return @status if defined?(@status)
-
- @status ||=
- begin
- Gitlab::Danger::RequestHelper.http_get_json(status_api_endpoint)
- rescue Gitlab::Danger::RequestHelper::HTTPError, JSON::ParserError
- nil # better no status than a crashing Danger
- end
- end
-
- # @return [Boolean]
- def available?
- !out_of_office? && has_capacity?
- end
-
private
- def status_api_endpoint
- "https://gitlab.com/api/v4/users/#{CGI.escape(username)}/status"
- end
-
- def status_emoji
- status&.dig("emoji")
- end
-
- # @return [Boolean]
- def out_of_office?
- status&.dig("message")&.match?(/OOO/i) || OOO_EMOJI.include?(status_emoji)
- end
-
- # @return [Boolean]
- def has_capacity?
- !AT_CAPACITY_EMOJI.include?(status_emoji)
- end
-
def has_capability?(project, category, kind, labels)
case category
when :test
diff --git a/spec/lib/gitlab/danger/roulette_spec.rb b/spec/lib/gitlab/danger/roulette_spec.rb
index b6148cd14075b..a9ea2dd1d6249 100644
--- a/spec/lib/gitlab/danger/roulette_spec.rb
+++ b/spec/lib/gitlab/danger/roulette_spec.rb
@@ -11,7 +11,9 @@
username: 'backend-maintainer',
name: 'Backend maintainer',
role: 'Backend engineer',
- projects: { 'gitlab' => 'maintainer backend' }
+ projects: { 'gitlab' => 'maintainer backend' },
+ available: true,
+ has_capacity: true
}
end
let(:frontend_reviewer) do
@@ -19,7 +21,9 @@
username: 'frontend-reviewer',
name: 'Frontend reviewer',
role: 'Frontend engineer',
- projects: { 'gitlab' => 'reviewer frontend' }
+ projects: { 'gitlab' => 'reviewer frontend' },
+ available: true,
+ has_capacity: true
}
end
let(:frontend_maintainer) do
@@ -27,7 +31,9 @@
username: 'frontend-maintainer',
name: 'Frontend maintainer',
role: 'Frontend engineer',
- projects: { 'gitlab' => "maintainer frontend" }
+ projects: { 'gitlab' => "maintainer frontend" },
+ available: true,
+ has_capacity: true
}
end
let(:software_engineer_in_test) do
@@ -38,7 +44,9 @@
projects: {
'gitlab' => 'reviewer qa',
'gitlab-qa' => 'maintainer'
- }
+ },
+ available: true,
+ has_capacity: true
}
end
let(:engineering_productivity_reviewer) do
@@ -46,7 +54,9 @@
username: 'eng-prod-reviewer',
name: 'EP engineer',
role: 'Engineering Productivity',
- projects: { 'gitlab' => 'reviewer backend' }
+ projects: { 'gitlab' => 'reviewer backend' },
+ available: true,
+ has_capacity: true
}
end
@@ -73,10 +83,17 @@ def matching_teammate(person)
def matching_spin(category, reviewer: { username: nil }, maintainer: { username: nil }, optional: nil)
satisfy do |spin|
- spin.category == category &&
- spin.reviewer&.username == reviewer[:username] &&
- spin.maintainer&.username == maintainer[:username] &&
- spin.optional_role == optional
+ bool = spin.category == category
+ bool &&= spin.reviewer&.username == reviewer[:username]
+
+ bool &&=
+ if maintainer
+ spin.maintainer&.username == maintainer[:username]
+ else
+ spin.maintainer.nil?
+ end
+
+ bool && spin.optional_role == optional
end
end
@@ -85,66 +102,76 @@ def matching_spin(category, reviewer: { username: nil }, maintainer: { username:
let!(:branch_name) { 'a-branch' }
let!(:mr_labels) { ['backend', 'devops::create'] }
let!(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa') }
-
- before do
- [
- backend_maintainer,
- frontend_reviewer,
- frontend_maintainer,
- software_engineer_in_test,
- engineering_productivity_reviewer
- ].each do |person|
- stub_person_status(instance_double(Gitlab::Danger::Teammate, username: person[:username]), message: 'making GitLab magic')
- end
-
+ let(:spins) do
+ # Stub the request at the latest time so that we can modify the raw data, e.g. available and has_capacity fields.
WebMock
.stub_request(:get, described_class::ROULETTE_DATA_URL)
.to_return(body: teammate_json)
+
+ subject.spin(project, categories, branch_name)
+ end
+
+ before do
allow(subject).to receive_message_chain(:gitlab, :mr_author).and_return(author.username)
allow(subject).to receive_message_chain(:gitlab, :mr_labels).and_return(mr_labels)
end
context 'when change contains backend category' do
- it 'assigns backend reviewer and maintainer' do
- categories = [:backend]
- spins = subject.spin(project, categories, branch_name)
+ let(:categories) { [:backend] }
+ it 'assigns backend reviewer and maintainer' do
expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer))
end
+
+ context 'when teammate is not available' do
+ before do
+ backend_maintainer[:available] = false
+ end
+
+ it 'assigns backend reviewer and no maintainer' do
+ expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil))
+ end
+ end
+
+ context 'when teammate has no capacity' do
+ before do
+ backend_maintainer[:has_capacity] = false
+ end
+
+ it 'assigns backend reviewer and no maintainer' do
+ expect(spins).to contain_exactly(matching_spin(:backend, reviewer: engineering_productivity_reviewer, maintainer: nil))
+ end
+ end
end
context 'when change contains frontend category' do
- it 'assigns frontend reviewer and maintainer' do
- categories = [:frontend]
- spins = subject.spin(project, categories, branch_name)
+ let(:categories) { [:frontend] }
+ it 'assigns frontend reviewer and maintainer' do
expect(spins).to contain_exactly(matching_spin(:frontend, reviewer: frontend_reviewer, maintainer: frontend_maintainer))
end
end
context 'when change contains QA category' do
- it 'assigns QA reviewer and sets optional QA maintainer' do
- categories = [:qa]
- spins = subject.spin(project, categories, branch_name)
+ let(:categories) { [:qa] }
+ it 'assigns QA reviewer and sets optional QA maintainer' do
expect(spins).to contain_exactly(matching_spin(:qa, reviewer: software_engineer_in_test, optional: :maintainer))
end
end
context 'when change contains Engineering Productivity category' do
- it 'assigns Engineering Productivity reviewer and fallback to backend maintainer' do
- categories = [:engineering_productivity]
- spins = subject.spin(project, categories, branch_name)
+ let(:categories) { [:engineering_productivity] }
+ it 'assigns Engineering Productivity reviewer and fallback to backend maintainer' do
expect(spins).to contain_exactly(matching_spin(:engineering_productivity, reviewer: engineering_productivity_reviewer, maintainer: backend_maintainer))
end
end
context 'when change contains test category' do
- it 'assigns corresponding SET and sets optional test maintainer' do
- categories = [:test]
- spins = subject.spin(project, categories, branch_name)
+ let(:categories) { [:test] }
+ it 'assigns corresponding SET and sets optional test maintainer' do
expect(spins).to contain_exactly(matching_spin(:test, reviewer: software_engineer_in_test, optional: :maintainer))
end
end
@@ -217,20 +244,13 @@ def matching_spin(category, reviewer: { username: nil }, maintainer: { username:
end
describe '#spin_for_person' do
- let(:person1) { Gitlab::Danger::Teammate.new('username' => 'rymai') }
- let(:person2) { Gitlab::Danger::Teammate.new('username' => 'godfat') }
- let(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa') }
- let(:ooo) { Gitlab::Danger::Teammate.new('username' => 'jacopo-beschi') }
- let(:no_capacity) { Gitlab::Danger::Teammate.new('username' => 'uncharged') }
+ let(:person1) { Gitlab::Danger::Teammate.new('username' => 'rymai', 'available' => true, 'has_capacity' => true) }
+ let(:person2) { Gitlab::Danger::Teammate.new('username' => 'godfat', 'available' => true, 'has_capacity' => true) }
+ let(:author) { Gitlab::Danger::Teammate.new('username' => 'filipa', 'available' => true, 'has_capacity' => true) }
+ let(:ooo) { Gitlab::Danger::Teammate.new('username' => 'jacopo-beschi', 'available' => false, 'has_capacity' => true) }
+ let(:no_capacity) { Gitlab::Danger::Teammate.new('username' => 'uncharged', 'available' => true, 'has_capacity' => false) }
before do
- stub_person_status(person1, message: 'making GitLab magic')
- stub_person_status(person2, message: 'making GitLab magic')
- stub_person_status(ooo, message: 'OOO till 15th')
- stub_person_status(no_capacity, message: 'At capacity for the next few days', emoji: 'red_circle')
- # we don't stub Filipa, as she is the author and
- # we should not fire request checking for her
-
allow(subject).to receive_message_chain(:gitlab, :mr_author).and_return(author.username)
end
@@ -254,14 +274,4 @@ def matching_spin(category, reviewer: { username: nil }, maintainer: { username:
expect(subject.spin_for_person([no_capacity], random: Random.new)).to be_nil
end
end
-
- private
-
- def stub_person_status(person, message: 'dummy message', emoji: 'unicorn')
- body = { message: message, emoji: emoji }.to_json
-
- WebMock
- .stub_request(:get, "https://gitlab.com/api/v4/users/#{person.username}/status")
- .to_return(body: body)
- end
end
diff --git a/spec/lib/gitlab/danger/teammate_spec.rb b/spec/lib/gitlab/danger/teammate_spec.rb
index ea5aecbc597df..497d1271787ef 100644
--- a/spec/lib/gitlab/danger/teammate_spec.rb
+++ b/spec/lib/gitlab/danger/teammate_spec.rb
@@ -114,79 +114,4 @@
expect(subject.maintainer?(project, :frontend, labels)).to be_falsey
end
end
-
- describe '#status' do
- let(:capabilities) { ['dish washing'] }
-
- context 'with empty cache' do
- context 'for successful request' do
- it 'returns the response' do
- mock_status = double(does_not: 'matter')
- expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json)
- .and_return(mock_status)
-
- expect(subject.status).to be mock_status
- end
- end
-
- context 'for failing request' do
- it 'returns nil' do
- expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json)
- .and_raise(Gitlab::Danger::RequestHelper::HTTPError.new)
-
- expect(subject.status).to be nil
- end
- end
- end
-
- context 'with filled cache' do
- it 'returns the cached response' do
- mock_status = double(does_not: 'matter')
- expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json)
- .and_return(mock_status)
- subject.status
-
- expect(Gitlab::Danger::RequestHelper).not_to receive(:http_get_json)
- expect(subject.status).to be mock_status
- end
- end
- end
-
- describe '#available?' do
- using RSpec::Parameterized::TableSyntax
-
- let(:capabilities) { ['dry head'] }
-
- where(:status, :result) do
- {} | true
- { message: 'dear reader' } | true
- { message: 'OOO: massage' } | false
- { message: 'love it SOOO much' } | false
- { emoji: 'red_circle' } | false
- { emoji: 'palm_tree' } | false
- { emoji: 'beach' } | false
- { emoji: 'beach_umbrella' } | false
- { emoji: 'beach_with_umbrella' } | false
- { emoji: nil } | true
- { emoji: '' } | true
- { emoji: 'dancer' } | true
- end
-
- with_them do
- before do
- expect(Gitlab::Danger::RequestHelper).to receive(:http_get_json)
- .and_return(status&.stringify_keys)
- end
-
- it { expect(subject.available?).to be result }
- end
-
- it 'returns true if request fails' do
- expect(Gitlab::Danger::RequestHelper)
- .to receive(:http_get_json)
- .and_raise(Gitlab::Danger::RequestHelper::HTTPError.new)
-
- expect(subject.available?).to be true
- end
- end
end
--
GitLab