diff options
| -rw-r--r-- | app/models/ci/build.rb | 37 | ||||
| -rw-r--r-- | app/models/external_issue.rb | 2 | ||||
| -rw-r--r-- | app/services/merge_requests/build_service.rb | 4 | ||||
| -rw-r--r-- | changelogs/unreleased/expired-ci-artifacts.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml | 5 | ||||
| -rw-r--r-- | db/migrate/20180119160751_optimize_ci_job_artifacts.rb | 23 | ||||
| -rw-r--r-- | db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb | 47 | ||||
| -rw-r--r-- | db/schema.rb | 4 | ||||
| -rw-r--r-- | doc/topics/autodevops/index.md | 4 | ||||
| -rw-r--r-- | lib/gitlab/background_migration/prepare_untracked_uploads.rb | 13 | ||||
| -rw-r--r-- | spec/factories/ci/builds.rb | 4 | ||||
| -rw-r--r-- | spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb | 170 | ||||
| -rw-r--r-- | spec/services/merge_requests/build_service_spec.rb | 172 |
13 files changed, 244 insertions, 246 deletions
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 20534b8eed0..7312e704932 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -41,12 +41,41 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } + + # This convoluted mess is because we need to handle two cases of + # artifact files during the migration. And a simple OR clause + # makes it impossible to optimize. + + # Instead we want to use UNION ALL and do two carefully + # constructed disjoint queries. But Rails cannot handle UNION or + # UNION ALL queries so we do the query in a subquery and wrap it + # in an otherwise redundant WHERE IN query (IN is fine for + # non-null columns). + + # This should all be ripped out when the migration is finished and + # replaced with just the new storage to avoid the extra work. + scope :with_artifacts, ->() do - where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', - '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) + old = Ci::Build.select(:id).where(%q[artifacts_file <> '']) + new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)], + Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) + where('ci_builds.id IN (? UNION ALL ?)', old, new) end - scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } - scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } + + scope :with_artifacts_not_expired, ->() do + old = Ci::Build.select(:id).where(%q[artifacts_file <> '' AND (artifacts_expire_at IS NULL OR artifacts_expire_at > ?)], Time.now) + new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)], + Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id AND (expire_at IS NULL OR expire_at > ?)', Time.now)) + where('ci_builds.id IN (? UNION ALL ?)', old, new) + end + + scope :with_expired_artifacts, ->() do + old = Ci::Build.select(:id).where(%q[artifacts_file <> '' AND artifacts_expire_at < ?], Time.now) + new = Ci::Build.select(:id).where(%q[(artifacts_file IS NULL OR artifacts_file = '') AND EXISTS (?)], + Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id AND expire_at < ?', Time.now)) + where('ci_builds.id IN (? UNION ALL ?)', old, new) + end + scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } diff --git a/app/models/external_issue.rb b/app/models/external_issue.rb index 2aaba2e4c90..282fd7edcb7 100644 --- a/app/models/external_issue.rb +++ b/app/models/external_issue.rb @@ -39,7 +39,7 @@ class ExternalIssue end def to_reference(_from = nil, full: nil) - id + reference_link_text end def reference_link_text(from = nil) diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index ab6f8ea44a9..4b186d93772 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -134,7 +134,7 @@ module MergeRequests end def append_closes_description - return unless issue + return unless issue&.to_reference.present? closes_issue = "Closes #{issue.to_reference}" @@ -163,7 +163,7 @@ module MergeRequests return if merge_request.title.present? if issue_iid.present? - merge_request.title = "Resolve #{issue_iid}" + merge_request.title = "Resolve #{issue.to_reference}" branch_title = source_branch.downcase.remove(issue_iid.downcase).titleize.humanize merge_request.title += " \"#{branch_title}\"" if branch_title.present? end diff --git a/changelogs/unreleased/expired-ci-artifacts.yml b/changelogs/unreleased/expired-ci-artifacts.yml new file mode 100644 index 00000000000..2fcbdb02f84 --- /dev/null +++ b/changelogs/unreleased/expired-ci-artifacts.yml @@ -0,0 +1,5 @@ +--- +title: Change SQL for expired artifacts to use new ci_job_artifacts.expire_at +merge_request: 16578 +author: +type: performance diff --git a/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml b/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml new file mode 100644 index 00000000000..fddfba94192 --- /dev/null +++ b/changelogs/unreleased/mk-fix-no-untracked-upload-files-error.yml @@ -0,0 +1,5 @@ +--- +title: Resolve PrepareUntrackedUploads PostgreSQL syntax error +merge_request: 17019 +author: +type: fixed diff --git a/db/migrate/20180119160751_optimize_ci_job_artifacts.rb b/db/migrate/20180119160751_optimize_ci_job_artifacts.rb new file mode 100644 index 00000000000..9b4340ed7b7 --- /dev/null +++ b/db/migrate/20180119160751_optimize_ci_job_artifacts.rb @@ -0,0 +1,23 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class OptimizeCiJobArtifacts < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + # job_id is just here to be a covering index for index only scans + # since we'll almost always be joining against ci_builds on job_id + add_concurrent_index(:ci_job_artifacts, [:expire_at, :job_id]) + add_concurrent_index(:ci_builds, [:artifacts_expire_at], where: "artifacts_file <> ''") + end + + def down + remove_concurrent_index(:ci_job_artifacts, [:expire_at, :job_id]) + remove_concurrent_index(:ci_builds, [:artifacts_expire_at], where: "artifacts_file <> ''") + end +end diff --git a/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb b/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb new file mode 100644 index 00000000000..e46e793d9d2 --- /dev/null +++ b/db/migrate/20180208183958_schedule_populate_untracked_uploads_if_needed.rb @@ -0,0 +1,47 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class SchedulePopulateUntrackedUploadsIfNeeded < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + FOLLOW_UP_MIGRATION = 'PopulateUntrackedUploads'.freeze + + class UntrackedFile < ActiveRecord::Base + include EachBatch + + self.table_name = 'untracked_files_for_uploads' + end + + def up + if table_exists?(:untracked_files_for_uploads) + process_or_remove_table + end + end + + def down + # nothing + end + + private + + def process_or_remove_table + if UntrackedFile.all.empty? + drop_temp_table + else + schedule_populate_untracked_uploads_jobs + end + end + + def drop_temp_table + drop_table(:untracked_files_for_uploads, if_exists: true) + end + + def schedule_populate_untracked_uploads_jobs + say "Scheduling #{FOLLOW_UP_MIGRATION} background migration jobs since there are rows in untracked_files_for_uploads." + + bulk_queue_background_migration_jobs_by_range( + UntrackedFile, FOLLOW_UP_MIGRATION) + end +end diff --git a/db/schema.rb b/db/schema.rb index d07a4c31618..6b43fc8403c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180206200543) do +ActiveRecord::Schema.define(version: 20180208183958) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -293,6 +293,7 @@ ActiveRecord::Schema.define(version: 20180206200543) do t.integer "failure_reason" end + add_index "ci_builds", ["artifacts_expire_at"], name: "index_ci_builds_on_artifacts_expire_at", where: "(artifacts_file <> ''::text)", using: :btree add_index "ci_builds", ["auto_canceled_by_id"], name: "index_ci_builds_on_auto_canceled_by_id", using: :btree add_index "ci_builds", ["commit_id", "stage_idx", "created_at"], name: "index_ci_builds_on_commit_id_and_stage_idx_and_created_at", using: :btree add_index "ci_builds", ["commit_id", "status", "type"], name: "index_ci_builds_on_commit_id_and_status_and_type", using: :btree @@ -333,6 +334,7 @@ ActiveRecord::Schema.define(version: 20180206200543) do t.string "file" end + add_index "ci_job_artifacts", ["expire_at", "job_id"], name: "index_ci_job_artifacts_on_expire_at_and_job_id", using: :btree add_index "ci_job_artifacts", ["job_id", "file_type"], name: "index_ci_job_artifacts_on_job_id_and_file_type", unique: true, using: :btree add_index "ci_job_artifacts", ["project_id"], name: "index_ci_job_artifacts_on_project_id", using: :btree diff --git a/doc/topics/autodevops/index.md b/doc/topics/autodevops/index.md index 01bd925bd6f..5f5ba2b69bc 100644 --- a/doc/topics/autodevops/index.md +++ b/doc/topics/autodevops/index.md @@ -95,7 +95,9 @@ Auto Deploy, and Auto Monitoring will be silently skipped. The Auto DevOps base domain is required if you want to make use of [Auto Review Apps](#auto-review-apps) and [Auto Deploy](#auto-deploy). It is defined -under the project's CI/CD settings while [enabling Auto DevOps](#enabling-auto-devops). +either under the project's CI/CD settings while +[enabling Auto DevOps](#enabling-auto-devops) or in instance-wide settings in +the CI/CD section. It can also be set at the project or group level as a variable, `AUTO_DEVOPS_DOMAIN`. A wildcard DNS A record matching the base domain is required, for example, diff --git a/lib/gitlab/background_migration/prepare_untracked_uploads.rb b/lib/gitlab/background_migration/prepare_untracked_uploads.rb index a7a1bbe1752..298de005b9b 100644 --- a/lib/gitlab/background_migration/prepare_untracked_uploads.rb +++ b/lib/gitlab/background_migration/prepare_untracked_uploads.rb @@ -43,7 +43,11 @@ module Gitlab store_untracked_file_paths - schedule_populate_untracked_uploads_jobs + if UntrackedFile.all.empty? + drop_temp_table + else + schedule_populate_untracked_uploads_jobs + end end private @@ -92,7 +96,7 @@ module Gitlab end end - yield(paths) + yield(paths) if paths.any? end def build_find_command(search_dir) @@ -165,6 +169,11 @@ module Gitlab bulk_queue_background_migration_jobs_by_range( UntrackedFile, FOLLOW_UP_MIGRATION) end + + def drop_temp_table + UntrackedFile.connection.drop_table(:untracked_files_for_uploads, + if_exists: true) + end end end end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 6ba599cdf83..f6ba3a581ca 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -180,8 +180,8 @@ FactoryBot.define do trait :artifacts do after(:create) do |build| - create(:ci_job_artifact, :archive, job: build) - create(:ci_job_artifact, :metadata, job: build) + create(:ci_job_artifact, :archive, job: build, expire_at: build.artifacts_expire_at) + create(:ci_job_artifact, :metadata, job: build, expire_at: build.artifacts_expire_at) build.reload end end diff --git a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb index 370c2490b97..4c4c143afd6 100644 --- a/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb +++ b/spec/lib/gitlab/background_migration/prepare_untracked_uploads_spec.rb @@ -8,8 +8,6 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do before do DatabaseCleaner.clean - - drop_temp_table_if_exists end after do @@ -23,57 +21,7 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do end end - # E.g. The installation is in use at the time of migration, and someone has - # just uploaded a file - shared_examples 'does not add files in /uploads/tmp' do - let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } - - before do - FileUtils.mkdir(File.dirname(tmp_file)) - FileUtils.touch(tmp_file) - end - - after do - FileUtils.rm(tmp_file) - end - - it 'does not add files from /uploads/tmp' do - described_class.new.perform - - expect(untracked_files_for_uploads.count).to eq(5) - end - end - - it 'ensures the untracked_files_for_uploads table exists' do - expect do - described_class.new.perform - end.to change { ActiveRecord::Base.connection.table_exists?(:untracked_files_for_uploads) }.from(false).to(true) - end - - it 'has a path field long enough for really long paths' do - described_class.new.perform - - component = 'a' * 255 - - long_path = [ - 'uploads', - component, # project.full_path - component # filename - ].flatten.join('/') - - record = untracked_files_for_uploads.create!(path: long_path) - expect(record.reload.path.size).to eq(519) - end - - context "test bulk insert with ON CONFLICT DO NOTHING or IGNORE" do - around do |example| - # If this is CI, we use Postgres 9.2 so this whole context should be - # skipped since we're unable to use ON CONFLICT DO NOTHING or IGNORE. - if described_class.new.send(:can_bulk_insert_and_ignore_duplicates?) - example.run - end - end - + shared_examples 'prepares the untracked_files_for_uploads table' do context 'when files were uploaded before and after hashed storage was enabled' do let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) } let!(:user) { create(:user, :with_avatar) } @@ -90,6 +38,21 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do UploadService.new(project2, uploaded_file, FileUploader).execute end + it 'has a path field long enough for really long paths' do + described_class.new.perform + + component = 'a' * 255 + + long_path = [ + 'uploads', + component, # project.full_path + component # filename + ].flatten.join('/') + + record = untracked_files_for_uploads.create!(path: long_path) + expect(record.reload.path.size).to eq(519) + end + it 'adds unhashed files to the untracked_files_for_uploads table' do described_class.new.perform @@ -130,91 +93,66 @@ describe Gitlab::BackgroundMigration::PrepareUntrackedUploads, :sidekiq do end end + # E.g. The installation is in use at the time of migration, and someone has + # just uploaded a file context 'when there are files in /uploads/tmp' do - it_behaves_like 'does not add files in /uploads/tmp' - end - end - end - - context 'test bulk insert without ON CONFLICT DO NOTHING or IGNORE' do - before do - # If this is CI, we use Postgres 9.2 so this stub has no effect. - # - # If this is being run on Postgres 9.5+ or MySQL, then this stub allows us - # to test the bulk insert functionality without ON CONFLICT DO NOTHING or - # IGNORE. - allow_any_instance_of(described_class).to receive(:postgresql_pre_9_5?).and_return(true) - end - - context 'when files were uploaded before and after hashed storage was enabled' do - let!(:appearance) { create_or_update_appearance(logo: uploaded_file, header_logo: uploaded_file) } - let!(:user) { create(:user, :with_avatar) } - let!(:project1) { create(:project, :with_avatar) } - let(:project2) { create(:project) } # instantiate after enabling hashed_storage - - before do - # Markdown upload before enabling hashed_storage - UploadService.new(project1, uploaded_file, FileUploader).execute + let(:tmp_file) { Rails.root.join(described_class::ABSOLUTE_UPLOAD_DIR, 'tmp', 'some_file.jpg') } - stub_application_setting(hashed_storage_enabled: true) - - # Markdown upload after enabling hashed_storage - UploadService.new(project2, uploaded_file, FileUploader).execute - end - - it 'adds unhashed files to the untracked_files_for_uploads table' do - described_class.new.perform - - expect(untracked_files_for_uploads.count).to eq(5) - end - - it 'adds files with paths relative to CarrierWave.root' do - described_class.new.perform - untracked_files_for_uploads.all.each do |file| - expect(file.path.start_with?('uploads/')).to be_truthy + before do + FileUtils.mkdir(File.dirname(tmp_file)) + FileUtils.touch(tmp_file) end - end - - it 'does not add hashed files to the untracked_files_for_uploads table' do - described_class.new.perform - hashed_file_path = project2.uploads.where(uploader: 'FileUploader').first.path - expect(untracked_files_for_uploads.where("path like '%#{hashed_file_path}%'").exists?).to be_falsey - end + after do + FileUtils.rm(tmp_file) + end - it 'correctly schedules the follow-up background migration jobs' do - described_class.new.perform + it 'does not add files from /uploads/tmp' do + described_class.new.perform - expect(described_class::FOLLOW_UP_MIGRATION).to be_scheduled_migration(1, 5) - expect(BackgroundMigrationWorker.jobs.size).to eq(1) + expect(untracked_files_for_uploads.count).to eq(5) + end end - # E.g. from a previous failed run of this background migration - context 'when there is existing data in untracked_files_for_uploads' do - before do - described_class.new.perform - end + context 'when the last batch size exactly matches the max batch size' do + it 'does not raise error' do + stub_const("#{described_class}::FIND_BATCH_SIZE", 5) - it 'does not error or produce duplicates of existing data' do expect do described_class.new.perform - end.not_to change { untracked_files_for_uploads.count }.from(5) + end.not_to raise_error + + expect(untracked_files_for_uploads.count).to eq(5) end end + end + end - context 'when there are files in /uploads/tmp' do - it_behaves_like 'does not add files in /uploads/tmp' - end + # If running on Postgres 9.2 (like on CI), this whole context is skipped + # since we're unable to use ON CONFLICT DO NOTHING or IGNORE. + context "test bulk insert with ON CONFLICT DO NOTHING or IGNORE", if: described_class.new.send(:can_bulk_insert_and_ignore_duplicates?) do + it_behaves_like 'prepares the untracked_files_for_uploads table' + end + + # If running on Postgres 9.2 (like on CI), the stubbed method has no effect. + # + # If running on Postgres 9.5+ or MySQL, then this context effectively tests + # the bulk insert functionality without ON CONFLICT DO NOTHING or IGNORE. + context 'test bulk insert without ON CONFLICT DO NOTHING or IGNORE' do + before do + allow_any_instance_of(described_class).to receive(:postgresql_pre_9_5?).and_return(true) end + + it_behaves_like 'prepares the untracked_files_for_uploads table' end # Very new or lightly-used installations that are running this migration # may not have an upload directory because they have no uploads. context 'when no files were ever uploaded' do - it 'does not add to the untracked_files_for_uploads table (and does not raise error)' do + it 'deletes the `untracked_files_for_uploads` table (and does not raise error)' do described_class.new.perform - expect(untracked_files_for_uploads.count).to eq(0) + expect(untracked_files_for_uploads.connection.table_exists?(:untracked_files_for_uploads)).to be_falsey end end end diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb index a0d0a4fd81b..3a935d98540 100644 --- a/spec/services/merge_requests/build_service_spec.rb +++ b/spec/services/merge_requests/build_service_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' describe MergeRequests::BuildService do + using RSpec::Parameterized::TableSyntax include RepoHelpers let(:project) { create(:project, :repository) } @@ -111,6 +112,7 @@ describe MergeRequests::BuildService do context 'one commit in the diff' do let(:commits) { Commit.decorate([commit_1], project) } + let(:commit_description) { commit_1.safe_message.split(/\n+/, 2).last } before do stub_compare @@ -125,7 +127,7 @@ describe MergeRequests::BuildService do end it 'uses the description of the commit as the description of the merge request' do - expect(merge_request.description).to eq(commit_1.safe_message.split(/\n+/, 2).last) + expect(merge_request.description).to eq(commit_description) end context 'merge request already has a description set' do @@ -148,68 +150,32 @@ describe MergeRequests::BuildService do end end - context 'branch starts with issue IID followed by a hyphen' do - let(:source_branch) { "#{issue.iid}-fix-issue" } - - it 'appends "Closes #$issue-iid" to the description' do - expect(merge_request.description).to eq("#{commit_1.safe_message.split(/\n+/, 2).last}\n\nCloses ##{issue.iid}") + context 'when the source branch matches an issue' do + where(:issue_tracker, :source_branch, :closing_message) do + :jira | 'FOO-123-fix-issue' | 'Closes FOO-123' + :jira | 'fix-issue' | nil + :custom_issue_tracker | '123-fix-issue' | 'Closes #123' + :custom_issue_tracker | 'fix-issue' | nil + :internal | '123-fix-issue' | 'Closes #123' + :internal | 'fix-issue' | nil end - context 'merge request already has a description set' do - let(:description) { 'Merge request description' } - - it 'appends "Closes #$issue-iid" to the description' do - expect(merge_request.description).to eq("#{description}\n\nCloses ##{issue.iid}") + with_them do + before do + if issue_tracker == :internal + issue.update!(iid: 123) + else + create(:"#{issue_tracker}_service", project: project) + end end - end - context 'commit has no description' do - let(:commits) { Commit.decorate([commit_2], project) } + it 'appends the closing description' do + expected_description = [commit_description, closing_message].compact.join("\n\n") - it 'sets the description to "Closes #$issue-iid"' do - expect(merge_request.description).to eq("Closes ##{issue.iid}") + expect(merge_request.description).to eq(expected_description) end end end - - context 'branch starts with numeric characters followed by a hyphen with no issue tracker' do - let(:source_branch) { '12345-fix-issue' } - - before do - allow(project).to receive(:external_issue_tracker).and_return(false) - allow(project).to receive(:issues_enabled?).and_return(false) - end - - it 'uses the title of the commit as the title of the merge request' do - expect(merge_request.title).to eq(commit_1.safe_message.split("\n").first) - end - - it 'uses the description of the commit as the description of the merge request' do - commit_description = commit_1.safe_message.split(/\n+/, 2).last - - expect(merge_request.description).to eq("#{commit_description}") - end - end - - context 'branch starts with JIRA-formatted external issue IID followed by a hyphen' do - let(:source_branch) { 'EXMPL-12345-fix-issue' } - - before do - allow(project).to receive(:external_issue_tracker).and_return(true) - allow(project).to receive(:issues_enabled?).and_return(false) - allow(project).to receive(:external_issue_reference_pattern).and_return(IssueTrackerService.reference_pattern) - end - - it 'uses the title of the commit as the title of the merge request' do - expect(merge_request.title).to eq(commit_1.safe_message.split("\n").first) - end - - it 'uses the description of the commit as the description of the merge request and appends the closes text' do - commit_description = commit_1.safe_message.split(/\n+/, 2).last - - expect(merge_request.description).to eq("#{commit_description}\n\nCloses EXMPL-12345") - end - end end context 'more than one commit in the diff' do @@ -239,90 +205,62 @@ describe MergeRequests::BuildService do end end - context 'branch starts with GitLab issue IID followed by a hyphen' do - let(:source_branch) { "#{issue.iid}-fix-issue" } - - it 'sets the title to: Resolves "$issue-title"' do - expect(merge_request.title).to eq("Resolve \"#{issue.title}\"") + context 'when the source branch matches an issue' do + where(:issue_tracker, :source_branch, :title, :closing_message) do + :jira | 'FOO-123-fix-issue' | 'Resolve FOO-123 "Fix issue"' | 'Closes FOO-123' + :jira | 'fix-issue' | 'Fix issue' | nil + :custom_issue_tracker | '123-fix-issue' | 'Resolve #123 "Fix issue"' | 'Closes #123' + :custom_issue_tracker | 'fix-issue' | 'Fix issue' | nil + :internal | '123-fix-issue' | 'Resolve "A bug"' | 'Closes #123' + :internal | 'fix-issue' | 'Fix issue' | nil + :internal | '124-fix-issue' | '124 fix issue' | nil end - context 'when issue is not accessible to user' do + with_them do before do - project.team.truncate - end - - it 'uses branch title as the merge request title' do - expect(merge_request.title).to eq("#{issue.iid} fix issue") + if issue_tracker == :internal + issue.update!(iid: 123) + else + create(:"#{issue_tracker}_service", project: project) + end end - end - - context 'issue does not exist' do - let(:source_branch) { "#{issue.iid.succ}-fix-issue" } - it 'uses the title of the branch as the merge request title' do - expect(merge_request.title).to eq("#{issue.iid.succ} fix issue") + it 'sets the correct title' do + expect(merge_request.title).to eq(title) end - end - - context 'issue is confidential' do - let(:issue_confidential) { true } - it 'uses the title of the branch as the merge request title' do - expect(merge_request.title).to eq("#{issue.iid} fix issue") + it 'sets the closing description' do + expect(merge_request.description).to eq(closing_message) end end end - context 'branch starts with numeric characters followed by a hyphen with no issue tracker' do - let(:source_branch) { '12345-fix-issue' } + context 'when the issue is not accessible to user' do + let(:source_branch) { "#{issue.iid}-fix-issue" } before do - allow(project).to receive(:external_issue_tracker).and_return(false) - allow(project).to receive(:issues_enabled?).and_return(false) + project.team.truncate end - it 'sets the title to the humanized branch title' do - expect(merge_request.title).to eq('12345 fix issue') + it 'uses branch title as the merge request title' do + expect(merge_request.title).to eq("#{issue.iid} fix issue") end - end - describe 'with JIRA enabled' do - before do - allow(project).to receive(:external_issue_tracker).and_return(true) - allow(project).to receive(:issues_enabled?).and_return(false) - allow(project).to receive(:external_issue_reference_pattern).and_return(IssueTrackerService.reference_pattern) + it 'does not set a description' do + expect(merge_request.description).to be_nil end + end - context 'branch does not start with JIRA-formatted external issue IID' do - let(:source_branch) { 'test-branch' } + context 'when the issue is confidential' do + let(:source_branch) { "#{issue.iid}-fix-issue" } + let(:issue_confidential) { true } - it 'sets the title to the humanized branch title' do - expect(merge_request.title).to eq('Test branch') - end + it 'uses the title of the branch as the merge request title' do + expect(merge_request.title).to eq("#{issue.iid} fix issue") end - context 'branch starts with JIRA-formatted external issue IID' do - let(:source_branch) { 'EXMPL-12345' } - - it 'sets the title to the humanized branch title' do - expect(merge_request.title).to eq('Resolve EXMPL-12345') - end - - it 'appends the closes text' do - expect(merge_request.description).to eq('Closes EXMPL-12345') - end - - context 'followed by hyphenated text' do - let(:source_branch) { 'EXMPL-12345-fix-issue' } - - it 'sets the title to the humanized branch title' do - expect(merge_request.title).to eq('Resolve EXMPL-12345 "Fix issue"') - end - - it 'appends the closes text' do - expect(merge_request.description).to eq('Closes EXMPL-12345') - end - end + it 'does not set a description' do + expect(merge_request.description).to be_nil end end end |