diff options
30 files changed, 938 insertions, 252 deletions
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb index fc708400657..d77f64a84f5 100644 --- a/app/controllers/projects/branches_controller.rb +++ b/app/controllers/projects/branches_controller.rb @@ -25,15 +25,6 @@ class Projects::BranchesController < Projects::ApplicationController @refs_pipelines = @project.ci_pipelines.latest_successful_for_refs(@branches.map(&:name)) @merged_branch_names = repository.merged_branch_names(@branches.map(&:name)) - # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/48097 - Gitlab::GitalyClient.allow_n_plus_1_calls do - @max_commits = @branches.reduce(0) do |memo, branch| - diverging_commit_counts = repository.diverging_commit_counts(branch) - [memo, diverging_commit_counts.values_at(:behind, :ahead, :distance)] - .flatten.compact.max - end - end - # https://gitlab.com/gitlab-org/gitlab-ce/issues/48097 Gitlab::GitalyClient.allow_n_plus_1_calls do render @@ -51,6 +42,19 @@ class Projects::BranchesController < Projects::ApplicationController @branches = @repository.recent_branches end + def diverging_commit_counts + respond_to do |format| + format.json do + service = Branches::DivergingCommitCountsService.new(repository) + branches = BranchesFinder.new(repository, params.permit(names: [])).execute + + Gitlab::GitalyClient.allow_n_plus_1_calls do + render json: branches.to_h { |branch| [branch.name, service.call(branch)] } + end + end + end + end + # rubocop: disable CodeReuse/ActiveRecord def create branch_name = strip_tags(sanitize(params[:branch_name])) @@ -64,8 +68,9 @@ class Projects::BranchesController < Projects::ApplicationController success = (result[:status] == :success) if params[:issue_iid] && success - issue = IssuesFinder.new(current_user, project_id: @project.id).find_by(iid: params[:issue_iid]) - SystemNoteService.new_issue_branch(issue, @project, current_user, branch_name) if issue + target_project = confidential_issue_project || @project + issue = IssuesFinder.new(current_user, project_id: target_project.id).find_by(iid: params[:issue_iid]) + SystemNoteService.new_issue_branch(issue, target_project, current_user, branch_name, branch_project: @project) if issue end respond_to do |format| @@ -162,4 +167,15 @@ class Projects::BranchesController < Projects::ApplicationController @branches = Kaminari.paginate_array(@branches).page(params[:page]) end end + + def confidential_issue_project + return unless Feature.enabled?(:create_confidential_merge_request, @project) + return if params[:confidential_issue_project_id].blank? + + confidential_issue_project = Project.find(params[:confidential_issue_project_id]) + + return unless can?(current_user, :update_issue, confidential_issue_project) + + confidential_issue_project + end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index f221f0363d3..e275b417784 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -172,6 +172,7 @@ class Projects::IssuesController < Projects::ApplicationController def create_merge_request create_params = params.slice(:branch_name, :ref).merge(issue_iid: issue.iid) + create_params[:target_project_id] = params[:target_project_id] if Feature.enabled?(:create_confidential_merge_request, @project) result = ::MergeRequests::CreateFromIssueService.new(project, current_user, create_params).execute if result[:status] == :success diff --git a/app/finders/branches_finder.rb b/app/finders/branches_finder.rb index 45d5591e81b..b462c8053fa 100644 --- a/app/finders/branches_finder.rb +++ b/app/finders/branches_finder.rb @@ -9,6 +9,7 @@ class BranchesFinder def execute branches = repository.branches_sorted_by(sort) branches = by_search(branches) + branches = by_names(branches) branches end @@ -16,6 +17,10 @@ class BranchesFinder attr_reader :repository, :params + def names + @params[:names].presence + end + def search @params[:search].presence end @@ -59,4 +64,13 @@ class BranchesFinder def find_exact_match_index(matches, term) matches.index { |branch| branch.name.casecmp(term) == 0 } end + + def by_names(branches) + return branches unless names + + branch_names = names.to_set + branches.filter do |branch| + branch_names.include?(branch.name) + end + end end diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index cd645850af3..fbd8036653a 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -23,11 +23,6 @@ class ApplicationSetting < ApplicationRecord serialize :domain_blacklist, Array # rubocop:disable Cop/ActiveRecordSerialize serialize :repository_storages # rubocop:disable Cop/ActiveRecordSerialize - ignore_column :circuitbreaker_failure_count_threshold - ignore_column :circuitbreaker_failure_reset_time - ignore_column :circuitbreaker_storage_timeout - ignore_column :circuitbreaker_access_retries - ignore_column :circuitbreaker_check_interval ignore_column :koding_url ignore_column :koding_enabled ignore_column :sentry_enabled diff --git a/app/models/project.rb b/app/models/project.rb index b102e0580e7..0f4fba5d0b6 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1445,11 +1445,6 @@ class Project < ApplicationRecord end def in_fork_network_of?(other_project) - # TODO: Remove this in a next release when all fork_networks are populated - # This makes sure all MergeRequests remain valid while the projects don't - # have a fork_network yet. - return true if forked_from?(other_project) - return false if fork_network.nil? || other_project.fork_network.nil? fork_network == other_project.fork_network diff --git a/app/models/repository.rb b/app/models/repository.rb index e05d3dd58ac..992ed7485e5 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -282,46 +282,6 @@ class Repository ref_exists?(keep_around_ref_name(sha)) end - def diverging_commit_counts(branch) - return diverging_commit_counts_without_max(branch) if Feature.enabled?('gitaly_count_diverging_commits_no_max') - - ## TODO: deprecate the below code after 12.0 - @root_ref_hash ||= raw_repository.commit(root_ref).id - cache.fetch(:"diverging_commit_counts_#{branch.name}") do - # Rugged seems to throw a `ReferenceError` when given branch_names rather - # than SHA-1 hashes - branch_sha = branch.dereferenced_target.sha - - number_commits_behind, number_commits_ahead = - raw_repository.diverging_commit_count( - @root_ref_hash, - branch_sha, - max_count: MAX_DIVERGING_COUNT) - - if number_commits_behind + number_commits_ahead >= MAX_DIVERGING_COUNT - { distance: MAX_DIVERGING_COUNT } - else - { behind: number_commits_behind, ahead: number_commits_ahead } - end - end - end - - def diverging_commit_counts_without_max(branch) - @root_ref_hash ||= raw_repository.commit(root_ref).id - cache.fetch(:"diverging_commit_counts_without_max_#{branch.name}") do - # Rugged seems to throw a `ReferenceError` when given branch_names rather - # than SHA-1 hashes - branch_sha = branch.dereferenced_target.sha - - number_commits_behind, number_commits_ahead = - raw_repository.diverging_commit_count( - @root_ref_hash, - branch_sha) - - { behind: number_commits_behind, ahead: number_commits_ahead } - end - end - def archive_metadata(ref, storage_path, format = "tar.gz", append_sha:, path: nil) raw_repository.archive_metadata( ref, diff --git a/app/services/branches/diverging_commit_counts_service.rb b/app/services/branches/diverging_commit_counts_service.rb new file mode 100644 index 00000000000..f947cec1663 --- /dev/null +++ b/app/services/branches/diverging_commit_counts_service.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Branches + class DivergingCommitCountsService + def initialize(repository) + @repository = repository + @cache = Gitlab::RepositoryCache.new(repository) + end + + def call(branch) + if Feature.enabled?('gitaly_count_diverging_commits_no_max') + diverging_commit_counts_without_max(branch) + else + diverging_commit_counts(branch) + end + end + + private + + attr_reader :repository, :cache + + delegate :raw_repository, to: :repository + + def diverging_commit_counts(branch) + ## TODO: deprecate the below code after 12.0 + @root_ref_hash ||= raw_repository.commit(repository.root_ref).id + cache.fetch(:"diverging_commit_counts_#{branch.name}") do + number_commits_behind, number_commits_ahead = + repository.raw_repository.diverging_commit_count( + @root_ref_hash, + branch.dereferenced_target.sha, + max_count: Repository::MAX_DIVERGING_COUNT) + + if number_commits_behind + number_commits_ahead >= Repository::MAX_DIVERGING_COUNT + { distance: Repository::MAX_DIVERGING_COUNT } + else + { behind: number_commits_behind, ahead: number_commits_ahead } + end + end + end + + def diverging_commit_counts_without_max(branch) + @root_ref_hash ||= raw_repository.commit(repository.root_ref).id + cache.fetch(:"diverging_commit_counts_without_max_#{branch.name}") do + number_commits_behind, number_commits_ahead = + raw_repository.diverging_commit_count( + @root_ref_hash, + branch.dereferenced_target.sha) + + { behind: number_commits_behind, ahead: number_commits_ahead } + end + end + end +end diff --git a/app/services/merge_requests/create_from_issue_service.rb b/app/services/merge_requests/create_from_issue_service.rb index e69791872cc..2a217a6f689 100644 --- a/app/services/merge_requests/create_from_issue_service.rb +++ b/app/services/merge_requests/create_from_issue_service.rb @@ -6,17 +6,20 @@ module MergeRequests # branch - the name of new branch # ref - the source of new branch. - @branch_name = params[:branch_name] - @issue_iid = params[:issue_iid] - @ref = params[:ref] + @branch_name = params[:branch_name] + @issue_iid = params[:issue_iid] + @ref = params[:ref] + @target_project_id = params[:target_project_id] super(project, user) end def execute + return error('Project not found') if target_project.blank? + return error('Not allowed to create merge request') unless can_create_merge_request? return error('Invalid issue iid') unless @issue_iid.present? && issue.present? - result = CreateBranchService.new(project, current_user).execute(branch_name, ref) + result = CreateBranchService.new(target_project, current_user).execute(branch_name, ref) return result if result[:status] == :error new_merge_request = create(merge_request) @@ -26,7 +29,7 @@ module MergeRequests success(new_merge_request) else - SystemNoteService.new_issue_branch(issue, project, current_user, branch_name) + SystemNoteService.new_issue_branch(issue, project, current_user, branch_name, branch_project: target_project) error(new_merge_request.errors) end @@ -34,6 +37,10 @@ module MergeRequests private + def can_create_merge_request? + can?(current_user, :create_merge_request_from, target_project) + end + # rubocop: disable CodeReuse/ActiveRecord def issue @issue ||= IssuesFinder.new(current_user, project_id: project.id).find_by(iid: @issue_iid) @@ -45,21 +52,21 @@ module MergeRequests end def ref - return @ref if project.repository.branch_exists?(@ref) + return @ref if target_project.repository.branch_exists?(@ref) - project.default_branch || 'master' + target_project.default_branch || 'master' end def merge_request - MergeRequests::BuildService.new(project, current_user, merge_request_params).execute + MergeRequests::BuildService.new(target_project, current_user, merge_request_params).execute end def merge_request_params { issue_iid: @issue_iid, - source_project_id: project.id, + source_project_id: target_project.id, source_branch: branch_name, - target_project_id: project.id, + target_project_id: target_project.id, target_branch: ref } end @@ -67,5 +74,14 @@ module MergeRequests def success(merge_request) super().merge(merge_request: merge_request) end + + def target_project + @target_project ||= + if @target_project_id.present? + project.forks.find_by_id(@target_project_id) + else + project + end + end end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 1390f7cdf46..8f7cfe582ca 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -404,8 +404,9 @@ module SystemNoteService # Example note text: # # "created branch `201-issue-branch-button`" - def new_issue_branch(issue, project, author, branch) - link = url_helpers.project_compare_path(project, from: project.default_branch, to: branch) + def new_issue_branch(issue, project, author, branch, branch_project: nil) + branch_project ||= project + link = url_helpers.project_compare_path(branch_project, from: branch_project.default_branch, to: branch) body = "created branch [`#{branch}`](#{link}) to address this issue" @@ -413,7 +414,7 @@ module SystemNoteService end def new_merge_request(issue, project, author, merge_request) - body = "created merge request #{merge_request.to_reference} to address this issue" + body = "created merge request #{merge_request.to_reference(project)} to address this issue" create_note(NoteSummary.new(issue, project, author, body, action: 'merge')) end diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index 3638334d61c..f97b259f8f2 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/app/views/projects/branches/_branch.html.haml @@ -1,6 +1,6 @@ - merged = local_assigns.fetch(:merged, false) - commit = @repository.commit(branch.dereferenced_target) -- diverging_commit_counts = @repository.diverging_commit_counts(branch) +- diverging_commit_counts = Branches::DivergingCommitCountsService.new(@repository).call(branch) - number_commits_distance = diverging_commit_counts[:distance] - number_commits_behind = diverging_commit_counts[:behind] - number_commits_ahead = diverging_commit_counts[:ahead] diff --git a/changelogs/unreleased/id-stale-branches.yml b/changelogs/unreleased/id-stale-branches.yml new file mode 100644 index 00000000000..2f35c5a12c9 --- /dev/null +++ b/changelogs/unreleased/id-stale-branches.yml @@ -0,0 +1,5 @@ +--- +title: Add endpoint for fetching diverging commit counts +merge_request: 29802 +author: +type: performance diff --git a/changelogs/unreleased/tc-rake-orphan-artifacts.yml b/changelogs/unreleased/tc-rake-orphan-artifacts.yml new file mode 100644 index 00000000000..7081bee640a --- /dev/null +++ b/changelogs/unreleased/tc-rake-orphan-artifacts.yml @@ -0,0 +1,5 @@ +--- +title: Add rake task to clean orphan artifact files +merge_request: 29681 +author: +type: added diff --git a/config/initializers/6_validations.rb b/config/initializers/6_validations.rb index bf9e5a50382..827b15e5c8d 100644 --- a/config/initializers/6_validations.rb +++ b/config/initializers/6_validations.rb @@ -1,24 +1,15 @@ -def storage_name_valid?(name) - !!(name =~ /\A[a-zA-Z0-9\-_]+\z/) -end - def storage_validation_error(message) raise "#{message}. Please fix this in your gitlab.yml before starting GitLab." end def validate_storages_config - storage_validation_error('No repository storage path defined') if Gitlab.config.repositories.storages.empty? - - Gitlab.config.repositories.storages.each do |name, repository_storage| - storage_validation_error("\"#{name}\" is not a valid storage name") unless storage_name_valid?(name) - - %w(failure_count_threshold failure_reset_time storage_timeout).each do |setting| - # Falling back to the defaults is fine! - next if repository_storage[setting].nil? + if Gitlab.config.repositories.storages.empty? + storage_validation_error('No repository storage path defined') + end - unless repository_storage[setting].to_f > 0 - storage_validation_error("`#{setting}` for storage `#{name}` needs to be greater than 0") - end + Gitlab.config.repositories.storages.keys.each do |name| + unless /\A[a-zA-Z0-9\-_]+\z/.match?(name) + storage_validation_error("\"#{name}\" is not a valid storage name") end end end diff --git a/config/routes/repository.rb b/config/routes/repository.rb index 1ea0ae72614..b89e1c7f9af 100644 --- a/config/routes/repository.rb +++ b/config/routes/repository.rb @@ -52,7 +52,10 @@ scope format: false do end get '/branches/:state', to: 'branches#index', as: :branches_filtered, constraints: { state: /active|stale|all/ } - resources :branches, only: [:index, :new, :create, :destroy] + resources :branches, only: [:index, :new, :create, :destroy] do + get :diverging_commit_counts, on: :collection + end + delete :merged_branches, controller: 'branches', action: :destroy_all_merged resources :tags, only: [:index, :show, :new, :create, :destroy] do resource :release, controller: 'tags/releases', only: [:edit, :update] diff --git a/doc/administration/geo/replication/troubleshooting.md b/doc/administration/geo/replication/troubleshooting.md index 5394e6dd763..5bd6cc81362 100644 --- a/doc/administration/geo/replication/troubleshooting.md +++ b/doc/administration/geo/replication/troubleshooting.md @@ -504,6 +504,15 @@ To resolve this, run the following command: sudo gitlab-rake geo:db:refresh_foreign_tables ``` +## Expired artifacts + +If you notice for some reason there are more artifacts on the Geo +secondary node than on the Geo primary node, you can use the rake task +to [cleanup orphan artifact files](../../../raketasks/cleanup.md#remove-orphan-artifact-files). + +On a Geo **secondary** node, this command will also clean up all Geo +registry record related to the orphan files on disk. + ## Fixing common errors This section documents common errors reported in the Admin UI and how to fix them. diff --git a/doc/raketasks/cleanup.md b/doc/raketasks/cleanup.md index f5c788af578..f880f31c39e 100644 --- a/doc/raketasks/cleanup.md +++ b/doc/raketasks/cleanup.md @@ -92,3 +92,48 @@ I, [2018-08-02T10:26:47.598424 #45087] INFO -- : Looking for orphaned remote up I, [2018-08-02T10:26:47.753131 #45087] INFO -- : Moved to lost and found: @hashed/6b/DSC_6152.JPG -> lost_and_found/@hashed/6b/DSC_6152.JPG I, [2018-08-02T10:26:47.764356 #45087] INFO -- : Moved to lost and found: @hashed/79/02/7902699be42c8a8e46fbbb4501726517e86b22c56a189f7625a6da49081b2451/711491b29d3eb08837798c4909e2aa4d/DSC00314.jpg -> lost_and_found/@hashed/79/02/7902699be42c8a8e46fbbb4501726517e86b22c56a189f7625a6da49081b2451/711491b29d3eb08837798c4909e2aa4d/DSC00314.jpg ``` + +## Remove orphan artifact files + +When you notice there are more job artifacts files on disk than there +should be, you can run: + +```shell +gitlab-rake gitlab:cleanup:orphan_job_artifact_files +``` + +This command: + +- Scans through the entire artifacts folder. +- Checks which files still have a record in the database. +- If no database record is found, the file is deleted from disk. + +By default, this task does not delete anything but shows what it can +delete. Run the command with `DRY_RUN=false` if you actually want to +delete the files: + +```shell +gitlab-rake gitlab:cleanup:orphan_job_artifact_files DRY_RUN=false +``` + +You can also limit the number of files to delete with `LIMIT`: + +```shell +gitlab-rake gitlab:cleanup:orphan_job_artifact_files LIMIT=100` +``` + +This will only delete up to 100 files from disk. You can use this to +delete a small set for testing purposes. + +If you provide `DEBUG=1`, you'll see the full path of every file that +is detected as being an orphan. + +If `ionice` is installed, the tasks uses it to ensure the command is +not causing too much load on the disk. You can configure the niceness +level with `NICENESS`. Below are the valid levels, but consult +`man 1 ionice` to be sure. + +- `0` or `None` +- `1` or `Realtime` +- `2` or `Best-effort` (default) +- `3` or `Idle` diff --git a/lib/gitlab/cleanup/orphan_job_artifact_files.rb b/lib/gitlab/cleanup/orphan_job_artifact_files.rb new file mode 100644 index 00000000000..ee7164b3e55 --- /dev/null +++ b/lib/gitlab/cleanup/orphan_job_artifact_files.rb @@ -0,0 +1,132 @@ +# frozen_string_literal: true + +module Gitlab + module Cleanup + class OrphanJobArtifactFiles + include Gitlab::Utils::StrongMemoize + + ABSOLUTE_ARTIFACT_DIR = ::JobArtifactUploader.root.freeze + LOST_AND_FOUND = File.join(ABSOLUTE_ARTIFACT_DIR, '-', 'lost+found').freeze + BATCH_SIZE = 500 + DEFAULT_NICENESS = 'Best-effort' + + attr_accessor :batch, :total_found, :total_cleaned + attr_reader :limit, :dry_run, :niceness, :logger + + def initialize(limit: nil, dry_run: true, niceness: nil, logger: nil) + @limit = limit + @dry_run = dry_run + @niceness = niceness || DEFAULT_NICENESS + @logger = logger || Rails.logger + @total_found = @total_cleaned = 0 + + new_batch! + end + + def run! + log_info('Looking for orphan job artifacts to clean up') + + find_artifacts do |artifact_file| + batch << artifact_file + + clean_batch! if batch.full? + break if limit_reached? + end + + clean_batch! + + log_info("Processed #{total_found} job artifacts to find and clean #{total_cleaned} orphans.") + end + + private + + def new_batch! + self.batch = ::Gitlab::Cleanup::OrphanJobArtifactFilesBatch + .new(batch_size: batch_size, logger: logger, dry_run: dry_run) + end + + def clean_batch! + batch.clean! + + update_stats!(batch) + + new_batch! + end + + def update_stats!(batch) + self.total_found += batch.artifact_files.count + self.total_cleaned += batch.lost_and_found.count + end + + def limit_reached? + return false unless limit + + total_cleaned >= limit + end + + def batch_size + return BATCH_SIZE unless limit + return if limit_reached? + + todo = limit - total_cleaned + [BATCH_SIZE, todo].min + end + + def find_artifacts + Open3.popen3(*find_command) do |stdin, stdout, stderr, status_thread| + stdout.each_line do |line| + yield line + end + + log_error(stderr.read.color(:red)) unless status_thread.value.success? + end + end + + def find_command + strong_memoize(:find_command) do + cmd = %W[find -L #{absolute_artifact_dir}] + + # Search for Job Artifact IDs, they are found 6 directory + # levels deep. For example: + # shared/artifacts/2c/62/2c...a3/2019_02_27/836/628/job.log + # 1 2 3 4 5 6 + # | | | ^- date | ^- Job Artifact ID + # | | | ^- Job ID + # ^--+--+- components of hashed storage project path + cmd += %w[-mindepth 6 -maxdepth 6] + + # Artifact directories are named on their ID + cmd += %w[-type d] + + if ionice + raise ArgumentError, 'Invalid niceness' unless niceness.match?(/^\w[\w\-]*$/) + + cmd.unshift(*%W[#{ionice} --class #{niceness}]) + end + + log_info("find command: '#{cmd.join(' ')}'") + + cmd + end + end + + def absolute_artifact_dir + File.absolute_path(ABSOLUTE_ARTIFACT_DIR) + end + + def ionice + strong_memoize(:ionice) do + Gitlab::Utils.which('ionice') + end + end + + def log_info(msg, params = {}) + logger.info("#{'[DRY RUN]' if dry_run} #{msg}") + end + + def log_error(msg, params = {}) + logger.error(msg) + end + end + end +end diff --git a/lib/gitlab/cleanup/orphan_job_artifact_files_batch.rb b/lib/gitlab/cleanup/orphan_job_artifact_files_batch.rb new file mode 100644 index 00000000000..5c30258c0fc --- /dev/null +++ b/lib/gitlab/cleanup/orphan_job_artifact_files_batch.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +module Gitlab + module Cleanup + class OrphanJobArtifactFilesBatch + BatchFull = Class.new(StandardError) + + class ArtifactFile + attr_accessor :path + + def initialize(path) + @path = path + end + + def artifact_id + path.split('/').last.to_i + end + end + + include Gitlab::Utils::StrongMemoize + + attr_reader :batch_size, :dry_run + attr_accessor :artifact_files + + def initialize(batch_size:, dry_run: true, logger: Rails.logger) + @batch_size = batch_size + @dry_run = dry_run + @logger = logger + @artifact_files = [] + end + + def clean! + return if artifact_files.empty? + + lost_and_found.each do |artifact| + clean_one!(artifact) + end + end + + def full? + artifact_files.count >= batch_size + end + + def <<(artifact_path) + raise BatchFull, "Batch full! Already contains #{artifact_files.count} artifacts" if full? + + artifact_files << ArtifactFile.new(artifact_path) + end + + def lost_and_found + strong_memoize(:lost_and_found) do + artifact_file_ids = artifact_files.map(&:artifact_id) + existing_artifact_ids = ::Ci::JobArtifact.id_in(artifact_file_ids).pluck_primary_key + + artifact_files.reject { |artifact| existing_artifact_ids.include?(artifact.artifact_id) } + end + end + + private + + def clean_one!(artifact_file) + log_debug("Found orphan job artifact file @ #{artifact_file.path}") + + remove_file!(artifact_file) unless dry_run + end + + def remove_file!(artifact_file) + FileUtils.rm_rf(artifact_file.path) + end + + def log_info(msg, params = {}) + @logger.info("#{'[DRY RUN]' if dry_run} #{msg}") + end + + def log_debug(msg, params = {}) + @logger.debug(msg) + end + end + end +end diff --git a/lib/tasks/gitlab/cleanup.rake b/lib/tasks/gitlab/cleanup.rake index 760331620ef..105ef417df3 100644 --- a/lib/tasks/gitlab/cleanup.rake +++ b/lib/tasks/gitlab/cleanup.rake @@ -115,6 +115,18 @@ namespace :gitlab do end end + desc 'GitLab | Cleanup | Clean orphan job artifact files' + task orphan_job_artifact_files: :gitlab_environment do + warn_user_is_not_gitlab + + cleaner = Gitlab::Cleanup::OrphanJobArtifactFiles.new(limit: limit, dry_run: dry_run?, niceness: niceness, logger: logger) + cleaner.run! + + if dry_run? + logger.info "To clean up these files run this command with DRY_RUN=false".color(:yellow) + end + end + def remove? ENV['REMOVE'] == 'true' end @@ -123,12 +135,25 @@ namespace :gitlab do ENV['DRY_RUN'] != 'false' end + def debug? + ENV['DEBUG'].present? + end + + def limit + ENV['LIMIT']&.to_i + end + + def niceness + ENV['NICENESS'].presence + end + def logger return @logger if defined?(@logger) @logger = if Rails.env.development? || Rails.env.production? Logger.new(STDOUT).tap do |stdout_logger| stdout_logger.extend(ActiveSupport::Logger.broadcast(Rails.logger)) + stdout_logger.level = debug? ? Logger::DEBUG : Logger::INFO end else Rails.logger diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb index cf201c9f735..b30966e70a7 100644 --- a/spec/controllers/projects/branches_controller_spec.rb +++ b/spec/controllers/projects/branches_controller_spec.rb @@ -92,7 +92,7 @@ describe Projects::BranchesController do end it 'posts a system note' do - expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, "1-feature-branch") + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, "1-feature-branch", branch_project: project) post :create, params: { @@ -103,6 +103,75 @@ describe Projects::BranchesController do } end + context 'confidential_issue_project_id is present' do + let(:confidential_issue_project) { create(:project) } + + def create_branch_with_confidential_issue_project + post( + :create, + params: { + namespace_id: project.namespace, + project_id: project, + branch_name: branch, + confidential_issue_project_id: confidential_issue_project.id, + issue_iid: issue.iid + } + ) + end + + context 'create_confidential_merge_request feature is enabled' do + before do + stub_feature_flags(create_confidential_merge_request: true) + end + + context 'user cannot update issue' do + let(:issue) { create(:issue, project: confidential_issue_project) } + + it 'does not post a system note' do + expect(SystemNoteService).not_to receive(:new_issue_branch) + + create_branch_with_confidential_issue_project + end + end + + context 'user can update issue' do + before do + confidential_issue_project.add_reporter(user) + end + + context 'issue is under the specified project' do + let(:issue) { create(:issue, project: confidential_issue_project) } + + it 'posts a system note' do + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, confidential_issue_project, user, "1-feature-branch", branch_project: project) + + create_branch_with_confidential_issue_project + end + end + + context 'issue is not under the specified project' do + it 'does not post a system note' do + expect(SystemNoteService).not_to receive(:new_issue_branch) + + create_branch_with_confidential_issue_project + end + end + end + end + + context 'create_confidential_merge_request feature is disabled' do + before do + stub_feature_flags(create_confidential_merge_request: false) + end + + it 'posts a system note on project' do + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, "1-feature-branch", branch_project: project) + + create_branch_with_confidential_issue_project + end + end + end + context 'repository-less project' do let(:project) { create :project } @@ -507,4 +576,27 @@ describe Projects::BranchesController do end end end + + describe 'GET diverging_commit_counts' do + before do + sign_in(user) + + get :diverging_commit_counts, + format: :json, + params: { + namespace_id: project.namespace, + project_id: project, + names: ['fix', 'add-pdf-file', 'branch-merged'] + } + end + + it 'returns the commit counts behind and ahead of default branch' do + parsed_response = JSON.parse(response.body) + expect(parsed_response).to eq( + "fix" => { "behind" => 29, "ahead" => 2 }, + "branch-merged" => { "behind" => 1, "ahead" => 0 }, + "add-pdf-file" => { "behind" => 0, "ahead" => 3 } + ) + end + end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index f82e3c8c7dc..bc5e0b4671e 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Projects::IssuesController do + include ProjectForksHelper + let(:project) { create(:project) } let(:user) { create(:user) } let(:issue) { create(:issue, project: project) } @@ -1130,6 +1132,7 @@ describe Projects::IssuesController do end describe 'POST create_merge_request' do + let(:target_project_id) { nil } let(:project) { create(:project, :repository, :public) } before do @@ -1163,13 +1166,42 @@ describe Projects::IssuesController do expect(response).to have_gitlab_http_status(404) end + context 'target_project_id is set' do + let(:target_project) { fork_project(project, user, repository: true) } + let(:target_project_id) { target_project.id } + + context 'create_confidential_merge_request feature is enabled' do + before do + stub_feature_flags(create_confidential_merge_request: true) + end + + it 'creates a new merge request' do + expect { create_merge_request }.to change(target_project.merge_requests, :count).by(1) + end + end + + context 'create_confidential_merge_request feature is disabled' do + before do + stub_feature_flags(create_confidential_merge_request: false) + end + + it 'creates a new merge request' do + expect { create_merge_request }.to change(project.merge_requests, :count).by(1) + end + end + end + def create_merge_request - post :create_merge_request, params: { - namespace_id: project.namespace.to_param, - project_id: project.to_param, - id: issue.to_param - }, - format: :json + post( + :create_merge_request, + params: { + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: issue.to_param, + target_project_id: target_project_id + }, + format: :json + ) end end diff --git a/spec/finders/branches_finder_spec.rb b/spec/finders/branches_finder_spec.rb index 7d164539d9a..3fc86f3e408 100644 --- a/spec/finders/branches_finder_spec.rb +++ b/spec/finders/branches_finder_spec.rb @@ -62,6 +62,15 @@ describe BranchesFinder do expect(result.count).to eq(0) end + + it 'filters branches by provided names' do + branches_finder = described_class.new(repository, { names: ['fix', 'csv', 'lfs', 'does-not-exist'] }) + + result = branches_finder.execute + + expect(result.count).to eq(3) + expect(result.map(&:name)).to eq(%w{csv fix lfs}) + end end context 'filter and sort' do diff --git a/spec/initializers/6_validations_spec.rb b/spec/initializers/6_validations_spec.rb index f96e5a2133f..73fbd4c7a44 100644 --- a/spec/initializers/6_validations_spec.rb +++ b/spec/initializers/6_validations_spec.rb @@ -2,16 +2,6 @@ require 'spec_helper' require_relative '../../config/initializers/6_validations.rb' describe '6_validations' do - before :all do - FileUtils.mkdir_p('tmp/tests/paths/a/b/c/d') - FileUtils.mkdir_p('tmp/tests/paths/a/b/c2') - FileUtils.mkdir_p('tmp/tests/paths/a/b/d') - end - - after :all do - FileUtils.rm_rf('tmp/tests/paths') - end - describe 'validate_storages_config' do context 'with correct settings' do before do @@ -23,16 +13,6 @@ describe '6_validations' do end end - context 'when one of the settings is incorrect' do - before do - mock_storages('foo' => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/paths/a/b/c', 'failure_count_threshold' => 'not a number')) - end - - it 'throws an error' do - expect { validate_storages_config }.to raise_error(/failure_count_threshold/) - end - end - context 'with invalid storage names' do before do mock_storages('name with spaces' => Gitlab::GitalyClient::StorageSettings.new('path' => 'tmp/tests/paths/a/b/c')) diff --git a/spec/lib/gitlab/cleanup/orphan_job_artifact_files_batch_spec.rb b/spec/lib/gitlab/cleanup/orphan_job_artifact_files_batch_spec.rb new file mode 100644 index 00000000000..4d8edfeac80 --- /dev/null +++ b/spec/lib/gitlab/cleanup/orphan_job_artifact_files_batch_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Cleanup::OrphanJobArtifactFilesBatch do + let(:batch_size) { 10 } + let(:dry_run) { true } + + subject(:batch) { described_class.new(batch_size: batch_size, dry_run: dry_run) } + + context 'no dry run' do + let(:dry_run) { false } + + it 'deletes only orphan job artifacts from disk' do + job_artifact = create(:ci_job_artifact, :archive) + orphan_artifact = create(:ci_job_artifact, :archive) + batch << artifact_path(job_artifact) + batch << artifact_path(orphan_artifact) + orphan_artifact.delete + + batch.clean! + + expect(batch.artifact_files.count).to eq(2) + expect(batch.lost_and_found.count).to eq(1) + expect(batch.lost_and_found.first.artifact_id).to eq(orphan_artifact.id) + end + + it 'does not mix up job ID and artifact ID' do + # take maximum ID of both tables to avoid any collision + max_id = [Ci::Build.maximum(:id), Ci::JobArtifact.maximum(:id)].compact.max.to_i + job_a = create(:ci_build, id: max_id + 1) + job_b = create(:ci_build, id: max_id + 2) + # reuse the build IDs for the job artifact IDs, but swap them + job_artifact_b = create(:ci_job_artifact, :archive, job: job_b, id: max_id + 1) + job_artifact_a = create(:ci_job_artifact, :archive, job: job_a, id: max_id + 2) + + batch << artifact_path(job_artifact_a) + batch << artifact_path(job_artifact_b) + + job_artifact_b.delete + + batch.clean! + + expect(File.exist?(job_artifact_a.file.path)).to be_truthy + expect(File.exist?(job_artifact_b.file.path)).to be_falsey + end + end + + context 'with dry run' do + it 'does not remove files' do + job_artifact = create(:ci_job_artifact, :archive) + batch << job_artifact.file.path + job_artifact.delete + + expect(batch).not_to receive(:remove_file!) + + batch.clean! + + expect(File.exist?(job_artifact.file.path)).to be_truthy + end + end + + def artifact_path(job_artifact) + Pathname.new(job_artifact.file.path).parent.to_s + end +end diff --git a/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb b/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb new file mode 100644 index 00000000000..974cc2c4660 --- /dev/null +++ b/spec/lib/gitlab/cleanup/orphan_job_artifact_files_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Cleanup::OrphanJobArtifactFiles do + let(:null_logger) { Logger.new('/dev/null') } + subject(:cleanup) { described_class.new(logger: null_logger) } + + before do + allow(null_logger).to receive(:info) + end + + it 'passes on dry_run' do + expect(Gitlab::Cleanup::OrphanJobArtifactFilesBatch) + .to receive(:new) + .with(dry_run: false, batch_size: anything, logger: anything) + .at_least(:once) + .and_call_original + + described_class.new(dry_run: false).run! + end + + it 'errors when invalid niceness is given' do + cleanup = described_class.new(logger: null_logger, niceness: 'FooBar') + + expect(null_logger).to receive(:error).with(/FooBar/) + + cleanup.run! + end + + it 'finds artifacts on disk' do + artifact = create(:ci_job_artifact, :archive) + + expect(cleanup).to receive(:find_artifacts).and_yield(artifact.file.path) + cleanup.run! + end + + it 'stops when limit is reached' do + cleanup = described_class.new(limit: 1) + + mock_artifacts_found(cleanup, 'tmp/foo/bar/1', 'tmp/foo/bar/2') + + cleanup.run! + + expect(cleanup.total_found).to eq(1) + end + + it 'cleans even if batch is not full' do + mock_artifacts_found(cleanup, 'tmp/foo/bar/1') + + expect(cleanup).to receive(:clean_batch!).and_call_original + cleanup.run! + end + + it 'cleans in batches' do + stub_const("#{described_class.name}::BATCH_SIZE", 2) + mock_artifacts_found(cleanup, 'tmp/foo/bar/1', 'tmp/foo/bar/2', 'tmp/foo/bar/3') + + expect(cleanup).to receive(:clean_batch!).twice.and_call_original + cleanup.run! + end + + def mock_artifacts_found(cleanup, *files) + mock = allow(cleanup).to receive(:find_artifacts) + + files.each { |file| mock.and_yield(file) } + end +end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 0bd17dbacd7..13da7bd7407 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -2298,48 +2298,6 @@ describe Repository do end end - describe '#diverging_commit_counts' do - let(:diverged_branch) { repository.find_branch('fix') } - let(:root_ref_sha) { repository.raw_repository.commit(repository.root_ref).id } - let(:diverged_branch_sha) { diverged_branch.dereferenced_target.sha } - - it 'returns the commit counts behind and ahead of default branch' do - result = repository.diverging_commit_counts(diverged_branch) - - expect(result).to eq(behind: 29, ahead: 2) - end - - context 'when gitaly_count_diverging_commits_no_max is enabled' do - before do - stub_feature_flags(gitaly_count_diverging_commits_no_max: true) - end - - it 'calls diverging_commit_count without max count' do - expect(repository.raw_repository) - .to receive(:diverging_commit_count) - .with(root_ref_sha, diverged_branch_sha) - .and_return([29, 2]) - - repository.diverging_commit_counts(diverged_branch) - end - end - - context 'when gitaly_count_diverging_commits_no_max is disabled' do - before do - stub_feature_flags(gitaly_count_diverging_commits_no_max: false) - end - - it 'calls diverging_commit_count with max count' do - expect(repository.raw_repository) - .to receive(:diverging_commit_count) - .with(root_ref_sha, diverged_branch_sha, max_count: Repository::MAX_DIVERGING_COUNT) - .and_return([29, 2]) - - repository.diverging_commit_counts(diverged_branch) - end - end - end - describe '#refresh_method_caches' do it 'refreshes the caches of the given types' do expect(repository).to receive(:expire_method_caches) diff --git a/spec/services/branches/diverging_commit_counts_service_spec.rb b/spec/services/branches/diverging_commit_counts_service_spec.rb new file mode 100644 index 00000000000..bfdbebdb7c1 --- /dev/null +++ b/spec/services/branches/diverging_commit_counts_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Branches::DivergingCommitCountsService do + let(:project) { create(:project, :repository) } + let(:repository) { project.repository } + + describe '#call' do + let(:diverged_branch) { repository.find_branch('fix') } + let(:root_ref_sha) { repository.raw_repository.commit(repository.root_ref).id } + let(:diverged_branch_sha) { diverged_branch.dereferenced_target.sha } + + let(:service) { described_class.new(repository) } + + it 'returns the commit counts behind and ahead of default branch' do + result = service.call(diverged_branch) + + expect(result).to eq(behind: 29, ahead: 2) + end + + context 'when gitaly_count_diverging_commits_no_max is enabled' do + before do + stub_feature_flags(gitaly_count_diverging_commits_no_max: true) + end + + it 'calls diverging_commit_count without max count' do + expect(repository.raw_repository) + .to receive(:diverging_commit_count) + .with(root_ref_sha, diverged_branch_sha) + .and_return([29, 2]) + + service.call(diverged_branch) + end + end + + context 'when gitaly_count_diverging_commits_no_max is disabled' do + before do + stub_feature_flags(gitaly_count_diverging_commits_no_max: false) + end + + it 'calls diverging_commit_count with max count' do + expect(repository.raw_repository) + .to receive(:diverging_commit_count) + .with(root_ref_sha, diverged_branch_sha, max_count: Repository::MAX_DIVERGING_COUNT) + .and_return([29, 2]) + + service.call(diverged_branch) + end + end + end +end diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb index a0ac7dba89d..0e0da6a13ab 100644 --- a/spec/services/merge_requests/create_from_issue_service_spec.rb +++ b/spec/services/merge_requests/create_from_issue_service_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe MergeRequests::CreateFromIssueService do + include ProjectForksHelper + let(:project) { create(:project, :repository) } let(:user) { create(:user) } let(:label_ids) { create_pair(:label, project: project).map(&:id) } @@ -10,139 +12,174 @@ describe MergeRequests::CreateFromIssueService do let(:issue) { create(:issue, project: project, milestone_id: milestone_id) } let(:custom_source_branch) { 'custom-source-branch' } - subject(:service) { described_class.new(project, user, issue_iid: issue.iid) } - subject(:service_with_custom_source_branch) { described_class.new(project, user, issue_iid: issue.iid, branch_name: custom_source_branch) } + subject(:service) { described_class.new(project, user, service_params) } + subject(:service_with_custom_source_branch) { described_class.new(project, user, branch_name: custom_source_branch, **service_params) } before do project.add_developer(user) end describe '#execute' do - it 'returns an error with invalid issue iid' do - result = described_class.new(project, user, issue_iid: -1).execute + shared_examples_for 'a service that creates a merge request from an issue' do + it 'returns an error when user can not create merge request on target project' do + result = described_class.new(project, create(:user), service_params).execute - expect(result[:status]).to eq(:error) - expect(result[:message]).to eq('Invalid issue iid') - end + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Not allowed to create merge request') + end - it 'delegates issue search to IssuesFinder' do - expect_any_instance_of(IssuesFinder).to receive(:find_by).once.and_call_original + it 'returns an error with invalid issue iid' do + result = described_class.new(project, user, issue_iid: -1).execute - described_class.new(project, user, issue_iid: -1).execute - end - - it "inherits labels" do - issue.assign_attributes(label_ids: label_ids) + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Invalid issue iid') + end - result = service.execute + it 'creates a branch based on issue title' do + service.execute - expect(result[:merge_request].label_ids).to eq(label_ids) - end + expect(target_project.repository.branch_exists?(issue.to_branch_name)).to be_truthy + end - it "inherits milestones" do - result = service.execute + it 'creates a branch using passed name' do + service_with_custom_source_branch.execute - expect(result[:merge_request].milestone_id).to eq(milestone_id) - end + expect(target_project.repository.branch_exists?(custom_source_branch)).to be_truthy + end - it 'delegates the branch creation to CreateBranchService' do - expect_any_instance_of(CreateBranchService).to receive(:execute).once.and_call_original + it 'creates the new_merge_request system note' do + expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest)) - service.execute - end + service.execute + end - it 'creates a branch based on issue title' do - service.execute + it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do + expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false) - expect(project.repository.branch_exists?(issue.to_branch_name)).to be_truthy - end + expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name, branch_project: target_project) - it 'creates a branch using passed name' do - service_with_custom_source_branch.execute + service.execute + end - expect(project.repository.branch_exists?(custom_source_branch)).to be_truthy - end + it 'creates a merge request' do + expect { service.execute }.to change(target_project.merge_requests, :count).by(1) + end - it 'creates the new_merge_request system note' do - expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest)) + it 'sets the merge request author to current user' do + result = service.execute - service.execute - end + expect(result[:merge_request].author).to eq(user) + end - it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do - project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED) + it 'sets the merge request source branch to the new issue branch' do + result = service.execute - expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name) + expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) + end - service.execute - end + it 'sets the merge request source branch to the passed branch name' do + result = service_with_custom_source_branch.execute - it 'creates a merge request' do - expect { service.execute }.to change(project.merge_requests, :count).by(1) - end + expect(result[:merge_request].source_branch).to eq(custom_source_branch) + end - it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do - result = service.execute + it 'sets the merge request target branch to the project default branch' do + result = service.execute - expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"") - end + expect(result[:merge_request].target_branch).to eq(target_project.default_branch) + end - it 'sets the merge request author to current user' do - result = service.execute + it 'executes quick actions if the build service sets them in the description' do + allow(service).to receive(:merge_request).and_wrap_original do |m, *args| + m.call(*args).tap do |merge_request| + merge_request.description = "/assign #{user.to_reference}" + end + end - expect(result[:merge_request].author).to eq(user) - end + result = service.execute - it 'sets the merge request source branch to the new issue branch' do - result = service.execute + expect(result[:merge_request].assignees).to eq([user]) + end - expect(result[:merge_request].source_branch).to eq(issue.to_branch_name) - end + context 'when ref branch is set' do + subject { described_class.new(project, user, ref: 'feature', **service_params).execute } - it 'sets the merge request source branch to the passed branch name' do - result = service_with_custom_source_branch.execute + it 'sets the merge request source branch to the new issue branch' do + expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name) + end - expect(result[:merge_request].source_branch).to eq(custom_source_branch) - end + it 'sets the merge request target branch to the ref branch' do + expect(subject[:merge_request].target_branch).to eq('feature') + end - it 'sets the merge request target branch to the project default branch' do - result = service.execute + context 'when ref branch does not exist' do + subject { described_class.new(project, user, ref: 'no-such-branch', **service_params).execute } - expect(result[:merge_request].target_branch).to eq(project.default_branch) - end + it 'creates a merge request' do + expect { subject }.to change(target_project.merge_requests, :count).by(1) + end - it 'executes quick actions if the build service sets them in the description' do - allow(service).to receive(:merge_request).and_wrap_original do |m, *args| - m.call(*args).tap do |merge_request| - merge_request.description = "/assign #{user.to_reference}" + it 'sets the merge request target branch to the project default branch' do + expect(subject[:merge_request].target_branch).to eq(target_project.default_branch) + end end end + end - result = service.execute + context 'no target_project_id specified' do + let(:service_params) { { issue_iid: issue.iid } } + let(:target_project) { project } - expect(result[:merge_request].assignees).to eq([user]) - end + it_behaves_like 'a service that creates a merge request from an issue' - context 'when ref branch is set' do - subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'feature').execute } + it "inherits labels" do + issue.assign_attributes(label_ids: label_ids) - it 'sets the merge request source branch to the new issue branch' do - expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name) + result = service.execute + + expect(result[:merge_request].label_ids).to eq(label_ids) end - it 'sets the merge request target branch to the ref branch' do - expect(subject[:merge_request].target_branch).to eq('feature') + it "inherits milestones" do + result = service.execute + + expect(result[:merge_request].milestone_id).to eq(milestone_id) end - context 'when ref branch does not exist' do - subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'no-such-branch').execute } + it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do + result = service.execute - it 'creates a merge request' do - expect { subject }.to change(project.merge_requests, :count).by(1) + expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"") + end + end + + context 'target_project_id is specified' do + let(:service_params) { { issue_iid: issue.iid, target_project_id: target_project.id } } + + context 'target project is not a fork of the project' do + let(:target_project) { create(:project, :repository) } + + it 'returns an error about not finding the project' do + result = service.execute + + expect(result[:status]).to eq(:error) + expect(result[:message]).to eq('Project not found') end - it 'sets the merge request target branch to the project default branch' do - expect(subject[:merge_request].target_branch).to eq(project.default_branch) + it 'does not create merge request' do + expect { service.execute }.to change(target_project.merge_requests, :count).by(0) + end + end + + context 'target project is a fork of project project' do + let(:target_project) { fork_project(project, user, repository: true) } + + it_behaves_like 'a service that creates a merge request from an issue' + + it 'sets the merge request title to: "WIP: $issue-branch-name' do + result = service.execute + + expect(result[:merge_request].title).to eq("WIP: #{issue.to_branch_name.titleize.humanize}") end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 93fe3290d8b..97377c2f560 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -454,16 +454,32 @@ describe SystemNoteService do end describe '.new_issue_branch' do - subject { described_class.new_issue_branch(noteable, project, author, "1-mepmep") } + let(:branch) { '1-mepmep' } - it_behaves_like 'a system note' do - let(:action) { 'branch' } - end + subject { described_class.new_issue_branch(noteable, project, author, branch, branch_project: branch_project) } - context 'when a branch is created from the new branch button' do - it 'sets the note text' do - expect(subject.note).to start_with("created branch [`1-mepmep`]") + shared_examples_for 'a system note for new issue branch' do + it_behaves_like 'a system note' do + let(:action) { 'branch' } end + + context 'when a branch is created from the new branch button' do + it 'sets the note text' do + expect(subject.note).to start_with("created branch [`#{branch}`]") + end + end + end + + context 'branch_project is set' do + let(:branch_project) { create(:project, :repository) } + + it_behaves_like 'a system note for new issue branch' + end + + context 'branch_project is not set' do + let(:branch_project) { nil } + + it_behaves_like 'a system note for new issue branch' end end @@ -477,7 +493,7 @@ describe SystemNoteService do end it 'sets the new merge request note text' do - expect(subject.note).to eq("created merge request #{merge_request.to_reference} to address this issue") + expect(subject.note).to eq("created merge request #{merge_request.to_reference(project)} to address this issue") end end diff --git a/spec/tasks/gitlab/cleanup_rake_spec.rb b/spec/tasks/gitlab/cleanup_rake_spec.rb index 19794227d9f..92c094f08a4 100644 --- a/spec/tasks/gitlab/cleanup_rake_spec.rb +++ b/spec/tasks/gitlab/cleanup_rake_spec.rb @@ -156,4 +156,33 @@ describe 'gitlab:cleanup rake tasks' do end end end + + describe 'gitlab:cleanup:orphan_job_artifact_files' do + subject(:rake_task) { run_rake_task('gitlab:cleanup:orphan_job_artifact_files') } + + it 'runs the task without errors' do + expect(Gitlab::Cleanup::OrphanJobArtifactFiles) + .to receive(:new).and_call_original + + expect { rake_task }.not_to raise_error + end + + context 'with DRY_RUN set to false' do + before do + stub_env('DRY_RUN', 'false') + end + + it 'passes dry_run correctly' do + expect(Gitlab::Cleanup::OrphanJobArtifactFiles) + .to receive(:new) + .with(limit: anything, + dry_run: false, + niceness: anything, + logger: anything) + .and_call_original + + rake_task + end + end + end end |