From e4b0901ab8b95d66ecce4edafb7c2f5d219e72b1 Mon Sep 17 00:00:00 2001
From: Tiger Watson <twatson@gitlab.com>
Date: Thu, 25 Jan 2024 10:50:19 +0000
Subject: [PATCH] gitlab-housekeeper: Add helper for determining group and
reviewer
This will be used by Keeps to determine which group is responsible
for a given feature category, which allows a suitable (backend)
reviewer to be selected.
---
.../lib/gitlab/housekeeper/change.rb | 37 ++++
.../lib/gitlab/housekeeper/git.rb | 10 +-
.../lib/gitlab/housekeeper/gitlab_client.rb | 167 ++++++------------
.../lib/gitlab/housekeeper/runner.rb | 23 +--
.../spec/gitlab/housekeeper/change_spec.rb | 54 ++++++
.../spec/gitlab/housekeeper/git_spec.rb | 21 ++-
.../gitlab/housekeeper/gitlab_client_spec.rb | 152 +++++++++++-----
.../spec/gitlab/housekeeper/runner_spec.rb | 57 +++---
gems/gitlab-housekeeper/spec/spec_helper.rb | 24 +++
keeps/helpers/groups.rb | 37 ++++
.../overdue_finalize_background_migration.rb | 16 +-
spec/keeps/helpers/groups_spec.rb | 60 +++++++
12 files changed, 430 insertions(+), 228 deletions(-)
create mode 100644 gems/gitlab-housekeeper/lib/gitlab/housekeeper/change.rb
create mode 100644 gems/gitlab-housekeeper/spec/gitlab/housekeeper/change_spec.rb
create mode 100644 keeps/helpers/groups.rb
create mode 100644 spec/keeps/helpers/groups_spec.rb
diff --git a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/change.rb b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/change.rb
new file mode 100644
index 0000000000000..749e05317d397
--- /dev/null
+++ b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/change.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module Gitlab
+ module Housekeeper
+ class Change
+ attr_accessor :identifiers,
+ :title,
+ :description,
+ :changed_files,
+ :labels,
+ :reviewers
+
+ def mr_description
+ <<~MARKDOWN
+ #{description}
+
+ This change was generated by
+ [gitlab-housekeeper](https://gitlab.com/gitlab-org/gitlab/-/tree/master/gems/gitlab-housekeeper)
+ MARKDOWN
+ end
+
+ def commit_message
+ <<~MARKDOWN
+ #{title}
+
+ #{mr_description}
+
+ Changelog: other
+ MARKDOWN
+ end
+
+ def valid?
+ @identifiers && @title && @description && @changed_files
+ end
+ end
+ end
+end
diff --git a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/git.rb b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/git.rb
index 329d32885eeef..b19b133c65d3b 100644
--- a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/git.rb
+++ b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/git.rb
@@ -44,15 +44,7 @@ def create_commit(branch_name, change)
Shell.execute("git", "checkout", "-b", branch_name)
Shell.execute("git", "add", *change.changed_files)
-
- commit_message = <<~MSG
- #{change.title}
-
- #{change.description}
- MSG
-
- Shell.execute("git", "commit", "-m", commit_message)
-
+ Shell.execute("git", "commit", "-m", change.commit_message)
ensure
Shell.execute("git", "checkout", current_branch)
end
diff --git a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/gitlab_client.rb b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/gitlab_client.rb
index 697ab9043ef80..91f1bcf894495 100644
--- a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/gitlab_client.rb
+++ b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/gitlab_client.rb
@@ -43,6 +43,9 @@ def non_housekeeper_changes(
changes << :title if note['body'].start_with?("changed title from")
changes << :description if note['body'] == "changed the description"
changes << :code if note['body'].match?(/added \d+ commit/)
+
+ changes << :reviewers if note['body'].include?('requested review from')
+ changes << :reviewers if note['body'].include?('removed review request for')
end
resource_label_events = get_merge_request_resource_label_events(target_project_id: target_project_id, iid: iid)
@@ -61,17 +64,17 @@ def non_housekeeper_changes(
changes.to_a
end
+ # rubocop:disable Metrics/ParameterLists
def create_or_update_merge_request(
+ change:,
source_project_id:,
- title:,
- description:,
- labels:,
source_branch:,
target_branch:,
target_project_id:,
update_title:,
update_description:,
- update_labels:
+ update_labels:,
+ update_reviewers:
)
existing_iid = get_existing_merge_request(
source_project_id: source_project_id,
@@ -82,105 +85,48 @@ def create_or_update_merge_request(
if existing_iid
update_existing_merge_request(
+ change: change,
existing_iid: existing_iid,
- title: title,
- description: description,
- labels: labels,
target_project_id: target_project_id,
update_title: update_title,
update_description: update_description,
- update_labels: update_labels
+ update_labels: update_labels,
+ update_reviewers: update_reviewers
)
else
create_merge_request(
+ change: change,
source_project_id: source_project_id,
- title: title,
- description: description,
- labels: labels,
source_branch: source_branch,
target_branch: target_branch,
target_project_id: target_project_id
)
end
end
+ # rubocop:enable Metrics/ParameterLists
private
def get_merge_request_notes(target_project_id:, iid:)
- response = HTTParty.get(
- "#{@base_uri}/projects/#{target_project_id}/merge_requests/#{iid}/notes",
- query: {
- per_page: 100
- },
- headers: {
- "Private-Token" => @token
- }
- )
-
- unless (200..299).cover?(response.code)
- raise Error,
- "Failed to get merge request notes with response code: #{response.code} and body:\n#{response.body}"
- end
-
- JSON.parse(response.body)
+ request(:get, "/projects/#{target_project_id}/merge_requests/#{iid}/notes", query: { per_page: 100 })
end
def get_merge_request_resource_label_events(target_project_id:, iid:)
- response = HTTParty.get(
- "#{@base_uri}/projects/#{target_project_id}/merge_requests/#{iid}/resource_label_events",
- query: {
- per_page: 100
- },
- headers: {
- "Private-Token" => @token
- }
- )
-
- unless (200..299).cover?(response.code)
- raise Error,
- "Failed to get merge request label events with response code: #{response.code} and body:\n#{response.body}"
- end
-
- JSON.parse(response.body)
+ request(:get, "/projects/#{target_project_id}/merge_requests/#{iid}/resource_label_events",
+ query: { per_page: 100 })
end
def current_user_id
- @current_user_id = begin
- response = HTTParty.get(
- "#{@base_uri}/user",
- headers: {
- "Private-Token" => @token
- }
- )
-
- unless (200..299).cover?(response.code)
- raise Error,
- "Failed with response code: #{response.code} and body:\n#{response.body}"
- end
-
- data = JSON.parse(response.body)
- data['id']
- end
+ @current_user_id ||= request(:get, "/user")['id']
end
def get_existing_merge_request(source_project_id:, source_branch:, target_branch:, target_project_id:)
- response = HTTParty.get("#{@base_uri}/projects/#{target_project_id}/merge_requests",
- query: {
- state: :opened,
- source_branch: source_branch,
- target_branch: target_branch,
- source_project_id: source_project_id
- },
- headers: {
- 'Private-Token' => @token
- })
-
- unless (200..299).cover?(response.code)
- raise Error,
- "Failed with response code: #{response.code} and body:\n#{response.body}"
- end
-
- data = JSON.parse(response.body)
+ data = request(:get, "/projects/#{target_project_id}/merge_requests", query: {
+ state: :opened,
+ source_branch: source_branch,
+ target_branch: target_branch,
+ source_project_id: source_project_id
+ })
return nil if data.empty?
@@ -192,63 +138,64 @@ def get_existing_merge_request(source_project_id:, source_branch:, target_branch
end
def create_merge_request(
+ change:,
source_project_id:,
- title:,
- description:,
- labels:,
source_branch:,
target_branch:,
target_project_id:
)
- response = HTTParty.post("#{@base_uri}/projects/#{source_project_id}/merge_requests", body: {
- title: title,
- description: description,
- labels: Array(labels).join(','),
+ request(:post, "/projects/#{source_project_id}/merge_requests", body: {
+ title: change.title,
+ description: change.mr_description,
+ labels: Array(change.labels).join(','),
source_branch: source_branch,
target_branch: target_branch,
target_project_id: target_project_id,
- remove_source_branch: true
- }.to_json,
- headers: {
- 'Private-Token' => @token,
- 'Content-Type' => 'application/json'
- })
-
- return if (200..299).cover?(response.code)
-
- raise Error,
- "Failed with response code: #{response.code} and body:\n#{response.body}"
+ remove_source_branch: true,
+ reviewer_ids: usernames_to_ids(change.reviewers)
+ })
end
def update_existing_merge_request(
+ change:,
existing_iid:,
- title:,
- description:,
- labels:,
target_project_id:,
update_title:,
update_description:,
- update_labels:
+ update_labels:,
+ update_reviewers:
)
body = {}
- body[:title] = title if update_title
- body[:description] = description if update_description
- body[:add_labels] = Array(labels).join(',') if update_labels
+ body[:title] = change.title if update_title
+ body[:description] = change.mr_description if update_description
+ body[:add_labels] = Array(change.labels).join(',') if update_labels
+ body[:reviewer_ids] = usernames_to_ids(change.reviewers) if update_reviewers
return if body.empty?
- response = HTTParty.put("#{@base_uri}/projects/#{target_project_id}/merge_requests/#{existing_iid}",
- body: body.to_json,
- headers: {
- 'Private-Token' => @token,
- 'Content-Type' => 'application/json'
- })
+ request(:put, "/projects/#{target_project_id}/merge_requests/#{existing_iid}", body: body)
+ end
+
+ def usernames_to_ids(usernames)
+ usernames.map do |username|
+ data = request(:get, "/users", query: { username: username })
+ data[0]['id']
+ end
+ end
+
+ def request(method, path, query: {}, body: {})
+ response = HTTParty.public_send(method, "#{@base_uri}#{path}", query: query, body: body.to_json, headers: { # rubocop:disable GitlabSecurity/PublicSend
+ 'Private-Token' => @token,
+ 'Content-Type' => 'application/json'
+ })
- return if (200..299).cover?(response.code)
+ unless (200..299).cover?(response.code)
+ raise Error,
+ "Failed with response code: #{response.code} and body:\n#{response.body}"
+ end
- raise Error,
- "Failed with response code: #{response.code} and body:\n#{response.body}"
+ JSON.parse(response.body)
end
end
end
diff --git a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/runner.rb b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/runner.rb
index cebe2acfe5b6f..c86da8b588bf3 100644
--- a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/runner.rb
+++ b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/runner.rb
@@ -4,13 +4,12 @@
require 'gitlab/housekeeper/keep'
require 'gitlab/housekeeper/gitlab_client'
require 'gitlab/housekeeper/git'
+require 'gitlab/housekeeper/change'
require 'awesome_print'
require 'digest'
module Gitlab
module Housekeeper
- Change = Struct.new(:identifiers, :title, :description, :changed_files, :labels)
-
class Runner
def initialize(max_mrs: 1, dry_run: false, require: [], keeps: nil)
@max_mrs = max_mrs
@@ -32,6 +31,11 @@ def run
@keeps.each do |keep_class|
keep = keep_class.new
keep.each_change do |change|
+ unless change.valid?
+ puts "Ignoring invalid change from: #{keep_class}"
+ next
+ end
+
branch_name = git.commit_in_branch(change)
add_standard_change_data(change)
@@ -54,14 +58,6 @@ def run
def add_standard_change_data(change)
change.labels ||= []
change.labels << 'automation:gitlab-housekeeper-authored'
-
- change.description += <<~MARKDOWN
-
- This change was generated by
- [gitlab-housekeeper](https://gitlab.com/gitlab-org/gitlab/-/tree/master/gems/gitlab-housekeeper)
-
- Changelog: other
- MARKDOWN
end
def git
@@ -112,16 +108,15 @@ def create(change, branch_name)
end
gitlab_client.create_or_update_merge_request(
+ change: change,
source_project_id: housekeeper_fork_project_id,
- title: change.title,
- description: change.description,
- labels: change.labels,
source_branch: branch_name,
target_branch: 'master',
target_project_id: housekeeper_target_project_id,
update_title: !non_housekeeper_changes.include?(:title),
update_description: !non_housekeeper_changes.include?(:description),
- update_labels: !non_housekeeper_changes.include?(:labels)
+ update_labels: !non_housekeeper_changes.include?(:labels),
+ update_reviewers: !non_housekeeper_changes.include?(:reviewers)
)
end
diff --git a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/change_spec.rb b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/change_spec.rb
new file mode 100644
index 0000000000000..8060507210085
--- /dev/null
+++ b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/change_spec.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Gitlab::Housekeeper::Change do
+ let(:change) { described_class.new }
+
+ before do
+ change.title = 'The title'
+ change.description = 'The description'
+ end
+
+ describe '#mr_description' do
+ it 'includes standard content' do
+ expect(change.mr_description).to eq(
+ <<~MARKDOWN
+ The description
+
+ This change was generated by
+ [gitlab-housekeeper](https://gitlab.com/gitlab-org/gitlab/-/tree/master/gems/gitlab-housekeeper)
+ MARKDOWN
+ )
+ end
+ end
+
+ describe '#commit_message' do
+ it 'includes standard content' do
+ expect(change.commit_message).to eq(
+ <<~MARKDOWN
+ The title
+
+ The description
+
+ This change was generated by
+ [gitlab-housekeeper](https://gitlab.com/gitlab-org/gitlab/-/tree/master/gems/gitlab-housekeeper)
+
+
+ Changelog: other
+ MARKDOWN
+ )
+ end
+ end
+
+ describe '#valid?' do
+ it 'is not valid if missing required attributes' do
+ [:identifiers, :title, :description, :changed_files].each do |attribute|
+ change = create_change
+ expect(change).to be_valid
+ change.public_send("#{attribute}=", nil)
+ expect(change).not_to be_valid
+ end
+ end
+ end
+end
diff --git a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/git_spec.rb b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/git_spec.rb
index 485c46f7ad0f3..81c4342b95fe7 100644
--- a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/git_spec.rb
+++ b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/git_spec.rb
@@ -50,27 +50,21 @@ def setup_and_checkout_another_branch
let(:test_file2) { 'files/test_file2.txt' }
it 'commits the given change details to the given branch name' do
- title = "The commit title"
- description = <<~COMMIT
+ change = ::Gitlab::Housekeeper::Change.new
+ change.title = "The commit title"
+ change.description = <<~COMMIT
The commit description can be
split over multiple lines!
COMMIT
- identifiers = %w[GitlabHousekeeper TestBranch]
+ change.identifiers = %w[GitlabHousekeeper TestBranch]
Dir.mkdir('files')
File.write(test_file1, "Content in file 1!")
File.write(test_file2, "Other content in file 2!")
File.write(file_not_to_commit, 'Do not commit!')
- changed_files = [test_file1, test_file2]
-
- change = ::Gitlab::Housekeeper::Change.new(
- identifiers,
- title,
- description,
- changed_files
- )
+ change.changed_files = [test_file1, test_file2]
branch_name = nil
git.with_branch_from_branch do
@@ -91,6 +85,11 @@ def setup_and_checkout_another_branch
The commit description can be
split over multiple lines!
+ This change was generated by
+ [gitlab-housekeeper](https://gitlab.com/gitlab-org/gitlab/-/tree/master/gems/gitlab-housekeeper)
+
+ Changelog: other
+
diff --git a/files/test_file2.txt b/files/test_file2.txt
new file mode 100644
diff --git a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/gitlab_client_spec.rb b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/gitlab_client_spec.rb
index 2a1d2cb79fb99..c52a98058e3a6 100644
--- a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/gitlab_client_spec.rb
+++ b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/gitlab_client_spec.rb
@@ -23,6 +23,24 @@
}
end
+ let(:added_reviewer_note) do
+ {
+ id: 1698248524,
+ body: "requested review from @bob",
+ author: { "id" => 1234 },
+ system: true
+ }
+ end
+
+ let(:removed_reviewer_note) do
+ {
+ id: 1698248524,
+ body: "removed review request for @bob",
+ author: { "id" => 1234 },
+ system: true
+ }
+ end
+
let(:irrelevant_note1) do
{
id: 1698248523,
@@ -138,7 +156,7 @@
context 'when all important things change' do
let(:notes) do
- [not_a_system_note, updated_title_note, updated_description_note, added_commit_note]
+ [not_a_system_note, updated_title_note, updated_description_note, added_commit_note, added_reviewer_note]
end
let(:resource_label_events) do
@@ -146,10 +164,7 @@
end
it 'returns :title, :description, :code, :labels' do
- expect(non_housekeeper_changes).to include(:title)
- expect(non_housekeeper_changes).to include(:description)
- expect(non_housekeeper_changes).to include(:code)
- expect(non_housekeeper_changes).to include(:labels)
+ expect(non_housekeeper_changes).to match_array([:title, :description, :code, :labels, :reviewers])
end
end
@@ -159,10 +174,7 @@
end
it 'returns :title' do
- expect(non_housekeeper_changes).to include(:title)
- expect(non_housekeeper_changes).not_to include(:description)
- expect(non_housekeeper_changes).not_to include(:code)
- expect(non_housekeeper_changes).not_to include(:labels)
+ expect(non_housekeeper_changes).to match_array([:title])
end
end
@@ -172,10 +184,7 @@
end
it 'returns :description' do
- expect(non_housekeeper_changes).not_to include(:title)
- expect(non_housekeeper_changes).to include(:description)
- expect(non_housekeeper_changes).not_to include(:code)
- expect(non_housekeeper_changes).not_to include(:labels)
+ expect(non_housekeeper_changes).to match_array([:description])
end
end
@@ -189,10 +198,27 @@
end
it 'returns :labels' do
- expect(non_housekeeper_changes).not_to include(:title)
- expect(non_housekeeper_changes).not_to include(:description)
- expect(non_housekeeper_changes).not_to include(:code)
- expect(non_housekeeper_changes).to include(:labels)
+ expect(non_housekeeper_changes).to match_array([:labels])
+ end
+ end
+
+ context 'when reviewers are added' do
+ let(:notes) do
+ [not_a_system_note, added_reviewer_note]
+ end
+
+ it 'returns :reviewers' do
+ expect(non_housekeeper_changes).to match_array([:reviewers])
+ end
+ end
+
+ context 'when reviewers are removed' do
+ let(:notes) do
+ [not_a_system_note, removed_reviewer_note]
+ end
+
+ it 'returns :reviewers' do
+ expect(non_housekeeper_changes).to match_array([:reviewers])
end
end
@@ -204,24 +230,39 @@
end
describe '#create_or_update_merge_request' do
+ let(:reviewer_id) { 999 }
+
+ let(:change) do
+ create_change
+ end
+
let(:params) do
{
+ change: change,
source_project_id: 123,
- title: 'A new merge request!',
- labels: %w[label-1 label-2],
- description: 'This merge request is pretty good.',
source_branch: 'the-source-branch',
target_branch: 'the-target-branch',
target_project_id: 456,
update_title: true,
update_description: true,
- update_labels: true
+ update_labels: true,
+ update_reviewers: true
}
end
let(:existing_mrs) { [] }
before do
+ # Stub the user id of the reviewers
+ stub_request(:get, "https://gitlab.com/api/v4/users")
+ .with(
+ query: { username: 'thegitlabreviewer' },
+ headers: {
+ 'Private-Token' => 'the-api-token'
+ }
+ )
+ .to_return(status: 200, body: [{ id: reviewer_id }].to_json)
+
# Stub the check to see if the merge request already exists
stub_request(:get, "https://gitlab.com/api/v4/projects/456/merge_requests?state=opened&source_branch=the-source-branch&target_branch=the-target-branch&source_project_id=123")
.with(
@@ -236,19 +277,20 @@
stub = stub_request(:post, "https://gitlab.com/api/v4/projects/123/merge_requests")
.with(
body: {
- title: "A new merge request!",
- description: "This merge request is pretty good.",
- labels: "label-1,label-2",
+ title: "The change title",
+ description: change.mr_description,
+ labels: "some-label-1,some-label-2",
source_branch: "the-source-branch",
target_branch: "the-target-branch",
target_project_id: 456,
- remove_source_branch: true
+ remove_source_branch: true,
+ reviewer_ids: [reviewer_id]
},
headers: {
'Content-Type' => 'application/json',
'Private-Token' => 'the-api-token'
})
- .to_return(status: 200, body: "")
+ .to_return(status: 200, body: '{}')
client.create_or_update_merge_request(**params)
@@ -264,15 +306,16 @@
stub = stub_request(:put, "https://gitlab.com/api/v4/projects/456/merge_requests/1234")
.with(
body: {
- title: "A new merge request!",
- description: "This merge request is pretty good.",
- add_labels: "label-1,label-2"
+ title: "The change title",
+ description: change.mr_description,
+ add_labels: "some-label-1,some-label-2",
+ reviewer_ids: [reviewer_id]
}.to_json,
headers: {
'Content-Type' => 'application/json',
'Private-Token' => 'the-api-token'
})
- .to_return(status: 200, body: "")
+ .to_return(status: 200, body: '{}')
client.create_or_update_merge_request(**params)
expect(stub).to have_been_requested
@@ -293,14 +336,15 @@
stub = stub_request(:put, "https://gitlab.com/api/v4/projects/456/merge_requests/1234")
.with(
body: {
- description: "This merge request is pretty good.",
- add_labels: "label-1,label-2"
+ description: change.mr_description,
+ add_labels: "some-label-1,some-label-2",
+ reviewer_ids: [reviewer_id]
}.to_json,
headers: {
'Content-Type' => 'application/json',
'Private-Token' => 'the-api-token'
}
- ).to_return(status: 200, body: "")
+ ).to_return(status: 200, body: '{}')
client.create_or_update_merge_request(**params.merge(update_title: false))
expect(stub).to have_been_requested
@@ -312,14 +356,15 @@
stub = stub_request(:put, "https://gitlab.com/api/v4/projects/456/merge_requests/1234")
.with(
body: {
- title: "A new merge request!",
- add_labels: "label-1,label-2"
+ title: "The change title",
+ add_labels: "some-label-1,some-label-2",
+ reviewer_ids: [reviewer_id]
}.to_json,
headers: {
'Content-Type' => 'application/json',
'Private-Token' => 'the-api-token'
}
- ).to_return(status: 200, body: "")
+ ).to_return(status: 200, body: '{}')
client.create_or_update_merge_request(**params.merge(update_description: false))
expect(stub).to have_been_requested
@@ -331,24 +376,49 @@
stub = stub_request(:put, "https://gitlab.com/api/v4/projects/456/merge_requests/1234")
.with(
body: {
- title: "A new merge request!",
- description: "This merge request is pretty good."
+ title: "The change title",
+ description: change.mr_description,
+ reviewer_ids: [reviewer_id]
}.to_json,
headers: {
'Content-Type' => 'application/json',
'Private-Token' => 'the-api-token'
}
- ).to_return(status: 200, body: "")
+ ).to_return(status: 200, body: '{}')
client.create_or_update_merge_request(**params.merge(update_labels: false))
expect(stub).to have_been_requested
end
end
+ context 'when update_reviewers: false' do
+ it 'does not update the reviewers' do
+ stub = stub_request(:put, "https://gitlab.com/api/v4/projects/456/merge_requests/1234")
+ .with(
+ body: {
+ title: "The change title",
+ description: change.mr_description,
+ add_labels: "some-label-1,some-label-2"
+ }.to_json,
+ headers: {
+ 'Content-Type' => 'application/json',
+ 'Private-Token' => 'the-api-token'
+ }
+ ).to_return(status: 200, body: '{}')
+
+ client.create_or_update_merge_request(**params.merge(update_reviewers: false))
+ expect(stub).to have_been_requested
+ end
+ end
+
context 'when there is nothing to update' do
it 'does not make a request' do
- client.create_or_update_merge_request(**params.merge(update_description: false, update_title: false,
- update_labels: false))
+ client.create_or_update_merge_request(**params.merge(
+ update_description: false,
+ update_title: false,
+ update_labels: false,
+ update_reviewers: false
+ ))
end
end
end
diff --git a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/runner_spec.rb b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/runner_spec.rb
index 51dcc3b763f96..a967d74db90a6 100644
--- a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/runner_spec.rb
+++ b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/runner_spec.rb
@@ -8,32 +8,23 @@
let(:fake_keep) { instance_double(Class) }
let(:change1) do
- ::Gitlab::Housekeeper::Change.new(
- %w[the identifier for the first change],
- "The title of MR1",
- "The description of the MR",
- ['change1.txt', 'change2.txt'],
- ['example-label']
+ create_change(
+ identifiers: %w[the identifier for the first change],
+ title: "The title of MR1"
)
end
let(:change2) do
- ::Gitlab::Housekeeper::Change.new(
- %w[the identifier for the second change],
- "The title of MR2",
- "The description of the MR",
- ['change1.txt', 'change2.txt'],
- ['example-label']
+ create_change(
+ identifiers: %w[the identifier for the second change],
+ title: "The title of MR2"
)
end
let(:change3) do
- ::Gitlab::Housekeeper::Change.new(
- %w[the identifier for the third change],
- "The title of MR3",
- "The description of the MR",
- ['change1.txt', 'change2.txt'],
- ['example-label']
+ create_change(
+ identifiers: %w[the identifier for the third change],
+ title: "The title of MR3"
)
end
@@ -98,29 +89,27 @@
# Merge requests get created
expect(gitlab_client).to receive(:create_or_update_merge_request)
.with(
+ change: change1,
source_project_id: '123',
- title: 'The title of MR1',
- description: /The description of the MR/,
- labels: %w[example-label automation:gitlab-housekeeper-authored],
source_branch: 'the-identifier-for-the-first-change',
target_branch: 'master',
target_project_id: '456',
update_title: true,
update_description: true,
- update_labels: true
+ update_labels: true,
+ update_reviewers: true
)
expect(gitlab_client).to receive(:create_or_update_merge_request)
.with(
+ change: change2,
source_project_id: '123',
- title: 'The title of MR2',
- description: /The description of the MR/,
- labels: %w[example-label automation:gitlab-housekeeper-authored],
source_branch: 'the-identifier-for-the-second-change',
target_branch: 'master',
target_project_id: '456',
update_title: true,
update_description: true,
- update_labels: true
+ update_labels: true,
+ update_reviewers: true
)
described_class.new(max_mrs: 2, keeps: [fake_keep]).run
@@ -135,7 +124,7 @@
source_branch: 'the-identifier-for-the-first-change',
target_branch: 'master',
target_project_id: '456'
- ).and_return([:code, :description])
+ ).and_return([:code, :description, :reviewers])
# Second change has updated title and description so it should push the code
expect(gitlab_client).to receive(:non_housekeeper_changes)
@@ -155,29 +144,27 @@
expect(gitlab_client).to receive(:create_or_update_merge_request)
.with(
+ change: change1,
source_project_id: '123',
- title: 'The title of MR1',
- description: /The description of the MR/,
- labels: %w[example-label automation:gitlab-housekeeper-authored],
source_branch: 'the-identifier-for-the-first-change',
target_branch: 'master',
target_project_id: '456',
update_title: true,
update_description: false,
- update_labels: true
+ update_labels: true,
+ update_reviewers: false
)
expect(gitlab_client).to receive(:create_or_update_merge_request)
.with(
+ change: change2,
source_project_id: '123',
- title: 'The title of MR2',
- description: /The description of the MR/,
- labels: %w[example-label automation:gitlab-housekeeper-authored],
source_branch: 'the-identifier-for-the-second-change',
target_branch: 'master',
target_project_id: '456',
update_title: false,
update_description: false,
- update_labels: true
+ update_labels: true,
+ update_reviewers: true
)
described_class.new(max_mrs: 2, keeps: [fake_keep]).run
diff --git a/gems/gitlab-housekeeper/spec/spec_helper.rb b/gems/gitlab-housekeeper/spec/spec_helper.rb
index f2ebb2298bd93..890a4675f3ed0 100644
--- a/gems/gitlab-housekeeper/spec/spec_helper.rb
+++ b/gems/gitlab-housekeeper/spec/spec_helper.rb
@@ -6,6 +6,28 @@
require 'webmock/rspec'
require 'gitlab/rspec/all'
+module HousekeeperFactory
+ def create_change(
+ identifiers: %w[the identifier],
+ title: 'The change title',
+ description: 'The change description',
+ changed_files: ['change1.txt', 'change2.txt'],
+ labels: %w[some-label-1 some-label-2],
+ reviewers: ['thegitlabreviewer']
+ )
+
+ change = ::Gitlab::Housekeeper::Change.new
+ change.identifiers = identifiers
+ change.title = title
+ change.description = description
+ change.changed_files = changed_files
+ change.labels = labels
+ change.reviewers = reviewers
+
+ change
+ end
+end
+
RSpec.configure do |config|
config.include StubENV
# Enable flags like --only-failures and --next-failure
@@ -17,4 +39,6 @@
config.expect_with :rspec do |c|
c.syntax = :expect
end
+
+ config.include(HousekeeperFactory)
end
diff --git a/keeps/helpers/groups.rb b/keeps/helpers/groups.rb
new file mode 100644
index 0000000000000..c66597fabdf8c
--- /dev/null
+++ b/keeps/helpers/groups.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module Keeps
+ module Helpers
+ class Groups
+ Error = Class.new(StandardError)
+
+ def group_for_feature_category(category)
+ @groups ||= {}
+ @groups[category] ||= fetch_groups.find do |_, group|
+ group['categories'].present? && group['categories'].include?(category)
+ end&.last
+ end
+
+ def pick_reviewer(group, identifiers)
+ random_engineer = Digest::SHA256.hexdigest(identifiers.join).to_i(16) % group['backend_engineers'].size
+
+ group['backend_engineers'][random_engineer]
+ end
+
+ private
+
+ def fetch_groups
+ @groups_json ||= begin
+ response = Gitlab::HTTP.get('https://about.gitlab.com/groups.json')
+
+ unless (200..299).cover?(response.code)
+ raise Error,
+ "Failed to get group information with response code: #{response.code} and body:\n#{response.body}"
+ end
+
+ Gitlab::Json.parse(response.body)
+ end
+ end
+ end
+ end
+end
diff --git a/keeps/overdue_finalize_background_migration.rb b/keeps/overdue_finalize_background_migration.rb
index 035db201b4857..ee937f08a27e7 100644
--- a/keeps/overdue_finalize_background_migration.rb
+++ b/keeps/overdue_finalize_background_migration.rb
@@ -40,15 +40,16 @@ def each_change
next unless migration_record
# Finalize the migration
- title = "Finalize migration #{job_name}"
+ change = ::Gitlab::Housekeeper::Change.new
+ change.title = "Finalize migration #{job_name}"
- identifiers = [self.class.name.demodulize, job_name]
+ change.identifiers = [self.class.name.demodulize, job_name]
last_migration_file = last_migration_for_job(job_name)
next unless last_migration_file
# rubocop:disable Gitlab/DocUrl -- Not running inside rails application
- description = <<~MARKDOWN
+ change.description = <<~MARKDOWN
This migration was finished at `#{migration_record.finished_at || migration_record.updated_at}`, you can confirm
the status using our
[batched background migration chatops commands](https://docs.gitlab.com/ee/development/database/batched_background_migrations.html#monitor-the-progress-and-status-of-a-batched-background-migration).
@@ -78,7 +79,7 @@ def each_change
.source_root('generator_templates/post_deployment_migration/post_deployment_migration/')
generator = ::PostDeploymentMigration::PostDeploymentMigrationGenerator.new([migration_name], force: true)
migration_file = generator.invoke_all.first
- changed_files = [migration_file]
+ change.changed_files = [migration_file]
add_ensure_call_to_migration(migration_file, queue_method_node, job_name)
::Gitlab::Housekeeper::Shell.execute('rubocop', '-a', migration_file)
@@ -89,11 +90,10 @@ def each_change
add_finalized_by_to_yaml(migration_yaml_file, generator.migration_number)
- changed_files << digest_file
- changed_files << migration_yaml_file
+ change.changed_files << digest_file
+ change.changed_files << migration_yaml_file
- to_create = ::Gitlab::Housekeeper::Change.new(identifiers, title, description, changed_files)
- yield(to_create)
+ yield(change)
end
end
diff --git a/spec/keeps/helpers/groups_spec.rb b/spec/keeps/helpers/groups_spec.rb
new file mode 100644
index 0000000000000..c3b9afdeef02a
--- /dev/null
+++ b/spec/keeps/helpers/groups_spec.rb
@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require './keeps/helpers/groups'
+
+RSpec.describe Keeps::Helpers::Groups, feature_category: :tooling do
+ let(:groups) do
+ {
+ 'tenant_scale' => {
+ 'name' => 'Tenant Scale',
+ 'section' => 'core_platform',
+ 'stage' => 'data_stores',
+ 'categories' => %w[cell groups_and_projects user_profile organization],
+ 'label' => 'group::tenant scale',
+ 'extra_labels' => [],
+ 'slack_channel' => 'g_tenant_scale',
+ 'backend_engineers' => %w[be1 be2 be3 be4 be5],
+ 'triage_ops_config' => nil
+ }
+ }
+ end
+
+ before do
+ stub_request(:get, "https://about.gitlab.com/groups.json").to_return(status: 200, body: groups.to_json)
+ end
+
+ describe '#group_for_feature_category' do
+ let(:category) { 'organization' }
+
+ subject(:group) { described_class.new.group_for_feature_category(category) }
+
+ it { is_expected.to eq(groups['tenant_scale']) }
+
+ context 'when the category does not exist' do
+ let(:category) { 'missing-category' }
+
+ it { is_expected.to eq(nil) }
+ end
+
+ context 'when the request to fetch groups fails' do
+ before do
+ stub_request(:get, "https://about.gitlab.com/groups.json").to_return(status: 404, body: '')
+ end
+
+ it 'raises an error' do
+ expect { group }.to raise_error(described_class::Error)
+ end
+ end
+ end
+
+ describe '#pick_reviewer' do
+ let(:group) { groups['tenant_scale'] }
+ let(:identifiers) { %w[example identifier] }
+ let(:expected_index) { Digest::SHA256.hexdigest(identifiers.join).to_i(16) % group['backend_engineers'].size }
+
+ subject { described_class.new.pick_reviewer(group, identifiers) }
+
+ it { is_expected.to eq(group['backend_engineers'][expected_index]) }
+ end
+end
--
GitLab