From 79214be727aaa0704a1be5b50aa6dd3011629bc2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 20 Jul 2016 16:18:18 -0600 Subject: Add Discussion model to represent MR/diff discussion --- app/assets/javascripts/notes.js.coffee | 6 +- app/controllers/projects/commit_controller.rb | 4 +- app/controllers/projects/compare_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 8 +- app/controllers/projects/notes_controller.rb | 65 +- app/helpers/diff_helper.rb | 16 +- app/helpers/notes_helper.rb | 37 +- app/models/concerns/note_on_diff.rb | 25 - app/models/discussion.rb | 91 +++ app/models/note.rb | 7 +- app/views/discussions/_diff_discussion.html.haml | 6 + app/views/discussions/_diff_with_notes.html.haml | 14 + app/views/discussions/_discussion.html.haml | 45 ++ app/views/discussions/_notes.html.haml | 5 + .../_parallel_diff_discussion.html.haml | 22 + .../projects/diffs/_match_line_parallel.html.haml | 4 - app/views/projects/diffs/_parallel_view.html.haml | 51 +- app/views/projects/diffs/_text_file.html.haml | 6 +- .../notes/_diff_notes_with_reply.html.haml | 7 - .../_diff_notes_with_reply_parallel.html.haml | 25 - app/views/projects/notes/_discussion.html.haml | 46 -- app/views/projects/notes/_notes.html.haml | 12 +- .../notes/discussions/_diff_with_notes.html.haml | 17 - .../projects/notes/discussions/_notes.html.haml | 6 - lib/gitlab/diff/parallel_diff.rb | 63 +- spec/fixtures/parallel_diff_result.yml | 800 --------------------- spec/lib/gitlab/diff/parallel_diff_spec.rb | 46 +- 27 files changed, 340 insertions(+), 1096 deletions(-) create mode 100644 app/models/discussion.rb create mode 100644 app/views/discussions/_diff_discussion.html.haml create mode 100644 app/views/discussions/_diff_with_notes.html.haml create mode 100644 app/views/discussions/_discussion.html.haml create mode 100644 app/views/discussions/_notes.html.haml create mode 100644 app/views/discussions/_parallel_diff_discussion.html.haml delete mode 100644 app/views/projects/diffs/_match_line_parallel.html.haml delete mode 100644 app/views/projects/notes/_diff_notes_with_reply.html.haml delete mode 100644 app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml delete mode 100644 app/views/projects/notes/_discussion.html.haml delete mode 100644 app/views/projects/notes/discussions/_diff_with_notes.html.haml delete mode 100644 app/views/projects/notes/discussions/_notes.html.haml delete mode 100644 spec/fixtures/parallel_diff_result.yml diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee index 0ea54faae1a..d4de712f88c 100644 --- a/app/assets/javascripts/notes.js.coffee +++ b/app/assets/javascripts/notes.js.coffee @@ -162,7 +162,7 @@ class @Notes @last_fetched_at = data.last_fetched_at @setPollingInterval(data.notes.length) $.each notes, (i, note) => - if note.discussion_with_diff_html? + if note.discussion_html? @renderDiscussionNote(note) else @renderNote(note) @@ -251,7 +251,7 @@ class @Notes discussionContainer = $(".notes[data-discussion-id='" + note.original_discussion_id + "']") if discussionContainer.length is 0 # insert the note and the reply button after the temp row - row.after note.discussion_html + row.after note.diff_discussion_html # remove the note (will be added again below) row.next().find(".note").remove() @@ -265,7 +265,7 @@ class @Notes # Init discussion on 'Discussion' page if it is merge request page if $('body').attr('data-page').indexOf('projects:merge_request') is 0 $('ul.main-notes-list') - .append(note.discussion_with_diff_html) + .append(note.discussion_html) .syntaxHighlight() else # append new note to all matching discussions diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index 727e84b40a1..7ae034f9398 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -115,11 +115,11 @@ class Projects::CommitController < Projects::ApplicationController end def define_note_vars - @grouped_diff_notes = commit.notes.grouped_diff_notes + @grouped_diff_discussions = commit.notes.grouped_diff_discussions @notes = commit.notes.non_diff_notes.fresh Banzai::NoteRenderer.render( - @grouped_diff_notes.values.flatten + @notes, + @grouped_diff_discussions.values.flat_map(&:notes) + @notes, @project, current_user, ) diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb index 10749d0fef8..8c004724f02 100644 --- a/app/controllers/projects/compare_controller.rb +++ b/app/controllers/projects/compare_controller.rb @@ -54,7 +54,7 @@ class Projects::CompareController < Projects::ApplicationController ) @diff_notes_disabled = true - @grouped_diff_notes = {} + @grouped_diff_discussions = {} end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 7beeb7d97d0..594a61464b9 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -97,7 +97,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController else build_merge_request @diff_notes_disabled = true - @grouped_diff_notes = {} + @grouped_diff_discussions = {} end define_commit_vars @@ -378,7 +378,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController # This is not executed lazily @notes = Banzai::NoteRenderer.render( - @discussions.flatten, + @discussions.flat_map(&:notes), @project, current_user, @path, @@ -404,10 +404,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController } @use_legacy_diff_notes = !@merge_request.support_new_diff_notes? - @grouped_diff_notes = @merge_request.notes.grouped_diff_notes + @grouped_diff_discussions = @merge_request.notes.grouped_diff_discussions Banzai::NoteRenderer.render( - @grouped_diff_notes.values.flatten, + @grouped_diff_discussions.values.flat_map(&:notes), @project, current_user, @path, diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 3eacdbbd067..766b7e9cf22 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -73,7 +73,7 @@ class Projects::NotesController < Projects::ApplicationController end alias_method :awardable, :note - def note_to_html(note) + def note_html(note) render_to_string( "projects/notes/_note", layout: false, @@ -82,20 +82,20 @@ class Projects::NotesController < Projects::ApplicationController ) end - def note_to_discussion_html(note) - return unless note.diff_note? + def diff_discussion_html(discussion) + return unless discussion.diff_discussion? if params[:view] == 'parallel' - template = "projects/notes/_diff_notes_with_reply_parallel" + template = "discussions/_parallel_diff_discussion" locals = if params[:line_type] == 'old' - { notes_left: [note], notes_right: [] } + { discussion_left: discussion, discussion_right: nil } else - { notes_left: [], notes_right: [note] } + { discussion_left: nil, discussion_right: discussion } end else - template = "projects/notes/_diff_notes_with_reply" - locals = { notes: [note] } + template = "discussions/_diff_discussion" + locals = { discussion: discussion } end render_to_string( @@ -106,14 +106,14 @@ class Projects::NotesController < Projects::ApplicationController ) end - def note_to_discussion_with_diff_html(note) - return unless note.diff_note? + def discussion_html(discussion) + return unless discussion.diff_discussion? render_to_string( - "projects/notes/_discussion", + "discussions/_discussion", layout: false, formats: [:html], - locals: { discussion_notes: [note] } + locals: { discussion: discussion } ) end @@ -132,26 +132,33 @@ class Projects::NotesController < Projects::ApplicationController valid: true, id: note.id, discussion_id: note.discussion_id, - html: note_to_html(note), + html: note_html(note), award: false, - note: note.note, - discussion_html: note_to_discussion_html(note), - discussion_with_diff_html: note_to_discussion_with_diff_html(note) + note: note.note } - # The discussion_id is used to add the comment to the correct discussion - # element on the merge request page. Among other things, the discussion_id - # contains the sha of head commit of the merge request. - # When new commits are pushed into the merge request after the initial - # load of the merge request page, the discussion elements will still have - # the old discussion_ids, with the old head commit sha. The new comment, - # however, will have the new discussion_id with the new commit sha. - # To ensure that these new comments will still end up in the correct - # discussion element, we also send the original discussion_id, with the - # old commit sha, along, and fall back on this value when no discussion - # element with the new discussion_id could be found. - if note.new_diff_note? && note.position != note.original_position - attrs[:original_discussion_id] = note.original_discussion_id + if note.diff_note? + discussion = Discussion.new([note]) + + attrs.merge!( + diff_discussion_html: diff_discussion_html(discussion), + discussion_html: discussion_html(discussion) + ) + + # The discussion_id is used to add the comment to the correct discussion + # element on the merge request page. Among other things, the discussion_id + # contains the sha of head commit of the merge request. + # When new commits are pushed into the merge request after the initial + # load of the merge request page, the discussion elements will still have + # the old discussion_ids, with the old head commit sha. The new comment, + # however, will have the new discussion_id with the new commit sha. + # To ensure that these new comments will still end up in the correct + # discussion element, we also send the original discussion_id, with the + # old commit sha, along, and fall back on this value when no discussion + # element with the new discussion_id could be found. + if note.new_diff_note? && note.position != note.original_position + attrs[:original_discussion_id] = note.original_discussion_id + end end attrs diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 75b029365f9..4c031942793 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -54,18 +54,20 @@ module DiffHelper end end - def organize_comments(left, right) - notes_left = notes_right = nil + def parallel_diff_discussions(left, right, diff_file) + discussion_left = discussion_right = nil - unless left[:type].nil? && right[:type] == 'new' - notes_left = @grouped_diff_notes[left[:line_code]] + if left && (left.unchanged? || left.removed?) + line_code = diff_file.line_code(left) + discussion_left = @grouped_diff_discussions[line_code] end - unless left[:type].nil? && right[:type].nil? - notes_right = @grouped_diff_notes[right[:line_code]] + if right && right.added? + line_code = diff_file.line_code(right) + discussion_right = @grouped_diff_discussions[line_code] end - [notes_left, notes_right] + [discussion_left, discussion_right] end def inline_diff_btn diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 98143dcee9b..0f60dd828ab 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -1,9 +1,4 @@ module NotesHelper - # Helps to distinguish e.g. commit notes in mr notes list - def note_for_main_target?(note) - @noteable.class.name == note.noteable_type && !note.diff_note? - end - def note_target_fields(note) if note.noteable hidden_field_tag(:target_type, note.noteable.class.name.underscore) + @@ -44,8 +39,8 @@ module NotesHelper # If we didn't, diff notes that would show for the same line on the changes # tab, would show in different discussions on the discussion tab. use_legacy_diff_note ||= begin - line_diff_notes = @grouped_diff_notes[line_code] - line_diff_notes && line_diff_notes.any?(&:legacy_diff_note?) + discussion = @grouped_diff_discussions[line_code] + discussion && discussion.legacy_diff_discussion? end data = { @@ -81,22 +76,10 @@ module NotesHelper data end - def link_to_reply_discussion(note, line_type = nil) + def link_to_reply_discussion(discussion, line_type = nil) return unless current_user - data = { - noteable_type: note.noteable_type, - noteable_id: note.noteable_id, - commit_id: note.commit_id, - discussion_id: note.discussion_id, - line_type: line_type - } - - if note.diff_note? - data[:note_type] = note.type - - data.merge!(note.diff_attributes) - end + data = discussion.reply_attributes.merge(line_type: line_type) content_tag(:div, class: "discussion-reply-holder") do button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button', @@ -114,13 +97,13 @@ module NotesHelper @max_access_by_user_id[full_key] end - def diff_note_path(note) - return unless note.diff_note? + def discussion_diff_path(discussion) + return unless discussion.diff_discussion? - if note.for_merge_request? && note.active? - diffs_namespace_project_merge_request_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code) - elsif note.for_commit? - namespace_project_commit_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code) + if discussion.for_merge_request? && discussion.active? + diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code) + elsif discussion.for_commit? + namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code) end end end diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb index 2785fbb21c9..4be6a2f621b 100644 --- a/app/models/concerns/note_on_diff.rb +++ b/app/models/concerns/note_on_diff.rb @@ -1,12 +1,6 @@ module NoteOnDiff extend ActiveSupport::Concern - NUMBER_OF_TRUNCATED_DIFF_LINES = 16 - - included do - delegate :blob, :highlighted_diff_lines, to: :diff_file, allow_nil: true - end - def diff_note? true end @@ -30,23 +24,4 @@ module NoteOnDiff def can_be_award_emoji? false end - - # Returns an array of at most 16 highlighted lines above a diff note - def truncated_diff_lines - prev_lines = [] - - highlighted_diff_lines.each do |line| - if line.meta? - prev_lines.clear - else - prev_lines << line - - break if for_line?(line) - - prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES - end - end - - prev_lines - end end diff --git a/app/models/discussion.rb b/app/models/discussion.rb new file mode 100644 index 00000000000..74facfd1c9c --- /dev/null +++ b/app/models/discussion.rb @@ -0,0 +1,91 @@ +class Discussion + NUMBER_OF_TRUNCATED_DIFF_LINES = 16 + + attr_reader :first_note, :notes + + delegate :created_at, + :project, + :author, + + :noteable, + :for_commit?, + :for_merge_request?, + + :line_code, + :diff_file, + :for_line?, + :active?, + + to: :first_note + + delegate :blob, :highlighted_diff_lines, to: :diff_file, allow_nil: true + + def self.for_notes(notes) + notes.group_by(&:discussion_id).values.map { |notes| new(notes) } + end + + def self.for_diff_notes(notes) + notes.group_by(&:line_code).values.map { |notes| new(notes) } + end + + def initialize(notes) + @first_note = notes.first + @notes = notes + end + + def id + first_note.discussion_id + end + + def diff_discussion? + first_note.diff_note? + end + + def legacy_diff_discussion? + notes.any?(&:legacy_diff_note?) + end + + def for_target?(target) + self.noteable == target && !diff_discussion? + end + + def expanded? + !diff_discussion? || active? + end + + def reply_attributes + data = { + noteable_type: first_note.noteable_type, + noteable_id: first_note.noteable_id, + commit_id: first_note.commit_id, + discussion_id: self.id, + } + + if diff_discussion? + data[:note_type] = first_note.type + + data.merge!(first_note.diff_attributes) + end + + data + end + + # Returns an array of at most 16 highlighted lines above a diff note + def truncated_diff_lines + prev_lines = [] + + highlighted_diff_lines.each do |line| + if line.meta? + prev_lines.clear + else + prev_lines << line + + break if for_line?(line) + + prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES + end + end + + prev_lines + end +end diff --git a/app/models/note.rb b/app/models/note.rb index 0ce10c77de9..9b0a7211b4e 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -82,11 +82,12 @@ class Note < ActiveRecord::Base end def discussions - all.group_by(&:discussion_id).values + Discussion.for_notes(all) end - def grouped_diff_notes - diff_notes.select(&:active?).sort_by(&:created_at).group_by(&:line_code) + def grouped_diff_discussions + notes = diff_notes.fresh.select(&:active?) + Discussion.for_diff_notes(notes).map { |d| [d.line_code, d] }.to_h end # Searches for notes matching the given query. diff --git a/app/views/discussions/_diff_discussion.html.haml b/app/views/discussions/_diff_discussion.html.haml new file mode 100644 index 00000000000..fa1ad9efa73 --- /dev/null +++ b/app/views/discussions/_diff_discussion.html.haml @@ -0,0 +1,6 @@ +%tr.notes_holder + %td.notes_line{ colspan: 2 } + %td.notes_content + %ul.notes{ data: { discussion_id: discussion.id } } + = render partial: "projects/notes/note", collection: discussion.notes, as: :note + = link_to_reply_discussion(discussion) diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml new file mode 100644 index 00000000000..02b159ffd45 --- /dev/null +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -0,0 +1,14 @@ +- diff_file = discussion.diff_file +- blob = discussion.blob + +.diff-file.file-holder + .file-title + = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: discussion.project, url: discussion_diff_path(discussion) + + .diff-content.code.js-syntax-highlight + %table + - discussion.truncated_diff_lines.each do |line| + = render "projects/diffs/line", line: line, diff_file: diff_file, plain: true + + - if discussion.for_line?(line) + = render "discussions/diff_discussion", discussion: discussion diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml new file mode 100644 index 00000000000..49702e048aa --- /dev/null +++ b/app/views/discussions/_discussion.html.haml @@ -0,0 +1,45 @@ +- expanded = discussion.expanded? +%li.note.note-discussion.timeline-entry + .timeline-entry-inner + .timeline-icon + = link_to user_path(discussion.author) do + = image_tag avatar_icon(discussion.author), class: "avatar s40" + .timeline-content + .discussion.js-toggle-container{ class: discussion.id } + .discussion-header + = link_to_member(@project, discussion.author, avatar: false) + + .inline.discussion-headline-light + = discussion.author.to_reference + started a discussion on + + - if discussion.for_commit? + - commit = discussion.noteable + - if commit + commit + = link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code), class: 'monospace' + - else + a deleted commit + - else + - if discussion.active? + = link_to diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code) do + the diff + - else + an outdated diff + + = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago") + + .discussion-actions + = link_to "#", class: "note-action-button discussion-toggle-button js-toggle-button" do + - if expanded + = icon("chevron-up") + - else + = icon("chevron-down") + + Toggle discussion + + .discussion-body.js-toggle-content{ class: ("hide" unless expanded) } + - if discussion.diff_discussion? && discussion.diff_file + = render "discussions/diff_with_notes", discussion: discussion + - else + = render "discussions/notes", discussion: discussion diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml new file mode 100644 index 00000000000..a2642b839f6 --- /dev/null +++ b/app/views/discussions/_notes.html.haml @@ -0,0 +1,5 @@ +.panel.panel-default + .notes{ data: { discussion_id: discussion.id } } + %ul.notes.timeline + = render partial: "projects/notes/note", collection: discussion.notes, as: :note + = link_to_reply_discussion(discussion) diff --git a/app/views/discussions/_parallel_diff_discussion.html.haml b/app/views/discussions/_parallel_diff_discussion.html.haml new file mode 100644 index 00000000000..a798c438ea0 --- /dev/null +++ b/app/views/discussions/_parallel_diff_discussion.html.haml @@ -0,0 +1,22 @@ +%tr.notes_holder + - if discussion_left + %td.notes_line.old + %td.notes_content.parallel.old + %ul.notes{ data: { discussion_id: discussion_left.id } } + = render partial: "projects/notes/note", collection: discussion_left.notes, as: :note + + = link_to_reply_discussion(discussion_left, 'old') + - else + %td.notes_line.old= "" + %td.notes_content.parallel.old= "" + + - if discussion_right + %td.notes_line.new + %td.notes_content.parallel.new + %ul.notes{ data: { discussion_id: discussion_right.id } } + = render partial: "projects/notes/note", collection: discussion_right.notes, as: :note + + = link_to_reply_discussion(discussion_right, 'new') + - else + %td.notes_line.new= "" + %td.notes_content.parallel.new= "" diff --git a/app/views/projects/diffs/_match_line_parallel.html.haml b/app/views/projects/diffs/_match_line_parallel.html.haml deleted file mode 100644 index b9c0d9dcdfd..00000000000 --- a/app/views/projects/diffs/_match_line_parallel.html.haml +++ /dev/null @@ -1,4 +0,0 @@ -%td.old_line.diff-line-num.empty-cell -%td.line_content.parallel.match= line -%td.new_line.diff-line-num.empty-cell -%td.line_content.parallel.match= line diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index d208fcee10b..7f30faa20d8 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -5,32 +5,35 @@ - left = line[:left] - right = line[:right] %tr.line_holder.parallel - - if left[:type] == 'match' - = render "projects/diffs/match_line_parallel", { line: left[:text] } - - elsif left[:type] == 'nonewline' - %td.old_line.diff-line-num.empty-cell - %td.line_content.parallel.match= left[:text] - %td.new_line.diff-line-num.empty-cell - %td.line_content.parallel.match= left[:text] + - if left + - if left.meta? + %td.old_line.diff-line-num.empty-cell + %td.line_content.parallel.match= left.text + - else + - left_line_code = diff_file.line_code(left) + - left_position = diff_file.position(left) + %td.old_line.diff-line-num{id: left_line_code, class: left.type, data: { linenumber: left.old_pos }} + %a{href: "##{left_line_code}" }= raw(left.old_pos) + %td.line_content.parallel.noteable_line{class: left.type, data: diff_view_line_data(left_line_code, left_position, 'old')}= diff_line_content(left.text) - else - %td.old_line.diff-line-num{id: left[:line_code], class: [left[:type], ('empty-cell' unless left[:number])], data: { linenumber: left[:number] }} - %a{href: "##{left[:line_code]}" }= raw(left[:number]) - %td.line_content.parallel.noteable_line{class: [left[:type], ('empty-cell' if left[:text].empty?)], data: diff_view_line_data(left[:line_code], left[:position], 'old')}= diff_line_content(left[:text]) + %td.old_line.diff-line-num.empty-cell + %td.line_content.parallel - - if right[:type] == 'new' - - new_line_type = 'new' - - new_line_code = right[:line_code] - - new_position = right[:position] + - if right + - if right.meta? + %td.old_line.diff-line-num.empty-cell + %td.line_content.parallel.match= left.text - else - - new_line_type = nil - - new_line_code = left[:line_code] - - new_position = left[:position] - - %td.new_line.diff-line-num{id: new_line_code, class: [new_line_type, ('empty-cell' unless right[:number])], data: { linenumber: right[:number] }} - %a{href: "##{new_line_code}" }= raw(right[:number]) - %td.line_content.parallel.noteable_line{class: [new_line_type, ('empty-cell' if right[:text].empty?)], data: diff_view_line_data(new_line_code, new_position, 'new')}= diff_line_content(right[:text]) + - right_line_code = diff_file.line_code(right) + - right_position = diff_file.position(right) + %td.new_line.diff-line-num{id: right_line_code, class: right.type, data: { linenumber: right.new_pos }} + %a{href: "##{right_line_code}" }= raw(right.new_pos) + %td.line_content.parallel.noteable_line{class: right.type, data: diff_view_line_data(right_line_code, right_position, 'new')}= diff_line_content(right.text) + - else + %td.old_line.diff-line-num.empty-cell + %td.line_content.parallel - unless @diff_notes_disabled - - notes_left, notes_right = organize_comments(left, right) - - if notes_left.present? || notes_right.present? - = render "projects/notes/diff_notes_with_reply_parallel", notes_left: notes_left, notes_right: notes_right + - discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file) + - if discussion_left || discussion_right + = render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index 196f8122db3..5970b9abf2b 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -11,9 +11,9 @@ - unless @diff_notes_disabled - line_code = diff_file.line_code(line) - - diff_notes = @grouped_diff_notes[line_code] if line_code - - if diff_notes - = render "projects/notes/diff_notes_with_reply", notes: diff_notes + - discussion = @grouped_diff_discussions[line_code] if line_code + - if discussion + = render "discussions/diff_discussion", discussion: discussion - if last_line > 0 = render "projects/diffs/match_line", { line: "", diff --git a/app/views/projects/notes/_diff_notes_with_reply.html.haml b/app/views/projects/notes/_diff_notes_with_reply.html.haml deleted file mode 100644 index ec6c4938efc..00000000000 --- a/app/views/projects/notes/_diff_notes_with_reply.html.haml +++ /dev/null @@ -1,7 +0,0 @@ -- note = notes.first -%tr.notes_holder - %td.notes_line{ colspan: 2 } - %td.notes_content - %ul.notes{ data: { discussion_id: note.discussion_id } } - = render partial: "projects/notes/note", collection: notes, as: :note - = link_to_reply_discussion(note) diff --git a/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml b/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml deleted file mode 100644 index e50a4f86d03..00000000000 --- a/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml +++ /dev/null @@ -1,25 +0,0 @@ -- note_left = notes_left.present? ? notes_left.first : nil -- note_right = notes_right.present? ? notes_right.first : nil - -%tr.notes_holder - - if note_left - %td.notes_line.old - %td.notes_content.parallel.old - %ul.notes{ data: { discussion_id: note_left.discussion_id } } - = render partial: "projects/notes/note", collection: notes_left, as: :note - - = link_to_reply_discussion(note_left, 'old') - - else - %td.notes_line.old= "" - %td.notes_content.parallel.old= "" - - - if note_right - %td.notes_line.new - %td.notes_content.parallel.new - %ul.notes{ data: { discussion_id: note_right.discussion_id } } - = render partial: "projects/notes/note", collection: notes_right, as: :note - - = link_to_reply_discussion(note_right, 'new') - - else - %td.notes_line.new= "" - %td.notes_content.parallel.new= "" diff --git a/app/views/projects/notes/_discussion.html.haml b/app/views/projects/notes/_discussion.html.haml deleted file mode 100644 index 7869d6413d8..00000000000 --- a/app/views/projects/notes/_discussion.html.haml +++ /dev/null @@ -1,46 +0,0 @@ -- note = discussion_notes.first -- expanded = !note.diff_note? || note.active? -%li.note.note-discussion.timeline-entry - .timeline-entry-inner - .timeline-icon - = link_to user_path(note.author) do - = image_tag avatar_icon(note.author), class: "avatar s40" - .timeline-content - .discussion.js-toggle-container{ class: note.discussion_id } - .discussion-header - = link_to_member(@project, note.author, avatar: false) - - .inline.discussion-headline-light - = note.author.to_reference - started a discussion on - - - if note.for_commit? - - commit = note.noteable - - if commit - commit - = link_to commit.short_id, namespace_project_commit_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code), class: 'monospace' - - else - a deleted commit - - else - - if note.active? - = link_to diffs_namespace_project_merge_request_path(note.project.namespace, note.project, note.noteable, anchor: note.line_code) do - the diff - - else - an outdated diff - - = time_ago_with_tooltip(note.created_at, placement: "bottom", html_class: "note-created-ago") - - .discussion-actions - = link_to "#", class: "note-action-button discussion-toggle-button js-toggle-button" do - - if expanded - = icon("chevron-up") - - else - = icon("chevron-down") - - Toggle discussion - - .discussion-body.js-toggle-content{ class: ("hide" unless expanded) } - - if note.diff_note? - = render "projects/notes/discussions/diff_with_notes", discussion_notes: discussion_notes - - else - = render "projects/notes/discussions/notes", discussion_notes: discussion_notes diff --git a/app/views/projects/notes/_notes.html.haml b/app/views/projects/notes/_notes.html.haml index ebf7e8a9cb3..022578bd6db 100644 --- a/app/views/projects/notes/_notes.html.haml +++ b/app/views/projects/notes/_notes.html.haml @@ -1,10 +1,8 @@ - if @discussions.present? - - @discussions.each do |discussion_notes| - - note = discussion_notes.first - - if note_for_main_target?(note) - = render partial: "projects/notes/note", object: note, as: :note + - @discussions.each do |discussion| + - if discussion.for_target?(@noteable) + = render partial: "projects/notes/note", object: discussion.first_note, as: :note - else - = render 'projects/notes/discussion', discussion_notes: discussion_notes + = render 'discussions/discussion', discussion: discussion - else - - @notes.each do |note| - = render partial: "projects/notes/note", object: note, as: :note + = render partial: "projects/notes/note", collection: @notes, as: :note diff --git a/app/views/projects/notes/discussions/_diff_with_notes.html.haml b/app/views/projects/notes/discussions/_diff_with_notes.html.haml deleted file mode 100644 index 4a69b8f8840..00000000000 --- a/app/views/projects/notes/discussions/_diff_with_notes.html.haml +++ /dev/null @@ -1,17 +0,0 @@ -- note = discussion_notes.first -- diff_file = note.diff_file -- return unless diff_file - -- blob = note.blob - -.diff-file.file-holder - .file-title - = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_file.content_commit, project: note.project, url: diff_note_path(note) - - .diff-content.code.js-syntax-highlight - %table - - note.truncated_diff_lines.each do |line| - = render "projects/diffs/line", line: line, diff_file: diff_file, plain: true - - - if note.for_line?(line) - = render "projects/notes/diff_notes_with_reply", notes: discussion_notes diff --git a/app/views/projects/notes/discussions/_notes.html.haml b/app/views/projects/notes/discussions/_notes.html.haml deleted file mode 100644 index a785149549d..00000000000 --- a/app/views/projects/notes/discussions/_notes.html.haml +++ /dev/null @@ -1,6 +0,0 @@ -- note = discussion_notes.first -.panel.panel-default - .notes{ data: { discussion_id: note.discussion_id } } - %ul.notes.timeline - = render partial: "projects/notes/note", collection: discussion_notes, as: :note - = link_to_reply_discussion(note) diff --git a/lib/gitlab/diff/parallel_diff.rb b/lib/gitlab/diff/parallel_diff.rb index b069afdd28c..481536a380b 100644 --- a/lib/gitlab/diff/parallel_diff.rb +++ b/lib/gitlab/diff/parallel_diff.rb @@ -8,72 +8,35 @@ module Gitlab end def parallelize - i = 0 free_right_index = nil lines = [] highlighted_diff_lines = diff_file.highlighted_diff_lines highlighted_diff_lines.each do |line| - line_code = diff_file.line_code(line) - position = diff_file.position(line) - - case line.type - when 'match', nil + if line.meta? || line.unchanged? # line in the right panel is the same as in the left one lines << { - left: { - type: line.type, - number: line.old_pos, - text: line.text, - line_code: line_code, - position: position - }, - right: { - type: line.type, - number: line.new_pos, - text: line.text, - line_code: line_code, - position: position - } + left: line, + right: line } free_right_index = nil i += 1 - when 'old' + elsif line.removed? lines << { - left: { - type: line.type, - number: line.old_pos, - text: line.text, - line_code: line_code, - position: position - }, - right: { - type: nil, - number: nil, - text: "", - line_code: line_code, - position: position - } + left: line, + right: nil } # Once we come upon a new line it can be put on the right of this old line free_right_index ||= i i += 1 - when 'new' - data = { - type: line.type, - number: line.new_pos, - text: line.text, - line_code: line_code, - position: position - } - + elsif line.added? if free_right_index # If an old line came before this without a line on the right, this # line can be put to the right of it. - lines[free_right_index][:right] = data + lines[free_right_index][:right] = line # If there are any other old lines on the left that don't yet have # a new counterpart on the right, update the free_right_index @@ -81,14 +44,8 @@ module Gitlab free_right_index = next_free_right_index < i ? next_free_right_index : nil else lines << { - left: { - type: nil, - number: nil, - text: "", - line_code: line_code, - position: position - }, - right: data + left: nil, + right: line } free_right_index = nil diff --git a/spec/fixtures/parallel_diff_result.yml b/spec/fixtures/parallel_diff_result.yml deleted file mode 100644 index 37066c8e930..00000000000 --- a/spec/fixtures/parallel_diff_result.yml +++ /dev/null @@ -1,800 +0,0 @@ ---- -- :left: - :type: match - :number: 6 - :text: "@@ -6,12 +6,18 @@ module Popen" - :line_code: - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: match - :number: 6 - :text: "@@ -6,12 +6,18 @@ module Popen" - :line_code: - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 6 - :text: |2 - - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 6 - :new_line: 6 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 6 - :text: |2 - - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_6_6 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 6 - :new_line: 6 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 7 - :text: |2 - def popen(cmd, path=nil) - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 7 - :new_line: 7 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 7 - :text: |2 - def popen(cmd, path=nil) - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 7 - :new_line: 7 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 8 - :text: |2 - unless cmd.is_a?(Array) - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 8 - :new_line: 8 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 8 - :text: |2 - unless cmd.is_a?(Array) - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_8_8 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 8 - :new_line: 8 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: old - :number: 9 - :text: | - - raise "System commands must be given as an array of strings" - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 9 - :new_line: - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 9 - :text: | - + raise RuntimeError, "System commands must be given as an array of strings" - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 9 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 10 - :text: |2 - end - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 10 - :new_line: 10 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 10 - :text: |2 - end - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_10 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 10 - :new_line: 10 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 11 - :text: |2 - - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 11 - :new_line: 11 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 11 - :text: |2 - - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_11_11 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 11 - :new_line: 11 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 12 - :text: |2 - path ||= Dir.pwd - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 12 - :new_line: 12 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 12 - :text: |2 - path ||= Dir.pwd - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_12_12 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 12 - :new_line: 12 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: old - :number: 13 - :text: | - - vars = { "PWD" => path } - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_13_13 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 13 - :new_line: - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 13 - :text: | - + - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_13 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 13 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: old - :number: 14 - :text: | - - options = { chdir: path } - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_14_13 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 14 - :new_line: - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 14 - :text: | - + vars = { - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_14 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 14 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: - :text: '' - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 15 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 15 - :text: | - + "PWD" => path - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 15 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: - :text: '' - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 16 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 16 - :text: | - + } - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_16 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 16 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: - :text: '' - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 17 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 17 - :text: | - + - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_17 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 17 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: - :text: '' - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 18 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 18 - :text: | - + options = { - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_18 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 18 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: - :text: '' - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 19 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 19 - :text: | - + chdir: path - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_19 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 19 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: - :text: '' - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 20 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 20 - :text: | - + } - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_20 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 20 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 15 - :text: |2 - - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 15 - :new_line: 21 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 21 - :text: |2 - - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_21 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 15 - :new_line: 21 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 16 - :text: |2 - unless File.directory?(path) - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 16 - :new_line: 22 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 22 - :text: |2 - unless File.directory?(path) - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_16_22 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 16 - :new_line: 22 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 17 - :text: |2 - FileUtils.mkdir_p(path) - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 17 - :new_line: 23 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 23 - :text: |2 - FileUtils.mkdir_p(path) - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_17_23 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 17 - :new_line: 23 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: match - :number: 19 - :text: "@@ -19,6 +25,7 @@ module Popen" - :line_code: - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: match - :number: 25 - :text: "@@ -19,6 +25,7 @@ module Popen" - :line_code: - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 19 - :text: |2 - - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 19 - :new_line: 25 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 25 - :text: |2 - - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_19_25 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 19 - :new_line: 25 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 20 - :text: |2 - @cmd_output = "" - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 20 - :new_line: 26 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 26 - :text: |2 - @cmd_output = "" - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_20_26 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 20 - :new_line: 26 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 21 - :text: |2 - @cmd_status = 0 - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 21 - :new_line: 27 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 27 - :text: |2 - @cmd_status = 0 - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_21_27 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 21 - :new_line: 27 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: - :text: '' - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 28 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: new - :number: 28 - :text: | - + - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_28 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: - :new_line: 28 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 22 - :text: |2 - Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 22 - :new_line: 29 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 29 - :text: |2 - Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_22_29 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 22 - :new_line: 29 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 23 - :text: |2 - @cmd_output << stdout.read - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 23 - :new_line: 30 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 30 - :text: |2 - @cmd_output << stdout.read - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_23_30 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 23 - :new_line: 30 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d -- :left: - :type: - :number: 24 - :text: |2 - @cmd_output << stderr.read - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 24 - :new_line: 31 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d - :right: - :type: - :number: 31 - :text: |2 - @cmd_output << stderr.read - :line_code: 2f6fcd96b88b36ce98c38da085c795a27d92a3dd_24_31 - :position: !ruby/object:Gitlab::Diff::Position - attributes: - :old_path: files/ruby/popen.rb - :new_path: files/ruby/popen.rb - :old_line: 24 - :new_line: 31 - :base_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :start_sha: 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 - :head_sha: 570e7b2abdd848b95f2f578043fc23bd6f6fd24d diff --git a/spec/lib/gitlab/diff/parallel_diff_spec.rb b/spec/lib/gitlab/diff/parallel_diff_spec.rb index 5f76b70c6f5..2aa5ae44f54 100644 --- a/spec/lib/gitlab/diff/parallel_diff_spec.rb +++ b/spec/lib/gitlab/diff/parallel_diff_spec.rb @@ -11,11 +11,51 @@ describe Gitlab::Diff::ParallelDiff, lib: true do let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: commit.diff_refs, repository: repository) } subject { described_class.new(diff_file) } - let(:parallel_diff_result_array) { YAML.load_file("#{Rails.root}/spec/fixtures/parallel_diff_result.yml") } - describe '#parallelize' do it 'should return an array of arrays containing the parsed diff' do - expect(subject.parallelize).to match_array(parallel_diff_result_array) + diff_lines = diff_file.highlighted_diff_lines + expected = [ + # Unchanged lines + { left: diff_lines[0], right: diff_lines[0] }, + { left: diff_lines[1], right: diff_lines[1] }, + { left: diff_lines[2], right: diff_lines[2] }, + { left: diff_lines[3], right: diff_lines[3] }, + { left: diff_lines[4], right: diff_lines[5] }, + { left: diff_lines[6], right: diff_lines[6] }, + { left: diff_lines[7], right: diff_lines[7] }, + { left: diff_lines[8], right: diff_lines[8] }, + + # Changed lines + { left: diff_lines[9], right: diff_lines[11] }, + { left: diff_lines[10], right: diff_lines[12] }, + + # Added lines + { left: nil, right: diff_lines[13] }, + { left: nil, right: diff_lines[14] }, + { left: nil, right: diff_lines[15] }, + { left: nil, right: diff_lines[16] }, + { left: nil, right: diff_lines[17] }, + { left: nil, right: diff_lines[18] }, + + # Unchanged lines + { left: diff_lines[19], right: diff_lines[19] }, + { left: diff_lines[20], right: diff_lines[20] }, + { left: diff_lines[21], right: diff_lines[21] }, + { left: diff_lines[22], right: diff_lines[22] }, + { left: diff_lines[23], right: diff_lines[23] }, + { left: diff_lines[24], right: diff_lines[24] }, + { left: diff_lines[25], right: diff_lines[25] }, + + # Added line + { left: nil, right: diff_lines[26] }, + + # Unchanged lines + { left: diff_lines[27], right: diff_lines[27] }, + { left: diff_lines[28], right: diff_lines[28] }, + { left: diff_lines[29], right: diff_lines[29] } + ] + + expect(subject.parallelize).to eq(expected) end end end -- cgit v1.2.1 From 97c4a1dceab97cd1388f5bdb1d3f4fd3d228f94b Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 20 Jul 2016 16:45:19 -0600 Subject: Refactor Issues::BulkUpdateService spec - Create fewer Issue objects; 2 is as good as 5 for these cases. This gives us a nice little speed improvement. - Don't `describe` Symbols. - Simplify object creation. - Lessen "mystery guest" antipattern --- spec/factories/issues.rb | 10 ++ spec/services/issues/bulk_update_service_spec.rb | 131 ++++++++++------------- 2 files changed, 65 insertions(+), 76 deletions(-) diff --git a/spec/factories/issues.rb b/spec/factories/issues.rb index e72aa9479b7..2c0a2dd94ca 100644 --- a/spec/factories/issues.rb +++ b/spec/factories/issues.rb @@ -18,5 +18,15 @@ FactoryGirl.define do factory :closed_issue, traits: [:closed] factory :reopened_issue, traits: [:reopened] + + factory :labeled_issue do + transient do + labels [] + end + + after(:create) do |issue, evaluator| + issue.update_attributes(labels: evaluator.labels) + end + end end end diff --git a/spec/services/issues/bulk_update_service_spec.rb b/spec/services/issues/bulk_update_service_spec.rb index ba3a4dfc048..321b54ac39d 100644 --- a/spec/services/issues/bulk_update_service_spec.rb +++ b/spec/services/issues/bulk_update_service_spec.rb @@ -1,118 +1,106 @@ require 'spec_helper' describe Issues::BulkUpdateService, services: true do - let(:user) { create(:user) } - let(:project) { Projects::CreateService.new(user, namespace: user.namespace, name: 'test').execute } + let(:user) { create(:user) } + let(:project) { create(:empty_project, namespace: user.namespace) } - let!(:result) { Issues::BulkUpdateService.new(project, user, params).execute } + def bulk_update(issues, extra_params = {}) + bulk_update_params = extra_params + .reverse_merge(issues_ids: Array(issues).map(&:id).join(',')) - describe :close_issue do - let(:issues) { create_list(:issue, 5, project: project) } - let(:params) do - { - state_event: 'close', - issues_ids: issues.map(&:id).join(',') - } - end + Issues::BulkUpdateService.new(project, user, bulk_update_params).execute + end + + describe 'close issues' do + let(:issues) { create_list(:issue, 2, project: project) } it 'succeeds and returns the correct number of issues updated' do + result = bulk_update(issues, state_event: 'close') + expect(result[:success]).to be_truthy expect(result[:count]).to eq(issues.count) end it 'closes all the issues passed' do + bulk_update(issues, state_event: 'close') + expect(project.issues.opened).to be_empty expect(project.issues.closed).not_to be_empty end end - describe :reopen_issues do - let(:issues) { create_list(:closed_issue, 5, project: project) } - let(:params) do - { - state_event: 'reopen', - issues_ids: issues.map(&:id).join(',') - } - end + describe 'reopen issues' do + let(:issues) { create_list(:closed_issue, 2, project: project) } it 'succeeds and returns the correct number of issues updated' do + result = bulk_update(issues, state_event: 'reopen') + expect(result[:success]).to be_truthy expect(result[:count]).to eq(issues.count) end it 'reopens all the issues passed' do + bulk_update(issues, state_event: 'reopen') + expect(project.issues.closed).to be_empty expect(project.issues.opened).not_to be_empty end end describe 'updating assignee' do - let(:issue) do - create(:issue, project: project) { |issue| issue.update_attributes(assignee: user) } - end - - let(:params) do - { - assignee_id: assignee_id, - issues_ids: issue.id.to_s - } - end + let(:issue) { create(:issue, project: project, assignee: user) } context 'when the new assignee ID is a valid user' do - let(:new_assignee) { create(:user) } - let(:assignee_id) { new_assignee.id } - it 'succeeds' do + result = bulk_update(issue, assignee_id: create(:user).id) + expect(result[:success]).to be_truthy expect(result[:count]).to eq(1) end it 'updates the assignee to the use ID passed' do - expect(issue.reload.assignee).to eq(new_assignee) + assignee = create(:user) + + expect { bulk_update(issue, assignee_id: assignee.id) } + .to change { issue.reload.assignee }.from(user).to(assignee) end end context 'when the new assignee ID is -1' do - let(:assignee_id) { -1 } - it 'unassigns the issues' do - expect(issue.reload.assignee).to be_nil + expect { bulk_update(issue, assignee_id: -1) } + .to change { issue.reload.assignee }.to(nil) end end context 'when the new assignee ID is not present' do - let(:assignee_id) { nil } - it 'does not unassign' do - expect(issue.reload.assignee).to eq(user) + expect { bulk_update(issue, assignee_id: nil) } + .not_to change { issue.reload.assignee } end end end describe 'updating milestones' do - let(:issue) { create(:issue, project: project) } + let(:issue) { create(:issue, project: project) } let(:milestone) { create(:milestone, project: project) } - let(:params) do - { - issues_ids: issue.id.to_s, - milestone_id: milestone.id - } - end - it 'succeeds' do + result = bulk_update(issue, milestone_id: milestone.id) + expect(result[:success]).to be_truthy expect(result[:count]).to eq(1) end it 'updates the issue milestone' do - expect(project.issues.first.milestone).to eq(milestone) + expect { bulk_update(issue, milestone_id: milestone.id) } + .to change { issue.reload.milestone }.from(nil).to(milestone) end end describe 'updating labels' do def create_issue_with_labels(labels) - create(:issue, project: project) { |issue| issue.update_attributes(labels: labels) } + create(:labeled_issue, project: project, labels: labels) end let(:bug) { create(:label, project: project) } @@ -129,15 +117,18 @@ describe Issues::BulkUpdateService, services: true do let(:add_labels) { [] } let(:remove_labels) { [] } - let(:params) do + let(:bulk_update_params) do { - label_ids: labels.map(&:id), - add_label_ids: add_labels.map(&:id), + label_ids: labels.map(&:id), + add_label_ids: add_labels.map(&:id), remove_label_ids: remove_labels.map(&:id), - issues_ids: issues.map(&:id).join(',') } end + before do + bulk_update(issues, bulk_update_params) + end + context 'when label_ids are passed' do let(:issues) { [issue_all_labels, issue_no_labels] } let(:labels) { [bug, regression] } @@ -263,40 +254,28 @@ describe Issues::BulkUpdateService, services: true do end end - describe :subscribe_issues do - let(:issues) { create_list(:issue, 5, project: project) } - let(:params) do - { - subscription_event: 'subscribe', - issues_ids: issues.map(&:id).join(',') - } - end + describe 'subscribe to issues' do + let(:issues) { create_list(:issue, 2, project: project) } it 'subscribes the given user' do - issues.each do |issue| - expect(issue.subscribed?(user)).to be_truthy - end - end - end + bulk_update(issues, subscription_event: 'subscribe') - describe :unsubscribe_issues do - let(:issues) { create_list(:closed_issue, 5, project: project) } - let(:params) do - { - subscription_event: 'unsubscribe', - issues_ids: issues.map(&:id).join(',') - } + expect(issues).to all(be_subscribed(user)) end + end - before do - issues.each do |issue| + describe 'unsubscribe from issues' do + let(:issues) do + create_list(:closed_issue, 2, project: project) do |issue| issue.subscriptions.create(user: user, subscribed: true) end end it 'unsubscribes the given user' do + bulk_update(issues, subscription_event: 'unsubscribe') + issues.each do |issue| - expect(issue.subscribed?(user)).to be_falsey + expect(issue).not_to be_subscribed(user) end end end -- cgit v1.2.1 From 5e0669e0eb0fe466bab64f45249b16073e314c99 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 21 Jul 2016 17:02:28 +0800 Subject: Since it's too hard to use JOIN with Rails... feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347#note_13204564 --- app/models/project.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 9b9d497fbd8..d14d15fdc5c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -431,13 +431,13 @@ class Project < ActiveRecord::Base # ref can't be HEAD, can only be branch/tag name or SHA def latest_successful_builds_for(ref = default_branch) - pipeline = pipelines.latest_successful_for(ref).to_sql - join_sql = "INNER JOIN (#{pipeline}) pipelines" + - " ON pipelines.id = #{Ci::Build.quoted_table_name}.commit_id" - builds.joins(join_sql).latest.with_artifacts - # TODO: Whenever we dropped support for MySQL, we could change to: - # pipeline = pipelines.latest_successful_for(ref) - # builds.where(pipeline: pipeline).latest.with_artifacts + latest_pipeline = pipelines.latest_successful_for(ref).first + + if latest_pipeline + latest_pipeline.builds.latest.with_artifacts + else + builds.none + end end def merge_base_commit(first_commit_id, second_commit_id) -- cgit v1.2.1 From 4211f50014faebf027fd2bc80e0cf1d3f012627c Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Thu, 21 Jul 2016 12:34:36 +0300 Subject: Add API documentation for downloading the latest successful build --- doc/api/builds.md | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/doc/api/builds.md b/doc/api/builds.md index 2adea11247e..443739ff845 100644 --- a/doc/api/builds.md +++ b/doc/api/builds.md @@ -283,6 +283,40 @@ Response: [ce-2893]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2893 +## Download the artifacts file + +> [Introduced][ce-5347] in GitLab 8.10. + +Download the artifacts file from the given reference name and job provided the +build finished successfully. + +``` +GET /projects/:id/artifacts/:ref_name/download?job=name +``` + +Parameters + +| Attribute | Type | Required | Description | +|-------------|---------|----------|-------------------------- | +| `id` | integer | yes | The ID of a project | +| `ref_name` | string | yes | The ref from a repository | +| `job` | string | yes | The name of the job | + +Example request: + +``` +curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/artifacts/master/download?job=test" +``` + +Example response: + +| Status | Description | +|-----------|---------------------------------| +| 200 | Serves the artifacts file | +| 404 | Build not found or no artifacts | + +[ce-5347]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5347 + ## Get a trace file Get a trace of a specific build of a project @@ -409,7 +443,7 @@ POST /projects/:id/builds/:build_id/erase Parameters -| Attribute | Type | required | Description | +| Attribute | Type | Required | Description | |-------------|---------|----------|---------------------| | `id` | integer | yes | The ID of a project | | `build_id` | integer | yes | The ID of a build | @@ -459,7 +493,7 @@ POST /projects/:id/builds/:build_id/artifacts/keep Parameters -| Attribute | Type | required | Description | +| Attribute | Type | Required | Description | |-------------|---------|----------|---------------------| | `id` | integer | yes | The ID of a project | | `build_id` | integer | yes | The ID of a build | -- cgit v1.2.1 From 880a40e7e94eb7a3c1e31eda9b6bd9f68b0a4068 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 21 Jul 2016 19:20:25 +0800 Subject: Fix URL in the documentation --- doc/api/builds.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/builds.md b/doc/api/builds.md index 443739ff845..24d90e22a9b 100644 --- a/doc/api/builds.md +++ b/doc/api/builds.md @@ -291,7 +291,7 @@ Download the artifacts file from the given reference name and job provided the build finished successfully. ``` -GET /projects/:id/artifacts/:ref_name/download?job=name +GET /projects/:id/builds/artifacts/:ref_name/download?job=name ``` Parameters @@ -305,7 +305,7 @@ Parameters Example request: ``` -curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/artifacts/master/download?job=test" +curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/builds/artifacts/master/download?job=test" ``` Example response: -- cgit v1.2.1 From 0b67945c99fde0d2c1ac6287f826001ef4c5d03b Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 21 Jul 2016 19:26:58 +0800 Subject: Also fix the URL in the comment --- lib/api/builds.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/builds.rb b/lib/api/builds.rb index 657d421fe97..be5a3484ec8 100644 --- a/lib/api/builds.rb +++ b/lib/api/builds.rb @@ -80,7 +80,7 @@ module API # ref_name (required) - The ref from repository # job (required) - The name for the build # Example Request: - # GET /projects/:id/artifacts/:ref_name/download?job=name + # GET /projects/:id/builds/artifacts/:ref_name/download?job=name get ':id/builds/artifacts/:ref_name/download', requirements: { ref_name: /.+/ } do authorize_read_builds! -- cgit v1.2.1 From 2ba5e6259e14f9bc9f60201da249a5a313bbd0db Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 21 Jul 2016 17:46:33 +0530 Subject: Don't use `params[:id]` while building `markdown_preview_path`. 1. Instead, use `@page.title`, since it's always guaranteed to be around in a wiki. 2. `params[:id]` is _not_ always guaranteed to be around - if a page is created with errors, `render :edit` is called, and `params[:id]` is `nil`. 3. More context: https://gitlab.com/gitlab-org/gitlab-ce/issues/20079#note_13223240 --- app/views/layouts/project.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index 2049b204956..d03d5e2ca6a 100644 --- a/app/views/layouts/project.html.haml +++ b/app/views/layouts/project.html.haml @@ -6,7 +6,7 @@ - content_for :scripts_body_top do - project = @target_project || @project - if @project_wiki && @page - - markdown_preview_path = namespace_project_wiki_markdown_preview_path(project.namespace, project, params[:id]) + - markdown_preview_path = namespace_project_wiki_markdown_preview_path(project.namespace, project, @page.title) - else - markdown_preview_path = markdown_preview_namespace_project_path(project.namespace, project) - if current_user -- cgit v1.2.1 From 36b9cc3fff04efe4c5f2e8609ecd4a6d5d613f22 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Thu, 21 Jul 2016 15:37:32 +0300 Subject: Add allow_failure CI documentation --- doc/ci/yaml/README.md | 81 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 58 insertions(+), 23 deletions(-) diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index ecca469e4b8..ea3fff1596e 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -13,34 +13,36 @@ If you want a quick introduction to GitLab CI, follow our **Table of Contents** *generated with [DocToc](https://github.com/thlorenz/doctoc)* - [.gitlab-ci.yml](#gitlab-ci-yml) - - [image and services](#image-and-services) - - [before_script](#before_script) - - [after_script](#after_script) - - [stages](#stages) - - [types](#types) - - [variables](#variables) - - [cache](#cache) - - [cache:key](#cache-key) + - [image and services](#image-and-services) + - [before_script](#before_script) + - [after_script](#after_script) + - [stages](#stages) + - [types](#types) + - [variables](#variables) + - [cache](#cache) + - [cache:key](#cache-key) - [Jobs](#jobs) - - [script](#script) - - [stage](#stage) - - [only and except](#only-and-except) - - [job variables](#job-variables) - - [tags](#tags) - - [when](#when) - - [environment](#environment) - - [artifacts](#artifacts) - - [artifacts:name](#artifactsname) - - [artifacts:when](#artifactswhen) - - [artifacts:expire_in](#artifactsexpire_in) - - [dependencies](#dependencies) - - [before_script and after_script](#before_script-and-after_script) + - [script](#script) + - [stage](#stage) + - [only and except](#only-and-except) + - [job variables](#job-variables) + - [tags](#tags) + - [allow_failure](#allow_failure) + - [when](#when) + - [Manual actions](#manual-actions) + - [environment](#environment) + - [artifacts](#artifacts) + - [artifacts:name](#artifacts-name) + - [artifacts:when](#artifacts-when) + - [artifacts:expire_in](#artifacts-expire_in) + - [dependencies](#dependencies) + - [before_script and after_script](#before_script-and-after_script) - [Git Strategy](#git-strategy) - [Shallow cloning](#shallow-cloning) - [Hidden jobs](#hidden-jobs) - [Special YAML features](#special-yaml-features) - - [Anchors](#anchors) -- [Validate the .gitlab-ci.yml](#validate-the-gitlab-ciyml) + - [Anchors](#anchors) +- [Validate the .gitlab-ci.yml](#validate-the-gitlab-ci-yml) - [Skipping builds](#skipping-builds) - [Examples](#examples) @@ -473,6 +475,39 @@ job: The specification above, will make sure that `job` is built by a Runner that has both `ruby` AND `postgres` tags defined. +### allow_failure + +`allow_failure` is used when you want to allow a build to fail without impacting +the rest of the CI suite. Failed builds don't contribute to the commit status. + +When enabled and the build fails, the pipeline will be successful/green for all +intents and purposes, but a "CI build passed with warnings" message will be +displayed on the merge request or commit or build page. This is to be used by +builds that are allowed to fail, but where failure indicates some other (manual) +steps should be taken elsewhere. + +In the example below, `job1` and `job2` will run in parallel, but if `job1` +fails, it will not stop the next stage from running, since it's marked with +`allow_failure: true`: + +```yaml +job1: + stage: test + script: + - execute_script_that_will_fail + allow_failure: true + +job2: + stage: test + script: + - execute_script_that_will_succeed + +job3: + stage: deploy + script: + - deploy_to_staging +``` + ### when `when` is used to implement jobs that are run in case of failure or despite the -- cgit v1.2.1 From d62656b081f8abd5affc764c1f92fc4c787522e7 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 15 Jul 2016 16:26:46 -0300 Subject: Update default path for repository_downloads_path in gitlab.yml.example --- config/gitlab.yml.example | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 325eca72862..1470a6e2550 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -106,8 +106,8 @@ production: &base ## Repository downloads directory # When a user clicks e.g. 'Download zip' on a project, a temporary zip file is created in the following directory. - # The default is 'tmp/repositories' relative to the root of the Rails app. - # repository_downloads_path: tmp/repositories + # The default is 'shared/cache/archive/' relative to the root of the Rails app. + # repository_downloads_path: shared/cache/archive/ ## Reply by email # Allow users to comment on issues and merge requests by replying to notification emails. -- cgit v1.2.1 From 6ae177ef53e57d98f95238df990da2225bca3fab Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 15 Jul 2016 16:27:19 -0300 Subject: Avoid data-integrity issue when repository_downloads_path is incorrectly --- config/initializers/1_settings.rb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 693507e0bec..86f55210487 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -211,7 +211,6 @@ Settings.gitlab.default_projects_features['snippets'] = false if Setti Settings.gitlab.default_projects_features['builds'] = true if Settings.gitlab.default_projects_features['builds'].nil? Settings.gitlab.default_projects_features['container_registry'] = true if Settings.gitlab.default_projects_features['container_registry'].nil? Settings.gitlab.default_projects_features['visibility_level'] = Settings.send(:verify_constant, Gitlab::VisibilityLevel, Settings.gitlab.default_projects_features['visibility_level'], Gitlab::VisibilityLevel::PRIVATE) -Settings.gitlab['repository_downloads_path'] = File.join(Settings.shared['path'], 'cache/archive') if Settings.gitlab['repository_downloads_path'].nil? Settings.gitlab['domain_whitelist'] ||= [] Settings.gitlab['import_sources'] ||= %w[github bitbucket gitlab gitorious google_code fogbugz git gitlab_project] Settings.gitlab['trusted_proxies'] ||= [] @@ -315,6 +314,21 @@ Settings.repositories['storages'] ||= {} # Setting gitlab_shell.repos_path is DEPRECATED and WILL BE REMOVED in version 9.0 Settings.repositories.storages['default'] ||= Settings.gitlab_shell['repos_path'] || Settings.gitlab['user_home'] + '/repositories/' +# +# The repository_downloads_path is used to remove outdated repository +# archives, if someone has it configured incorrectly, and it points +# to the path where repositories are stored this can cause some +# data-integrity issue. In this case, we sets it to the default +# repository_downloads_path value. +# +repositories_storages_path = Settings.repositories.storages.values +repository_downloads_path = Settings.gitlab['repository_downloads_path'].to_s.gsub(/\/$/, '') +repository_downloads_full_path = File.expand_path(repository_downloads_path, Settings.gitlab['user_home']) + +if repository_downloads_path.blank? || repositories_storages_path.any? { |path| [repository_downloads_path, repository_downloads_full_path].include?(path.gsub(/\/$/, '')) } + Settings.gitlab['repository_downloads_path'] = File.join(Settings.shared['path'], 'cache/archive') +end + # # Backup # -- cgit v1.2.1 From a6de806498c405355380be4b80f63d134658b779 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 18 Jul 2016 16:38:36 -0300 Subject: Add service to clean up repository archive cache Replace invocation of `find` with Ruby code that matches old cached files in a better, and safe way to avoid data-integrity issues. --- app/models/repository.rb | 10 ------ .../repository_archive_clean_up_service.rb | 42 ++++++++++++++++++++++ app/workers/repository_archive_cache_worker.rb | 2 +- spec/models/repository_spec.rb | 24 ------------- .../repository_archive_clean_up_service_spec.rb | 31 ++++++++++++++++ 5 files changed, 74 insertions(+), 35 deletions(-) create mode 100644 app/services/repository_archive_clean_up_service.rb create mode 100644 spec/services/repository_archive_clean_up_service_spec.rb diff --git a/app/models/repository.rb b/app/models/repository.rb index 511df2d67c6..a6580e85498 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -11,16 +11,6 @@ class Repository attr_accessor :path_with_namespace, :project - def self.clean_old_archives - Gitlab::Metrics.measure(:clean_old_archives) do - repository_downloads_path = Gitlab.config.gitlab.repository_downloads_path - - return unless File.directory?(repository_downloads_path) - - Gitlab::Popen.popen(%W(find #{repository_downloads_path} -not -path #{repository_downloads_path} -mmin +120 -delete)) - end - end - def initialize(path_with_namespace, project) @path_with_namespace = path_with_namespace @project = project diff --git a/app/services/repository_archive_clean_up_service.rb b/app/services/repository_archive_clean_up_service.rb new file mode 100644 index 00000000000..9764df6492d --- /dev/null +++ b/app/services/repository_archive_clean_up_service.rb @@ -0,0 +1,42 @@ +class RepositoryArchiveCleanUpService + ALLOWED_ARCHIVE_EXTENSIONS = %w[tar tar.bz2 tar.gz zip].join(',').freeze + LAST_MODIFIED_TIME_IN_MINUTES = 120 + + def initialize(mmin = LAST_MODIFIED_TIME_IN_MINUTES) + @mmin = mmin + @path = Gitlab.config.gitlab.repository_downloads_path + end + + def execute + Gitlab::Metrics.measure(:repository_archive_clean_up) do + return unless File.directory?(path) + + clean_up_old_archives + clean_up_empty_directories + end + end + + private + + attr_reader :mmin, :path + + def clean_up_old_archives + Dir.glob("#{path}/**.git/*{#{ALLOWED_ARCHIVE_EXTENSIONS}}") do |filename| + File.delete(filename) if older?(filename) + end + end + + def older?(filename) + File.exist?(filename) && File.new(filename).mtime < (Time.now - mmin * 60) + end + + def clean_up_empty_directories + Dir.glob("#{path}/**.git/").reverse_each do |dir| + Dir.rmdir(dir) if empty?(dir) + end + end + + def empty?(dir) + (Dir.entries(dir) - %w[. ..]).empty? + end +end diff --git a/app/workers/repository_archive_cache_worker.rb b/app/workers/repository_archive_cache_worker.rb index 47c5a670ed4..a2e49c61f59 100644 --- a/app/workers/repository_archive_cache_worker.rb +++ b/app/workers/repository_archive_cache_worker.rb @@ -4,6 +4,6 @@ class RepositoryArchiveCacheWorker sidekiq_options queue: :default def perform - Repository.clean_old_archives + RepositoryArchiveCleanUpService.new.execute end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 59c5732c075..38b0c345b48 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1172,30 +1172,6 @@ describe Repository, models: true do end end - describe '.clean_old_archives' do - let(:path) { Gitlab.config.gitlab.repository_downloads_path } - - context 'when the downloads directory does not exist' do - it 'does not remove any archives' do - expect(File).to receive(:directory?).with(path).and_return(false) - - expect(Gitlab::Popen).not_to receive(:popen) - - described_class.clean_old_archives - end - end - - context 'when the downloads directory exists' do - it 'removes old archives' do - expect(File).to receive(:directory?).with(path).and_return(true) - - expect(Gitlab::Popen).to receive(:popen) - - described_class.clean_old_archives - end - end - end - describe "#keep_around" do it "stores a reference to the specified commit sha so it isn't garbage collected" do repository.keep_around(sample_commit.id) diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb new file mode 100644 index 00000000000..a9ac21258da --- /dev/null +++ b/spec/services/repository_archive_clean_up_service_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe RepositoryArchiveCleanUpService, services: true do + describe '#execute' do + let(:path) { Gitlab.config.gitlab.repository_downloads_path } + + subject(:service) { described_class.new } + + context 'when the downloads directory does not exist' do + it 'does not remove any archives' do + expect(File).to receive(:directory?).with(path).and_return(false) + + expect(service).not_to receive(:clean_up_old_archives) + expect(service).not_to receive(:clean_up_empty_directories) + + service.execute + end + end + + context 'when the downloads directory exists' do + it 'removes old archives' do + expect(File).to receive(:directory?).with(path).and_return(true) + + expect(service).to receive(:clean_up_old_archives) + expect(service).to receive(:clean_up_empty_directories) + + service.execute + end + end + end +end -- cgit v1.2.1 From 1bda45178977f613a9f4adc1d00e2694ca3963b5 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 19 Jul 2016 20:45:31 -0300 Subject: Cover the behavior RepositoryArchiveCleanUpService with tests --- .../repository_archive_clean_up_service_spec.rb | 96 ++++++++++++++++++++-- 1 file changed, 89 insertions(+), 7 deletions(-) diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb index a9ac21258da..321cc6daf9d 100644 --- a/spec/services/repository_archive_clean_up_service_spec.rb +++ b/spec/services/repository_archive_clean_up_service_spec.rb @@ -2,14 +2,17 @@ require 'spec_helper' describe RepositoryArchiveCleanUpService, services: true do describe '#execute' do - let(:path) { Gitlab.config.gitlab.repository_downloads_path } + let(:path) { File.join(Rails.root, 'tmp/tests/shared/cache/archive') } subject(:service) { described_class.new } + before do + allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path) + end + context 'when the downloads directory does not exist' do it 'does not remove any archives' do expect(File).to receive(:directory?).with(path).and_return(false) - expect(service).not_to receive(:clean_up_old_archives) expect(service).not_to receive(:clean_up_empty_directories) @@ -18,13 +21,92 @@ describe RepositoryArchiveCleanUpService, services: true do end context 'when the downloads directory exists' do - it 'removes old archives' do - expect(File).to receive(:directory?).with(path).and_return(true) + before do + FileUtils.mkdir_p(path) + end - expect(service).to receive(:clean_up_old_archives) - expect(service).to receive(:clean_up_empty_directories) + after do + FileUtils.rm_rf(path) + end - service.execute + context 'when archives older than 2 hours exists' do + before do + allow_any_instance_of(File).to receive(:mtime).and_return(2.hours.ago) + end + + it 'removes old files that matches valid archive extensions' do + dirname = File.join(path, 'sample.git') + files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip]) + + service.execute + + files.each { |file| expect(File.exist?(file)).to eq false } + expect(File.directory?(dirname)).to eq false + end + + it 'keeps old files that does not matches valid archive extensions' do + dirname = File.join(path, 'sample.git') + files = create_temporary_files(dirname, %w[conf rb]) + + service.execute + + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end + + it 'keeps old files inside invalid directories' do + dirname = File.join(path, 'john_doe/sample.git') + files = create_temporary_files(dirname, %w[conf rb tar tar.gz]) + + service.execute + + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end + end + + context 'when archives older than 2 hours does not exist' do + before do + allow_any_instance_of(File).to receive(:mtime).and_return(1.hour.ago) + end + + it 'keeps files that matches valid archive extensions' do + dirname = File.join(path, 'sample.git') + files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip]) + + service.execute + + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end + + it 'keeps files that does not matches valid archive extensions' do + dirname = File.join(path, 'sample.git') + files = create_temporary_files(dirname, %w[conf rb]) + + service.execute + + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end + + it 'keeps files inside invalid directories' do + dirname = File.join(path, 'john_doe/sample.git') + files = create_temporary_files(dirname, %w[conf rb tar tar.gz]) + + service.execute + + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end + end + + def create_temporary_files(dirname, extensions) + FileUtils.mkdir_p(dirname) + + extensions.flat_map do |extension| + FileUtils.touch(File.join(dirname, "sample.#{extension}")) + end end end end -- cgit v1.2.1 From 79983afbbf052476eee3d86e0b970326e64f8514 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 20 Jul 2016 20:29:25 -0300 Subject: Use find instead Ruby to remove files due to performance reasons --- .../repository_archive_clean_up_service.rb | 17 ++++---------- .../repository_archive_clean_up_service_spec.rb | 27 +++++++--------------- 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/app/services/repository_archive_clean_up_service.rb b/app/services/repository_archive_clean_up_service.rb index 9764df6492d..0b56b09738d 100644 --- a/app/services/repository_archive_clean_up_service.rb +++ b/app/services/repository_archive_clean_up_service.rb @@ -1,5 +1,4 @@ class RepositoryArchiveCleanUpService - ALLOWED_ARCHIVE_EXTENSIONS = %w[tar tar.bz2 tar.gz zip].join(',').freeze LAST_MODIFIED_TIME_IN_MINUTES = 120 def initialize(mmin = LAST_MODIFIED_TIME_IN_MINUTES) @@ -21,22 +20,14 @@ class RepositoryArchiveCleanUpService attr_reader :mmin, :path def clean_up_old_archives - Dir.glob("#{path}/**.git/*{#{ALLOWED_ARCHIVE_EXTENSIONS}}") do |filename| - File.delete(filename) if older?(filename) - end - end - - def older?(filename) - File.exist?(filename) && File.new(filename).mtime < (Time.now - mmin * 60) + run(%W(find #{path} -not -path #{path} -type f \( -name \*.tar -o -name \*.bz2 -o -name \*.tar.gz -o -name \*.zip \) -maxdepth 2 -mmin +#{mmin} -delete)) end def clean_up_empty_directories - Dir.glob("#{path}/**.git/").reverse_each do |dir| - Dir.rmdir(dir) if empty?(dir) - end + run(%W(find #{path} -not -path #{path} -type d -empty -name \*.git -maxdepth 1 -delete)) end - def empty?(dir) - (Dir.entries(dir) - %w[. ..]).empty? + def run(cmd) + Gitlab::Popen.popen(cmd) end end diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb index 321cc6daf9d..6173d6cb51c 100644 --- a/spec/services/repository_archive_clean_up_service_spec.rb +++ b/spec/services/repository_archive_clean_up_service_spec.rb @@ -30,13 +30,9 @@ describe RepositoryArchiveCleanUpService, services: true do end context 'when archives older than 2 hours exists' do - before do - allow_any_instance_of(File).to receive(:mtime).and_return(2.hours.ago) - end - it 'removes old files that matches valid archive extensions' do dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip]) + files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip], 2.hours) service.execute @@ -46,7 +42,7 @@ describe RepositoryArchiveCleanUpService, services: true do it 'keeps old files that does not matches valid archive extensions' do dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[conf rb]) + files = create_temporary_files(dirname, %w[conf rb], 2.hours) service.execute @@ -56,7 +52,7 @@ describe RepositoryArchiveCleanUpService, services: true do it 'keeps old files inside invalid directories' do dirname = File.join(path, 'john_doe/sample.git') - files = create_temporary_files(dirname, %w[conf rb tar tar.gz]) + files = create_temporary_files(dirname, %w[conf rb tar tar.gz], 2.hours) service.execute @@ -66,13 +62,9 @@ describe RepositoryArchiveCleanUpService, services: true do end context 'when archives older than 2 hours does not exist' do - before do - allow_any_instance_of(File).to receive(:mtime).and_return(1.hour.ago) - end - it 'keeps files that matches valid archive extensions' do dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip]) + files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip], 1.hour) service.execute @@ -82,7 +74,7 @@ describe RepositoryArchiveCleanUpService, services: true do it 'keeps files that does not matches valid archive extensions' do dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[conf rb]) + files = create_temporary_files(dirname, %w[conf rb], 1.hour) service.execute @@ -92,7 +84,7 @@ describe RepositoryArchiveCleanUpService, services: true do it 'keeps files inside invalid directories' do dirname = File.join(path, 'john_doe/sample.git') - files = create_temporary_files(dirname, %w[conf rb tar tar.gz]) + files = create_temporary_files(dirname, %w[conf rb tar tar.gz], 1.hour) service.execute @@ -101,12 +93,9 @@ describe RepositoryArchiveCleanUpService, services: true do end end - def create_temporary_files(dirname, extensions) + def create_temporary_files(dirname, extensions, mtime) FileUtils.mkdir_p(dirname) - - extensions.flat_map do |extension| - FileUtils.touch(File.join(dirname, "sample.#{extension}")) - end + FileUtils.touch(extensions.map { |ext| File.join(dirname, "sample.#{ext}") }, mtime: Time.now - mtime) end end end -- cgit v1.2.1 From 6130376ad6673facb729b78878f9156a29948a5b Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Wed, 20 Jul 2016 09:17:52 -0700 Subject: Bug fixes --- app/assets/stylesheets/pages/commit.scss | 8 ++++++++ app/assets/stylesheets/pages/pipelines.scss | 6 +++++- app/views/projects/ci/builds/_build.html.haml | 10 ++++++---- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/pages/commit.scss b/app/assets/stylesheets/pages/commit.scss index 35ab28b3fea..29d3bf8ae6f 100644 --- a/app/assets/stylesheets/pages/commit.scss +++ b/app/assets/stylesheets/pages/commit.scss @@ -66,6 +66,14 @@ margin-left: 8px; } } + + .ci-status-link { + svg { + position: relative; + top: 2px; + margin: 0 3px; + } + } } .commit-box { diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index a404f108dc4..a45c675e307 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -93,7 +93,7 @@ .commit-title { margin-top: 4px; - max-width: 320px; + max-width: 300px; overflow: hidden; white-space: nowrap; text-overflow: ellipsis; @@ -139,6 +139,10 @@ width: 18px; vertical-align: middle; } + + .light { + width: 3px; + } } .duration, diff --git a/app/views/projects/ci/builds/_build.html.haml b/app/views/projects/ci/builds/_build.html.haml index 9264289987d..a9fb3c58431 100644 --- a/app/views/projects/ci/builds/_build.html.haml +++ b/app/views/projects/ci/builds/_build.html.haml @@ -14,16 +14,19 @@ %span ##{build.id} - if build.stuck? - = icon('warning', class: 'text-warning has-tooltip', title: 'Build is stuck. Check runners.') + .icon-container + = icon('warning', class: 'text-warning has-tooltip', title: 'Build is stuck. Check runners.') - if defined?(retried) && retried - = icon('warning', class: 'text-warning has-tooltip', title: 'Build was retried.') + .icon-container + = icon('warning', class: 'text-warning has-tooltip', title: 'Build was retried.') - if defined?(ref) && ref - if build.ref = link_to build.ref, namespace_project_commits_path(build.project.namespace, build.project, build.ref), class: "monospace branch-name" - else .light none - = custom_icon("icon_commit") + .icon-container + = custom_icon("icon_commit") - if defined?(commit_sha) && commit_sha = link_to build.short_sha, namespace_project_commit_path(build.project.namespace, build.project, build.sha), class: "commit-id monospace" @@ -88,4 +91,3 @@ - elsif build.playable? = link_to play_namespace_project_build_path(build.project.namespace, build.project, build, return_to: request.original_url), method: :post, title: 'Play', class: 'btn btn-build' do = icon('play') - -- cgit v1.2.1 From 23a437f10454125219d5510191d058af7f8afe76 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Wed, 20 Jul 2016 20:43:43 -0700 Subject: Add new fork SVG to fix weird styling of other SVGs --- app/views/shared/icons/_icon_fork.svg | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/views/shared/icons/_icon_fork.svg b/app/views/shared/icons/_icon_fork.svg index 420ffe3a55b..a21f8f3a951 100644 --- a/app/views/shared/icons/_icon_fork.svg +++ b/app/views/shared/icons/_icon_fork.svg @@ -1 +1,3 @@ - \ No newline at end of file + + + -- cgit v1.2.1 From 76a27d7c5803f031a8fe541238ee25f6776aeeab Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 21 Jul 2016 06:51:12 -0700 Subject: Fix icons on commits page and builds page --- app/assets/stylesheets/pages/builds.scss | 8 ++++++++ app/views/projects/commits/_commit.html.haml | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss index 99a2cd306cf..e26f8f7080d 100644 --- a/app/assets/stylesheets/pages/builds.scss +++ b/app/assets/stylesheets/pages/builds.scss @@ -53,6 +53,14 @@ left: 70px; } } + + .nav-links { + svg { + position: relative; + top: 2px; + margin-right: 3px; + } + } } .build-header { diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index ab9afb06afb..04cf2965454 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -25,7 +25,7 @@ .commit-actions.hidden-xs - if commit.status - = render_commit_status(commit, cssclass: 'btn btn-transparent') + = render_commit_status(commit) = clipboard_button(clipboard_text: commit.id) = link_to commit.short_id, namespace_project_commit_path(project.namespace, project, commit), class: "commit-short-id btn btn-transparent" = link_to_browse_code(project, commit) -- cgit v1.2.1 From 0b36dcb8b043e987eca4f5efc75fb00c161d1a2b Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 21 Jul 2016 07:28:41 -0700 Subject: Fix firefox rendering of SVGs --- app/assets/stylesheets/pages/merge_requests.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 4e806e60e7e..db295935b00 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -64,6 +64,7 @@ margin-right: 4px; position: relative; top: 1px; + overflow: visible; } &.ci-success { -- cgit v1.2.1 From 92ee8c5e6489a91d672f51a8807d755e52db4c05 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 20 Jul 2016 20:57:34 -0300 Subject: Use Dir.mktmpdir instead of FileUtils.mkdir_p in the spec --- .../repository_archive_clean_up_service_spec.rb | 104 +++++++++++---------- 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb index 6173d6cb51c..0a3c262df32 100644 --- a/spec/services/repository_archive_clean_up_service_spec.rb +++ b/spec/services/repository_archive_clean_up_service_spec.rb @@ -2,16 +2,13 @@ require 'spec_helper' describe RepositoryArchiveCleanUpService, services: true do describe '#execute' do - let(:path) { File.join(Rails.root, 'tmp/tests/shared/cache/archive') } - subject(:service) { described_class.new } - before do - allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path) - end - context 'when the downloads directory does not exist' do it 'does not remove any archives' do + path = '/invalid/path/' + stub_repository_downloads_path(path) + expect(File).to receive(:directory?).with(path).and_return(false) expect(service).not_to receive(:clean_up_old_archives) expect(service).not_to receive(:clean_up_empty_directories) @@ -21,82 +18,93 @@ describe RepositoryArchiveCleanUpService, services: true do end context 'when the downloads directory exists' do - before do - FileUtils.mkdir_p(path) - end - - after do - FileUtils.rm_rf(path) - end - context 'when archives older than 2 hours exists' do it 'removes old files that matches valid archive extensions' do - dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip], 2.hours) + Dir.mktmpdir do |path| + stub_repository_downloads_path(path) + dirname = File.join(path, 'sample.git') + files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip], 2.hours) - service.execute + service.execute - files.each { |file| expect(File.exist?(file)).to eq false } - expect(File.directory?(dirname)).to eq false + files.each { |file| expect(File.exist?(file)).to eq false } + expect(File.directory?(dirname)).to eq false + end end it 'keeps old files that does not matches valid archive extensions' do - dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[conf rb], 2.hours) + Dir.mktmpdir do |path| + stub_repository_downloads_path(path) + dirname = File.join(path, 'sample.git') + files = create_temporary_files(dirname, %w[conf rb], 2.hours) - service.execute + service.execute - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end end it 'keeps old files inside invalid directories' do - dirname = File.join(path, 'john_doe/sample.git') - files = create_temporary_files(dirname, %w[conf rb tar tar.gz], 2.hours) + Dir.mktmpdir do |path| + stub_repository_downloads_path(path) + dirname = File.join(path, 'john_doe/sample.git') + files = create_temporary_files(dirname, %w[conf rb tar tar.gz], 2.hours) - service.execute + service.execute - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end end end context 'when archives older than 2 hours does not exist' do it 'keeps files that matches valid archive extensions' do - dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip], 1.hour) + Dir.mktmpdir do |path| + dirname = File.join(path, 'sample.git') + files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip], 1.hour) - service.execute + service.execute - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end end it 'keeps files that does not matches valid archive extensions' do - dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[conf rb], 1.hour) + Dir.mktmpdir do |path| + dirname = File.join(path, 'sample.git') + files = create_temporary_files(dirname, %w[conf rb], 1.hour) - service.execute + service.execute - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end end it 'keeps files inside invalid directories' do - dirname = File.join(path, 'john_doe/sample.git') - files = create_temporary_files(dirname, %w[conf rb tar tar.gz], 1.hour) + Dir.mktmpdir do |path| + dirname = File.join(path, 'john_doe/sample.git') + files = create_temporary_files(dirname, %w[conf rb tar tar.gz], 1.hour) - service.execute + service.execute - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true + files.each { |file| expect(File.exist?(file)).to eq true } + expect(File.directory?(dirname)).to eq true + end end end + end - def create_temporary_files(dirname, extensions, mtime) - FileUtils.mkdir_p(dirname) - FileUtils.touch(extensions.map { |ext| File.join(dirname, "sample.#{ext}") }, mtime: Time.now - mtime) - end + def create_temporary_files(dirname, extensions, mtime) + FileUtils.mkdir_p(dirname) + FileUtils.touch(extensions.map { |ext| File.join(dirname, "sample.#{ext}") }, mtime: Time.now - mtime) + end + + def stub_repository_downloads_path(path) + allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path) end end end -- cgit v1.2.1 From 2053d89172b5edfa56c7f4e0451fcff74a5e26b5 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 21 Jul 2016 11:42:44 -0300 Subject: Extract helper methods to clean up RepositoryArchiveCleanUpService spec --- .../repository_archive_clean_up_service_spec.rb | 103 ++++++++------------- 1 file changed, 37 insertions(+), 66 deletions(-) diff --git a/spec/services/repository_archive_clean_up_service_spec.rb b/spec/services/repository_archive_clean_up_service_spec.rb index 0a3c262df32..842585f9e54 100644 --- a/spec/services/repository_archive_clean_up_service_spec.rb +++ b/spec/services/repository_archive_clean_up_service_spec.rb @@ -18,93 +18,64 @@ describe RepositoryArchiveCleanUpService, services: true do end context 'when the downloads directory exists' do - context 'when archives older than 2 hours exists' do - it 'removes old files that matches valid archive extensions' do - Dir.mktmpdir do |path| - stub_repository_downloads_path(path) - dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip], 2.hours) - - service.execute - - files.each { |file| expect(File.exist?(file)).to eq false } - expect(File.directory?(dirname)).to eq false - end - end - - it 'keeps old files that does not matches valid archive extensions' do - Dir.mktmpdir do |path| - stub_repository_downloads_path(path) - dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[conf rb], 2.hours) - - service.execute - - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true - end - end - - it 'keeps old files inside invalid directories' do - Dir.mktmpdir do |path| - stub_repository_downloads_path(path) - dirname = File.join(path, 'john_doe/sample.git') - files = create_temporary_files(dirname, %w[conf rb tar tar.gz], 2.hours) - + shared_examples 'invalid archive files' do |dirname, extensions, mtime| + it 'does not remove files and directoy' do + in_directory_with_files(dirname, extensions, mtime) do |dir, files| service.execute files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true + expect(File.directory?(dir)).to eq true end end end - context 'when archives older than 2 hours does not exist' do - it 'keeps files that matches valid archive extensions' do - Dir.mktmpdir do |path| - dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[tar tar.bz2 tar.gz zip], 1.hour) + it 'removes files older than 2 hours that matches valid archive extensions' do + in_directory_with_files('sample.git', %w[tar tar.bz2 tar.gz zip], 2.hours) do |dir, files| + service.execute - service.execute - - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true - end + files.each { |file| expect(File.exist?(file)).to eq false } + expect(File.directory?(dir)).to eq false end + end - it 'keeps files that does not matches valid archive extensions' do - Dir.mktmpdir do |path| - dirname = File.join(path, 'sample.git') - files = create_temporary_files(dirname, %w[conf rb], 1.hour) - - service.execute + context 'with files older than 2 hours that does not matches valid archive extensions' do + it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb], 2.hours + end - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true - end - end + context 'with files older than 2 hours inside invalid directories' do + it_behaves_like 'invalid archive files', 'john_doe/sample.git', %w[conf rb tar tar.gz], 2.hours + end - it 'keeps files inside invalid directories' do - Dir.mktmpdir do |path| - dirname = File.join(path, 'john_doe/sample.git') - files = create_temporary_files(dirname, %w[conf rb tar tar.gz], 1.hour) + context 'with files newer than 2 hours that matches valid archive extensions' do + it_behaves_like 'invalid archive files', 'sample.git', %w[tar tar.bz2 tar.gz zip], 1.hour + end - service.execute + context 'with files newer than 2 hours that does not matches valid archive extensions' do + it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb], 1.hour + end - files.each { |file| expect(File.exist?(file)).to eq true } - expect(File.directory?(dirname)).to eq true - end - end + context 'with files newer than 2 hours inside invalid directories' do + it_behaves_like 'invalid archive files', 'sample.git', %w[conf rb tar tar.gz], 1.hour end end - def create_temporary_files(dirname, extensions, mtime) - FileUtils.mkdir_p(dirname) - FileUtils.touch(extensions.map { |ext| File.join(dirname, "sample.#{ext}") }, mtime: Time.now - mtime) + def in_directory_with_files(dirname, extensions, mtime) + Dir.mktmpdir do |tmpdir| + stub_repository_downloads_path(tmpdir) + dir = File.join(tmpdir, dirname) + files = create_temporary_files(dir, extensions, mtime) + + yield(dir, files) + end end def stub_repository_downloads_path(path) allow(Gitlab.config.gitlab).to receive(:repository_downloads_path).and_return(path) end + + def create_temporary_files(dir, extensions, mtime) + FileUtils.mkdir_p(dir) + FileUtils.touch(extensions.map { |ext| File.join(dir, "sample.#{ext}") }, mtime: Time.now - mtime) + end end end -- cgit v1.2.1 From 3b2c17a9a203aa8e82a3367a77d28eacaa5a0ab7 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 21 Jul 2016 11:48:07 -0300 Subject: Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 017cf92160f..8fd65f76ccb 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -21,6 +21,7 @@ v 8.10.0 (unreleased) - Delete award emoji when deleting a user - Remove pinTo from Flash and make inline flash messages look nicer. !4854 (winniehell) - Add an API for downloading latest successful build from a particular branch or tag. !5347 + - Avoid data-integrity issue when cleaning up repository archive cache. - Add link to profile to commit avatar. !5163 (winniehell) - Wrap code blocks on Activies and Todos page. !4783 (winniehell) - Align flash messages with left side of page content. !4959 (winniehell) -- cgit v1.2.1 From 6b5708e073cfb8003b82dc3b6c16fa479973ae87 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 21 Jul 2016 07:37:31 -0700 Subject: Fix ci icons getting cut off --- app/assets/stylesheets/pages/commit.scss | 10 ++++------ app/assets/stylesheets/pages/pipelines.scss | 1 + app/assets/stylesheets/pages/status.scss | 1 + app/helpers/ci_status_helper.rb | 4 ++-- app/views/projects/commits/_commit.html.haml | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/pages/commit.scss b/app/assets/stylesheets/pages/commit.scss index 29d3bf8ae6f..bbe0c6c5f1f 100644 --- a/app/assets/stylesheets/pages/commit.scss +++ b/app/assets/stylesheets/pages/commit.scss @@ -66,13 +66,11 @@ margin-left: 8px; } } +} - .ci-status-link { - svg { - position: relative; - top: 2px; - margin: 0 3px; - } +.ci-status-link { + svg { + overflow: visible; } } diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index a45c675e307..081bd7204fe 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -138,6 +138,7 @@ height: 18px; width: 18px; vertical-align: middle; + overflow: visible; } .light { diff --git a/app/assets/stylesheets/pages/status.scss b/app/assets/stylesheets/pages/status.scss index 99f64b5e4cc..210ace357c0 100644 --- a/app/assets/stylesheets/pages/status.scss +++ b/app/assets/stylesheets/pages/status.scss @@ -49,6 +49,7 @@ position: relative; top: 1px; margin: 0 3px; + overflow: visible; } } diff --git a/app/helpers/ci_status_helper.rb b/app/helpers/ci_status_helper.rb index ffac28f78a0..ea2f5f9281a 100644 --- a/app/helpers/ci_status_helper.rb +++ b/app/helpers/ci_status_helper.rb @@ -45,10 +45,10 @@ module CiStatusHelper custom_icon(icon_name) end - def render_commit_status(commit, tooltip_placement: 'auto left', cssclass: '') + def render_commit_status(commit, tooltip_placement: 'auto left') project = commit.project path = builds_namespace_project_commit_path(project.namespace, project, commit) - render_status_with_link('commit', commit.status, path, tooltip_placement, cssclass: cssclass) + render_status_with_link('commit', commit.status, path, tooltip_placement) end def render_pipeline_status(pipeline, tooltip_placement: 'auto left') diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 04cf2965454..b195822d184 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -19,7 +19,7 @@ · = commit.short_id - if commit.status - = render_commit_status(commit, cssclass: 'visible-xs-inline') + = render_commit_status(commit) - if commit.description? %a.text-expander.hidden-xs.js-toggle-button ... -- cgit v1.2.1 From 420f117df98faef0ff06ea7eceabe46994518559 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 21 Jul 2016 08:31:05 -0700 Subject: Mobile view for commit status --- app/assets/stylesheets/pages/status.scss | 8 ++++++++ app/views/projects/commits/_commit.html.haml | 3 ++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/pages/status.scss b/app/assets/stylesheets/pages/status.scss index 210ace357c0..587f2d9f3c1 100644 --- a/app/assets/stylesheets/pages/status.scss +++ b/app/assets/stylesheets/pages/status.scss @@ -75,3 +75,11 @@ color: $gl-gray; } } + +.visible-xs-inline { + .ci-status-link { + position: relative; + top: 2px; + left: 5px; + } +} diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index b195822d184..fd888f41b1e 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -19,7 +19,8 @@ · = commit.short_id - if commit.status - = render_commit_status(commit) + .visible-xs-inline + = render_commit_status(commit) - if commit.description? %a.text-expander.hidden-xs.js-toggle-button ... -- cgit v1.2.1 From b7c5cf9edb0b58d885be7783dd5f6ada756ccacd Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 21 Jul 2016 17:43:43 +0200 Subject: Don't drop in DropAndReaddHasExternalWikiInProjects Dropping a column and then re-adding it can lead to the application throwing errors as the column may temporarily not exist. To work around this we'll reset the various project rows in batches _without_ removing any columns. --- .../20160721081015_drop_and_readd_has_external_wiki_in_projects.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb b/db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb index 459a120155d..1eb99feb40c 100644 --- a/db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb +++ b/db/migrate/20160721081015_drop_and_readd_has_external_wiki_in_projects.rb @@ -5,8 +5,9 @@ class DropAndReaddHasExternalWikiInProjects < ActiveRecord::Migration DOWNTIME = false def up - remove_column :projects, :has_external_wiki, :boolean - add_column :projects, :has_external_wiki, :boolean + update_column_in_batches(:projects, :has_external_wiki, nil) do |table, query| + query.where(table[:has_external_wiki].not_eq(nil)) + end end def down -- cgit v1.2.1 From a6780a77df17214093166d43799443d4a15ff6ae Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 21 Jul 2016 08:38:19 -0700 Subject: Fix sha icon positioning on safari --- app/assets/stylesheets/pages/pipelines.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index 081bd7204fe..843f04ab1f6 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -76,7 +76,7 @@ svg { height: 14px; - width: auto; + width: 14px; vertical-align: middle; fill: $table-text-gray; } @@ -158,7 +158,7 @@ svg { width: 12px; - height: auto; + height: 12px; vertical-align: middle; margin-right: 4px; } -- cgit v1.2.1 From b2a4b6defefb081e97c7c57123b9e64b209bdd20 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 21 Jul 2016 10:00:45 -0700 Subject: Bump vmstat version to fix issues reporting on FreeBSD --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index ead64a6d4df..92e666c1bb7 100644 --- a/Gemfile +++ b/Gemfile @@ -347,5 +347,5 @@ gem 'paranoia', '~> 2.0' gem 'health_check', '~> 2.1.0' # System information -gem 'vmstat', '~> 2.1.0' +gem 'vmstat', '~> 2.1.1' gem 'sys-filesystem', '~> 1.1.6' diff --git a/Gemfile.lock b/Gemfile.lock index 8739f8579d5..e2b3d55ee0c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -775,7 +775,7 @@ GEM coercible (~> 1.0) descendants_tracker (~> 0.0, >= 0.0.3) equalizer (~> 0.0, >= 0.0.9) - vmstat (2.1.0) + vmstat (2.1.1) warden (1.2.6) rack (>= 1.0) web-console (2.3.0) @@ -980,7 +980,7 @@ DEPENDENCIES unicorn-worker-killer (~> 0.4.2) version_sorter (~> 2.0.0) virtus (~> 1.0.1) - vmstat (~> 2.1.0) + vmstat (~> 2.1.1) web-console (~> 2.0) webmock (~> 1.21.0) wikicloth (= 0.8.1) -- cgit v1.2.1 From 4e321b01c6d2e26dae52573092764dec9faf2060 Mon Sep 17 00:00:00 2001 From: Mark Pundsack Date: Thu, 21 Jul 2016 11:33:07 -0700 Subject: Explain CI_PROJECT_NAMESPACE better --- doc/ci/variables/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index 4bf610f0e9a..4a7c21f811d 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -38,7 +38,7 @@ The `API_TOKEN` will take the Secure Variable value: `SECURE`. | **CI_PIPELINE_ID** | 8.10 | 0.5 | The unique id of the current pipeline that GitLab CI uses internally | | **CI_PROJECT_ID** | all | all | The unique id of the current project that GitLab CI uses internally | | **CI_PROJECT_NAME** | 8.10 | 0.5 | The project name that is currently being built | -| **CI_PROJECT_NAMESPACE**| 8.10 | 0.5 | The project namespace that is currently being built | +| **CI_PROJECT_NAMESPACE**| 8.10 | 0.5 | The project namespace (username or groupname) that is currently being built | | **CI_PROJECT_PATH** | 8.10 | 0.5 | The namespace with project name | | **CI_PROJECT_URL** | 8.10 | 0.5 | The HTTP address to access project | | **CI_PROJECT_DIR** | all | all | The full path where the repository is cloned and where the build is run | -- cgit v1.2.1 From 0c14c6332d38704a7bfd8916a8deedd5c5808978 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Thu, 21 Jul 2016 19:15:31 +0200 Subject: Retrieve rendered HTML from cache in one request See #19985 --- CHANGELOG | 1 + lib/banzai/reference_extractor.rb | 9 +++++---- spec/models/note_spec.rb | 40 +++++++++++++++++++++++---------------- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 017cf92160f..8fe2caac7bd 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.11.0 (unreleased) - Fix of 'Commits being passed to custom hooks are already reachable when using the UI' - Limit git rev-list output count to one in forced push check + - Retrieve rendered HTML from cache in one request v 8.10.0 (unreleased) - Fix profile activity heatmap to show correct day name (eanplatter) diff --git a/lib/banzai/reference_extractor.rb b/lib/banzai/reference_extractor.rb index bf366962aef..b26a41a1f3b 100644 --- a/lib/banzai/reference_extractor.rb +++ b/lib/banzai/reference_extractor.rb @@ -2,11 +2,11 @@ module Banzai # Extract possible GFM references from an arbitrary String for further processing. class ReferenceExtractor def initialize - @texts = [] + @texts_and_contexts = [] end def analyze(text, context = {}) - @texts << Renderer.render(text, context) + @texts_and_contexts << { text: text, context: context } end def references(type, project, current_user = nil) @@ -21,9 +21,10 @@ module Banzai def html_documents # This ensures that we don't memoize anything until we have a number of # text blobs to parse. - return [] if @texts.empty? + return [] if @texts_and_contexts.empty? - @html_documents ||= @texts.map { |html| Nokogiri::HTML.fragment(html) } + @html_documents ||= Renderer.cache_collection_render(@texts_and_contexts) + .map { |html| Nokogiri::HTML.fragment(html) } end end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 7d0697dab42..1243f5420a7 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -135,22 +135,30 @@ describe Note, models: true do let!(:note2) { create(:note_on_issue) } it "reads the rendered note body from the cache" do - expect(Banzai::Renderer).to receive(:render). - with(note1.note, - pipeline: :note, - cache_key: [note1, "note"], - project: note1.project, - author: note1.author) - - expect(Banzai::Renderer).to receive(:render). - with(note2.note, - pipeline: :note, - cache_key: [note2, "note"], - project: note2.project, - author: note2.author) - - note1.all_references - note2.all_references + expect(Banzai::Renderer).to receive(:cache_collection_render). + with([{ + text: note1.note, + context: { + pipeline: :note, + cache_key: [note1, "note"], + project: note1.project, + author: note1.author + } + }]).and_call_original + + expect(Banzai::Renderer).to receive(:cache_collection_render). + with([{ + text: note2.note, + context: { + pipeline: :note, + cache_key: [note2, "note"], + project: note2.project, + author: note2.author + } + }]).and_call_original + + note1.all_references.users + note2.all_references.users end end -- cgit v1.2.1 From 25ce5e7e707ddaf08e134c56419d95065b9479e8 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 21 Jul 2016 12:42:23 -0700 Subject: Reduce min width of pipeline table --- app/assets/stylesheets/pages/pipelines.scss | 9 +++++++++ app/views/projects/commit/_pipeline.html.haml | 4 ++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index 843f04ab1f6..c58e2ffe7f5 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -29,9 +29,18 @@ } } +.pipeline-holder { + width: 100%; + overflow: auto; +} + .table.builds { min-width: 1200px; + &.pipeline { + min-width: 650px; + } + tr { th { padding: 16px 8px; diff --git a/app/views/projects/commit/_pipeline.html.haml b/app/views/projects/commit/_pipeline.html.haml index 41fd5459429..540689f4a61 100644 --- a/app/views/projects/commit/_pipeline.html.haml +++ b/app/views/projects/commit/_pipeline.html.haml @@ -35,8 +35,8 @@ .bs-callout.bs-callout-warning \.gitlab-ci.yml not found in this commit -.table-holder - %table.table.builds +.table-holder.pipeline-holder + %table.table.builds.pipeline %thead %tr %th Status -- cgit v1.2.1 From 04ad02bc9f1c06b4f87ae105be74e8b47072ee02 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Date: Thu, 21 Jul 2016 14:12:08 -0700 Subject: Change nav link snippet controller --- app/views/layouts/nav/_dashboard.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 21668698814..3a14751ea8e 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -30,7 +30,7 @@ %span Merge Requests %span.count= number_with_delimiter(current_user.assigned_merge_requests.opened.count) - = nav_link(controller: :snippets) do + = nav_link(controller: 'dashboard/snippets') do = link_to dashboard_snippets_path, title: 'Snippets' do %span Snippets -- cgit v1.2.1 From 04fff5cd8f4322ddc7729715dcc8098fb5211b24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Thu, 21 Jul 2016 17:48:57 -0400 Subject: Update documentation according to the new multiple git mount points feature --- doc/administration/repository_storages.md | 3 ++- doc/raketasks/cleanup.md | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/administration/repository_storages.md b/doc/administration/repository_storages.md index 81bfe173151..fd2b4f00960 100644 --- a/doc/administration/repository_storages.md +++ b/doc/administration/repository_storages.md @@ -1,7 +1,8 @@ # Repository storages GitLab allows you to define repository storage paths to enable distribution of -storage load between several mount points. +storage load between several mount points. You can choose where new projects are +stored via the `Application Settings` in the Admin interface. ## For installations from source diff --git a/doc/raketasks/cleanup.md b/doc/raketasks/cleanup.md index 8fbcbb983e9..cf891cd90ad 100644 --- a/doc/raketasks/cleanup.md +++ b/doc/raketasks/cleanup.md @@ -2,7 +2,7 @@ ## Remove garbage from filesystem. Important! Data loss! -Remove namespaces(dirs) from `/home/git/repositories` if they don't exist in GitLab database. +Remove namespaces(dirs) from all repository storage paths if they don't exist in GitLab database. ``` # omnibus-gitlab @@ -12,7 +12,7 @@ sudo gitlab-rake gitlab:cleanup:dirs bundle exec rake gitlab:cleanup:dirs RAILS_ENV=production ``` -Rename repositories from `/home/git/repositories` if they don't exist in GitLab database. +Rename repositories from all repository storage paths if they don't exist in GitLab database. The repositories get a `+orphaned+TIMESTAMP` suffix so that they cannot block new repositories from being created. ``` -- cgit v1.2.1 From 065a65adfe3508581664358779821b802476ee0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Thu, 21 Jul 2016 16:49:43 -0400 Subject: Update to gitlab_git 10.4.1 and take advantage of preserved Ref objects --- CHANGELOG | 1 + Gemfile | 2 +- Gemfile.lock | 4 ++-- app/models/repository.rb | 25 ++++++++++++++----------- app/services/delete_branch_service.rb | 2 +- app/services/delete_tag_service.rb | 2 +- app/services/git_tag_push_service.rb | 4 ++-- app/views/projects/branches/_commit.html.haml | 2 +- spec/models/repository_spec.rb | 26 ++++++-------------------- 9 files changed, 29 insertions(+), 39 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 938e3a496a6..bc09a51cc1e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -82,6 +82,7 @@ v 8.10.0 (unreleased) - API: Todos. !3188 (Robert Schilling) - API: Expose shared groups for projects and shared projects for groups. !5050 (Robert Schilling) - API: Expose `developers_can_push` and `developers_can_merge` for branches. !5208 (Robert Schilling) + - Update to gitlab_git 10.4.1 and take advantage of preserved Ref objects - Add "Enabled Git access protocols" to Application Settings - Diffs will create button/diff form on demand no on server side - Reduce size of HTML used by diff comment forms diff --git a/Gemfile b/Gemfile index 92e666c1bb7..0504e643ed7 100644 --- a/Gemfile +++ b/Gemfile @@ -52,7 +52,7 @@ gem 'browser', '~> 2.2' # Extracting information from a git repository # Provide access to Gitlab::Git library -gem 'gitlab_git', '~> 10.3.2' +gem 'gitlab_git', '~> 10.4.1' # LDAP Auth # GitLab fork with several improvements to original library. For full list of changes diff --git a/Gemfile.lock b/Gemfile.lock index e2b3d55ee0c..195516d1bf1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -274,7 +274,7 @@ GEM diff-lcs (~> 1.1) mime-types (>= 1.16, < 3) posix-spawn (~> 0.3) - gitlab_git (10.3.2) + gitlab_git (10.4.1) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) @@ -861,7 +861,7 @@ DEPENDENCIES github-linguist (~> 4.7.0) github-markup (~> 1.4) gitlab-flowdock-git-hook (~> 1.0.1) - gitlab_git (~> 10.3.2) + gitlab_git (~> 10.4.1) gitlab_meta (= 7.0) gitlab_omniauth-ldap (~> 1.2.1) gollum-lib (~> 4.2) diff --git a/app/models/repository.rb b/app/models/repository.rb index a6580e85498..46a04eb80cd 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -70,7 +70,12 @@ class Repository def commit(ref = 'HEAD') return nil unless exists? - commit = Gitlab::Git::Commit.find(raw_repository, ref) + commit = + if ref.is_a?(Gitlab::Git::Commit) + ref + else + Gitlab::Git::Commit.find(raw_repository, ref) + end commit = ::Commit.new(commit, @project) if commit commit rescue Rugged::OdbError @@ -247,10 +252,10 @@ class Repository # Rugged seems to throw a `ReferenceError` when given branch_names rather # than SHA-1 hashes number_commits_behind = raw_repository. - count_commits_between(branch.target, root_ref_hash) + count_commits_between(branch.target.sha, root_ref_hash) number_commits_ahead = raw_repository. - count_commits_between(root_ref_hash, branch.target) + count_commits_between(root_ref_hash, branch.target.sha) { behind: number_commits_behind, ahead: number_commits_ahead } end @@ -674,9 +679,7 @@ class Repository end def local_branches - @local_branches ||= rugged.branches.each(:local).map do |branch| - Gitlab::Git::Branch.new(branch.name, branch.target) - end + @local_branches ||= raw_repository.local_branches end alias_method :branches, :local_branches @@ -817,7 +820,7 @@ class Repository end def revert(user, commit, base_branch, revert_tree_id = nil) - source_sha = find_branch(base_branch).target + source_sha = find_branch(base_branch).target.sha revert_tree_id ||= check_revert_content(commit, base_branch) return false unless revert_tree_id @@ -834,7 +837,7 @@ class Repository end def cherry_pick(user, commit, base_branch, cherry_pick_tree_id = nil) - source_sha = find_branch(base_branch).target + source_sha = find_branch(base_branch).target.sha cherry_pick_tree_id ||= check_cherry_pick_content(commit, base_branch) return false unless cherry_pick_tree_id @@ -855,7 +858,7 @@ class Repository end def check_revert_content(commit, base_branch) - source_sha = find_branch(base_branch).target + source_sha = find_branch(base_branch).target.sha args = [commit.id, source_sha] args << { mainline: 1 } if commit.merge_commit? @@ -869,7 +872,7 @@ class Repository end def check_cherry_pick_content(commit, base_branch) - source_sha = find_branch(base_branch).target + source_sha = find_branch(base_branch).target.sha args = [commit.id, source_sha] args << 1 if commit.merge_commit? @@ -1034,7 +1037,7 @@ class Repository end def tags_sorted_by_committed_date - tags.sort_by { |tag| commit(tag.target).committed_date } + tags.sort_by { |tag| tag.target.committed_date } end def keep_around_ref_name(sha) diff --git a/app/services/delete_branch_service.rb b/app/services/delete_branch_service.rb index 332c55581a1..87f066edb6f 100644 --- a/app/services/delete_branch_service.rb +++ b/app/services/delete_branch_service.rb @@ -40,6 +40,6 @@ class DeleteBranchService < BaseService def build_push_data(branch) Gitlab::PushDataBuilder - .build(project, current_user, branch.target, Gitlab::Git::BLANK_SHA, "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}", []) + .build(project, current_user, branch.target.sha, Gitlab::Git::BLANK_SHA, "#{Gitlab::Git::BRANCH_REF_PREFIX}#{branch.name}", []) end end diff --git a/app/services/delete_tag_service.rb b/app/services/delete_tag_service.rb index 1e41fbe34b6..32e0eed6b63 100644 --- a/app/services/delete_tag_service.rb +++ b/app/services/delete_tag_service.rb @@ -34,6 +34,6 @@ class DeleteTagService < BaseService def build_push_data(tag) Gitlab::PushDataBuilder - .build(project, current_user, tag.target, Gitlab::Git::BLANK_SHA, "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}", []) + .build(project, current_user, tag.target.sha, Gitlab::Git::BLANK_SHA, "#{Gitlab::Git::TAG_REF_PREFIX}#{tag.name}", []) end end diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb index 58573078048..969530c4fdc 100644 --- a/app/services/git_tag_push_service.rb +++ b/app/services/git_tag_push_service.rb @@ -26,8 +26,8 @@ class GitTagPushService < BaseService unless Gitlab::Git.blank_ref?(params[:newrev]) tag_name = Gitlab::Git.ref_name(params[:ref]) tag = project.repository.find_tag(tag_name) - - if tag && tag.target == params[:newrev] + + if tag && tag.object_sha == params[:newrev] commit = project.commit(tag.target) commits = [commit].compact message = tag.message diff --git a/app/views/projects/branches/_commit.html.haml b/app/views/projects/branches/_commit.html.haml index 9fe65cbb104..d54c76ff9c8 100644 --- a/app/views/projects/branches/_commit.html.haml +++ b/app/views/projects/branches/_commit.html.haml @@ -1,5 +1,5 @@ .branch-commit - = link_to commit.short_id, namespace_project_commit_path(project.namespace, project, commit), class: "commit-id monospace" + = link_to commit.short_id, namespace_project_commit_path(project.namespace, project, commit.id), class: "commit-id monospace" · %span.str-truncated = link_to_gfm commit.title, namespace_project_commit_path(project.namespace, project, commit.id), class: "commit-row-message" diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 38b0c345b48..3e133143bbc 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -50,8 +50,9 @@ describe Repository, models: true do double_first = double(committed_date: Time.now) double_last = double(committed_date: Time.now - 1.second) - allow(repository).to receive(:commit).with(tag_a.target).and_return(double_first) - allow(repository).to receive(:commit).with(tag_b.target).and_return(double_last) + allow(tag_a).to receive(:target).and_return(double_first) + allow(tag_b).to receive(:target).and_return(double_last) + allow(repository).to receive(:tags).and_return([tag_a, tag_b]) end it { is_expected.to eq(['v1.0.0', 'v1.1.0']) } @@ -64,8 +65,9 @@ describe Repository, models: true do double_first = double(committed_date: Time.now - 1.second) double_last = double(committed_date: Time.now) - allow(repository).to receive(:commit).with(tag_a.target).and_return(double_last) - allow(repository).to receive(:commit).with(tag_b.target).and_return(double_first) + allow(tag_a).to receive(:target).and_return(double_last) + allow(tag_b).to receive(:target).and_return(double_first) + allow(repository).to receive(:tags).and_return([tag_a, tag_b]) end it { is_expected.to eq(['v1.1.0', 'v1.0.0']) } @@ -1161,17 +1163,6 @@ describe Repository, models: true do end end - describe '#local_branches' do - it 'returns the local branches' do - masterrev = repository.find_branch('master').target - create_remote_branch('joe', 'remote_branch', masterrev) - repository.add_branch(user, 'local_branch', masterrev) - - expect(repository.local_branches.any? { |branch| branch.name == 'remote_branch' }).to eq(false) - expect(repository.local_branches.any? { |branch| branch.name == 'local_branch' }).to eq(true) - end - end - describe "#keep_around" do it "stores a reference to the specified commit sha so it isn't garbage collected" do repository.keep_around(sample_commit.id) @@ -1179,9 +1170,4 @@ describe Repository, models: true do expect(repository.kept_around?(sample_commit.id)).to be_truthy end end - - def create_remote_branch(remote_name, branch_name, target) - rugged = repository.rugged - rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target) - end end -- cgit v1.2.1 From 72f59ddf4c9d276bd565892c0cf79d5622906090 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Mon, 11 Jul 2016 21:29:13 -0400 Subject: Use Pathname to make the repository storage path validations more robust --- config/initializers/6_validations.rb | 11 ++++++----- spec/initializers/6_validations_spec.rb | 26 +++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/config/initializers/6_validations.rb b/config/initializers/6_validations.rb index 3ba9e36c567..83acdc8ab54 100644 --- a/config/initializers/6_validations.rb +++ b/config/initializers/6_validations.rb @@ -3,22 +3,23 @@ def storage_name_valid?(name) end def find_parent_path(name, path) + parent = Pathname.new(path).realpath.parent Gitlab.config.repositories.storages.detect do |n, p| - name != n && path.chomp('/').start_with?(p.chomp('/')) + name != n && Pathname.new(p).realpath == parent end end -def error(message) +def storage_validation_error(message) raise "#{message}. Please fix this in your gitlab.yml before starting GitLab." end -error('No repository storage path defined') if Gitlab.config.repositories.storages.empty? +storage_validation_error('No repository storage path defined') if Gitlab.config.repositories.storages.empty? Gitlab.config.repositories.storages.each do |name, path| - error("\"#{name}\" is not a valid storage name") unless storage_name_valid?(name) + storage_validation_error("\"#{name}\" is not a valid storage name") unless storage_name_valid?(name) parent_name, _parent_path = find_parent_path(name, path) if parent_name - error("#{name} is a nested path of #{parent_name}. Nested paths are not supported for repository storages") + storage_validation_error("#{name} is a nested path of #{parent_name}. Nested paths are not supported for repository storages") end end diff --git a/spec/initializers/6_validations_spec.rb b/spec/initializers/6_validations_spec.rb index 5178bd130f4..05d0aa63488 100644 --- a/spec/initializers/6_validations_spec.rb +++ b/spec/initializers/6_validations_spec.rb @@ -1,9 +1,19 @@ require 'spec_helper' describe '6_validations', lib: true 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 + context 'with correct settings' do before do - mock_storages('foo' => '/a/b/c', 'bar' => 'a/b/d') + mock_storages('foo' => 'tmp/tests/paths/a/b/c', 'bar' => 'tmp/tests/paths/a/b/d') end it 'passes through' do @@ -13,7 +23,7 @@ describe '6_validations', lib: true do context 'with invalid storage names' do before do - mock_storages('name with spaces' => '/a/b/c') + mock_storages('name with spaces' => 'tmp/tests/paths/a/b/c') end it 'throws an error' do @@ -23,7 +33,7 @@ describe '6_validations', lib: true do context 'with nested storage paths' do before do - mock_storages('foo' => '/a/b/c', 'bar' => '/a/b/c/d') + mock_storages('foo' => 'tmp/tests/paths/a/b/c', 'bar' => 'tmp/tests/paths/a/b/c/d') end it 'throws an error' do @@ -31,6 +41,16 @@ describe '6_validations', lib: true do end end + context 'with similar but un-nested storage paths' do + before do + mock_storages('foo' => 'tmp/tests/paths/a/b/c', 'bar' => 'tmp/tests/paths/a/b/c2') + end + + it 'passes through' do + expect { load_validations }.not_to raise_error + end + end + def mock_storages(storages) allow(Gitlab.config.repositories).to receive(:storages).and_return(storages) end -- cgit v1.2.1 From 89589007aee6780c3dea2d44df12e9509a22f975 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Mon, 18 Jul 2016 17:28:26 -0400 Subject: Skip repository storage path valitaions on test environment Storage path are not created until `TestEnv.init`, so we must skip their validation on initialization. --- config/initializers/6_validations.rb | 16 ++++++++++------ spec/initializers/6_validations_spec.rb | 13 +++++-------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/config/initializers/6_validations.rb b/config/initializers/6_validations.rb index 83acdc8ab54..37746968675 100644 --- a/config/initializers/6_validations.rb +++ b/config/initializers/6_validations.rb @@ -13,13 +13,17 @@ def storage_validation_error(message) raise "#{message}. Please fix this in your gitlab.yml before starting GitLab." end -storage_validation_error('No repository storage path defined') if Gitlab.config.repositories.storages.empty? +def validate_storages + storage_validation_error('No repository storage path defined') if Gitlab.config.repositories.storages.empty? -Gitlab.config.repositories.storages.each do |name, path| - storage_validation_error("\"#{name}\" is not a valid storage name") unless storage_name_valid?(name) + Gitlab.config.repositories.storages.each do |name, path| + storage_validation_error("\"#{name}\" is not a valid storage name") unless storage_name_valid?(name) - parent_name, _parent_path = find_parent_path(name, path) - if parent_name - storage_validation_error("#{name} is a nested path of #{parent_name}. Nested paths are not supported for repository storages") + parent_name, _parent_path = find_parent_path(name, path) + if parent_name + storage_validation_error("#{name} is a nested path of #{parent_name}. Nested paths are not supported for repository storages") + end end end + +validate_storages unless Rails.env.test? diff --git a/spec/initializers/6_validations_spec.rb b/spec/initializers/6_validations_spec.rb index 05d0aa63488..baab30f482f 100644 --- a/spec/initializers/6_validations_spec.rb +++ b/spec/initializers/6_validations_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require_relative '../../config/initializers/6_validations.rb' describe '6_validations', lib: true do before :all do @@ -17,7 +18,7 @@ describe '6_validations', lib: true do end it 'passes through' do - expect { load_validations }.not_to raise_error + expect { validate_storages }.not_to raise_error end end @@ -27,7 +28,7 @@ describe '6_validations', lib: true do end it 'throws an error' do - expect { load_validations }.to raise_error('"name with spaces" is not a valid storage name. Please fix this in your gitlab.yml before starting GitLab.') + expect { validate_storages }.to raise_error('"name with spaces" is not a valid storage name. Please fix this in your gitlab.yml before starting GitLab.') end end @@ -37,7 +38,7 @@ describe '6_validations', lib: true do end it 'throws an error' do - expect { load_validations }.to raise_error('bar is a nested path of foo. Nested paths are not supported for repository storages. Please fix this in your gitlab.yml before starting GitLab.') + expect { validate_storages }.to raise_error('bar is a nested path of foo. Nested paths are not supported for repository storages. Please fix this in your gitlab.yml before starting GitLab.') end end @@ -47,15 +48,11 @@ describe '6_validations', lib: true do end it 'passes through' do - expect { load_validations }.not_to raise_error + expect { validate_storages }.not_to raise_error end end def mock_storages(storages) allow(Gitlab.config.repositories).to receive(:storages).and_return(storages) end - - def load_validations - load File.join(__dir__, '../../config/initializers/6_validations.rb') - end end -- cgit v1.2.1 From c3cbee10bd438a2d8ded30f1a11865693a3d3c6a Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 22 Jul 2016 08:25:59 +0530 Subject: Add a spec for #20079. The issue was fixed in 2ba5e62. The spec is going in separately just so the fix could go in as soon as possible. --- features/project/wiki.feature | 6 ++++++ features/steps/project/wiki.rb | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/features/project/wiki.feature b/features/project/wiki.feature index d4811b1ff54..63ce3ccb536 100644 --- a/features/project/wiki.feature +++ b/features/project/wiki.feature @@ -8,6 +8,12 @@ Feature: Project Wiki Given I create the Wiki Home page Then I should see the newly created wiki page + Scenario: Add new page with errors + Given I create the Wiki Home page with no content + Then I should see a "Content can't be blank" error message + When I create the Wiki Home page + Then I should see the newly created wiki page + Scenario: Pressing Cancel while editing a brand new Wiki Given I click on the Cancel button Then I should be redirected back to the Edit Home Wiki page diff --git a/features/steps/project/wiki.rb b/features/steps/project/wiki.rb index 3cbf832c728..5ad82a9b3c1 100644 --- a/features/steps/project/wiki.rb +++ b/features/steps/project/wiki.rb @@ -19,6 +19,11 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps click_on "Create page" end + step 'I create the Wiki Home page with no content' do + fill_in "wiki_content", with: '' + click_on "Create page" + end + step 'I should see the newly created wiki page' do expect(page).to have_content "Home" expect(page).to have_content "link test" @@ -173,6 +178,11 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps find('a[href*="?version_id"]') end + step 'I should see a "Content can\'t be blank" error message' do + expect(page).to have_content('The form contains the following error:') + expect(page).to have_content('Content can\'t be blank') + end + def wiki @project_wiki = ProjectWiki.new(project, current_user) end -- cgit v1.2.1 From 1a5b7e7d70cfe4802e858f129779e388307fbbab Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Fri, 22 Jul 2016 10:56:25 +0300 Subject: Refactor protected branches documentation --- .../projects/protected_branches/index.html.haml | 13 +-- doc/user/project/img/project_settings_list.png | Bin 0 -> 10788 bytes .../img/protected_branches_choose_branch.png | Bin 0 -> 20659 bytes .../img/protected_branches_devs_can_push.png | Bin 0 -> 23976 bytes .../project/img/protected_branches_error_ui.png | Bin 0 -> 37750 bytes doc/user/project/img/protected_branches_list.png | Bin 0 -> 16817 bytes .../project/img/protected_branches_matches.png | Bin 0 -> 32145 bytes doc/user/project/protected_branches.md | 106 +++++++++++++++++++++ doc/workflow/README.md | 2 +- doc/workflow/protected_branches.md | 56 +---------- .../protected_branches/protected_branches1.png | Bin 195061 -> 0 bytes .../protected_branches/protected_branches2.png | Bin 41179 -> 0 bytes .../protected_branches/protected_branches3.png | Bin 110160 -> 0 bytes 13 files changed, 115 insertions(+), 62 deletions(-) create mode 100644 doc/user/project/img/project_settings_list.png create mode 100644 doc/user/project/img/protected_branches_choose_branch.png create mode 100644 doc/user/project/img/protected_branches_devs_can_push.png create mode 100644 doc/user/project/img/protected_branches_error_ui.png create mode 100644 doc/user/project/img/protected_branches_list.png create mode 100644 doc/user/project/img/protected_branches_matches.png create mode 100644 doc/user/project/protected_branches.md delete mode 100644 doc/workflow/protected_branches/protected_branches1.png delete mode 100644 doc/workflow/protected_branches/protected_branches2.png delete mode 100644 doc/workflow/protected_branches/protected_branches3.png diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml index 151e1d64851..950df740bbc 100644 --- a/app/views/projects/protected_branches/index.html.haml +++ b/app/views/projects/protected_branches/index.html.haml @@ -6,12 +6,13 @@ = page_title %p Keep stable branches secure and force developers to use merge requests. %p.prepend-top-20 - Protected branches are designed to: + By default, protected branches are designed to: %ul - %li prevent pushes from everybody except #{link_to "masters", help_page_path("user/permissions"), class: "vlink"} - %li prevent anyone from force pushing to the branch - %li prevent anyone from deleting the branch - %p.append-bottom-0 Read more about #{link_to "project permissions", help_page_path("user/permissions"), class: "underlined-link"} + %li prevent their creation, if not already created, from everybody except Masters + %li prevent pushes from everybody except Masters + %li prevent anyone from force pushing to the branch + %li prevent anyone from deleting the branch + %p.append-bottom-0 Read more about #{link_to "protected branches", help_page_path("user/project/protected_branches"), class: "underlined-link"} and #{link_to "project permissions", help_page_path("user/permissions"), class: "underlined-link"}. .col-lg-9 %h5.prepend-top-0 Protect a branch @@ -23,7 +24,7 @@ = f.label :name, "Branch", class: "label-light" = render partial: "dropdown", locals: { f: f } %p.help-block - = link_to "Wildcards", help_page_path('workflow/protected_branches', anchor: "wildcard-protected-branches") + = link_to "Wildcards", help_page_path('user/project/protected_branches', anchor: "wildcard-protected-branches") such as %code *-stable or diff --git a/doc/user/project/img/project_settings_list.png b/doc/user/project/img/project_settings_list.png new file mode 100644 index 00000000000..57ca2ac5f9e Binary files /dev/null and b/doc/user/project/img/project_settings_list.png differ diff --git a/doc/user/project/img/protected_branches_choose_branch.png b/doc/user/project/img/protected_branches_choose_branch.png new file mode 100644 index 00000000000..26328143717 Binary files /dev/null and b/doc/user/project/img/protected_branches_choose_branch.png differ diff --git a/doc/user/project/img/protected_branches_devs_can_push.png b/doc/user/project/img/protected_branches_devs_can_push.png new file mode 100644 index 00000000000..9c33db36586 Binary files /dev/null and b/doc/user/project/img/protected_branches_devs_can_push.png differ diff --git a/doc/user/project/img/protected_branches_error_ui.png b/doc/user/project/img/protected_branches_error_ui.png new file mode 100644 index 00000000000..cc61df7ca97 Binary files /dev/null and b/doc/user/project/img/protected_branches_error_ui.png differ diff --git a/doc/user/project/img/protected_branches_list.png b/doc/user/project/img/protected_branches_list.png new file mode 100644 index 00000000000..9f070f7a208 Binary files /dev/null and b/doc/user/project/img/protected_branches_list.png differ diff --git a/doc/user/project/img/protected_branches_matches.png b/doc/user/project/img/protected_branches_matches.png new file mode 100644 index 00000000000..30ce53f704e Binary files /dev/null and b/doc/user/project/img/protected_branches_matches.png differ diff --git a/doc/user/project/protected_branches.md b/doc/user/project/protected_branches.md new file mode 100644 index 00000000000..6a8170b5ecb --- /dev/null +++ b/doc/user/project/protected_branches.md @@ -0,0 +1,106 @@ +# Protected Branches + +[Permissions](../permissions.md) in GitLab are fundamentally defined around the +idea of having read or write permission to the repository and branches. To +prevent people from messing with history or pushing code without review, we've +created protected branches. + +By default, a protected branch does four simple things: + +- it prevents its creation, if not already created, from everybody except users + with Master permission +- it prevents pushes from everybody except users with Master permission +- it prevents **anyone** from force pushing to the branch +- it prevents **anyone** from deleting the branch + +See the [Changelog](#changelog) section for changes over time. + +## Configuring protected branches + +To protect a branch, you need to have at least Master permission level. Note +that the `master` branch is protected by default. + +1. Navigate to the main page of the project. +1. In the upper right corner, click the settings wheel and select **Protected branches**. + + ![Project settings list](img/project_settings_list.png) + +1. From the **Branch** dropdown menu, select the branch you want to protect and + click **Protect**. In the screenshot below, we chose the `develop` branch. + + ![Choose protected branch](img/protected_branches_choose_branch.png) + +1. Once done, the protected branch will appear in the "Already protected" list. + + ![Protected branches list](img/protected_branches_list.png) + + +Since GitLab 8.10, we added another layer of branch protection which provides +more granular management of protected branches. You can now choose the option +"Developers can merge" so that Developer users can merge a merge request but +not directly push. In that case, your branches are protected from direct pushes, +yet Developers don't need elevated permissions or wait for someone with a higher +permission level to press merge. + +You can set this option while creating the protected branch or after its +creation. + +## Wildcard protected branches + +>**Note:** +This feature was [introduced][ce-4665] in GitLab 8.10. + +You can specify a wildcard protected branch, which will protect all branches +matching the wildcard. For example: + +| Wildcard Protected Branch | Matching Branches | +|---------------------------+--------------------------------------------------------| +| `*-stable` | `production-stable`, `staging-stable` | +| `production/*` | `production/app-server`, `production/load-balancer` | +| `*gitlab*` | `gitlab`, `gitlab/staging`, `master/gitlab/production` | + +Protected branch settings (like "Developers can push") apply to all matching +branches. + +Two different wildcards can potentially match the same branch. For example, +`*-stable` and `production-*` would both match a `production-stable` branch. +In that case, if _any_ of these protected branches have a setting like +"Allowed to push", then `production-stable` will also inherit this setting. + +If you click on a protected branch's name that is created using a wildcard, +you will be presented with a list of all matching branches: + +![Protected branch matches](img/protected_branches_matches.png) + +## Restrict the creation of protected branches + +Creating a protected branch or a list of protected branches using the wildcard +feature, not only you are restricting pushes to those branches, but also their +creation if not already created. + +## Error messages when pushing to a protected branch + +A user with insufficient permissions will be presented with an error when +creating or pushing to a branch that's prohibited, either through GitLab's UI: + +![Protected branch error GitLab UI](img/protected_branches_error_ui.png) + +or using Git from their terminal: + +```bash +remote: GitLab: You are not allowed to push code to protected branches on this project. +To https://gitlab.example.com/thedude/bowling.git + ! [remote rejected] staging-stable -> staging-stable (pre-receive hook declined) +error: failed to push some refs to 'https://gitlab.example.com/thedude/bowling.git' +``` + +## Changelog + +**8.10.0** + +- Allow specifying protected branches using wildcards [gitlab-org/gitlab-ce!5081][ce-4665] + +--- + +[ce-4665]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4665 "Allow specifying protected branches using wildcards" +[ce-5081]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5081 "Allow creating protected branches that can't be pushed to" diff --git a/doc/workflow/README.md b/doc/workflow/README.md index ddb2f7281b1..49dec613716 100644 --- a/doc/workflow/README.md +++ b/doc/workflow/README.md @@ -12,7 +12,7 @@ - [Project Features](project_features.md) - [Project forking workflow](forking_workflow.md) - [Project users](add-user/add-user.md) -- [Protected branches](protected_branches.md) +- [Protected branches](../user/project/protected_branches.md) - [Sharing a project with a group](share_with_group.md) - [Share projects with other groups](share_projects_with_other_groups.md) - [Web Editor](web_editor.md) diff --git a/doc/workflow/protected_branches.md b/doc/workflow/protected_branches.md index 5c1c7b47c8a..aa48b8f750e 100644 --- a/doc/workflow/protected_branches.md +++ b/doc/workflow/protected_branches.md @@ -1,55 +1 @@ -# Protected Branches - -Permissions in GitLab are fundamentally defined around the idea of having read or write permission to the repository and branches. - -To prevent people from messing with history or pushing code without review, we've created protected branches. - -A protected branch does three simple things: - -* it prevents pushes from everybody except users with Master permission -* it prevents anyone from force pushing to the branch -* it prevents anyone from deleting the branch - -You can make any branch a protected branch. GitLab makes the master branch a protected branch by default. - -To protect a branch, user needs to have at least a Master permission level, see [permissions document](../user/permissions.md). - -![protected branches page](protected_branches/protected_branches1.png) - -Navigate to project settings page and select `protected branches`. From the `Branch` dropdown menu select the branch you want to protect. - -Some workflows, like [GitLab workflow](gitlab_flow.md), require all users with write access to submit a Merge request in order to get the code into a protected branch. - -Since Masters and Owners can already push to protected branches, that means Developers cannot push to protected branch and need to submit a Merge request. - -However, there are workflows where that is not needed and only protecting from force pushes and branch removal is useful. - -For those workflows, you can allow everyone with write access to push to a protected branch by selecting `Developers can push` check box. - -On already protected branches you can also allow developers to push to the repository by selecting the `Developers can push` check box. - -![Developers can push](protected_branches/protected_branches2.png) - -## Wildcard Protected Branches - ->**Note:** -This feature was added in GitLab 8.10. - -1. You can specify a wildcard protected branch, which will protect all branches matching the wildcard. For example: - - | Wildcard Protected Branch | Matching Branches | - |---------------------------+--------------------------------------------------------| - | `*-stable` | `production-stable`, `staging-stable` | - | `production/*` | `production/app-server`, `production/load-balancer` | - | `*gitlab*` | `gitlab`, `gitlab/staging`, `master/gitlab/production` | - -1. Protected branch settings (like "Developers Can Push") apply to all matching branches. - -1. Two different wildcards can potentially match the same branch. For example, `*-stable` and `production-*` would both match a `production-stable` branch. - >**Note:** - If _any_ of these protected branches have "Developers Can Push" set to true, then `production-stable` has it set to true. - -1. If you click on a protected branch's name, you will be presented with a list of all matching branches: - - ![protected branch matches](protected_branches/protected_branches3.png) - +This document is moved to [user/project/protected_branches.md](../user/project/protected_branches.md) diff --git a/doc/workflow/protected_branches/protected_branches1.png b/doc/workflow/protected_branches/protected_branches1.png deleted file mode 100644 index c00443803de..00000000000 Binary files a/doc/workflow/protected_branches/protected_branches1.png and /dev/null differ diff --git a/doc/workflow/protected_branches/protected_branches2.png b/doc/workflow/protected_branches/protected_branches2.png deleted file mode 100644 index a4f664d3b21..00000000000 Binary files a/doc/workflow/protected_branches/protected_branches2.png and /dev/null differ diff --git a/doc/workflow/protected_branches/protected_branches3.png b/doc/workflow/protected_branches/protected_branches3.png deleted file mode 100644 index 2a50cb174bb..00000000000 Binary files a/doc/workflow/protected_branches/protected_branches3.png and /dev/null differ -- cgit v1.2.1 From b48fd097ff22965bbb8d89780bc2a35385ffdfb5 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Fri, 22 Jul 2016 11:41:17 +0300 Subject: Fix failing spec on help controller --- spec/controllers/help_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/help_controller_spec.rb b/spec/controllers/help_controller_spec.rb index 347bef1e129..33c75e7584f 100644 --- a/spec/controllers/help_controller_spec.rb +++ b/spec/controllers/help_controller_spec.rb @@ -36,7 +36,7 @@ describe HelpController do context 'when requested file exists' do it 'renders the raw file' do get :show, - path: 'workflow/protected_branches/protected_branches1', + path: 'user/project/img/labels_filter', format: :png expect(response).to be_success expect(response.content_type).to eq 'image/png' -- cgit v1.2.1 From 7a3b20ed36617aab2b56f12391903882c05f3ec0 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Fri, 22 Jul 2016 12:18:05 +0300 Subject: Move admin application settings to own path [ci skip] --- doc/README.md | 2 +- doc/administration/access_restrictions.md | 56 --------------------- doc/administration/img/access_restrictions.png | Bin 317529 -> 0 bytes doc/administration/img/domain_blacklist.png | Bin 178444 -> 0 bytes doc/administration/img/restricted_url.png | Bin 188210 -> 0 bytes .../settings/img/access_restrictions.png | Bin 0 -> 7435 bytes .../admin_area/settings/img/domain_blacklist.png | Bin 0 -> 34684 bytes .../admin_area/settings/img/restricted_url.png | Bin 0 -> 47539 bytes .../admin_area/settings/sign_up_restrictions.md | 22 ++++++++ .../settings/visibility_and_access_controls.md | 40 +++++++++++++++ 10 files changed, 63 insertions(+), 57 deletions(-) delete mode 100644 doc/administration/access_restrictions.md delete mode 100644 doc/administration/img/access_restrictions.png delete mode 100644 doc/administration/img/domain_blacklist.png delete mode 100644 doc/administration/img/restricted_url.png create mode 100644 doc/user/admin_area/settings/img/access_restrictions.png create mode 100644 doc/user/admin_area/settings/img/domain_blacklist.png create mode 100644 doc/user/admin_area/settings/img/restricted_url.png create mode 100644 doc/user/admin_area/settings/sign_up_restrictions.md create mode 100644 doc/user/admin_area/settings/visibility_and_access_controls.md diff --git a/doc/README.md b/doc/README.md index cc0b6e0c1e5..ad5267352b0 100644 --- a/doc/README.md +++ b/doc/README.md @@ -21,7 +21,7 @@ ## Administrator documentation -- [Access restrictions](administration/access_restrictions.md) Define which Git access protocols can be used to talk to GitLab +- [Access restrictions](user/admin_area/settings/visibility_and_access_controls.md#enabled-git-access-protocols) Define which Git access protocols can be used to talk to GitLab - [Authentication/Authorization](administration/auth/README.md) Configure external authentication with LDAP, SAML, CAS and additional Omniauth providers. - [Custom Git hooks](administration/custom_hooks.md) Custom Git hooks (on the filesystem) for when webhooks aren't enough. diff --git a/doc/administration/access_restrictions.md b/doc/administration/access_restrictions.md deleted file mode 100644 index eb08cf139d4..00000000000 --- a/doc/administration/access_restrictions.md +++ /dev/null @@ -1,56 +0,0 @@ -# Access Restrictions - -> **Note:** These features are only available on versions 8.10 and above. - -With GitLab's Access restrictions you can choose which Git access protocols you -want your users to use to communicate with GitLab. This feature can be enabled -via the `Application Settings` in the Admin interface. - -The setting is called `Enabled Git access protocols`, and it gives you the option -to choose between: - -- Both SSH and HTTP(S) -- Only SSH -- Only HTTP(s) - -![Settings Overview](img/access_restrictions.png) - -## Enabled Protocol - -When both SSH and HTTP(S) are enabled, GitLab will behave as usual, it will give -your users the option to choose which protocol they would like to use. - -When you choose to allow only one of the protocols, a couple of things will happen: - -- The project page will only show the allowed protocol's URL, with no option to - change it. -- A tooltip will be shown when you hover over the URL's protocol, if an action - on the user's part is required, e.g. adding an SSH key, or setting a password. - -![Project URL with SSH only access](img/restricted_url.png) - -On top of these UI restrictions, GitLab will deny all Git actions on the protocol -not selected. - -> **Note:** Please keep in mind that disabling an access protocol does not actually - block access to the server itself. The ports used for the protocol, be it SSH or - HTTP, will still be accessible. What GitLab does is restrict access on the - application level. - -## Blacklist email domains - -With this feature enabled, you can block email addresses of a specific domain -from creating an account on your GitLab server. This is particularly useful to -prevent spam. Disposable email addresses are usually used by malicious users to -create dummy accounts and spam issues. - -This feature can be activated via the `Application Settings` in the Admin area, -and you have the option of entering the list manually, or uploading a file with -the list. - -The blacklist accepts wildcards, so you can use `*.test.com` to block every -`test.com` subdomain, or `*.io` to block all domains ending in `.io`. Domains -should be separated by a whitespace, semicolon, comma, or a new line. - -![Domain Blacklist](img/domain_blacklist.png) - diff --git a/doc/administration/img/access_restrictions.png b/doc/administration/img/access_restrictions.png deleted file mode 100644 index 66fd9491e85..00000000000 Binary files a/doc/administration/img/access_restrictions.png and /dev/null differ diff --git a/doc/administration/img/domain_blacklist.png b/doc/administration/img/domain_blacklist.png deleted file mode 100644 index a7894e5f08d..00000000000 Binary files a/doc/administration/img/domain_blacklist.png and /dev/null differ diff --git a/doc/administration/img/restricted_url.png b/doc/administration/img/restricted_url.png deleted file mode 100644 index 0a677433dcf..00000000000 Binary files a/doc/administration/img/restricted_url.png and /dev/null differ diff --git a/doc/user/admin_area/settings/img/access_restrictions.png b/doc/user/admin_area/settings/img/access_restrictions.png new file mode 100644 index 00000000000..8eea84320d7 Binary files /dev/null and b/doc/user/admin_area/settings/img/access_restrictions.png differ diff --git a/doc/user/admin_area/settings/img/domain_blacklist.png b/doc/user/admin_area/settings/img/domain_blacklist.png new file mode 100644 index 00000000000..bd87b73cf9e Binary files /dev/null and b/doc/user/admin_area/settings/img/domain_blacklist.png differ diff --git a/doc/user/admin_area/settings/img/restricted_url.png b/doc/user/admin_area/settings/img/restricted_url.png new file mode 100644 index 00000000000..8b00a18320b Binary files /dev/null and b/doc/user/admin_area/settings/img/restricted_url.png differ diff --git a/doc/user/admin_area/settings/sign_up_restrictions.md b/doc/user/admin_area/settings/sign_up_restrictions.md new file mode 100644 index 00000000000..4b540473a6e --- /dev/null +++ b/doc/user/admin_area/settings/sign_up_restrictions.md @@ -0,0 +1,22 @@ +# Sign-up restrictions + +## Blacklist email domains + +> [Introduced][ce-5259] in GitLab 8.10. + +With this feature enabled, you can block email addresses of a specific domain +from creating an account on your GitLab server. This is particularly useful to +prevent spam. Disposable email addresses are usually used by malicious users to +create dummy accounts and spam issues. + +This feature can be activated via the **Application Settings** in the Admin area, +and you have the option of entering the list manually, or uploading a file with +the list. + +The blacklist accepts wildcards, so you can use `*.test.com` to block every +`test.com` subdomain, or `*.io` to block all domains ending in `.io`. Domains +should be separated by a whitespace, semicolon, comma, or a new line. + +![Domain Blacklist](img/domain_blacklist.png) + +[ce-5259]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5259 diff --git a/doc/user/admin_area/settings/visibility_and_access_controls.md b/doc/user/admin_area/settings/visibility_and_access_controls.md new file mode 100644 index 00000000000..633f16a617c --- /dev/null +++ b/doc/user/admin_area/settings/visibility_and_access_controls.md @@ -0,0 +1,40 @@ +# Visibility and access controls + +## Enabled Git access protocols + +> [Introduced][ce-4696] in GitLab 8.10. + +With GitLab's Access restrictions you can choose which Git access protocols you +want your users to use to communicate with GitLab. This feature can be enabled +via the `Application Settings` in the Admin interface. + +The setting is called `Enabled Git access protocols`, and it gives you the option +to choose between: + +- Both SSH and HTTP(S) +- Only SSH +- Only HTTP(s) + +![Settings Overview](img/access_restrictions.png) + +When both SSH and HTTP(S) are enabled, GitLab will behave as usual, it will give +your users the option to choose which protocol they would like to use. + +When you choose to allow only one of the protocols, a couple of things will happen: + +- The project page will only show the allowed protocol's URL, with no option to + change it. +- A tooltip will be shown when you hover over the URL's protocol, if an action + on the user's part is required, e.g. adding an SSH key, or setting a password. + +![Project URL with SSH only access](img/restricted_url.png) + +On top of these UI restrictions, GitLab will deny all Git actions on the protocol +not selected. + +> **Note:** Please keep in mind that disabling an access protocol does not actually + block access to the server itself. The ports used for the protocol, be it SSH or + HTTP, will still be accessible. What GitLab does is restrict access on the + application level. + +[ce-4696]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4696 -- cgit v1.2.1 From a93323cd9449ec59464727c7758401d25c4801d4 Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Tue, 19 Jul 2016 19:33:02 +0300 Subject: Refactor Slack service documentation [ci skip] --- doc/integration/README.md | 1 - doc/integration/slack.md | 41 +----------------- doc/project_services/img/slack_configuration.png | Bin 72072 -> 75762 bytes doc/project_services/slack.md | 52 +++++++++++++++++------ 4 files changed, 39 insertions(+), 55 deletions(-) diff --git a/doc/integration/README.md b/doc/integration/README.md index fd330dd7a7d..ddbd570ac6c 100644 --- a/doc/integration/README.md +++ b/doc/integration/README.md @@ -11,7 +11,6 @@ See the documentation below for details on how to configure these services. - [OmniAuth](omniauth.md) Sign in via Twitter, GitHub, GitLab.com, Google, Bitbucket, Facebook, Shibboleth, SAML, Crowd and Azure - [SAML](saml.md) Configure GitLab as a SAML 2.0 Service Provider - [CAS](cas.md) Configure GitLab to sign in using CAS -- [Slack](slack.md) Integrate with the Slack chat service - [OAuth2 provider](oauth_provider.md) OAuth2 application creation - [Gmail actions buttons](gmail_action_buttons_for_gitlab.md) Adds GitLab actions to messages - [reCAPTCHA](recaptcha.md) Configure GitLab to use Google reCAPTCHA for new users diff --git a/doc/integration/slack.md b/doc/integration/slack.md index 11f956fed3e..8cd151fbf95 100644 --- a/doc/integration/slack.md +++ b/doc/integration/slack.md @@ -1,40 +1 @@ -# Slack integration - -## On Slack - -To enable Slack integration you must create an Incoming WebHooks integration on Slack: - -1. [Sign in to Slack](https://slack.com/signin) - -1. Visit [Incoming WebHooks](https://my.slack.com/services/new/incoming-webhook/) - -1. Choose the channel name you want to send notifications to. - -1. Click **Add Incoming WebHooks Integration** - - Optional step; You can change bot's name and avatar by clicking modifying the bot name or avatar under **Integration Settings**. - -1. Copy the **Webhook URL**, we'll need this later for GitLab. - - -## On GitLab - -After Slack is ready we need to setup GitLab. Here are the steps to achieve this. - -1. Sign in to GitLab - -1. Pick the repository you want. - -1. Navigate to Settings -> Services -> Slack - -1. Pick the triggers you want to activate and respective channel (`#general` by default). - -1. Fill in your Slack details - - Webhook: Paste the Webhook URL from the step above - - Username: Fill this in if you want to change the username of the bot - - Mark it as active - -1. Save your settings - -Have fun :) - -*P.S. You can set "branch,pushed,Compare changes" as highlight words on your Slack profile settings, so that you can be aware of new commits when somebody pushes them.* +This document was moved to [project_services/slack.md](../project_services/slack.md). diff --git a/doc/project_services/img/slack_configuration.png b/doc/project_services/img/slack_configuration.png index d3ebe5969af..b8de8a56db7 100644 Binary files a/doc/project_services/img/slack_configuration.png and b/doc/project_services/img/slack_configuration.png differ diff --git a/doc/project_services/slack.md b/doc/project_services/slack.md index 4ed33838f7c..3cfe77c9f85 100644 --- a/doc/project_services/slack.md +++ b/doc/project_services/slack.md @@ -1,26 +1,50 @@ # Slack Service -Go to your project's **Settings > Services > Slack** and you will see a checkbox with the following events that can be triggered: +## On Slack -* Push -* Issue -* Merge request -* Note -* Tag push -* Build -* Wiki page +To enable Slack integration you must create an incoming webhook integration on +Slack: -Below each of these event checkboxes you will have an input to insert which Slack channel do you want to send that event message, -`#general` channel is default. +1. [Sign in to Slack](https://slack.com/signin) +1. Visit [Incoming WebHooks](https://my.slack.com/services/new/incoming-webhook/) +1. Choose the channel name you want to send notifications to. +1. Click **Add Incoming WebHooks Integration** +1. Copy the **Webhook URL**, we'll need this later for GitLab. +## On GitLab -![Slack configuration](img/slack_configuration.png) +After you set up Slack, it's time to set up GitLab. + +Go to your project's **Settings > Services > Slack** and you will see a +checkbox with the following events that can be triggered: + +- Push +- Issue +- Merge request +- Note +- Tag push +- Build +- Wiki page + +Bellow each of these event checkboxes, you will have an input field to insert +which Slack channel you want to send that event message, with `#general` +being the default. Enter your preferred channel **without** the hash sign (`#`). +At the end, fill in your Slack details: | Field | Description | | ----- | ----------- | -| `Webhook` | The incoming webhook url which you have to setup on slack. (https://my.slack.com/services/new/incoming-webhook/) | -| `Username` | Optional username which can be on messages sent to slack. | -| `notify only broken builds` | Notify only about broken builds, when build events are marked to be sent.| +| **Webhook** | The [incoming webhook URL][slackhook] which you have to setup on Slack. | +| **Username** | Optional username which can be on messages sent to slack. Fill this in if you want to change the username of the bot. | +| **Notify only broken builds** | If you choose to enable the **Build** event and you want to be only notified about failed builds. | +After you are all done, click **Save changes** for the changes to take effect. + +>**Note:** +You can set "branch,pushed,Compare changes" as highlight words on your Slack +profile settings, so that you can be aware of new commits when somebody pushes +them. + +![Slack configuration](img/slack_configuration.png) +[slackhook]: https://my.slack.com/services/new/incoming-webhook -- cgit v1.2.1 From 12dd0f62012c6df8bd67abc2d9c5c54bd82366f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 22 Jul 2016 13:18:34 +0200 Subject: Update VERSION to 8.11.0-pre MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- CHANGELOG | 2 +- VERSION | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index bc09a51cc1e..e2104338f5c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -5,7 +5,7 @@ v 8.11.0 (unreleased) - Limit git rev-list output count to one in forced push check - Retrieve rendered HTML from cache in one request -v 8.10.0 (unreleased) +v 8.10.0 - Fix profile activity heatmap to show correct day name (eanplatter) - Speed up ExternalWikiHelper#get_project_wiki_path - Expose {should,force}_remove_source_branch (Ben Boeckel) diff --git a/VERSION b/VERSION index 213504430f3..542e7824102 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -8.10.0-pre +8.11.0-pre -- cgit v1.2.1 From de3f8ad397e8505ca56e1a81235ce36f6f145c51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 22 Jul 2016 13:12:30 +0200 Subject: Make Notify specs more robust by setting up assignee names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- spec/mailers/notify_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 0a9b10bebea..3685b2b17b5 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -12,7 +12,7 @@ describe Notify do context 'for a project' do describe 'items that are assignable, the email' do let(:current_user) { create(:user, email: "current@email.com") } - let(:assignee) { create(:user, email: 'assignee@example.com') } + let(:assignee) { create(:user, email: 'assignee@example.com', name: 'John Doe') } let(:previous_assignee) { create(:user, name: 'Previous Assignee') } shared_examples 'an assignee email' do -- cgit v1.2.1 From 679bb671be0225d6e6b65a1cd7a47cf2814e849c Mon Sep 17 00:00:00 2001 From: Achilleas Pipinellis Date: Fri, 22 Jul 2016 14:57:45 +0300 Subject: Refactor repository storages documentation [ci skip] --- doc/README.md | 1 + .../img/repository_storages_admin_ui.png | Bin 0 -> 17081 bytes doc/administration/repository_storages.md | 82 ++++++++++++++++++--- 3 files changed, 73 insertions(+), 10 deletions(-) create mode 100644 doc/administration/img/repository_storages_admin_ui.png diff --git a/doc/README.md b/doc/README.md index ad5267352b0..b5b377822e6 100644 --- a/doc/README.md +++ b/doc/README.md @@ -50,6 +50,7 @@ - [Sidekiq Troubleshooting](administration/troubleshooting/sidekiq.md) Debug when Sidekiq appears hung and is not processing jobs. - [High Availability](administration/high_availability/README.md) Configure multiple servers for scaling or high availability. - [Container Registry](administration/container_registry.md) Configure Docker Registry with GitLab. +- [Multiple mountpoints for the repositories storage](administration/repository_storages.md) Define multiple repository storage paths to distribute the storage load. ## Contributor documentation diff --git a/doc/administration/img/repository_storages_admin_ui.png b/doc/administration/img/repository_storages_admin_ui.png new file mode 100644 index 00000000000..599350bc098 Binary files /dev/null and b/doc/administration/img/repository_storages_admin_ui.png differ diff --git a/doc/administration/repository_storages.md b/doc/administration/repository_storages.md index fd2b4f00960..a9e22e2bdaa 100644 --- a/doc/administration/repository_storages.md +++ b/doc/administration/repository_storages.md @@ -1,19 +1,81 @@ # Repository storages -GitLab allows you to define repository storage paths to enable distribution of -storage load between several mount points. You can choose where new projects are -stored via the `Application Settings` in the Admin interface. +> [Introduced][ce-4578] in GitLab 8.10. -## For installations from source - -Add your repository storage paths in your `gitlab.yml` under repositories -> storages, using key -> value pairs. +GitLab allows you to define multiple repository storage paths to distribute the +storage load between several mount points. >**Notes:** +> - You must have at least one storage path called `default`. +- The paths are defined in key-value pairs. The key is an arbitrary name you + can pick to name the file path. +- The target directories and any of its subpaths must not be a symlink. + +## Configure GitLab + +>**Warning:** - In order for backups to work correctly the storage path must **not** be a -mount point and the GitLab user should have correct permissions for the parent -directory of the path. + mount point and the GitLab user should have correct permissions for the parent + directory of the path. + +Edit the configuration files and add the full paths of the alternative repository +storage paths. In the example below we added two more mountpoints that we named +`nfs` and `cephfs` respectively. + +**For installations from source** + +1. Edit `gitlab.yml` and add the storage paths: + + ```yaml + repositories: + # Paths where repositories can be stored. Give the canonicalized absolute pathname. + # NOTE: REPOS PATHS MUST NOT CONTAIN ANY SYMLINK!!! + storages: # You must have at least a 'default' storage path. + default: /home/git/repositories + nfs: /mnt/nfs/repositories + cephfs: /mnt/cephfs/repositories + ``` + +1. [Restart GitLab] for the changes to take effect. + +The `gitlab_shell: repos_path` entry in `gitlab.yml` will be deprecated and +replaced by `repositories: storages` in the future, so if you are upgrading +from a version prior to 8.10, make sure to add the configuration as described +in the step above. After you make the changes and confirm they are working, +you can remove: + +```yaml +repos_path: /home/git/repositories +``` + +which is located under the `gitlab_shell` section. + +--- + +**For Omnibus installations** + +1. Edit `/etc/gitlab/gitlab.rb` by appending the rest of the paths to the + default one: + + ```ruby + git_data_dirs({ + "default" => "/var/opt/gitlab/git-data", + "nfs" => "/mnt/nfs/git-data", + "cephfs" => "/mnt/cephfs/git-data" + }) + ``` + + Note that Omnibus stores the repositories in a `repositories` subdirectory + of the `git-data` directory. + +## Choose where new project repositories will be stored + +Once you set the multiple storage paths, you can choose where new projects will +be stored via the **Application Settings** in the Admin area. -## For omnibus installations +![Choose repository storage path in Admin area](img/repository_storages_admin_ui.png) -Follow the instructions at https://gitlab.com/gitlab-org/omnibus-gitlab/blob/master/doc/settings/configuration.md#storing-git-data-in-an-alternative-directory +[ce-4578]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4578 +[restart gitlab]: restart_gitlab.md#installations-from-source +[reconfigure gitlab]: restart_gitlab.md#omnibus-gitlab-reconfigure -- cgit v1.2.1