Commit dda02409 authored by Rémy Coutable's avatar Rémy Coutable

Address the latest round of review

Signed-off-by: Rémy Coutable's avatarRémy Coutable <remy@rymai.me>
parent dede854c
......@@ -123,10 +123,16 @@ task :upstream_merge do
if result.success?
upstream_mr = result.payload[:upstream_mr]
$stdout.puts <<~SUCCESS_MESSAGE.colorize(upstream_mr.exists? ? :green : :yellow)
--> Merge request "#{upstream_mr.title}" #{'not ' unless upstream_mr.exists?}created.
#{upstream_mr.url}
SUCCESS_MESSAGE
if upstream_mr.exists?
$stdout.puts <<~SUCCESS_MESSAGE.colorize(:green)
--> Merge request "#{upstream_mr.title}" created.
#{upstream_mr.url}
SUCCESS_MESSAGE
else
$stdout.puts <<~SUCCESS_MESSAGE.colorize(:yellow)
--> Merge request "#{upstream_mr.title}" not created.
SUCCESS_MESSAGE
end
elsif result.payload[:in_progress_mr_url]
$stdout.puts <<~ERROR_MESSAGE.colorize(:red)
--> An upstream merge request already exists.
......
......@@ -183,7 +183,7 @@ TEST=true bundle exec rake "security_release[8.2.1]"
This task will:
1. Merge the latest CE `master` into the latest EE `master`
1. Push the merge to a new –unique per day– branch
1. Push the merge to a new (unique per day) branch
1. Create a Merge Request that will include:
1. A list the files for which conflicts need to be resolved
1. Mentions of the last ones who updated the conflicting files
......@@ -194,7 +194,7 @@ This task will:
| ------ | ------- |
| `MENTION=false` | Don't mention people in the MR description (wrap their usernames in backticks) |
| `TEST=true` | Don't push the new branch, nor create a MR for it |
| `FORCE=true` | Create a branch and MR even if one is already in progress |
| `FORCE=true` | Create a branch and MR even if another upstream merge is already in progress |
### Examples
......
......@@ -9,9 +9,9 @@ class CommitAuthor
@git_names_to_team_names = git_names_to_team_names
end
def to_gitlab(reference: false)
def to_gitlab
if gitlab_username
reference ? "@#{gitlab_username}" : gitlab_username
"@#{gitlab_username}"
else
git_name
end
......
......@@ -8,7 +8,7 @@ class Issuable < OpenStruct
end
def description
self[:description] || ERB.new(template).result(binding)
ERB.new(template).result(binding)
end
def project
......
......@@ -5,33 +5,42 @@ require_relative '../upstream_merge_request'
module Services
class UpstreamMergeService
UpstreamMergeAlreadyInProgressError = Class.new(StandardError)
UpstreamMergeInProgressError = Class.new(StandardError)
Result = Struct.new(:success?, :payload)
def perform(dry_run: false, mention_people: false, force: false)
check_for_open_upstream_mrs!(force)
attr_reader :dry_run, :mention_people, :force
def initialize(dry_run: false, mention_people: false, force: false)
@dry_run = dry_run
@mention_people = mention_people
@force = force
end
def perform
check_for_open_upstream_mrs! unless force
merge_request = UpstreamMergeRequest.new(mention_people: mention_people)
merge = UpstreamMerge.new(
origin: Project::GitlabEe.remotes[:gitlab],
upstream: Project::GitlabCe.remotes[:gitlab],
merge_branch: merge_request.source_branch)
merge_request.conflicts = merge.execute
merge_branch: upstream_merge_request.source_branch)
upstream_merge_request.conflicts = merge.execute
merge_request.create unless dry_run
upstream_merge_request.create unless dry_run
Result.new(true, { upstream_mr: merge_request })
rescue UpstreamMergeAlreadyInProgressError
Result.new(true, { upstream_mr: upstream_merge_request })
rescue UpstreamMergeInProgressError
return Result.new(false, { in_progress_mr_url: open_merge_requests.first.web_url })
end
def upstream_merge_request
@upstream_merge_request ||= UpstreamMergeRequest.new(mention_people: mention_people)
end
private
def check_for_open_upstream_mrs!(force = false)
if !force && open_merge_requests.any?
raise UpstreamMergeAlreadyInProgressError
end
def check_for_open_upstream_mrs!
raise UpstreamMergeInProgressError if open_merge_requests.any?
end
def open_merge_requests
......
......@@ -3,7 +3,7 @@ require_relative 'remote_repository'
class UpstreamMerge
attr_reader :origin, :upstream, :merge_branch
CONFLICT_MARKER_REGEX = /\A[A-Z]{2} /
CONFLICT_MARKER_REGEX = /\A(?<conflict_type>[ADU]{2}) /
def initialize(origin:, upstream:, merge_branch:)
@origin = origin
......@@ -31,7 +31,9 @@ class UpstreamMerge
end
def execute_upstream_merge
merge_no_ff
repository.fetch('master', remote: :upstream)
repository.merge('upstream/master', merge_branch, no_ff: true)
conflicts = compute_conflicts
conflicting_files = conflicts.map { |conflict_data| conflict_data[:path] }
......@@ -49,16 +51,13 @@ class UpstreamMerge
repository.cleanup
end
def merge_no_ff
repository.fetch('master', remote: :upstream)
repository.merge('upstream/master', merge_branch, no_ff: true)
end
def compute_conflicts
repository.status(short: true).lines.each_with_object([]) do |line, files|
path = line.sub(CONFLICT_MARKER_REGEX, '').chomp
# Store the file as key and conflict type as value, e.g.: { path: 'foo.rb', conflict_type: 'UU' }
files << { user: last_modifier(path), path: path, conflict_type: line[0, 2] } if line =~ CONFLICT_MARKER_REGEX
if line =~ CONFLICT_MARKER_REGEX
files << { user: last_modifier(path), path: path, conflict_type: $LAST_MATCH_INFO[:conflict_type] }
end
end
end
......
......@@ -2,25 +2,26 @@ require_relative 'commit_author'
require_relative 'merge_request'
class UpstreamMergeRequest < MergeRequest
PROJECT = Project::GitlabEe
LABELS = 'CE upstream'.freeze
UPSTREAM_MR_DESCRIPTION = <<~DESCRIPTION.freeze
Try to resolve one file per commit, and then push (no force-push!) to the `%s` branch.
Thanks in advance! ❤️
def self.project
Project::GitlabEe
end
Note: This merge request was created by an automated script.
Please report any issue at https://gitlab.com/gitlab-org/release-tools/issues!
DESCRIPTION
def self.labels
'CE upstream'.freeze
end
def self.open_mrs
GitlabClient
.merge_requests(PROJECT, labels: LABELS, state: 'opened')
.merge_requests(project, labels: labels, state: 'opened')
.select { |mr| mr.target_branch == 'master' }
end
def project
PROJECT
self.class.project
end
def labels
self.class.labels
end
def title
......@@ -29,25 +30,29 @@ class UpstreamMergeRequest < MergeRequest
def description
if conflicts.empty?
'Congrats, no conflicts!'
'**Congrats, no conflicts!** :tada:'
else
out = StringIO.new
out.puts("Files to resolve:\n\n")
conflicts.each do |conflict|
username = CommitAuthor.new(conflict[:user]).to_gitlab(reference: true)
username = CommitAuthor.new(conflict[:user]).to_gitlab
username = "`#{username}`" unless self[:mention_people]
out.puts conflict_checklist_item(user: username, file: conflict[:path], conflict_type: conflict[:conflict_type])
end
out.puts "\n#{UPSTREAM_MR_DESCRIPTION % source_branch}"
out.puts
out.puts <<~DESCRIPTION.freeze
Try to resolve one file per commit, and then push (no force-push!) to the `#{source_branch}` branch.
Thanks in advance! :heart:
Note: This merge request was created by an automated script.
Please report any issue at https://gitlab.com/gitlab-org/release-tools/issues!
DESCRIPTION
out.string
end
end
def labels
LABELS
end
def source_branch
self[:source_branch] || "ce-to-ee-#{Date.today.iso8601}"
end
......
......@@ -42,14 +42,12 @@ describe CommitAuthor do
shared_examples 'an author not from the team' do |git_name|
it "returns their Git name: #{git_name}" do
expect(subject.to_gitlab).to eq(git_name)
expect(subject.to_gitlab(reference: true)).to eq(git_name)
end
end
shared_examples 'an author from the team' do |gitlab_username|
it "returns their GitLab username: #{gitlab_username}" do
expect(subject.to_gitlab).to eq(gitlab_username)
expect(subject.to_gitlab(reference: true)).to eq("@#{gitlab_username}")
expect(subject.to_gitlab).to eq("@#{gitlab_username}")
end
end
......
......@@ -16,8 +16,7 @@ describe GitlabClient do
labels: 'CE upstream',
source_branch: 'feature',
target_branch: 'master',
milestone: nil,
remove_source_branch: true)
milestone: nil)
end
let(:default_params) do
......
......@@ -16,14 +16,6 @@ describe Issuable do
describe '#description' do
it { expect(subject.description).to eq RUBY_VERSION }
context 'when a description is set' do
subject { described_class.new(description: 'Hello World!') }
it 'returns the given description' do
expect(subject.description).to eq('Hello World!')
end
end
end
describe '#project' do
......
......@@ -260,13 +260,12 @@ describe RemoteRepository do
before do
allow(subject).to receive(:run_git).with(%w[add README.md])
expect(subject).to receive(:run_git).with(%w[commit])
.and_return([error, double(success?: false)])
.and_return([error, double(success?: false)])
end
it 'raises a CannotCommitError exception' do
expect do
subject.commit(%w[README.md])
end.to raise_error(RemoteRepository::CannotCommitError, error)
expect { subject.commit(%w[README.md]) }
.to raise_error(RemoteRepository::CannotCommitError, error)
end
end
end
......
......@@ -4,47 +4,48 @@ require 'services/upstream_merge_service'
describe Services::UpstreamMergeService do
around do |example|
Timecop.freeze do
Timecop.freeze(2017, 11, 15) do
example.run
end
end
shared_examples 'successful MR creation' do
let(:options) { {} }
let(:calls_create) { true }
let(:upstream_mr) do
double(
source_branch: 'ce-to-ee',
'conflicts=': nil,
title: 'Upstream MR',
url: 'http://foo.bar')
end
shared_context 'stub collaborators' do
before do
allow(subject).to receive(:check_for_open_upstream_mrs!).and_return(true)
expect(UpstreamMergeRequest).to receive(:new)
.with(
mention_people: options.fetch(:mention_people, false)
).and_return(upstream_mr)
.with(mention_people: subject.mention_people).and_call_original
expect(UpstreamMerge).to receive(:new)
.with(
origin: Project::GitlabEe.remotes[:gitlab],
upstream: Project::GitlabCe.remotes[:gitlab],
merge_branch: 'ce-to-ee'
merge_branch: 'ce-to-ee-2017-11-15'
).and_return(double(execute: []))
end
end
shared_examples 'successful MR creation' do
include_context 'stub collaborators'
it 'returns a successful result object' do
if calls_create
expect(upstream_mr).to receive(:create)
else
expect(upstream_mr).not_to receive(:create)
end
expect(subject.upstream_merge_request).to receive(:create)
result = subject.perform(options)
result = subject.perform
expect(result).to be_success
expect(result.payload).to eq({ upstream_mr: upstream_mr })
expect(result.payload).to eq({ upstream_mr: subject.upstream_merge_request })
end
end
shared_examples 'dry-run MR creation' do
include_context 'stub collaborators'
it 'returns a successful result object' do
expect(subject.upstream_merge_request).not_to receive(:create)
result = subject.perform
expect(result).to be_success
expect(result.payload).to eq({ upstream_mr: subject.upstream_merge_request })
end
end
......@@ -64,32 +65,47 @@ describe Services::UpstreamMergeService do
end
context 'when forced' do
subject { described_class.new(force: true) }
before do
expect(UpstreamMergeRequest).not_to receive(:open_mrs)
end
it_behaves_like 'successful MR creation' do
let(:options) { { force: true } }
context 'when real run (default)' do
it_behaves_like 'successful MR creation'
end
context 'when dry run' do
subject { described_class.new(dry_run: true, force: true) }
before do
expect(UpstreamMergeRequest).not_to receive(:open_mrs)
end
it_behaves_like 'dry-run MR creation'
end
end
end
context 'when no upstream MR exist' do
before do
expect(UpstreamMergeRequest).to receive(:open_mrs).and_return([])
end
context 'when real run (default)' do
it_behaves_like 'successful MR creation'
end
context 'when dry run' do
it_behaves_like 'successful MR creation' do
let(:calls_create) { false }
let(:options) { { dry_run: true } }
end
subject { described_class.new(dry_run: true) }
it_behaves_like 'dry-run MR creation'
end
context 'when mentioning people' do
it_behaves_like 'successful MR creation' do
let(:options) { { mention_people: true } }
end
subject { described_class.new(mention_people: true) }
it_behaves_like 'successful MR creation'
end
end
end
......
......@@ -4,68 +4,68 @@ require 'upstream_merge_request'
describe UpstreamMergeRequest do
around do |example|
Timecop.freeze do
Timecop.freeze(2017, 11, 15) do
example.run
end
end
describe '.project' do
it { expect(described_class.project).to eq Project::GitlabEe }
end
describe '.labels' do
it { expect(described_class.labels).to eq 'CE upstream' }
end
describe '.open_mrs' do
before do
expect(GitlabClient).to receive(:merge_requests)
.with(described_class.project, labels: described_class.labels, state: 'opened')
.and_return(merge_requests)
end
context 'when no open upstream MR exists' do
before do
allow(GitlabClient).to receive(:merge_requests)
.with(Project::GitlabEe, labels: described_class::LABELS, state: 'opened')
.and_return([])
end
let(:merge_requests) { [] }
it { expect(described_class.open_mrs).to be_empty }
end
context 'when an open upstream MR exists' do
let(:mr) { double(target_branch: 'master') }
let(:merge_requests) { [double(target_branch: 'master')] }
before do
allow(GitlabClient).to receive(:merge_requests)
.with(Project::GitlabEe, labels: described_class::LABELS, state: 'opened')
.and_return([mr])
context 'and the target branch is master' do
it { expect(described_class.open_mrs).to eq(merge_requests) }
end
it { expect(described_class.open_mrs).to eq([mr]) }
end
context 'and the target_branch is not master' do
let(:merge_requests) { [double(target_branch: '9-5-stable')] }
context 'when an open MR exists but the target_branch is not master' do
let(:mr) { double(target_branch: '9-5-stable') }
before do
allow(GitlabClient).to receive(:merge_requests)
.with(Project::GitlabEe, labels: described_class::LABELS, state: 'opened')
.and_return([mr])
it { expect(described_class.open_mrs).to be_empty }
end
it { expect(described_class.open_mrs).to be_empty }
end
end
describe '#project' do
it { expect(subject.project).to eq Project::GitlabEe }
it { expect(subject.project).to eq described_class.project }
end
describe '#labels' do
it { expect(subject.labels).to eq described_class.labels }
end
describe '#title' do
it 'generates a relavant title' do
Timecop.freeze do
expect(subject.title).to eq "CE upstream - #{Date.today.strftime('%A')}"
end
it 'generates a relevant title' do
expect(subject.title).to eq 'CE upstream - Wednesday'
end
end
describe '#labels' do
it { expect(subject.labels).to eq described_class::LABELS }
it { expect(subject.labels).to eq described_class.labels }
end
describe '#source_branch' do
it 'generates a relavant source branch name' do
Timecop.freeze do
expect(subject.source_branch).to eq "ce-to-ee-#{Date.today.iso8601}"
end
expect(subject.source_branch).to eq 'ce-to-ee-2017-11-15'
end
end
......@@ -79,22 +79,18 @@ describe UpstreamMergeRequest do
context 'conflicts is empty' do
it 'returns a nice description' do
expect(subject.description).to eq('Congrats, no conflicts!')
expect(subject.description).to eq('**Congrats, no conflicts!** :tada:')
end
end
context 'conflicts is not empty' do
let(:conflicts) do
[
before do
subject.conflicts = [
{ path: 'foo/bar.rb', user: 'John Doe', conflict_type: 'UU' },
{ path: 'bar/baz.rb', user: 'Rémy Coutable', conflict_type: 'AA' }
]
end
before do
subject.conflicts = conflicts
end
it 'returns a description with checklist items for conflicting files' do
expect(subject.description).to eq <<~CONTENT
Files to resolve:
......@@ -104,7 +100,7 @@ describe UpstreamMergeRequest do
Try to resolve one file per commit, and then push (no force-push!) to the `ce-to-ee-123` branch.
Thanks in advance! ❤️
Thanks in advance! :heart:
Note: This merge request was created by an automated script.
Please report any issue at https://gitlab.com/gitlab-org/release-tools/issues!
......@@ -116,20 +112,9 @@ describe UpstreamMergeRequest do
subject.mention_people = true
end
it 'returns a description with checklist items for conflicting files with usernames wrapped in backticks' do
expect(subject.description).to eq <<~CONTENT
Files to resolve:
- [ ] John Doe Please resolve [(UU) `foo/bar.rb`](https://gitlab.com/gitlab-org/gitlab-ee/blob/ce-to-ee-123/foo/bar.rb)
- [ ] @rymai Please resolve [(AA) `bar/baz.rb`](https://gitlab.com/gitlab-org/gitlab-ee/blob/ce-to-ee-123/bar/baz.rb)
Try to resolve one file per commit, and then push (no force-push!) to the `ce-to-ee-123` branch.
Thanks in advance! ❤️
Note: This merge request was created by an automated script.
Please report any issue at https://gitlab.com/gitlab-org/release-tools/issues!
CONTENT
it 'does not wrap usernames in backticks' do
expect(subject.description).to include('@rymai')
expect(subject.description).not_to include('`@rymai`')
end
end
end
......
......@@ -7,44 +7,43 @@ describe UpstreamMerge, :silence_stdout, :aggregate_failures do
let(:ee_repo_name) { 'gitlab-ee-upstream-merge' }
let(:ce_repo_name) { 'gitlab-ce-upstream-merge' }
let(:ee_fixture) { ConflictualFixture.new(File.expand_path("../fixtures/repositories/#{ee_repo_name}", __dir__)) }
let(:ce_fixture) { ConflictualFixture.new(File.expand_path("../fixtures/repositories/#{ce_repo_name}", __dir__)) }
let(:ee_fixture) { ConflictFixture.new(File.expand_path("../fixtures/repositories/#{ee_repo_name}", __dir__)) }
let(:ce_fixture) { ConflictFixture.new(File.expand_path("../fixtures/repositories/#{ce_repo_name}", __dir__)) }
let(:ee_repo_path) { File.join('/tmp', ee_repo_name) }
let(:ce_repo_path) { File.join('/tmp', ce_repo_name) }
let(:ee_repo_url) { "file://#{ee_fixture.fixture_path}" }
let(:ce_repo_url) { "file://#{ce_fixture.fixture_path}" }
let(:default_options) do
{
origin: ee_repo_url,
upstream: ce_repo_url,
upstream: "file://#{ce_fixture.fixture_path}",
merge_branch: "ce-to-ee-#{SecureRandom.hex}"
}
end
let(:current_git_author_name) { 'Your Name' }
let(:current_git_author) { { name: current_git_author_name, email: 'author@example.org' } }
let(:git_author_name) { 'Your Name' }
let(:git_author) { { name: git_author_name, email: 'author@example.org' } }
subject { described_class.new(default_options) }
before do
ee_fixture.rebuild_fixture!(author: current_git_author)
ce_fixture.rebuild_fixture!(author: current_git_author)
ee_fixture.rebuild_fixture!(author: git_author)
ce_fixture.rebuild_fixture!(author: git_author)
# Disable cleanup so that we can see what's the state of the temp Git repos
# allow_any_instance_of(RemoteRepository).to receive(:cleanup).and_return(true)
allow(subject).to receive(:after_upstream_merge).and_return(true)
allow_any_instance_of(RemoteRepository).to receive(:cleanup).and_return(true)
end
after do
# Manually perform the cleanup we disabled in the `before` block
allow(subject).to receive(:after_upstream_merge).and_call_original
subject.__send__(:after_upstream_merge)
FileUtils.rm_r(ee_repo_path, secure: true) if File.exist?(ee_repo_path)
FileUtils.rm_r(ce_repo_path, secure: true) if File.exist?(ce_repo_path)
end
describe '#execute' do
let(:repository) { subject.__send__(:repository) }
let(:ee_rugged_repo) { Rugged::Repository.new(repository.path) }
let(:ee_rugged_repo) { Rugged::Repository.new(ee_repo_path) }
before do
ce_fixture.unique_update_to_file!('CONTRIBUTING.md', author: current_git_author)
ee_fixture.unique_update_to_file!('README.md', author: current_git_author)
ce_fixture.unique_update_to_file('CONTRIBUTING.md', author: git_author)
ee_fixture.unique_update_to_file('README.md', author: git_author)
end
context 'when no conflict is detected' do
......@@ -52,47 +51,50 @@ describe UpstreamMerge, :silence_stdout, :aggregate_failures do
subject.execute
expect(ee_rugged_repo).to have_head(default_options[:merge_branch])
expect(File.read(File.join(repository.path, 'CONTRIBUTING.md'))).to match(/\AContent of CONTRIBUTING\.md in #{ce_fixture.fixture_path} is \h+\z/)
expect(File.read(File.join(repository.path, 'README.md'))).to match(/\AContent of README\.md in #{ee_fixture.fixture_path} is \h+\z/)
expect(File.read(File.join(ee_repo_path, 'CONTRIBUTING.md'))).to match(/\AContent of CONTRIBUTING\.md in #{ce_fixture.fixture_path} is \h+\z/)
expect(File.read(File.join(ee_repo_path, 'README.md'))).to match(/\AContent of README\.md in #{ee_fixture.fixture_path} is \h+\z/)
commits = `git -C #{repository.path} log -1 --date-order --pretty=format:'%B'`.lines
expect(commits).to start_with("Merge remote-tracking branch 'upstream/master' into #{default_options[:merge_branch]}\n")
expect(commits).not_to include('[ci skip]')
expect(ee_rugged_repo).to have_commit_message <<~COMMIT_MESSAGE
Merge remote-tracking branch 'upstream/master' into #{default_options[:merge_branch]}
COMMIT_MESSAGE
end
end
context 'when a conflict is detected' do
before do
ce_fixture.unique_update_to_file!('README.md', author: current_git_author)
ce_fixture.unique_update_to_file('README.md', author: git_author)
end
it 'returns the conflicts data' do
expect(subject.execute).to eq(
[{ user: current_git_author_name, path: 'README.md', conflict_type: 'UU' }])
[{ user: git_author_name, path: 'README.md', conflict_type: 'UU' }])
end
it 'commits the conflicts and includes `[ci skip]` in the commit message' do
subject.execute
expect(ee_rugged_repo).to have_head(default_options[:merge_branch])
expect(File.read(File.join(repository.path, 'README.md'))).to match <<~CONTENT
expect(File.read(File.join(ee_repo_path, 'README.md'))).to match <<~CONTENT
<<<<<<< HEAD
Content of README.md in #{ee_fixture.fixture_path} is \\h+
=======
Content of README.md in #{ce_fixture.fixture_path} is \\h+
>>>>>>> upstream/master
CONTENT
CONTENT
expect(ee_rugged_repo).to have_commit_message <<~COMMIT_MESSAGE
Merge remote-tracking branch 'upstream/master' into #{default_options[:merge_branch]}
last_commit = `git -C #{repository.path} log -1`
# Conflicts:
#\tREADME.md
expect(last_commit).to include("Merge remote-tracking branch 'upstream/master' into #{default_options[:merge_branch]}\n")
expect(last_commit).to include("[ci skip]")
[ci skip]
COMMIT_MESSAGE
end
end
it 'pushed the merge branch' do
expect(repository).to receive(:push).with(ee_repo_url, default_options[:merge_branch])
expect(subject.__send__(:repository)).to receive(:push).with(ee_repo_url, default_options[:merge_branch])
subject.execute
end
......
......@@ -3,11 +3,11 @@ require 'rugged'
require_relative 'repository_fixture'