diff options
125 files changed, 2545 insertions, 787 deletions
diff --git a/.rubocop.yml b/.rubocop.yml index 6adbda53456..556a5d11a39 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -510,6 +510,15 @@ Metrics/PerceivedComplexity: #################### Lint ################################ +# Checks for useless access modifiers. +Lint/UselessAccessModifier: + Enabled: true + +# Checks for attempts to use `private` or `protected` to set the visibility +# of a class method, which does not work. +Lint/IneffectiveAccessModifier: + Enabled: false + # Checks for ambiguous operators in the first argument of a method invocation # without parentheses. Lint/AmbiguousOperator: diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index b622b9239d4..76ae5952753 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -19,10 +19,6 @@ Lint/AssignmentInCondition: Lint/HandleExceptions: Enabled: false -# Offense count: 21 -Lint/IneffectiveAccessModifier: - Enabled: false - # Offense count: 2 Lint/Loop: Enabled: false @@ -48,10 +44,6 @@ Lint/UnusedBlockArgument: Lint/UnusedMethodArgument: Enabled: false -# Offense count: 11 -Lint/UselessAccessModifier: - Enabled: false - # Offense count: 12 # Cop supports --auto-correct. Performance/PushSplat: diff --git a/CHANGELOG b/CHANGELOG index 8759bb04dec..9075972e6d0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,16 +2,21 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.11.0 (unreleased) - Fix the title of the toggle dropdown button. !5515 (herminiotorres) + - Improve diff performance by eliminating redundant checks for text blobs - Remove magic comments (`# encoding: UTF-8`) from Ruby files. !5456 (winniehell) - Fix CI status icon link underline (ClemMakesApps) + - Cache the commit author in RequestStore to avoid extra lookups in PostReceive - Fix of 'Commits being passed to custom hooks are already reachable when using the UI' - Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable - Optimize maximum user access level lookup in loading of notes + - Add "No one can push" as an option for protected branches. !5081 - Limit git rev-list output count to one in forced push check - Clean up unused routes (Josef Strzibny) - Add green outline to New Branch button. !5447 (winniehell) + - Update to gitlab_git 10.4.1 and take advantage of preserved Ref objects - Retrieve rendered HTML from cache in one request - Fix renaming repository when name contains invalid chararacters under project settings + - Optimize checking if a user has read access to a list of issues !5370 - Nokogiri's various parsing methods are now instrumented - Add a way to send an email and create an issue based on private personal token. Find the email address from issues page. !3363 - Add build event color in HipChat messages (David Eisner) @@ -20,7 +25,9 @@ v 8.11.0 (unreleased) - The overhead of instrumented method calls has been reduced - Remove `search_id` of labels dropdown filter to fix 'Missleading URI for labels in Merge Requests and Issues view'. !5368 (Scott Le) - Load project invited groups and members eagerly in `ProjectTeam#fetch_members` + - Bump gitlab_git to speedup DiffCollection iterations - Make branches sortable without push permission !5462 (winniehell) + - Check for Ci::Build artifacts at database level on pipeline partial - Add GitLab Workhorse version to admin dashboard (Katarzyna Kobierska Ula Budziszewska) - Add the `sprockets-es6` gem - Multiple trigger variables show in separate lines (Katarzyna Kobierska Ula Budziszewska) @@ -28,8 +35,12 @@ v 8.11.0 (unreleased) - Add commit stats in commit api. !5517 (dixpac) - Make error pages responsive (Takuya Noguchi) - Change requests_profiles resource constraint to catch virtually any file + - Reduce number of queries made for merge_requests/:id/diffs v 8.10.3 (unreleased) + - Fix hooks missing on imported GitLab projects + - Properly abort a merge when merge conflicts occur + - Ignore invalid IPs in X-Forwarded-For when trusted proxies are configured. v 8.10.2 - User can now search branches by name. !5144 @@ -53,7 +53,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.2' # LDAP Auth # GitLab fork with several improvements to original library. For full list of changes diff --git a/Gemfile.lock b/Gemfile.lock index 670578dec6d..7b4175ea824 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -278,7 +278,7 @@ GEM diff-lcs (~> 1.1) mime-types (>= 1.16, < 3) posix-spawn (~> 0.3) - gitlab_git (10.3.2) + gitlab_git (10.4.2) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) @@ -870,7 +870,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.2) gitlab_meta (= 7.0) gitlab_omniauth-ldap (~> 1.2.1) gollum-lib (~> 4.2) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index d212d66da1b..9e6901962c6 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -171,6 +171,11 @@ break; case 'search:show': new Search(); + break; + case 'projects:protected_branches:index': + new ProtectedBranchesAccessSelect($(".new_protected_branch"), false, true); + new ProtectedBranchesAccessSelect($(".protected-branches-list"), true, false); + break; } switch (path.first()) { case 'admin': diff --git a/app/assets/javascripts/protected_branches.js b/app/assets/javascripts/protected_branches.js deleted file mode 100644 index db21a19964d..00000000000 --- a/app/assets/javascripts/protected_branches.js +++ /dev/null @@ -1,35 +0,0 @@ -(function() { - $(function() { - return $(".protected-branches-list :checkbox").change(function(e) { - var can_push, id, name, obj, url; - name = $(this).attr("name"); - if (name === "developers_can_push" || name === "developers_can_merge") { - id = $(this).val(); - can_push = $(this).is(":checked"); - url = $(this).data("url"); - return $.ajax({ - type: "PATCH", - url: url, - dataType: "json", - data: { - id: id, - protected_branch: ( - obj = {}, - obj["" + name] = can_push, - obj - ) - }, - success: function() { - var row; - row = $(e.target); - return row.closest('tr').effect('highlight'); - }, - error: function() { - return new Flash("Failed to update branch!", "alert"); - } - }); - } - }); - }); - -}).call(this); diff --git a/app/assets/javascripts/protected_branches_access_select.js.es6 b/app/assets/javascripts/protected_branches_access_select.js.es6 new file mode 100644 index 00000000000..e98312bbf37 --- /dev/null +++ b/app/assets/javascripts/protected_branches_access_select.js.es6 @@ -0,0 +1,63 @@ +class ProtectedBranchesAccessSelect { + constructor(container, saveOnSelect, selectDefault) { + this.container = container; + this.saveOnSelect = saveOnSelect; + + this.container.find(".allowed-to-merge").each((i, element) => { + var fieldName = $(element).data('field-name'); + var dropdown = $(element).glDropdown({ + data: gon.merge_access_levels, + selectable: true, + fieldName: fieldName, + clicked: _.chain(this.onSelect).partial(element).bind(this).value() + }); + + if (selectDefault) { + dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0); + } + }); + + + this.container.find(".allowed-to-push").each((i, element) => { + var fieldName = $(element).data('field-name'); + var dropdown = $(element).glDropdown({ + data: gon.push_access_levels, + selectable: true, + fieldName: fieldName, + clicked: _.chain(this.onSelect).partial(element).bind(this).value() + }); + + if (selectDefault) { + dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0); + } + }); + } + + onSelect(dropdown, selected, element, e) { + $(dropdown).find('.dropdown-toggle-text').text(selected.text); + if (this.saveOnSelect) { + return $.ajax({ + type: "POST", + url: $(dropdown).data('url'), + dataType: "json", + data: { + _method: 'PATCH', + id: $(dropdown).data('id'), + protected_branch: { + ["" + ($(dropdown).data('type')) + "_attributes"]: { + "access_level": selected.id + } + } + }, + success: function() { + var row; + row = $(e.target); + return row.closest('tr').effect('highlight'); + }, + error: function() { + return new Flash("Failed to update branch!", "alert"); + } + }); + } + } +} diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index cc3aef5199e..4409477916f 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -661,14 +661,28 @@ pre.light-well { } } +.new_protected_branch { + .dropdown { + display: inline; + margin-left: 15px; + } + + label { + min-width: 120px; + } +} + .protected-branches-list { a { color: $gl-gray; - font-weight: 600; &:hover { color: $gl-link-color; } + + &.is-active { + font-weight: 600; + } } } diff --git a/app/controllers/import/bitbucket_controller.rb b/app/controllers/import/bitbucket_controller.rb index 25e58724860..944c73d139a 100644 --- a/app/controllers/import/bitbucket_controller.rb +++ b/app/controllers/import/bitbucket_controller.rb @@ -82,8 +82,6 @@ class Import::BitbucketController < Import::BaseController go_to_bitbucket_for_permissions end - private - def access_params { bitbucket_access_token: session[:bitbucket_access_token], diff --git a/app/controllers/import/gitlab_controller.rb b/app/controllers/import/gitlab_controller.rb index 23a396e8084..08130ee8176 100644 --- a/app/controllers/import/gitlab_controller.rb +++ b/app/controllers/import/gitlab_controller.rb @@ -61,8 +61,6 @@ class Import::GitlabController < Import::BaseController go_to_gitlab_for_permissions end - private - def access_params { gitlab_access_token: session[:gitlab_access_token] } end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 23252fa59cc..47c21a18b33 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -407,7 +407,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController } @use_legacy_diff_notes = !@merge_request.support_new_diff_notes? - @grouped_diff_discussions = @merge_request.notes.grouped_diff_discussions + @grouped_diff_discussions = @merge_request.notes.inc_author_project_award_emoji.grouped_diff_discussions Banzai::NoteRenderer.render( @grouped_diff_discussions.values.flat_map(&:notes), diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index 10dca47fded..d28ec6e2eac 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -3,19 +3,24 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController before_action :require_non_empty_project before_action :authorize_admin_project! before_action :load_protected_branch, only: [:show, :update, :destroy] + before_action :load_protected_branches, only: [:index] layout "project_settings" def index - @protected_branches = @project.protected_branches.order(:name).page(params[:page]) @protected_branch = @project.protected_branches.new - gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } }) + load_protected_branches_gon_variables end def create - @project.protected_branches.create(protected_branch_params) - redirect_to namespace_project_protected_branches_path(@project.namespace, - @project) + @protected_branch = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute + if @protected_branch.persisted? + redirect_to namespace_project_protected_branches_path(@project.namespace, @project) + else + load_protected_branches + load_protected_branches_gon_variables + render :index + end end def show @@ -23,7 +28,9 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController end def update - if @protected_branch && @protected_branch.update_attributes(protected_branch_params) + @protected_branch = ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch) + + if @protected_branch.valid? respond_to do |format| format.json { render json: @protected_branch, status: :ok } end @@ -50,6 +57,18 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController end def protected_branch_params - params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge) + params.require(:protected_branch).permit(:name, + merge_access_level_attributes: [:access_level], + push_access_level_attributes: [:access_level]) + end + + def load_protected_branches + @protected_branches = @project.protected_branches.order(:name).page(params[:page]) + end + + def load_protected_branches_gon_variables + gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } }, + push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } }, + merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } }) end end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index ec7a2e63b9a..a6e1aa5ccc1 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -97,7 +97,7 @@ class ProjectsController < Projects::ApplicationController end if @project.pending_delete? - flash[:alert] = "Project queued for delete." + flash[:alert] = "Project #{@project.name} queued for deletion." end respond_to do |format| diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index abe115d8c68..48c27828219 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -13,7 +13,7 @@ module BlobHelper blob = project.repository.blob_at(ref, path) rescue nil - return unless blob && blob_text_viewable?(blob) + return unless blob from_mr = options[:from_merge_request_id] link_opts = {} diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index 4c031942793..f35e2f6ddcd 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -144,8 +144,6 @@ module DiffHelper toggle_whitespace_link(url, options) end - private - def hide_whitespace? params[:w] == '1' end diff --git a/app/models/ability.rb b/app/models/ability.rb index e47c5539f60..d95a2507199 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -47,6 +47,16 @@ class Ability end end + # Returns an Array of Issues that can be read by the given user. + # + # issues - The issues to reduce down to those readable by the user. + # user - The User for which to check the issues + def issues_readable_by_user(issues, user = nil) + return issues if user && user.admin? + + issues.select { |issue| issue.visible_to_user?(user) } + end + # List of possible abilities for anonymous user def anonymous_abilities(user, subject) if subject.is_a?(PersonalSnippet) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index aac78d75f57..08f396210c9 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -13,6 +13,7 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->() { where.not(artifacts_file: [nil, '']) } + scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } scope :manual_actions, ->() { where(when: :manual) } diff --git a/app/models/commit.rb b/app/models/commit.rb index f80f1063406..486ad6714d9 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -178,7 +178,18 @@ class Commit end def author - @author ||= User.find_by_any_email(author_email.downcase) + if RequestStore.active? + key = "commit_author:#{author_email.downcase}" + # nil is a valid value since no author may exist in the system + if RequestStore.store.has_key?(key) + @author = RequestStore.store[key] + else + @author = find_author_by_any_email + RequestStore.store[key] = @author + end + else + @author ||= find_author_by_any_email + end end def committer @@ -306,6 +317,10 @@ class Commit private + def find_author_by_any_email + User.find_by_any_email(author_email.downcase) + end + def repo_changes changes = { added: [], modified: [], removed: [] } diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index acb6f5a2998..cbae1cd439b 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -17,7 +17,7 @@ module Issuable belongs_to :assignee, class_name: "User" belongs_to :updated_by, class_name: "User" belongs_to :milestone - has_many :notes, as: :noteable, dependent: :destroy do + has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :destroy do def authors_loaded? # We check first if we're loaded to not load unnecessarily. loaded? && to_a.all? { |note| note.association(:author).loaded? } diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 885deaf78d2..24c7b26d223 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -1,12 +1,26 @@ module TokenAuthenticatable extend ActiveSupport::Concern + private + + def write_new_token(token_field) + new_token = generate_token(token_field) + write_attribute(token_field, new_token) + end + + def generate_token(token_field) + loop do + token = Devise.friendly_token + break token unless self.class.unscoped.find_by(token_field => token) + end + end + class_methods do def authentication_token_fields @token_fields || [] end - private + private # rubocop:disable Lint/UselessAccessModifier def add_authentication_token_field(token_field) @token_fields = [] unless @token_fields @@ -32,18 +46,4 @@ module TokenAuthenticatable end end end - - private - - def write_new_token(token_field) - new_token = generate_token(token_field) - write_attribute(token_field, new_token) - end - - def generate_token(token_field) - loop do - token = Devise.friendly_token - break token unless self.class.unscoped.find_by(token_field => token) - end - end end diff --git a/app/models/issue.rb b/app/models/issue.rb index d9428ebc9fb..11f734cfc6d 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -230,6 +230,34 @@ class Issue < ActiveRecord::Base self.closed_by_merge_requests(current_user).empty? end + # Returns `true` if the current issue can be viewed by either a logged in User + # or an anonymous user. + def visible_to_user?(user = nil) + user ? readable_by?(user) : publicly_visible? + end + + # Returns `true` if the given User can read the current Issue. + def readable_by?(user) + if user.admin? + true + elsif project.owner == user + true + elsif confidential? + author == user || + assignee == user || + project.team.member?(user, Gitlab::Access::REPORTER) + else + project.public? || + project.internal? && !user.external? || + project.team.member?(user) + end + end + + # Returns `true` if this Issue is visible to everybody. + def publicly_visible? + project.public? && !confidential? + end + def overdue? due_date.try(:past?) || false end diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 04a651d50ab..865712268a0 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -25,6 +25,14 @@ class LegacyDiffNote < Note @discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code) end + def project_repository + if RequestStore.active? + RequestStore.fetch("project:#{project_id}:repository") { self.project.repository } + else + self.project.repository + end + end + def diff_file_hash line_code.split('_')[0] if line_code end @@ -34,7 +42,7 @@ class LegacyDiffNote < Note end def diff_file - @diff_file ||= Gitlab::Diff::File.new(diff, repository: self.project.repository) if diff + @diff_file ||= Gitlab::Diff::File.new(diff, repository: project_repository) if diff end def diff_line diff --git a/app/models/project.rb b/app/models/project.rb index dc44a757b4b..7aecd7860c5 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -874,14 +874,6 @@ class Project < ActiveRecord::Base ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present? end - def developers_can_push_to_protected_branch?(branch_name) - protected_branches.matching(branch_name).any?(&:developers_can_push) - end - - def developers_can_merge_to_protected_branch?(branch_name) - protected_branches.matching(branch_name).any?(&:developers_can_merge) - end - def forked? !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?) end diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index b7011d7afdf..226b3f54342 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -5,6 +5,12 @@ class ProtectedBranch < ActiveRecord::Base validates :name, presence: true validates :project, presence: true + has_one :merge_access_level, dependent: :destroy + has_one :push_access_level, dependent: :destroy + + accepts_nested_attributes_for :push_access_level + accepts_nested_attributes_for :merge_access_level + def commit project.commit(self.name) end diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb new file mode 100644 index 00000000000..b1112ee737d --- /dev/null +++ b/app/models/protected_branch/merge_access_level.rb @@ -0,0 +1,24 @@ +class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base + belongs_to :protected_branch + delegate :project, to: :protected_branch + + validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER, + Gitlab::Access::DEVELOPER] } + + def self.human_access_levels + { + Gitlab::Access::MASTER => "Masters", + Gitlab::Access::DEVELOPER => "Developers + Masters" + }.with_indifferent_access + end + + def check_access(user) + return true if user.is_admin? + + project.team.max_member_access(user.id) >= access_level + end + + def humanize + self.class.human_access_levels[self.access_level] + end +end diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb new file mode 100644 index 00000000000..6a5e49cf453 --- /dev/null +++ b/app/models/protected_branch/push_access_level.rb @@ -0,0 +1,27 @@ +class ProtectedBranch::PushAccessLevel < ActiveRecord::Base + belongs_to :protected_branch + delegate :project, to: :protected_branch + + validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER, + Gitlab::Access::DEVELOPER, + Gitlab::Access::NO_ACCESS] } + + def self.human_access_levels + { + Gitlab::Access::MASTER => "Masters", + Gitlab::Access::DEVELOPER => "Developers + Masters", + Gitlab::Access::NO_ACCESS => "No one" + }.with_indifferent_access + end + + def check_access(user) + return false if access_level == Gitlab::Access::NO_ACCESS + return true if user.is_admin? + + project.team.max_member_access(user.id) >= access_level + end + + def humanize + self.class.human_access_levels[self.access_level] + end +end diff --git a/app/models/repository.rb b/app/models/repository.rb index af65e5b20ec..bac37483c47 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 @@ -158,7 +163,7 @@ class Repository before_remove_branch branch = find_branch(branch_name) - oldrev = branch.try(:target) + oldrev = branch.try(:target).try(:id) newrev = Gitlab::Git::BLANK_SHA ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name @@ -259,10 +264,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 @@ -688,9 +693,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 @@ -831,7 +834,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 @@ -848,7 +851,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 @@ -869,7 +872,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? @@ -883,7 +886,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? @@ -974,7 +977,7 @@ class Repository was_empty = empty? if !was_empty && target_branch - oldrev = target_branch.target + oldrev = target_branch.target.id end # Make commit @@ -994,7 +997,7 @@ class Repository after_create_branch else # Update head - current_head = find_branch(branch).target + current_head = find_branch(branch).target.id # Make sure target branch was not changed during pre-receive hook if current_head == oldrev @@ -1052,7 +1055,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/auth/container_registry_authentication_service.rb b/app/services/auth/container_registry_authentication_service.rb index e294a962352..6072123b851 100644 --- a/app/services/auth/container_registry_authentication_service.rb +++ b/app/services/auth/container_registry_authentication_service.rb @@ -24,10 +24,14 @@ module Auth token[:access] = names.map do |name| { type: 'repository', name: name, actions: %w(*) } end - + token.encoded end + def self.token_expire_at + Time.now + current_application_settings.container_registry_token_expire_delay.minutes + end + private def authorized_token(*accesses) @@ -35,7 +39,7 @@ module Auth token.issuer = registry.issuer token.audience = params[:service] token.subject = current_user.try(:username) - token.expire_time = ContainerRegistryAuthenticationService.token_expire_at + token.expire_time = self.class.token_expire_at token[:access] = accesses.compact token end @@ -81,9 +85,5 @@ module Auth def registry Gitlab.config.registry end - - def self.token_expire_at - Time.now + current_application_settings.container_registry_token_expire_delay.minutes - end end end 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_push_service.rb b/app/services/git_push_service.rb index e02b50ff9a2..3f6a177bf3a 100644 --- a/app/services/git_push_service.rb +++ b/app/services/git_push_service.rb @@ -88,9 +88,18 @@ class GitPushService < BaseService # Set protection on the default branch if configured if current_application_settings.default_branch_protection != PROTECTION_NONE - developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false - developers_can_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? true : false - @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push, developers_can_merge: developers_can_merge }) + + params = { + name: @project.default_branch, + push_access_level_attributes: { + access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER + }, + merge_access_level_attributes: { + access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER + } + } + + ProtectedBranches::CreateService.new(@project, current_user, params).execute 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/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 0dac0614141..b037780c431 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -35,7 +35,13 @@ module MergeRequests } commit_id = repository.merge(current_user, merge_request, options) - merge_request.update(merge_commit_sha: commit_id) + + if commit_id + merge_request.update(merge_commit_sha: commit_id) + else + merge_request.update(merge_error: 'Conflicts detected during merge') + false + end rescue GitHooksService::PreReceiveError => e merge_request.update(merge_error: e.message) false diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb new file mode 100644 index 00000000000..6150a2a83c9 --- /dev/null +++ b/app/services/protected_branches/create_service.rb @@ -0,0 +1,27 @@ +module ProtectedBranches + class CreateService < BaseService + attr_reader :protected_branch + + def execute + raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) + + protected_branch = project.protected_branches.new(params) + + ProtectedBranch.transaction do + protected_branch.save! + + if protected_branch.push_access_level.blank? + protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER) + end + + if protected_branch.merge_access_level.blank? + protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER) + end + end + + protected_branch + rescue ActiveRecord::RecordInvalid + protected_branch + end + end +end diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb new file mode 100644 index 00000000000..89d8ba60134 --- /dev/null +++ b/app/services/protected_branches/update_service.rb @@ -0,0 +1,13 @@ +module ProtectedBranches + class UpdateService < BaseService + attr_reader :protected_branch + + def execute(protected_branch) + raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) + + @protected_branch = protected_branch + @protected_branch.update(params) + @protected_branch + end + end +end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 1ab3b5789bc..e13dc9265b8 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -2,7 +2,9 @@ # # Used for creating system notes (e.g., when a user references a merge request # from an issue, an issue's assignee changes, an issue is closed, etc.) -class SystemNoteService +module SystemNoteService + extend self + # Called when commits are added to a Merge Request # # noteable - Noteable object @@ -15,7 +17,7 @@ class SystemNoteService # See new_commit_summary and existing_commit_summary. # # Returns the created Note object - def self.add_commits(noteable, project, author, new_commits, existing_commits = [], oldrev = nil) + def add_commits(noteable, project, author, new_commits, existing_commits = [], oldrev = nil) total_count = new_commits.length + existing_commits.length commits_text = "#{total_count} commit".pluralize(total_count) @@ -40,7 +42,7 @@ class SystemNoteService # "Reassigned to @rspeicher" # # Returns the created Note object - def self.change_assignee(noteable, project, author, assignee) + def change_assignee(noteable, project, author, assignee) body = assignee.nil? ? 'Assignee removed' : "Reassigned to #{assignee.to_reference}" create_note(noteable: noteable, project: project, author: author, note: body) @@ -63,7 +65,7 @@ class SystemNoteService # "Removed ~5 label" # # Returns the created Note object - def self.change_label(noteable, project, author, added_labels, removed_labels) + def change_label(noteable, project, author, added_labels, removed_labels) labels_count = added_labels.count + removed_labels.count references = ->(label) { label.to_reference(format: :id) } @@ -101,7 +103,7 @@ class SystemNoteService # "Miletone changed to 7.11" # # Returns the created Note object - def self.change_milestone(noteable, project, author, milestone) + def change_milestone(noteable, project, author, milestone) body = 'Milestone ' body += milestone.nil? ? 'removed' : "changed to #{milestone.to_reference(project)}" @@ -123,7 +125,7 @@ class SystemNoteService # "Status changed to closed by bc17db76" # # Returns the created Note object - def self.change_status(noteable, project, author, status, source) + def change_status(noteable, project, author, status, source) body = "Status changed to #{status}" body << " by #{source.gfm_reference(project)}" if source @@ -131,26 +133,26 @@ class SystemNoteService end # Called when 'merge when build succeeds' is executed - def self.merge_when_build_succeeds(noteable, project, author, last_commit) + def merge_when_build_succeeds(noteable, project, author, last_commit) body = "Enabled an automatic merge when the build for #{last_commit.to_reference(project)} succeeds" create_note(noteable: noteable, project: project, author: author, note: body) end # Called when 'merge when build succeeds' is canceled - def self.cancel_merge_when_build_succeeds(noteable, project, author) + def cancel_merge_when_build_succeeds(noteable, project, author) body = 'Canceled the automatic merge' create_note(noteable: noteable, project: project, author: author, note: body) end - def self.remove_merge_request_wip(noteable, project, author) + def remove_merge_request_wip(noteable, project, author) body = 'Unmarked this merge request as a Work In Progress' create_note(noteable: noteable, project: project, author: author, note: body) end - def self.add_merge_request_wip(noteable, project, author) + def add_merge_request_wip(noteable, project, author) body = 'Marked this merge request as a **Work In Progress**' create_note(noteable: noteable, project: project, author: author, note: body) @@ -168,7 +170,7 @@ class SystemNoteService # "Title changed from **Old** to **New**" # # Returns the created Note object - def self.change_title(noteable, project, author, old_title) + def change_title(noteable, project, author, old_title) new_title = noteable.title.dup old_diffs, new_diffs = Gitlab::Diff::InlineDiff.new(old_title, new_title).inline_diffs @@ -191,7 +193,7 @@ class SystemNoteService # "Made the issue confidential" # # Returns the created Note object - def self.change_issue_confidentiality(issue, project, author) + def change_issue_confidentiality(issue, project, author) body = issue.confidential ? 'Made the issue confidential' : 'Made the issue visible' create_note(noteable: issue, project: project, author: author, note: body) end @@ -210,7 +212,7 @@ class SystemNoteService # "Target branch changed from `Old` to `New`" # # Returns the created Note object - def self.change_branch(noteable, project, author, branch_type, old_branch, new_branch) + def change_branch(noteable, project, author, branch_type, old_branch, new_branch) body = "#{branch_type} branch changed from `#{old_branch}` to `#{new_branch}`".capitalize create_note(noteable: noteable, project: project, author: author, note: body) end @@ -229,7 +231,7 @@ class SystemNoteService # "Restored target branch `feature`" # # Returns the created Note object - def self.change_branch_presence(noteable, project, author, branch_type, branch, presence) + def change_branch_presence(noteable, project, author, branch_type, branch, presence) verb = if presence == :add 'restored' @@ -245,7 +247,7 @@ class SystemNoteService # Example note text: # # "Started branch `201-issue-branch-button`" - def self.new_issue_branch(issue, project, author, branch) + def new_issue_branch(issue, project, author, branch) h = Gitlab::Routing.url_helpers link = h.namespace_project_compare_url(project.namespace, project, from: project.default_branch, to: branch) @@ -270,7 +272,7 @@ class SystemNoteService # See cross_reference_note_content. # # Returns the created Note object - def self.cross_reference(noteable, mentioner, author) + def cross_reference(noteable, mentioner, author) return if cross_reference_disallowed?(noteable, mentioner) gfm_reference = mentioner.gfm_reference(noteable.project) @@ -294,7 +296,7 @@ class SystemNoteService end end - def self.cross_reference?(note_text) + def cross_reference?(note_text) note_text.start_with?(cross_reference_note_prefix) end @@ -308,7 +310,7 @@ class SystemNoteService # mentioner - Mentionable object # # Returns Boolean - def self.cross_reference_disallowed?(noteable, mentioner) + def cross_reference_disallowed?(noteable, mentioner) return true if noteable.is_a?(ExternalIssue) && !noteable.project.jira_tracker_active? return false unless mentioner.is_a?(MergeRequest) return false unless noteable.is_a?(Commit) @@ -328,7 +330,7 @@ class SystemNoteService # # Returns Boolean - def self.cross_reference_exists?(noteable, mentioner) + def cross_reference_exists?(noteable, mentioner) # Initial scope should be system notes of this noteable type notes = Note.system.where(noteable_type: noteable.class) @@ -342,9 +344,60 @@ class SystemNoteService notes_for_mentioner(mentioner, noteable, notes).count > 0 end + # Build an Array of lines detailing each commit added in a merge request + # + # new_commits - Array of new Commit objects + # + # Returns an Array of Strings + def new_commit_summary(new_commits) + new_commits.collect do |commit| + "* #{commit.short_id} - #{escape_html(commit.title)}" + end + end + + # Called when the status of a Task has changed + # + # noteable - Noteable object. + # project - Project owning noteable + # author - User performing the change + # new_task - TaskList::Item object. + # + # Example Note text: + # + # "Soandso marked the task Whatever as completed." + # + # Returns the created Note object + def change_task_status(noteable, project, author, new_task) + status_label = new_task.complete? ? Taskable::COMPLETED : Taskable::INCOMPLETE + body = "Marked the task **#{new_task.source}** as #{status_label}" + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when noteable has been moved to another project + # + # direction - symbol, :to or :from + # noteable - Noteable object + # noteable_ref - Referenced noteable + # author - User performing the move + # + # Example Note text: + # + # "Moved to some_namespace/project_new#11" + # + # Returns the created Note object + def noteable_moved(noteable, project, noteable_ref, author, direction:) + unless [:to, :from].include?(direction) + raise ArgumentError, "Invalid direction `#{direction}`" + end + + cross_reference = noteable_ref.to_reference(project) + body = "Moved #{direction} #{cross_reference}" + create_note(noteable: noteable, project: project, author: author, note: body) + end + private - def self.notes_for_mentioner(mentioner, noteable, notes) + def notes_for_mentioner(mentioner, noteable, notes) if mentioner.is_a?(Commit) notes.where('note LIKE ?', "#{cross_reference_note_prefix}%#{mentioner.to_reference(nil)}") else @@ -353,29 +406,18 @@ class SystemNoteService end end - def self.create_note(args = {}) + def create_note(args = {}) Note.create(args.merge(system: true)) end - def self.cross_reference_note_prefix + def cross_reference_note_prefix 'mentioned in ' end - def self.cross_reference_note_content(gfm_reference) + def cross_reference_note_content(gfm_reference) "#{cross_reference_note_prefix}#{gfm_reference}" end - # Build an Array of lines detailing each commit added in a merge request - # - # new_commits - Array of new Commit objects - # - # Returns an Array of Strings - def self.new_commit_summary(new_commits) - new_commits.collect do |commit| - "* #{commit.short_id} - #{escape_html(commit.title)}" - end - end - # Build a single line summarizing existing commits being added in a merge # request # @@ -392,7 +434,7 @@ class SystemNoteService # "* ea0f8418 - 1 commit from branch `feature`" # # Returns a newline-terminated String - def self.existing_commit_summary(noteable, existing_commits, oldrev = nil) + def existing_commit_summary(noteable, existing_commits, oldrev = nil) return '' if existing_commits.empty? count = existing_commits.size @@ -415,47 +457,7 @@ class SystemNoteService "* #{commit_ids} - #{commits_text} from branch `#{branch}`\n" end - # Called when the status of a Task has changed - # - # noteable - Noteable object. - # project - Project owning noteable - # author - User performing the change - # new_task - TaskList::Item object. - # - # Example Note text: - # - # "Soandso marked the task Whatever as completed." - # - # Returns the created Note object - def self.change_task_status(noteable, project, author, new_task) - status_label = new_task.complete? ? Taskable::COMPLETED : Taskable::INCOMPLETE - body = "Marked the task **#{new_task.source}** as #{status_label}" - create_note(noteable: noteable, project: project, author: author, note: body) - end - - # Called when noteable has been moved to another project - # - # direction - symbol, :to or :from - # noteable - Noteable object - # noteable_ref - Referenced noteable - # author - User performing the move - # - # Example Note text: - # - # "Moved to some_namespace/project_new#11" - # - # Returns the created Note object - def self.noteable_moved(noteable, project, noteable_ref, author, direction:) - unless [:to, :from].include?(direction) - raise ArgumentError, "Invalid direction `#{direction}`" - end - - cross_reference = noteable_ref.to_reference(project) - body = "Moved #{direction} #{cross_reference}" - create_note(noteable: noteable, project: project, author: author, note: body) - end - - def self.escape_html(text) + def escape_html(text) Rack::Utils.escape_html(text) end end diff --git a/app/views/projects/blob/_actions.html.haml b/app/views/projects/blob/_actions.html.haml index cdac50f7a8d..ff893ea74e1 100644 --- a/app/views/projects/blob/_actions.html.haml +++ b/app/views/projects/blob/_actions.html.haml @@ -16,6 +16,7 @@ - if current_user .btn-group{ role: "group" } - = edit_blob_link + - if blob_text_viewable?(@blob) + = edit_blob_link = replace_blob_link = delete_blob_link 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/app/views/projects/ci/pipelines/_pipeline.html.haml b/app/views/projects/ci/pipelines/_pipeline.html.haml index 2f7d54f0bdd..558c35553da 100644 --- a/app/views/projects/ci/pipelines/_pipeline.html.haml +++ b/app/views/projects/ci/pipelines/_pipeline.html.haml @@ -57,7 +57,7 @@ %td.pipeline-actions .controls.hidden-xs.pull-right - - artifacts = pipeline.builds.latest.select { |b| b.artifacts? } + - artifacts = pipeline.builds.latest.with_artifacts_not_expired - actions = pipeline.manual_actions - if artifacts.present? || actions.any? .btn-group.inline diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index c306909fb1a..1854c64cbd7 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -9,10 +9,11 @@ = icon('comment') \ - - if editable_diff?(diff_file) - = edit_blob_link(@merge_request.source_project, - @merge_request.source_branch, diff_file.new_path, - from_merge_request_id: @merge_request.id) + - if editable_diff?(diff_file) + = edit_blob_link(@merge_request.source_project, + @merge_request.source_branch, diff_file.new_path, + from_merge_request_id: @merge_request.id, + skip_visible_check: true) = view_file_btn(diff_commit.id, diff_file, project) diff --git a/app/views/projects/diffs/_stats.html.haml b/app/views/projects/diffs/_stats.html.haml index ea2a3e01277..e751dabdf99 100644 --- a/app/views/projects/diffs/_stats.html.haml +++ b/app/views/projects/diffs/_stats.html.haml @@ -2,7 +2,7 @@ .commit-stat-summary Showing = link_to '#', class: 'js-toggle-button' do - %strong #{pluralize(diff_files.count, "changed file")} + %strong #{pluralize(diff_files.size, "changed file")} with %strong.cgreen #{diff_files.sum(&:added_lines)} additions and diff --git a/app/views/projects/diffs/_warning.html.haml b/app/views/projects/diffs/_warning.html.haml index 10fa1ddf2e5..295a1b62535 100644 --- a/app/views/projects/diffs/_warning.html.haml +++ b/app/views/projects/diffs/_warning.html.haml @@ -11,5 +11,5 @@ = link_to "Email patch", merge_request_path(@merge_request, format: :patch), class: "btn btn-sm" %p To preserve performance only - %strong #{diff_files.count} of #{diff_files.real_size} + %strong #{diff_files.size} of #{diff_files.real_size} files are displayed. diff --git a/app/views/projects/issues/_related_branches.html.haml b/app/views/projects/issues/_related_branches.html.haml index c6fc499a7b8..6ea9f612d13 100644 --- a/app/views/projects/issues/_related_branches.html.haml +++ b/app/views/projects/issues/_related_branches.html.haml @@ -4,8 +4,8 @@ %ul.unstyled-list - @related_branches.each do |branch| %li - - sha = @project.repository.find_branch(branch).target - - pipeline = @project.pipeline(sha, branch) if sha + - target = @project.repository.find_branch(branch).target + - pipeline = @project.pipeline(target.sha, branch) if target - if pipeline %span.related-branch-ci-status = render_pipeline_status(pipeline) diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml index 720d67dff7c..0603a014008 100644 --- a/app/views/projects/protected_branches/_branches_list.html.haml +++ b/app/views/projects/protected_branches/_branches_list.html.haml @@ -5,24 +5,22 @@ No branches are protected, protect a branch with the form above. - else - can_admin_project = can?(current_user, :admin_project, @project) - .table-responsive - %table.table.protected-branches-list - %colgroup - %col{ width: "20%" } - %col{ width: "30%" } - %col{ width: "25%" } - %col{ width: "25%" } + + %table.table.protected-branches-list + %colgroup + %col{ width: "20%" } + %col{ width: "30%" } + %col{ width: "25%" } + %col{ width: "25%" } + %thead + %tr + %th Branch + %th Last commit + %th Allowed to merge + %th Allowed to push - if can_admin_project - %col - %thead - %tr - %th Protected Branch - %th Commit - %th Developers Can Push - %th Developers Can Merge - - if can_admin_project - %th - %tbody - = render partial: @protected_branches, locals: { can_admin_project: can_admin_project } + %th + %tbody + = render partial: @protected_branches, locals: { can_admin_project: can_admin_project } = paginate @protected_branches, theme: 'gitlab' diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml index 7fda7f96047..498e412235e 100644 --- a/app/views/projects/protected_branches/_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_protected_branch.html.haml @@ -15,9 +15,15 @@ - else (branch was removed from repository) %td - = check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url }) + = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level + = dropdown_tag(protected_branch.merge_access_level.humanize, + options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge', + data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "merge_access_level" }}) %td - = check_box_tag("developers_can_merge", protected_branch.id, protected_branch.developers_can_merge, data: { url: url }) + = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level + = dropdown_tag(protected_branch.push_access_level.humanize, + options: { title: "Allowed to push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push', + data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "push_access_level" }}) - if can_admin_project %td = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right" diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml index 950df740bbc..4efe44c7233 100644 --- a/app/views/projects/protected_branches/index.html.haml +++ b/app/views/projects/protected_branches/index.html.haml @@ -32,18 +32,22 @@ are supported. .form-group - = f.check_box :developers_can_push, class: "pull-left" - .prepend-left-20 - = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0" - %p.light.append-bottom-0 - Allow developers to push to this branch + = hidden_field_tag 'protected_branch[merge_access_level_attributes][access_level]' + = label_tag "Allowed to merge: ", nil, class: "label-light append-bottom-0" + = dropdown_tag("<Make a selection>", + options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge', + dropdown_class: 'dropdown-menu-selectable', + data: { field_name: "protected_branch[merge_access_level_attributes][access_level]" }}) .form-group - = f.check_box :developers_can_merge, class: "pull-left" - .prepend-left-20 - = f.label :developers_can_merge, "Developers can merge", class: "label-light append-bottom-0" - %p.light.append-bottom-0 - Allow developers to accept merge requests to this branch + = hidden_field_tag 'protected_branch[push_access_level_attributes][access_level]' + = label_tag "Allowed to push: ", nil, class: "label-light append-bottom-0" + = dropdown_tag("<Make a selection>", + options: { title: "Allowed to push", toggle_class: 'allowed-to-push', + dropdown_class: 'dropdown-menu-selectable', + data: { field_name: "protected_branch[push_access_level_attributes][access_level]" }}) + + = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true %hr diff --git a/config/initializers/trusted_proxies.rb b/config/initializers/trusted_proxies.rb index 30770b71e24..cd869657c53 100644 --- a/config/initializers/trusted_proxies.rb +++ b/config/initializers/trusted_proxies.rb @@ -7,6 +7,8 @@ module Rack class Request def trusted_proxy?(ip) Rails.application.config.action_dispatch.trusted_proxies.any? { |proxy| proxy === ip } + rescue IPAddr::InvalidAddressError + false end end end diff --git a/db/fixtures/development/16_protected_branches.rb b/db/fixtures/development/16_protected_branches.rb new file mode 100644 index 00000000000..103c7f9445c --- /dev/null +++ b/db/fixtures/development/16_protected_branches.rb @@ -0,0 +1,12 @@ +Gitlab::Seeder.quiet do + admin_user = User.find(1) + + Project.all.each do |project| + params = { + name: 'master' + } + + ProtectedBranches::CreateService.new(project, admin_user, params).execute + print '.' + end +end diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb new file mode 100644 index 00000000000..f27295524e1 --- /dev/null +++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb @@ -0,0 +1,17 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddProtectedBranchesPushAccess < ActiveRecord::Migration + DOWNTIME = false + + def change + create_table :protected_branch_push_access_levels do |t| + t.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false + + # Gitlab::Access::MASTER == 40 + t.integer :access_level, default: 40, null: false + + t.timestamps null: false + end + end +end diff --git a/db/migrate/20160705054952_add_protected_branches_merge_access.rb b/db/migrate/20160705054952_add_protected_branches_merge_access.rb new file mode 100644 index 00000000000..32adfa266cd --- /dev/null +++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb @@ -0,0 +1,17 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddProtectedBranchesMergeAccess < ActiveRecord::Migration + DOWNTIME = false + + def change + create_table :protected_branch_merge_access_levels do |t| + t.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false + + # Gitlab::Access::MASTER == 40 + t.integer :access_level, default: 40, null: false + + t.timestamps null: false + end + end +end diff --git a/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb new file mode 100644 index 00000000000..fa93936ced7 --- /dev/null +++ b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb @@ -0,0 +1,29 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MoveFromDevelopersCanMergeToProtectedBranchesMergeAccess < ActiveRecord::Migration + DOWNTIME = true + DOWNTIME_REASON = <<-HEREDOC + We're creating a `merge_access_level` for each `protected_branch`. If a user creates a `protected_branch` while this + is running, we might be left with a `protected_branch` _without_ an associated `merge_access_level`. The `protected_branches` + table must not change while this is running, so downtime is required. + + https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5081#note_13247410 + HEREDOC + + def up + execute <<-HEREDOC + INSERT into protected_branch_merge_access_levels (protected_branch_id, access_level, created_at, updated_at) + SELECT id, (CASE WHEN developers_can_merge THEN 1 ELSE 0 END), now(), now() + FROM protected_branches + HEREDOC + end + + def down + execute <<-HEREDOC + UPDATE protected_branches SET developers_can_merge = TRUE + WHERE id IN (SELECT protected_branch_id FROM protected_branch_merge_access_levels + WHERE access_level = 1); + HEREDOC + end +end diff --git a/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb new file mode 100644 index 00000000000..56f6159d1d8 --- /dev/null +++ b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb @@ -0,0 +1,29 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class MoveFromDevelopersCanPushToProtectedBranchesPushAccess < ActiveRecord::Migration + DOWNTIME = true + DOWNTIME_REASON = <<-HEREDOC + We're creating a `push_access_level` for each `protected_branch`. If a user creates a `protected_branch` while this + is running, we might be left with a `protected_branch` _without_ an associated `push_access_level`. The `protected_branches` + table must not change while this is running, so downtime is required. + + https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5081#note_13247410 + HEREDOC + + def up + execute <<-HEREDOC + INSERT into protected_branch_push_access_levels (protected_branch_id, access_level, created_at, updated_at) + SELECT id, (CASE WHEN developers_can_push THEN 1 ELSE 0 END), now(), now() + FROM protected_branches + HEREDOC + end + + def down + execute <<-HEREDOC + UPDATE protected_branches SET developers_can_push = TRUE + WHERE id IN (SELECT protected_branch_id FROM protected_branch_push_access_levels + WHERE access_level = 1); + HEREDOC + end +end diff --git a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb new file mode 100644 index 00000000000..f563f660ddf --- /dev/null +++ b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveDevelopersCanPushFromProtectedBranches < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # This is only required for `#down` + disable_ddl_transaction! + + DOWNTIME = false + + def up + remove_column :protected_branches, :developers_can_push, :boolean + end + + def down + add_column_with_default(:protected_branches, :developers_can_push, :boolean, default: false, null: false) + end +end diff --git a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb new file mode 100644 index 00000000000..aa71e06d36e --- /dev/null +++ b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveDevelopersCanMergeFromProtectedBranches < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # This is only required for `#down` + disable_ddl_transaction! + + DOWNTIME = false + + def up + remove_column :protected_branches, :developers_can_merge, :boolean + end + + def down + add_column_with_default(:protected_branches, :developers_can_merge, :boolean, default: false, null: false) + end +end diff --git a/db/schema.rb b/db/schema.rb index 15cee55a7bf..2d2ae5fd840 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -867,13 +867,29 @@ ActiveRecord::Schema.define(version: 20160722221922) do add_index "projects", ["star_count"], name: "index_projects_on_star_count", using: :btree add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree + create_table "protected_branch_merge_access_levels", force: :cascade do |t| + t.integer "protected_branch_id", null: false + t.integer "access_level", default: 40, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "protected_branch_merge_access_levels", ["protected_branch_id"], name: "index_protected_branch_merge_access", using: :btree + + create_table "protected_branch_push_access_levels", force: :cascade do |t| + t.integer "protected_branch_id", null: false + t.integer "access_level", default: 40, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "protected_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree + create_table "protected_branches", force: :cascade do |t| - t.integer "project_id", null: false - t.string "name", null: false + t.integer "project_id", null: false + t.string "name", null: false t.datetime "created_at" t.datetime "updated_at" - t.boolean "developers_can_push", default: false, null: false - t.boolean "developers_can_merge", default: false, null: false end add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree @@ -1136,5 +1152,7 @@ ActiveRecord::Schema.define(version: 20160722221922) do add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree add_foreign_key "personal_access_tokens", "users" + add_foreign_key "protected_branch_merge_access_levels", "protected_branches" + add_foreign_key "protected_branch_push_access_levels", "protected_branches" add_foreign_key "u2f_registrations", "users" end diff --git a/features/steps/project/commits/branches.rb b/features/steps/project/commits/branches.rb index 0a42931147d..4bfb7e92e99 100644 --- a/features/steps/project/commits/branches.rb +++ b/features/steps/project/commits/branches.rb @@ -25,7 +25,7 @@ class Spinach::Features::ProjectCommitsBranches < Spinach::FeatureSteps step 'project "Shop" has protected branches' do project = Project.find_by(name: "Shop") - project.protected_branches.create(name: "stable") + create(:protected_branch, project: project, name: "stable") end step 'I click new branch link' do diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb index 0fe046dcbf6..9a8896acb15 100644 --- a/features/steps/project/source/browse_files.rb +++ b/features/steps/project/source/browse_files.rb @@ -293,7 +293,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps first('.js-project-refs-dropdown').click page.within '.project-refs-form' do - click_link 'test' + click_link "'test'" end end diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 66b853eb342..a77afe634f6 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -35,6 +35,10 @@ module API # Protect a single branch # + # Note: The internal data model moved from `developers_can_{merge,push}` to `allowed_to_{merge,push}` + # in `gitlab-org/gitlab-ce!5081`. The API interface has not been changed (to maintain compatibility), + # but it works with the changed data model to infer `developers_can_merge` and `developers_can_push`. + # # Parameters: # id (required) - The ID of a project # branch (required) - The name of the branch @@ -49,17 +53,36 @@ module API @branch = user_project.repository.find_branch(params[:branch]) not_found!('Branch') unless @branch protected_branch = user_project.protected_branches.find_by(name: @branch.name) - developers_can_push = to_boolean(params[:developers_can_push]) + developers_can_merge = to_boolean(params[:developers_can_merge]) + developers_can_push = to_boolean(params[:developers_can_push]) + + protected_branch_params = { + name: @branch.name + } + + unless developers_can_merge.nil? + protected_branch_params.merge!({ + merge_access_level_attributes: { + access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER + } + }) + end + + unless developers_can_push.nil? + protected_branch_params.merge!({ + push_access_level_attributes: { + access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER + } + }) + end if protected_branch - protected_branch.developers_can_push = developers_can_push unless developers_can_push.nil? - protected_branch.developers_can_merge = developers_can_merge unless developers_can_merge.nil? - protected_branch.save + service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params) + service.execute(protected_branch) else - user_project.protected_branches.create(name: @branch.name, - developers_can_push: developers_can_push || false, - developers_can_merge: developers_can_merge || false) + service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params) + service.execute end present @branch, with: Entities::RepoBranch, project: user_project diff --git a/lib/api/entities.rb b/lib/api/entities.rb index e76e7304674..4eb95d8a215 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -126,11 +126,13 @@ module API end expose :developers_can_push do |repo_branch, options| - options[:project].developers_can_push_to_protected_branch? repo_branch.name + project = options[:project] + project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.access_level == Gitlab::Access::DEVELOPER } end expose :developers_can_merge do |repo_branch, options| - options[:project].developers_can_merge_to_protected_branch? repo_branch.name + project = options[:project] + project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.access_level == Gitlab::Access::DEVELOPER } end end diff --git a/lib/banzai/filter/emoji_filter.rb b/lib/banzai/filter/emoji_filter.rb index ae7d31cf191..2492b5213ac 100644 --- a/lib/banzai/filter/emoji_filter.rb +++ b/lib/banzai/filter/emoji_filter.rb @@ -38,6 +38,11 @@ module Banzai end end + # Build a regexp that matches all valid :emoji: names. + def self.emoji_pattern + @emoji_pattern ||= /:(#{Gitlab::Emoji.emojis_names.map { |name| Regexp.escape(name) }.join('|')}):/ + end + private def emoji_url(name) @@ -59,11 +64,6 @@ module Banzai ActionController::Base.helpers.url_to_image(image) end - # Build a regexp that matches all valid :emoji: names. - def self.emoji_pattern - @emoji_pattern ||= /:(#{Gitlab::Emoji.emojis_names.map { |name| Regexp.escape(name) }.join('|')}):/ - end - def emoji_pattern self.class.emoji_pattern end diff --git a/lib/banzai/filter/markdown_filter.rb b/lib/banzai/filter/markdown_filter.rb index 9b209533a89..ff580ec68f8 100644 --- a/lib/banzai/filter/markdown_filter.rb +++ b/lib/banzai/filter/markdown_filter.rb @@ -12,7 +12,12 @@ module Banzai html end - private + def self.renderer + @renderer ||= begin + renderer = Redcarpet::Render::HTML.new + Redcarpet::Markdown.new(renderer, redcarpet_options) + end + end def self.redcarpet_options # https://github.com/vmg/redcarpet#and-its-like-really-simple-to-use @@ -28,12 +33,7 @@ module Banzai }.freeze end - def self.renderer - @renderer ||= begin - renderer = Redcarpet::Render::HTML.new - Redcarpet::Markdown.new(renderer, redcarpet_options) - end - end + private_class_method :redcarpet_options end end end diff --git a/lib/banzai/reference_parser/issue_parser.rb b/lib/banzai/reference_parser/issue_parser.rb index f306079d833..6c20dec5734 100644 --- a/lib/banzai/reference_parser/issue_parser.rb +++ b/lib/banzai/reference_parser/issue_parser.rb @@ -9,10 +9,11 @@ module Banzai issues = issues_for_nodes(nodes) - nodes.select do |node| - issue = issue_for_node(issues, node) + readable_issues = Ability. + issues_readable_by_user(issues.values, user).to_set - issue ? can?(user, :read_issue, issue) : false + nodes.select do |node| + readable_issues.include?(issue_for_node(issues, node)) end end diff --git a/lib/banzai/renderer.rb b/lib/banzai/renderer.rb index 910687a7b6a..a4ae27eefd8 100644 --- a/lib/banzai/renderer.rb +++ b/lib/banzai/renderer.rb @@ -1,5 +1,7 @@ module Banzai module Renderer + extend self + # Convert a Markdown String into an HTML-safe String of HTML # # Note that while the returned HTML will have been sanitized of dangerous @@ -14,7 +16,7 @@ module Banzai # context - Hash of context options passed to our HTML Pipeline # # Returns an HTML-safe String - def self.render(text, context = {}) + def render(text, context = {}) cache_key = context.delete(:cache_key) cache_key = full_cache_key(cache_key, context[:pipeline]) @@ -52,7 +54,7 @@ module Banzai # texts_and_contexts # => [{ text: '### Hello', # context: { cache_key: [note, :note] } }] - def self.cache_collection_render(texts_and_contexts) + def cache_collection_render(texts_and_contexts) items_collection = texts_and_contexts.each_with_index do |item, index| context = item[:context] cache_key = full_cache_multi_key(context.delete(:cache_key), context[:pipeline]) @@ -81,7 +83,7 @@ module Banzai items_collection.map { |item| item[:rendered] } end - def self.render_result(text, context = {}) + def render_result(text, context = {}) text = Pipeline[:pre_process].to_html(text, context) if text Pipeline[context[:pipeline]].call(text, context) @@ -100,7 +102,7 @@ module Banzai # :user - User object # # Returns an HTML-safe String - def self.post_process(html, context) + def post_process(html, context) context = Pipeline[context[:pipeline]].transform_context(context) pipeline = Pipeline[:post_process] @@ -113,7 +115,7 @@ module Banzai private - def self.cacheless_render(text, context = {}) + def cacheless_render(text, context = {}) Gitlab::Metrics.measure(:banzai_cacheless_render) do result = render_result(text, context) @@ -126,7 +128,7 @@ module Banzai end end - def self.full_cache_key(cache_key, pipeline_name) + def full_cache_key(cache_key, pipeline_name) return unless cache_key ["banzai", *cache_key, pipeline_name || :full] end @@ -134,7 +136,7 @@ module Banzai # To map Rails.cache.read_multi results we need to know the Rails.cache.expanded_key. # Other option will be to generate stringified keys on our side and don't delegate to Rails.cache.expanded_key # method. - def self.full_cache_multi_key(cache_key, pipeline_name) + def full_cache_multi_key(cache_key, pipeline_name) return unless cache_key Rails.cache.send(:expanded_key, full_cache_key(cache_key, pipeline_name)) end diff --git a/lib/ci/gitlab_ci_yaml_processor.rb b/lib/ci/gitlab_ci_yaml_processor.rb index 83afed9f49f..a2e8bd22a52 100644 --- a/lib/ci/gitlab_ci_yaml_processor.rb +++ b/lib/ci/gitlab_ci_yaml_processor.rb @@ -4,21 +4,11 @@ module Ci include Gitlab::Ci::Config::Node::LegacyValidationHelpers - DEFAULT_STAGE = 'test' - ALLOWED_YAML_KEYS = [:before_script, :after_script, :image, :services, :types, :stages, :variables, :cache] - ALLOWED_JOB_KEYS = [:tags, :script, :only, :except, :type, :image, :services, - :allow_failure, :type, :stage, :when, :artifacts, :cache, - :dependencies, :before_script, :after_script, :variables, - :environment] - ALLOWED_CACHE_KEYS = [:key, :untracked, :paths] - ALLOWED_ARTIFACTS_KEYS = [:name, :untracked, :paths, :when, :expire_in] - attr_reader :path, :cache, :stages def initialize(config, path = nil) @ci_config = Gitlab::Ci::Config.new(config) @config = @ci_config.to_hash - @path = path unless @ci_config.valid? @@ -26,7 +16,6 @@ module Ci end initial_parsing - validate! rescue Gitlab::Ci::Config::Loader::FormatError => e raise ValidationError, e.message end @@ -73,7 +62,7 @@ module Ci # - before script should be a concatenated command commands: [job[:before_script] || @before_script, job[:script]].flatten.compact.join("\n"), tag_list: job[:tags] || [], - name: name, + name: job[:name], allow_failure: job[:allow_failure] || false, when: job[:when] || 'on_success', environment: job[:environment], @@ -92,6 +81,9 @@ module Ci private def initial_parsing + ## + # Global config + # @before_script = @ci_config.before_script @image = @ci_config.image @after_script = @ci_config.after_script @@ -100,34 +92,28 @@ module Ci @stages = @ci_config.stages @cache = @ci_config.cache - @jobs = {} - - @config.except!(*ALLOWED_YAML_KEYS) - @config.each { |name, param| add_job(name, param) } - - raise ValidationError, "Please define at least one job" if @jobs.none? - end - - def add_job(name, job) - return if name.to_s.start_with?('.') + ## + # Jobs + # + @jobs = @ci_config.jobs - raise ValidationError, "Unknown parameter: #{name}" unless job.is_a?(Hash) && job.has_key?(:script) + @jobs.each do |name, job| + # logical validation for job - stage = job[:stage] || job[:type] || DEFAULT_STAGE - @jobs[name] = { stage: stage }.merge(job) + validate_job_stage!(name, job) + validate_job_dependencies!(name, job) + end end def yaml_variables(name) - variables = global_variables.merge(job_variables(name)) + variables = (@variables || {}) + .merge(job_variables(name)) + variables.map do |key, value| { key: key, value: value, public: true } end end - def global_variables - @variables || {} - end - def job_variables(name) job = @jobs[name.to_sym] return {} unless job @@ -135,154 +121,16 @@ module Ci job[:variables] || {} end - def validate! - @jobs.each do |name, job| - validate_job!(name, job) - end - - true - end - - def validate_job!(name, job) - validate_job_name!(name) - validate_job_keys!(name, job) - validate_job_types!(name, job) - validate_job_script!(name, job) - - validate_job_stage!(name, job) if job[:stage] - validate_job_variables!(name, job) if job[:variables] - validate_job_cache!(name, job) if job[:cache] - validate_job_artifacts!(name, job) if job[:artifacts] - validate_job_dependencies!(name, job) if job[:dependencies] - end - - def validate_job_name!(name) - if name.blank? || !validate_string(name) - raise ValidationError, "job name should be non-empty string" - end - end - - def validate_job_keys!(name, job) - job.keys.each do |key| - unless ALLOWED_JOB_KEYS.include? key - raise ValidationError, "#{name} job: unknown parameter #{key}" - end - end - end - - def validate_job_types!(name, job) - if job[:image] && !validate_string(job[:image]) - raise ValidationError, "#{name} job: image should be a string" - end - - if job[:services] && !validate_array_of_strings(job[:services]) - raise ValidationError, "#{name} job: services should be an array of strings" - end - - if job[:tags] && !validate_array_of_strings(job[:tags]) - raise ValidationError, "#{name} job: tags parameter should be an array of strings" - end - - if job[:only] && !validate_array_of_strings_or_regexps(job[:only]) - raise ValidationError, "#{name} job: only parameter should be an array of strings or regexps" - end - - if job[:except] && !validate_array_of_strings_or_regexps(job[:except]) - raise ValidationError, "#{name} job: except parameter should be an array of strings or regexps" - end - - if job[:allow_failure] && !validate_boolean(job[:allow_failure]) - raise ValidationError, "#{name} job: allow_failure parameter should be an boolean" - end - - if job[:when] && !job[:when].in?(%w[on_success on_failure always manual]) - raise ValidationError, "#{name} job: when parameter should be on_success, on_failure, always or manual" - end - - if job[:environment] && !validate_environment(job[:environment]) - raise ValidationError, "#{name} job: environment parameter #{Gitlab::Regex.environment_name_regex_message}" - end - end - - def validate_job_script!(name, job) - if !validate_string(job[:script]) && !validate_array_of_strings(job[:script]) - raise ValidationError, "#{name} job: script should be a string or an array of a strings" - end - - if job[:before_script] && !validate_array_of_strings(job[:before_script]) - raise ValidationError, "#{name} job: before_script should be an array of strings" - end - - if job[:after_script] && !validate_array_of_strings(job[:after_script]) - raise ValidationError, "#{name} job: after_script should be an array of strings" - end - end - def validate_job_stage!(name, job) + return unless job[:stage] + unless job[:stage].is_a?(String) && job[:stage].in?(@stages) raise ValidationError, "#{name} job: stage parameter should be #{@stages.join(", ")}" end end - def validate_job_variables!(name, job) - unless validate_variables(job[:variables]) - raise ValidationError, - "#{name} job: variables should be a map of key-value strings" - end - end - - def validate_job_cache!(name, job) - job[:cache].keys.each do |key| - unless ALLOWED_CACHE_KEYS.include? key - raise ValidationError, "#{name} job: cache unknown parameter #{key}" - end - end - - if job[:cache][:key] && !validate_string(job[:cache][:key]) - raise ValidationError, "#{name} job: cache:key parameter should be a string" - end - - if job[:cache][:untracked] && !validate_boolean(job[:cache][:untracked]) - raise ValidationError, "#{name} job: cache:untracked parameter should be an boolean" - end - - if job[:cache][:paths] && !validate_array_of_strings(job[:cache][:paths]) - raise ValidationError, "#{name} job: cache:paths parameter should be an array of strings" - end - end - - def validate_job_artifacts!(name, job) - job[:artifacts].keys.each do |key| - unless ALLOWED_ARTIFACTS_KEYS.include? key - raise ValidationError, "#{name} job: artifacts unknown parameter #{key}" - end - end - - if job[:artifacts][:name] && !validate_string(job[:artifacts][:name]) - raise ValidationError, "#{name} job: artifacts:name parameter should be a string" - end - - if job[:artifacts][:untracked] && !validate_boolean(job[:artifacts][:untracked]) - raise ValidationError, "#{name} job: artifacts:untracked parameter should be an boolean" - end - - if job[:artifacts][:paths] && !validate_array_of_strings(job[:artifacts][:paths]) - raise ValidationError, "#{name} job: artifacts:paths parameter should be an array of strings" - end - - if job[:artifacts][:when] && !job[:artifacts][:when].in?(%w[on_success on_failure always]) - raise ValidationError, "#{name} job: artifacts:when parameter should be on_success, on_failure or always" - end - - if job[:artifacts][:expire_in] && !validate_duration(job[:artifacts][:expire_in]) - raise ValidationError, "#{name} job: artifacts:expire_in parameter should be a duration" - end - end - def validate_job_dependencies!(name, job) - unless validate_array_of_strings(job[:dependencies]) - raise ValidationError, "#{name} job: dependencies parameter should be an array of strings" - end + return unless job[:dependencies] stage_index = @stages.index(job[:stage]) diff --git a/lib/gitlab/ci/config.rb b/lib/gitlab/ci/config.rb index e6cc1529760..ae82c0db3f1 100644 --- a/lib/gitlab/ci/config.rb +++ b/lib/gitlab/ci/config.rb @@ -8,7 +8,7 @@ module Gitlab # Temporary delegations that should be removed after refactoring # delegate :before_script, :image, :services, :after_script, :variables, - :stages, :cache, to: :@global + :stages, :cache, :jobs, to: :@global def initialize(config) @config = Loader.new(config).load! diff --git a/lib/gitlab/ci/config/node/artifacts.rb b/lib/gitlab/ci/config/node/artifacts.rb new file mode 100644 index 00000000000..844bd2fe998 --- /dev/null +++ b/lib/gitlab/ci/config/node/artifacts.rb @@ -0,0 +1,35 @@ +module Gitlab + module Ci + class Config + module Node + ## + # Entry that represents a configuration of job artifacts. + # + class Artifacts < Entry + include Validatable + include Attributable + + ALLOWED_KEYS = %i[name untracked paths when expire_in] + + attributes ALLOWED_KEYS + + validations do + validates :config, type: Hash + validates :config, allowed_keys: ALLOWED_KEYS + + with_options allow_nil: true do + validates :name, type: String + validates :untracked, boolean: true + validates :paths, array_of_strings: true + validates :when, + inclusion: { in: %w[on_success on_failure always], + message: 'should be on_success, on_failure ' \ + 'or always' } + validates :expire_in, duration: true + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/attributable.rb b/lib/gitlab/ci/config/node/attributable.rb new file mode 100644 index 00000000000..221b666f9f6 --- /dev/null +++ b/lib/gitlab/ci/config/node/attributable.rb @@ -0,0 +1,23 @@ +module Gitlab + module Ci + class Config + module Node + module Attributable + extend ActiveSupport::Concern + + class_methods do + def attributes(*attributes) + attributes.flatten.each do |attribute| + define_method(attribute) do + return unless config.is_a?(Hash) + + config[attribute] + end + end + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/cache.rb b/lib/gitlab/ci/config/node/cache.rb index cdf8ba2e35d..b4bda2841ac 100644 --- a/lib/gitlab/ci/config/node/cache.rb +++ b/lib/gitlab/ci/config/node/cache.rb @@ -8,6 +8,12 @@ module Gitlab class Cache < Entry include Configurable + ALLOWED_KEYS = %i[key untracked paths] + + validations do + validates :config, allowed_keys: ALLOWED_KEYS + end + node :key, Node::Key, description: 'Cache key used to define a cache affinity.' @@ -16,10 +22,6 @@ module Gitlab node :paths, Node::Paths, description: 'Specify which paths should be cached across builds.' - - validations do - validates :config, allowed_keys: true - end end end end diff --git a/lib/gitlab/ci/config/node/commands.rb b/lib/gitlab/ci/config/node/commands.rb new file mode 100644 index 00000000000..d7657ae314b --- /dev/null +++ b/lib/gitlab/ci/config/node/commands.rb @@ -0,0 +1,33 @@ +module Gitlab + module Ci + class Config + module Node + ## + # Entry that represents a job script. + # + class Commands < Entry + include Validatable + + validations do + include LegacyValidationHelpers + + validate do + unless string_or_array_of_strings?(config) + errors.add(:config, + 'should be a string or an array of strings') + end + end + + def string_or_array_of_strings?(field) + validate_string(field) || validate_array_of_strings(field) + end + end + + def value + Array(@config) + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/configurable.rb b/lib/gitlab/ci/config/node/configurable.rb index 37936fc8242..2de82d40c9d 100644 --- a/lib/gitlab/ci/config/node/configurable.rb +++ b/lib/gitlab/ci/config/node/configurable.rb @@ -25,10 +25,14 @@ module Gitlab private - def create_node(key, factory) - factory.with(value: @config[key], key: key, parent: self) + def compose! + self.class.nodes.each do |key, factory| + factory + .value(@config[key]) + .with(key: key, parent: self) - factory.create! + @entries[key] = factory.create! + end end class_methods do @@ -36,24 +40,25 @@ module Gitlab Hash[(@nodes || {}).map { |key, factory| [key, factory.dup] }] end - private + private # rubocop:disable Lint/UselessAccessModifier - def node(symbol, entry_class, metadata) - factory = Node::Factory.new(entry_class) + def node(key, node, metadata) + factory = Node::Factory.new(node) .with(description: metadata[:description]) - (@nodes ||= {}).merge!(symbol.to_sym => factory) + (@nodes ||= {}).merge!(key.to_sym => factory) end def helpers(*nodes) nodes.each do |symbol| define_method("#{symbol}_defined?") do - @nodes[symbol].try(:defined?) + @entries[symbol].specified? if @entries[symbol] end define_method("#{symbol}_value") do - raise Entry::InvalidError unless valid? - @nodes[symbol].try(:value) + return unless @entries[symbol] && @entries[symbol].valid? + + @entries[symbol].value end alias_method symbol.to_sym, "#{symbol}_value".to_sym diff --git a/lib/gitlab/ci/config/node/entry.rb b/lib/gitlab/ci/config/node/entry.rb index 9e79e170a4f..0c782c422b5 100644 --- a/lib/gitlab/ci/config/node/entry.rb +++ b/lib/gitlab/ci/config/node/entry.rb @@ -8,30 +8,31 @@ module Gitlab class Entry class InvalidError < StandardError; end - attr_reader :config + attr_reader :config, :metadata attr_accessor :key, :parent, :description - def initialize(config) + def initialize(config, **metadata) @config = config - @nodes = {} + @metadata = metadata + @entries = {} + @validator = self.class.validator.new(self) - @validator.validate + @validator.validate(:new) end def process! - return if leaf? return unless valid? compose! - process_nodes! + descendants.each(&:process!) end - def nodes - @nodes.values + def leaf? + @entries.none? end - def leaf? - self.class.nodes.none? + def descendants + @entries.values end def ancestors @@ -43,27 +44,30 @@ module Gitlab end def errors - @validator.messages + nodes.flat_map(&:errors) + @validator.messages + descendants.flat_map(&:errors) end def value if leaf? @config else - defined = @nodes.select { |_key, value| value.defined? } - Hash[defined.map { |key, node| [key, node.value] }] + meaningful = @entries.select do |_key, value| + value.specified? && value.relevant? + end + + Hash[meaningful.map { |key, entry| [key, entry.value] }] end end - def defined? + def specified? true end - def self.default + def relevant? + true end - def self.nodes - {} + def self.default end def self.validator @@ -73,17 +77,6 @@ module Gitlab private def compose! - self.class.nodes.each do |key, essence| - @nodes[key] = create_node(key, essence) - end - end - - def process_nodes! - nodes.each(&:process!) - end - - def create_node(key, essence) - raise NotImplementedError end end end diff --git a/lib/gitlab/ci/config/node/factory.rb b/lib/gitlab/ci/config/node/factory.rb index 5919a283283..707b052e6a8 100644 --- a/lib/gitlab/ci/config/node/factory.rb +++ b/lib/gitlab/ci/config/node/factory.rb @@ -10,35 +10,60 @@ module Gitlab def initialize(node) @node = node + @metadata = {} @attributes = {} end + def value(value) + @value = value + self + end + + def metadata(metadata) + @metadata.merge!(metadata) + self + end + def with(attributes) @attributes.merge!(attributes) self end def create! - raise InvalidFactory unless @attributes.has_key?(:value) + raise InvalidFactory unless defined?(@value) - fabricate.tap do |entry| - entry.key = @attributes[:key] - entry.parent = @attributes[:parent] - entry.description = @attributes[:description] + ## + # We assume that unspecified entry is undefined. + # See issue #18775. + # + if @value.nil? + Node::Undefined.new( + fabricate_undefined + ) + else + fabricate(@node, @value) end end private - def fabricate + def fabricate_undefined ## - # We assume that unspecified entry is undefined. - # See issue #18775. + # If node has a default value we fabricate concrete node + # with default value. # - if @attributes[:value].nil? - Node::Undefined.new(@node) + if @node.default.nil? + fabricate(Node::Null) else - @node.new(@attributes[:value]) + fabricate(@node, @node.default) + end + end + + def fabricate(node, value = nil) + node.new(value, @metadata).tap do |entry| + entry.key = @attributes[:key] + entry.parent = @attributes[:parent] + entry.description = @attributes[:description] end end end diff --git a/lib/gitlab/ci/config/node/global.rb b/lib/gitlab/ci/config/node/global.rb index f92e1eccbcf..ccd539fb003 100644 --- a/lib/gitlab/ci/config/node/global.rb +++ b/lib/gitlab/ci/config/node/global.rb @@ -34,10 +34,36 @@ module Gitlab description: 'Configure caching between build jobs.' helpers :before_script, :image, :services, :after_script, - :variables, :stages, :types, :cache + :variables, :stages, :types, :cache, :jobs - def stages - stages_defined? ? stages_value : types_value + private + + def compose! + super + + compose_jobs! + compose_deprecated_entries! + end + + def compose_jobs! + factory = Node::Factory.new(Node::Jobs) + .value(@config.except(*self.class.nodes.keys)) + .with(key: :jobs, parent: self, + description: 'Jobs definition for this pipeline') + + @entries[:jobs] = factory.create! + end + + def compose_deprecated_entries! + ## + # Deprecated `:types` key workaround - if types are defined and + # stages are not defined we use types definition as stages. + # + if types_defined? && !stages_defined? + @entries[:stages] = @entries[:types] + end + + @entries.delete(:types) end end end diff --git a/lib/gitlab/ci/config/node/hidden_job.rb b/lib/gitlab/ci/config/node/hidden_job.rb new file mode 100644 index 00000000000..073044b66f8 --- /dev/null +++ b/lib/gitlab/ci/config/node/hidden_job.rb @@ -0,0 +1,23 @@ +module Gitlab + module Ci + class Config + module Node + ## + # Entry that represents a hidden CI/CD job. + # + class HiddenJob < Entry + include Validatable + + validations do + validates :config, type: Hash + validates :config, presence: true + end + + def relevant? + false + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/job.rb b/lib/gitlab/ci/config/node/job.rb new file mode 100644 index 00000000000..e84737acbb9 --- /dev/null +++ b/lib/gitlab/ci/config/node/job.rb @@ -0,0 +1,123 @@ +module Gitlab + module Ci + class Config + module Node + ## + # Entry that represents a concrete CI/CD job. + # + class Job < Entry + include Configurable + include Attributable + + ALLOWED_KEYS = %i[tags script only except type image services allow_failure + type stage when artifacts cache dependencies before_script + after_script variables environment] + + attributes :tags, :allow_failure, :when, :environment, :dependencies + + validations do + validates :config, allowed_keys: ALLOWED_KEYS + + validates :config, presence: true + validates :name, presence: true + validates :name, type: Symbol + + with_options allow_nil: true do + validates :tags, array_of_strings: true + validates :allow_failure, boolean: true + validates :when, + inclusion: { in: %w[on_success on_failure always manual], + message: 'should be on_success, on_failure, ' \ + 'always or manual' } + validates :environment, + type: { + with: String, + message: Gitlab::Regex.environment_name_regex_message } + validates :environment, + format: { + with: Gitlab::Regex.environment_name_regex, + message: Gitlab::Regex.environment_name_regex_message } + + validates :dependencies, array_of_strings: true + end + end + + node :before_script, Script, + description: 'Global before script overridden in this job.' + + node :script, Commands, + description: 'Commands that will be executed in this job.' + + node :stage, Stage, + description: 'Pipeline stage this job will be executed into.' + + node :type, Stage, + description: 'Deprecated: stage this job will be executed into.' + + node :after_script, Script, + description: 'Commands that will be executed when finishing job.' + + node :cache, Cache, + description: 'Cache definition for this job.' + + node :image, Image, + description: 'Image that will be used to execute this job.' + + node :services, Services, + description: 'Services that will be used to execute this job.' + + node :only, Trigger, + description: 'Refs policy this job will be executed for.' + + node :except, Trigger, + description: 'Refs policy this job will be executed for.' + + node :variables, Variables, + description: 'Environment variables available for this job.' + + node :artifacts, Artifacts, + description: 'Artifacts configuration for this job.' + + helpers :before_script, :script, :stage, :type, :after_script, + :cache, :image, :services, :only, :except, :variables, + :artifacts + + def name + @metadata[:name] + end + + def value + @config.merge(to_hash.compact) + end + + private + + def to_hash + { name: name, + before_script: before_script, + script: script, + image: image, + services: services, + stage: stage, + cache: cache, + only: only, + except: except, + variables: variables_defined? ? variables : nil, + artifacts: artifacts, + after_script: after_script } + end + + def compose! + super + + if type_defined? && !stage_defined? + @entries[:stage] = @entries[:type] + end + + @entries.delete(:type) + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/jobs.rb b/lib/gitlab/ci/config/node/jobs.rb new file mode 100644 index 00000000000..51683c82ceb --- /dev/null +++ b/lib/gitlab/ci/config/node/jobs.rb @@ -0,0 +1,48 @@ +module Gitlab + module Ci + class Config + module Node + ## + # Entry that represents a set of jobs. + # + class Jobs < Entry + include Validatable + + validations do + validates :config, type: Hash + + validate do + unless has_visible_job? + errors.add(:config, 'should contain at least one visible job') + end + end + + def has_visible_job? + config.any? { |name, _| !hidden?(name) } + end + end + + def hidden?(name) + name.to_s.start_with?('.') + end + + private + + def compose! + @config.each do |name, config| + node = hidden?(name) ? Node::HiddenJob : Node::Job + + factory = Node::Factory.new(node) + .value(config || {}) + .metadata(name: name) + .with(key: name, parent: self, + description: "#{name} job definition.") + + @entries[name] = factory.create! + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/legacy_validation_helpers.rb b/lib/gitlab/ci/config/node/legacy_validation_helpers.rb index 4d9a508796a..0c291efe6a5 100644 --- a/lib/gitlab/ci/config/node/legacy_validation_helpers.rb +++ b/lib/gitlab/ci/config/node/legacy_validation_helpers.rb @@ -41,10 +41,6 @@ module Gitlab false end - def validate_environment(value) - value.is_a?(String) && value =~ Gitlab::Regex.environment_name_regex - end - def validate_boolean(value) value.in?([true, false]) end diff --git a/lib/gitlab/ci/config/node/null.rb b/lib/gitlab/ci/config/node/null.rb new file mode 100644 index 00000000000..88a5f53f13c --- /dev/null +++ b/lib/gitlab/ci/config/node/null.rb @@ -0,0 +1,34 @@ +module Gitlab + module Ci + class Config + module Node + ## + # This class represents an undefined node. + # + # Implements the Null Object pattern. + # + class Null < Entry + def value + nil + end + + def valid? + true + end + + def errors + [] + end + + def specified? + false + end + + def relevant? + false + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/stage.rb b/lib/gitlab/ci/config/node/stage.rb new file mode 100644 index 00000000000..cbc97641f5a --- /dev/null +++ b/lib/gitlab/ci/config/node/stage.rb @@ -0,0 +1,22 @@ +module Gitlab + module Ci + class Config + module Node + ## + # Entry that represents a stage for a job. + # + class Stage < Entry + include Validatable + + validations do + validates :config, type: String + end + + def self.default + 'test' + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/trigger.rb b/lib/gitlab/ci/config/node/trigger.rb new file mode 100644 index 00000000000..d8b31975088 --- /dev/null +++ b/lib/gitlab/ci/config/node/trigger.rb @@ -0,0 +1,26 @@ +module Gitlab + module Ci + class Config + module Node + ## + # Entry that represents a trigger policy for the job. + # + class Trigger < Entry + include Validatable + + validations do + include LegacyValidationHelpers + + validate :array_of_strings_or_regexps + + def array_of_strings_or_regexps + unless validate_array_of_strings_or_regexps(config) + errors.add(:config, 'should be an array of strings or regexps') + end + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/node/undefined.rb b/lib/gitlab/ci/config/node/undefined.rb index 699605e1e3a..45fef8c3ae5 100644 --- a/lib/gitlab/ci/config/node/undefined.rb +++ b/lib/gitlab/ci/config/node/undefined.rb @@ -3,24 +3,13 @@ module Gitlab class Config module Node ## - # This class represents an undefined entry node. + # This class represents an unspecified entry node. # - # It takes original entry class as configuration and returns default - # value of original entry as self value. + # It decorates original entry adding method that indicates it is + # unspecified. # - # - class Undefined < Entry - include Validatable - - validations do - validates :config, type: Class - end - - def value - @config.default - end - - def defined? + class Undefined < SimpleDelegator + def specified? false end end diff --git a/lib/gitlab/ci/config/node/validator.rb b/lib/gitlab/ci/config/node/validator.rb index 758a6cf4356..43c7e102b50 100644 --- a/lib/gitlab/ci/config/node/validator.rb +++ b/lib/gitlab/ci/config/node/validator.rb @@ -21,18 +21,19 @@ module Gitlab 'Validator' end - def unknown_keys - return [] unless config.is_a?(Hash) - - config.keys - @node.class.nodes.keys - end - private def location predecessors = ancestors.map(&:key).compact - current = key || @node.class.name.demodulize.underscore - predecessors.append(current).join(':') + predecessors.append(key_name).join(':') + end + + def key_name + if key.blank? + @node.class.name.demodulize.underscore.humanize + else + key + end end end end diff --git a/lib/gitlab/ci/config/node/validators.rb b/lib/gitlab/ci/config/node/validators.rb index 7b2f57990b5..e20908ad3cb 100644 --- a/lib/gitlab/ci/config/node/validators.rb +++ b/lib/gitlab/ci/config/node/validators.rb @@ -5,10 +5,11 @@ module Gitlab module Validators class AllowedKeysValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - if record.unknown_keys.any? - unknown_list = record.unknown_keys.join(', ') - record.errors.add(:config, - "contains unknown keys: #{unknown_list}") + unknown_keys = record.config.try(:keys).to_a - options[:in] + + if unknown_keys.any? + record.errors.add(:config, 'contains unknown keys: ' + + unknown_keys.join(', ')) end end end @@ -33,6 +34,16 @@ module Gitlab end end + class DurationValidator < ActiveModel::EachValidator + include LegacyValidationHelpers + + def validate_each(record, attribute, value) + unless validate_duration(value) + record.errors.add(attribute, 'should be a duration') + end + end + end + class KeyValidator < ActiveModel::EachValidator include LegacyValidationHelpers @@ -49,7 +60,8 @@ module Gitlab raise unless type.is_a?(Class) unless value.is_a?(type) - record.errors.add(attribute, "should be a #{type.name}") + message = options[:message] || "should be a #{type.name}" + record.errors.add(attribute, message) end end end diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 078609c86f1..55b8f888d53 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -55,12 +55,12 @@ module Gitlab end end - private - def self.connection ActiveRecord::Base.connection end + private_class_method :connection + def self.database_version row = connection.execute("SELECT VERSION()").first @@ -70,5 +70,7 @@ module Gitlab row.first end end + + private_class_method :database_version end end diff --git a/lib/gitlab/diff/inline_diff.rb b/lib/gitlab/diff/inline_diff.rb index 28ad637fda4..55708d42161 100644 --- a/lib/gitlab/diff/inline_diff.rb +++ b/lib/gitlab/diff/inline_diff.rb @@ -19,24 +19,6 @@ module Gitlab attr_accessor :old_line, :new_line, :offset - def self.for_lines(lines) - changed_line_pairs = self.find_changed_line_pairs(lines) - - inline_diffs = [] - - changed_line_pairs.each do |old_index, new_index| - old_line = lines[old_index] - new_line = lines[new_index] - - old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs - - inline_diffs[old_index] = old_diffs - inline_diffs[new_index] = new_diffs - end - - inline_diffs - end - def initialize(old_line, new_line, offset: 0) @old_line = old_line[offset..-1] @new_line = new_line[offset..-1] @@ -63,32 +45,54 @@ module Gitlab [old_diffs, new_diffs] end - private + class << self + def for_lines(lines) + changed_line_pairs = find_changed_line_pairs(lines) - # Finds pairs of old/new line pairs that represent the same line that changed - def self.find_changed_line_pairs(lines) - # Prefixes of all diff lines, indicating their types - # For example: `" - + -+ ---+++ --+ -++"` - line_prefixes = lines.each_with_object("") { |line, s| s << line[0] }.gsub(/[^ +-]/, ' ') + inline_diffs = [] - changed_line_pairs = [] - line_prefixes.scan(LINE_PAIRS_PATTERN) do - # For `"---+++"`, `begin_index == 0`, `end_index == 6` - begin_index, end_index = Regexp.last_match.offset(:del_ins) + changed_line_pairs.each do |old_index, new_index| + old_line = lines[old_index] + new_line = lines[new_index] - # For `"---+++"`, `changed_line_count == 3` - changed_line_count = (end_index - begin_index) / 2 + old_diffs, new_diffs = new(old_line, new_line, offset: 1).inline_diffs - halfway_index = begin_index + changed_line_count - (begin_index...halfway_index).each do |i| - # For `"---+++"`, index 1 maps to 1 + 3 = 4 - changed_line_pairs << [i, i + changed_line_count] + inline_diffs[old_index] = old_diffs + inline_diffs[new_index] = new_diffs end + + inline_diffs end - changed_line_pairs + private + + # Finds pairs of old/new line pairs that represent the same line that changed + def find_changed_line_pairs(lines) + # Prefixes of all diff lines, indicating their types + # For example: `" - + -+ ---+++ --+ -++"` + line_prefixes = lines.each_with_object("") { |line, s| s << line[0] }.gsub(/[^ +-]/, ' ') + + changed_line_pairs = [] + line_prefixes.scan(LINE_PAIRS_PATTERN) do + # For `"---+++"`, `begin_index == 0`, `end_index == 6` + begin_index, end_index = Regexp.last_match.offset(:del_ins) + + # For `"---+++"`, `changed_line_count == 3` + changed_line_count = (end_index - begin_index) / 2 + + halfway_index = begin_index + changed_line_count + (begin_index...halfway_index).each do |i| + # For `"---+++"`, index 1 maps to 1 + 3 = 4 + changed_line_pairs << [i, i + changed_line_count] + end + end + + changed_line_pairs + end end + private + def longest_common_prefix(a, b) max_length = [a.length, b.length].max diff --git a/lib/gitlab/import_export/command_line_util.rb b/lib/gitlab/import_export/command_line_util.rb index 5dd0e34c18e..e522a0fc8f6 100644 --- a/lib/gitlab/import_export/command_line_util.rb +++ b/lib/gitlab/import_export/command_line_util.rb @@ -17,6 +17,10 @@ module Gitlab execute(%W(#{git_bin_path} clone --bare #{bundle_path} #{repo_path})) end + def git_restore_hooks + execute(%W(#{Gitlab.config.gitlab_shell.path}/bin/create-hooks) + repository_storage_paths_args) + end + private def tar_with_options(archive:, dir:, options:) @@ -45,6 +49,10 @@ module Gitlab FileUtils.copy_entry(source, destination) true end + + def repository_storage_paths_args + Gitlab.config.repositories.storages.values + end end end end diff --git a/lib/gitlab/import_export/repo_restorer.rb b/lib/gitlab/import_export/repo_restorer.rb index f84de652a57..6d9379acf25 100644 --- a/lib/gitlab/import_export/repo_restorer.rb +++ b/lib/gitlab/import_export/repo_restorer.rb @@ -14,7 +14,7 @@ module Gitlab FileUtils.mkdir_p(path_to_repo) - git_unbundle(repo_path: path_to_repo, bundle_path: @path_to_bundle) + git_unbundle(repo_path: path_to_repo, bundle_path: @path_to_bundle) && repo_restore_hooks rescue => e @shared.error(e) false @@ -29,6 +29,16 @@ module Gitlab def path_to_repo @project.repository.path_to_repo end + + def repo_restore_hooks + return true if wiki? + + git_restore_hooks + end + + def wiki? + @project.class.name == 'ProjectWiki' + end end end end diff --git a/lib/gitlab/metrics.rb b/lib/gitlab/metrics.rb index 86a5b9a201a..081576a440c 100644 --- a/lib/gitlab/metrics.rb +++ b/lib/gitlab/metrics.rb @@ -141,10 +141,10 @@ module Gitlab end end - private - def self.current_transaction Transaction.current end + + private_class_method :current_transaction end end diff --git a/lib/gitlab/themes.rb b/lib/gitlab/themes.rb index 83f91de810c..d4020af76f9 100644 --- a/lib/gitlab/themes.rb +++ b/lib/gitlab/themes.rb @@ -2,6 +2,8 @@ module Gitlab # Module containing GitLab's application theme definitions and helper methods # for accessing them. module Themes + extend self + # Theme ID used when no `default_theme` configuration setting is provided. APPLICATION_DEFAULT = 2 @@ -22,7 +24,7 @@ module Gitlab # classes that might be applied to the `body` element # # Returns a String - def self.body_classes + def body_classes THEMES.collect(&:css_class).uniq.join(' ') end @@ -33,26 +35,26 @@ module Gitlab # id - Integer ID # # Returns a Theme - def self.by_id(id) + def by_id(id) THEMES.detect { |t| t.id == id } || default end # Returns the number of defined Themes - def self.count + def count THEMES.size end # Get the default Theme # # Returns a Theme - def self.default + def default by_id(default_id) end # Iterate through each Theme # # Yields the Theme object - def self.each(&block) + def each(&block) THEMES.each(&block) end @@ -61,7 +63,7 @@ module Gitlab # user - User record # # Returns a Theme - def self.for_user(user) + def for_user(user) if user by_id(user.theme_id) else @@ -71,7 +73,7 @@ module Gitlab private - def self.default_id + def default_id id = Gitlab.config.gitlab.default_theme.to_i # Prevent an invalid configuration setting from causing an infinite loop diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index c0f85e9b3a8..3a69027368f 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -29,8 +29,9 @@ module Gitlab def can_push_to_branch?(ref) return false unless user - if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref) - user.can?(:push_code_to_protected_branches, project) + if project.protected_branch?(ref) + access_levels = project.protected_branches.matching(ref).map(&:push_access_level) + access_levels.any? { |access_level| access_level.check_access(user) } else user.can?(:push_code, project) end @@ -39,8 +40,9 @@ module Gitlab def can_merge_to_branch?(ref) return false unless user - if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref) - user.can?(:push_code_to_protected_branches, project) + if project.protected_branch?(ref) + access_levels = project.protected_branches.matching(ref).map(&:merge_access_level) + access_levels.any? { |access_level| access_level.check_access(user) } else user.can?(:push_code, project) end diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index 3001d32e719..df902da86f8 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -24,15 +24,6 @@ describe Projects::CommitController do get :show, params.merge(extra_params) end - let(:project) { create(:project) } - - before do - user = create(:user) - project.team << [user, :master] - - sign_in(user) - end - context 'with valid id' do it 'responds with 200' do go(id: commit.id) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 5e19e403c6b..1b32d560b16 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -90,5 +90,21 @@ FactoryGirl.define do build.save! end end + + trait :artifacts_expired do + after(:create) do |build, _| + build.artifacts_file = + fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), + 'application/zip') + + build.artifacts_metadata = + fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), + 'application/x-gzip') + + build.artifacts_expire_at = 1.minute.ago + + build.save! + end + end end end diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb index 28ed8078157..5575852c2d7 100644 --- a/spec/factories/protected_branches.rb +++ b/spec/factories/protected_branches.rb @@ -2,5 +2,28 @@ FactoryGirl.define do factory :protected_branch do name project + + after(:create) do |protected_branch| + protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER) + protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER) + end + + trait :developers_can_push do + after(:create) do |protected_branch| + protected_branch.push_access_level.update!(access_level: Gitlab::Access::DEVELOPER) + end + end + + trait :developers_can_merge do + after(:create) do |protected_branch| + protected_branch.merge_access_level.update!(access_level: Gitlab::Access::DEVELOPER) + end + end + + trait :no_one_can_push do + after(:create) do |protected_branch| + protected_branch.push_access_level.update!(access_level: Gitlab::Access::NO_ACCESS) + end + end end end diff --git a/spec/features/pipelines_spec.rb b/spec/features/pipelines_spec.rb index 7f861db1969..377a9aba60d 100644 --- a/spec/features/pipelines_spec.rb +++ b/spec/features/pipelines_spec.rb @@ -116,9 +116,19 @@ describe "Pipelines" do it { expect(page).to have_link(with_artifacts.name) } end + context 'with artifacts expired' do + let!(:with_artifacts_expired) { create(:ci_build, :artifacts_expired, :success, pipeline: pipeline, name: 'rspec', stage: 'test') } + + before { visit namespace_project_pipelines_path(project.namespace, project) } + + it { expect(page).not_to have_selector('.build-artifacts') } + end + context 'without artifacts' do let!(:without_artifacts) { create(:ci_build, :success, pipeline: pipeline, name: 'rspec', stage: 'test') } + before { visit namespace_project_pipelines_path(project.namespace, project) } + it { expect(page).not_to have_selector('.build-artifacts') } end end diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index 2d1e3bbebe5..7835e1678ad 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -8,6 +8,7 @@ feature 'project import', feature: true, js: true do let(:file) { File.join(Rails.root, 'spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz') } let(:export_path) { "#{Dir::tmpdir}/import_file_spec" } let(:project) { Project.last } + let(:project_hook) { Gitlab::Git::Hook.new('post-receive', project.repository.path) } background do allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) @@ -37,7 +38,7 @@ feature 'project import', feature: true, js: true do expect(project).not_to be_nil expect(project.issues).not_to be_empty expect(project.merge_requests).not_to be_empty - expect(project.repo_exists?).to be true + expect(project_hook).to exist expect(wiki_exists?).to be true expect(project.import_status).to eq('finished') end diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index d94dee0c797..57734b33a44 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' feature 'Projected Branches', feature: true, js: true do + include WaitForAjax + let(:user) { create(:user, :admin) } let(:project) { create(:project) } @@ -81,4 +83,68 @@ feature 'Projected Branches', feature: true, js: true do end end end + + describe "access control" do + ProtectedBranch::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| + it "allows creating protected branches that #{access_type_name} can push to" do + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('master') + within('.new_protected_branch') do + find(".allowed-to-push").click + within(".dropdown.open .dropdown-menu") { click_on access_type_name } + end + click_on "Protect" + + expect(ProtectedBranch.count).to eq(1) + expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id) + end + + it "allows updating protected branches so that #{access_type_name} can push to them" do + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('master') + click_on "Protect" + + expect(ProtectedBranch.count).to eq(1) + + within(".protected-branches-list") do + find(".allowed-to-push").click + within('.dropdown-menu.push') { click_on access_type_name } + end + + wait_for_ajax + expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id) + end + end + + ProtectedBranch::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| + it "allows creating protected branches that #{access_type_name} can merge to" do + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('master') + within('.new_protected_branch') do + find(".allowed-to-merge").click + within(".dropdown.open .dropdown-menu") { click_on access_type_name } + end + click_on "Protect" + + expect(ProtectedBranch.count).to eq(1) + expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id) + end + + it "allows updating protected branches so that #{access_type_name} can merge to them" do + visit namespace_project_protected_branches_path(project.namespace, project) + set_protected_branch_name('master') + click_on "Protect" + + expect(ProtectedBranch.count).to eq(1) + + within(".protected-branches-list") do + find(".allowed-to-merge").click + within('.dropdown-menu.merge') { click_on access_type_name } + end + + wait_for_ajax + expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id) + end + end + end end diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index bd0108f9938..b2d6d59b1ee 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/spec/helpers/blob_helper_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe BlobHelper do + include TreeHelper + let(:blob_name) { 'test.lisp' } let(:no_context_content) { ":type \"assem\"))" } let(:blob_content) { "(make-pathname :defaults name\n#{no_context_content}" } @@ -65,4 +67,20 @@ describe BlobHelper do expect(sanitize_svg(blob).data).to eq(expected) end end + + describe "#edit_blob_link" do + let(:project) { create(:project) } + + before do + allow(self).to receive(:current_user).and_return(double) + end + + it 'verifies blob is text' do + expect(self).not_to receive(:blob_text_viewable?) + + button = edit_blob_link(project, 'refs/heads/master', 'README.md') + + expect(button).to start_with('<button') + end + end end diff --git a/spec/initializers/trusted_proxies_spec.rb b/spec/initializers/trusted_proxies_spec.rb index 52d5a7dffc9..290e47763eb 100644 --- a/spec/initializers/trusted_proxies_spec.rb +++ b/spec/initializers/trusted_proxies_spec.rb @@ -47,6 +47,12 @@ describe 'trusted_proxies', lib: true do expect(request.remote_ip).to eq('1.1.1.1') expect(request.ip).to eq('1.1.1.1') end + + it 'handles invalid ip addresses' do + request = stub_request('HTTP_X_FORWARDED_FOR' => '(null), 1.1.1.1:12345, 1.1.1.1') + expect(request.remote_ip).to eq('1.1.1.1') + expect(request.ip).to eq('1.1.1.1') + end end def stub_request(headers = {}) diff --git a/spec/lib/banzai/reference_parser/issue_parser_spec.rb b/spec/lib/banzai/reference_parser/issue_parser_spec.rb index 514c752546d..85cfe728b6a 100644 --- a/spec/lib/banzai/reference_parser/issue_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/issue_parser_spec.rb @@ -16,17 +16,17 @@ describe Banzai::ReferenceParser::IssueParser, lib: true do end it 'returns the nodes when the user can read the issue' do - expect(Ability.abilities).to receive(:allowed?). - with(user, :read_issue, issue). - and_return(true) + expect(Ability).to receive(:issues_readable_by_user). + with([issue], user). + and_return([issue]) expect(subject.nodes_visible_to_user(user, [link])).to eq([link]) end it 'returns an empty Array when the user can not read the issue' do - expect(Ability.abilities).to receive(:allowed?). - with(user, :read_issue, issue). - and_return(false) + expect(Ability).to receive(:issues_readable_by_user). + with([issue], user). + and_return([]) expect(subject.nodes_visible_to_user(user, [link])).to eq([]) end diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb index d20fd4ab7dd..61490555ff5 100644 --- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb +++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb @@ -162,7 +162,7 @@ module Ci shared_examples 'raises an error' do it do - expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'rspec job: only parameter should be an array of strings or regexps') + expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'jobs:rspec:only config should be an array of strings or regexps') end end @@ -318,7 +318,7 @@ module Ci shared_examples 'raises an error' do it do - expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'rspec job: except parameter should be an array of strings or regexps') + expect { processor }.to raise_error(GitlabCiYamlProcessor::ValidationError, 'jobs:rspec:except config should be an array of strings or regexps') end end @@ -559,7 +559,7 @@ module Ci it 'raises error' do expect { subject } .to raise_error(GitlabCiYamlProcessor::ValidationError, - /job: variables should be a map/) + /jobs:rspec:variables config should be a hash of key value pairs/) end end @@ -774,7 +774,7 @@ module Ci let(:environment) { 1 } it 'raises error' do - expect { builds }.to raise_error("deploy_to_production job: environment parameter #{Gitlab::Regex.environment_name_regex_message}") + expect { builds }.to raise_error("jobs:deploy_to_production environment #{Gitlab::Regex.environment_name_regex_message}") end end @@ -782,7 +782,7 @@ module Ci let(:environment) { 'production staging' } it 'raises error' do - expect { builds }.to raise_error("deploy_to_production job: environment parameter #{Gitlab::Regex.environment_name_regex_message}") + expect { builds }.to raise_error("jobs:deploy_to_production environment #{Gitlab::Regex.environment_name_regex_message}") end end end @@ -973,7 +973,7 @@ EOT config = YAML.dump({ rspec: { script: "test", tags: "mysql" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: tags parameter should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec tags should be an array of strings") end it "returns errors if before_script parameter is invalid" do @@ -987,7 +987,7 @@ EOT config = YAML.dump({ rspec: { script: "test", before_script: [10, "test"] } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: before_script should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:before_script config should be an array of strings") end it "returns errors if after_script parameter is invalid" do @@ -1001,7 +1001,7 @@ EOT config = YAML.dump({ rspec: { script: "test", after_script: [10, "test"] } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: after_script should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:after_script config should be an array of strings") end it "returns errors if image parameter is invalid" do @@ -1015,21 +1015,21 @@ EOT config = YAML.dump({ '' => { script: "test" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "job name should be non-empty string") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:job name can't be blank") end it "returns errors if job name is non-string" do config = YAML.dump({ 10 => { script: "test" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "job name should be non-empty string") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:10 name should be a symbol") end it "returns errors if job image parameter is invalid" do config = YAML.dump({ rspec: { script: "test", image: ["test"] } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: image should be a string") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:image config should be a string") end it "returns errors if services parameter is not an array" do @@ -1050,49 +1050,56 @@ EOT config = YAML.dump({ rspec: { script: "test", services: "test" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: services should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:services config should be an array of strings") end it "returns errors if job services parameter is not an array of strings" do config = YAML.dump({ rspec: { script: "test", services: [10, "test"] } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: services should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:services config should be an array of strings") end - it "returns errors if there are unknown parameters" do + it "returns error if job configuration is invalid" do config = YAML.dump({ extra: "bundle update" }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Unknown parameter: extra") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:extra config should be a hash") end it "returns errors if there are unknown parameters that are hashes, but doesn't have a script" do config = YAML.dump({ extra: { services: "test" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Unknown parameter: extra") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:extra:services config should be an array of strings") end it "returns errors if there are no jobs defined" do config = YAML.dump({ before_script: ["bundle update"] }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "Please define at least one job") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs config should contain at least one visible job") + end + + it "returns errors if there are no visible jobs defined" do + config = YAML.dump({ before_script: ["bundle update"], '.hidden'.to_sym => { script: 'ls' } }) + expect do + GitlabCiYamlProcessor.new(config, path) + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs config should contain at least one visible job") end it "returns errors if job allow_failure parameter is not an boolean" do config = YAML.dump({ rspec: { script: "test", allow_failure: "string" } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: allow_failure parameter should be an boolean") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec allow failure should be a boolean value") end it "returns errors if job stage is not a string" do config = YAML.dump({ rspec: { script: "test", type: 1 } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: stage parameter should be build, test, deploy") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:type config should be a string") end it "returns errors if job stage is not a pre-defined stage" do @@ -1141,49 +1148,49 @@ EOT config = YAML.dump({ rspec: { script: "test", when: 1 } }) expect do GitlabCiYamlProcessor.new(config, path) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: when parameter should be on_success, on_failure, always or manual") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec when should be on_success, on_failure, always or manual") end it "returns errors if job artifacts:name is not an a string" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { name: 1 } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:name parameter should be a string") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:artifacts name should be a string") end it "returns errors if job artifacts:when is not an a predefined value" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { when: 1 } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:when parameter should be on_success, on_failure or always") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:artifacts when should be on_success, on_failure or always") end it "returns errors if job artifacts:expire_in is not an a string" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { expire_in: 1 } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:expire_in parameter should be a duration") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:artifacts expire in should be a duration") end it "returns errors if job artifacts:expire_in is not an a valid duration" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { expire_in: "7 elephants" } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:expire_in parameter should be a duration") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:artifacts expire in should be a duration") end it "returns errors if job artifacts:untracked is not an array of strings" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { untracked: "string" } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:untracked parameter should be an boolean") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:artifacts untracked should be a boolean value") end it "returns errors if job artifacts:paths is not an array of strings" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", artifacts: { paths: "string" } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: artifacts:paths parameter should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:artifacts paths should be an array of strings") end it "returns errors if cache:untracked is not an array of strings" do @@ -1211,28 +1218,28 @@ EOT config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", cache: { key: 1 } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: cache:key parameter should be a string") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:cache:key config should be a string or symbol") end it "returns errors if job cache:untracked is not an array of strings" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", cache: { untracked: "string" } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: cache:untracked parameter should be an boolean") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:cache:untracked config should be a boolean value") end it "returns errors if job cache:paths is not an array of strings" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", cache: { paths: "string" } } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: cache:paths parameter should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec:cache:paths config should be an array of strings") end it "returns errors if job dependencies is not an array of strings" do config = YAML.dump({ types: ["build", "test"], rspec: { script: "test", dependencies: "string" } }) expect do GitlabCiYamlProcessor.new(config) - end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: dependencies parameter should be an array of strings") + end.to raise_error(GitlabCiYamlProcessor::ValidationError, "jobs:rspec dependencies should be an array of strings") end end diff --git a/spec/lib/gitlab/ci/config/node/artifacts_spec.rb b/spec/lib/gitlab/ci/config/node/artifacts_spec.rb new file mode 100644 index 00000000000..c09a0a9c793 --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/artifacts_spec.rb @@ -0,0 +1,45 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Artifacts do + let(:entry) { described_class.new(config) } + + describe 'validation' do + context 'when entry config value is correct' do + let(:config) { { paths: %w[public/] } } + + describe '#value' do + it 'returns artifacs configuration' do + expect(entry.value).to eq config + end + end + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when entry value is not correct' do + describe '#errors' do + context 'when value of attribute is invalid' do + let(:config) { { name: 10 } } + + it 'reports error' do + expect(entry.errors) + .to include 'artifacts name should be a string' + end + end + + context 'when there is an unknown key present' do + let(:config) { { test: 100 } } + + it 'reports error' do + expect(entry.errors) + .to include 'artifacts config contains unknown keys: test' + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/attributable_spec.rb b/spec/lib/gitlab/ci/config/node/attributable_spec.rb new file mode 100644 index 00000000000..24d9daafd88 --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/attributable_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Attributable do + let(:node) { Class.new } + let(:instance) { node.new } + + before do + node.include(described_class) + + node.class_eval do + attributes :name, :test + end + end + + context 'config is a hash' do + before do + allow(instance) + .to receive(:config) + .and_return({ name: 'some name', test: 'some test' }) + end + + it 'returns the value of config' do + expect(instance.name).to eq 'some name' + expect(instance.test).to eq 'some test' + end + + it 'returns no method error for unknown attributes' do + expect { instance.unknown }.to raise_error(NoMethodError) + end + end + + context 'config is not a hash' do + before do + allow(instance) + .to receive(:config) + .and_return('some test') + end + + it 'returns nil' do + expect(instance.test).to be_nil + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/commands_spec.rb b/spec/lib/gitlab/ci/config/node/commands_spec.rb new file mode 100644 index 00000000000..e373c40706f --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/commands_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Commands do + let(:entry) { described_class.new(config) } + + context 'when entry config value is an array' do + let(:config) { ['ls', 'pwd'] } + + describe '#value' do + it 'returns array of strings' do + expect(entry.value).to eq config + end + end + + describe '#errors' do + it 'does not append errors' do + expect(entry.errors).to be_empty + end + end + end + + context 'when entry config value is a string' do + let(:config) { 'ls' } + + describe '#value' do + it 'returns array with single element' do + expect(entry.value).to eq ['ls'] + end + end + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when entry value is not valid' do + let(:config) { 1 } + + describe '#errors' do + it 'saves errors' do + expect(entry.errors) + .to include 'commands config should be a ' \ + 'string or an array of strings' + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/factory_spec.rb b/spec/lib/gitlab/ci/config/node/factory_spec.rb index 91ddef7bfbf..d26185ba585 100644 --- a/spec/lib/gitlab/ci/config/node/factory_spec.rb +++ b/spec/lib/gitlab/ci/config/node/factory_spec.rb @@ -2,13 +2,13 @@ require 'spec_helper' describe Gitlab::Ci::Config::Node::Factory do describe '#create!' do - let(:factory) { described_class.new(entry_class) } - let(:entry_class) { Gitlab::Ci::Config::Node::Script } + let(:factory) { described_class.new(node) } + let(:node) { Gitlab::Ci::Config::Node::Script } - context 'when setting up a value' do + context 'when setting a concrete value' do it 'creates entry with valid value' do entry = factory - .with(value: ['ls', 'pwd']) + .value(['ls', 'pwd']) .create! expect(entry.value).to eq ['ls', 'pwd'] @@ -17,7 +17,7 @@ describe Gitlab::Ci::Config::Node::Factory do context 'when setting description' do it 'creates entry with description' do entry = factory - .with(value: ['ls', 'pwd']) + .value(['ls', 'pwd']) .with(description: 'test description') .create! @@ -29,7 +29,8 @@ describe Gitlab::Ci::Config::Node::Factory do context 'when setting key' do it 'creates entry with custom key' do entry = factory - .with(value: ['ls', 'pwd'], key: 'test key') + .value(['ls', 'pwd']) + .with(key: 'test key') .create! expect(entry.key).to eq 'test key' @@ -37,19 +38,20 @@ describe Gitlab::Ci::Config::Node::Factory do end context 'when setting a parent' do - let(:parent) { Object.new } + let(:object) { Object.new } it 'creates entry with valid parent' do entry = factory - .with(value: 'ls', parent: parent) + .value('ls') + .with(parent: object) .create! - expect(entry.parent).to eq parent + expect(entry.parent).to eq object end end end - context 'when not setting up a value' do + context 'when not setting a value' do it 'raises error' do expect { factory.create! }.to raise_error( Gitlab::Ci::Config::Node::Factory::InvalidFactory @@ -60,11 +62,25 @@ describe Gitlab::Ci::Config::Node::Factory do context 'when creating entry with nil value' do it 'creates an undefined entry' do entry = factory - .with(value: nil) + .value(nil) .create! expect(entry).to be_an_instance_of Gitlab::Ci::Config::Node::Undefined end end + + context 'when passing metadata' do + let(:node) { spy('node') } + + it 'passes metadata as a parameter' do + factory + .value('some value') + .metadata(some: 'hash') + .create! + + expect(node).to have_received(:new) + .with('some value', { some: 'hash' }) + end + end end end diff --git a/spec/lib/gitlab/ci/config/node/global_spec.rb b/spec/lib/gitlab/ci/config/node/global_spec.rb index c87c9e97bc8..2f87d270b36 100644 --- a/spec/lib/gitlab/ci/config/node/global_spec.rb +++ b/spec/lib/gitlab/ci/config/node/global_spec.rb @@ -22,38 +22,40 @@ describe Gitlab::Ci::Config::Node::Global do variables: { VAR: 'value' }, after_script: ['make clean'], stages: ['build', 'pages'], - cache: { key: 'k', untracked: true, paths: ['public/'] } } + cache: { key: 'k', untracked: true, paths: ['public/'] }, + rspec: { script: %w[rspec ls] }, + spinach: { script: 'spinach' } } end describe '#process!' do before { global.process! } it 'creates nodes hash' do - expect(global.nodes).to be_an Array + expect(global.descendants).to be_an Array end it 'creates node object for each entry' do - expect(global.nodes.count).to eq 8 + expect(global.descendants.count).to eq 8 end it 'creates node object using valid class' do - expect(global.nodes.first) + expect(global.descendants.first) .to be_an_instance_of Gitlab::Ci::Config::Node::Script - expect(global.nodes.second) + expect(global.descendants.second) .to be_an_instance_of Gitlab::Ci::Config::Node::Image end it 'sets correct description for nodes' do - expect(global.nodes.first.description) + expect(global.descendants.first.description) .to eq 'Script that will be executed before each job.' - expect(global.nodes.second.description) + expect(global.descendants.second.description) .to eq 'Docker image that will be used to execute jobs.' end - end - describe '#leaf?' do - it 'is not leaf' do - expect(global).not_to be_leaf + describe '#leaf?' do + it 'is not leaf' do + expect(global).not_to be_leaf + end end end @@ -63,6 +65,12 @@ describe Gitlab::Ci::Config::Node::Global do expect(global.before_script).to be nil end end + + describe '#leaf?' do + it 'is leaf' do + expect(global).to be_leaf + end + end end context 'when processed' do @@ -106,7 +114,10 @@ describe Gitlab::Ci::Config::Node::Global do end context 'when deprecated types key defined' do - let(:hash) { { types: ['test', 'deploy'] } } + let(:hash) do + { types: ['test', 'deploy'], + rspec: { script: 'rspec' } } + end it 'returns array of types as stages' do expect(global.stages).to eq %w[test deploy] @@ -120,20 +131,33 @@ describe Gitlab::Ci::Config::Node::Global do .to eq(key: 'k', untracked: true, paths: ['public/']) end end + + describe '#jobs' do + it 'returns jobs configuration' do + expect(global.jobs).to eq( + rspec: { name: :rspec, + script: %w[rspec ls], + stage: 'test' }, + spinach: { name: :spinach, + script: %w[spinach], + stage: 'test' } + ) + end + end end end context 'when most of entires not defined' do - let(:hash) { { cache: { key: 'a' }, rspec: {} } } + let(:hash) { { cache: { key: 'a' }, rspec: { script: %w[ls] } } } before { global.process! } describe '#nodes' do it 'instantizes all nodes' do - expect(global.nodes.count).to eq 8 + expect(global.descendants.count).to eq 8 end it 'contains undefined nodes' do - expect(global.nodes.first) + expect(global.descendants.first) .to be_an_instance_of Gitlab::Ci::Config::Node::Undefined end end @@ -164,7 +188,7 @@ describe Gitlab::Ci::Config::Node::Global do # details. # context 'when entires specified but not defined' do - let(:hash) { { variables: nil } } + let(:hash) { { variables: nil, rspec: { script: 'rspec' } } } before { global.process! } describe '#variables' do @@ -196,10 +220,8 @@ describe Gitlab::Ci::Config::Node::Global do end describe '#before_script' do - it 'raises error' do - expect { global.before_script }.to raise_error( - Gitlab::Ci::Config::Node::Entry::InvalidError - ) + it 'returns nil' do + expect(global.before_script).to be_nil end end end @@ -220,9 +242,9 @@ describe Gitlab::Ci::Config::Node::Global do end end - describe '#defined?' do + describe '#specified?' do it 'is concrete entry that is defined' do - expect(global.defined?).to be true + expect(global.specified?).to be true end end end diff --git a/spec/lib/gitlab/ci/config/node/hidden_job_spec.rb b/spec/lib/gitlab/ci/config/node/hidden_job_spec.rb new file mode 100644 index 00000000000..cc44e2cc054 --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/hidden_job_spec.rb @@ -0,0 +1,58 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::HiddenJob do + let(:entry) { described_class.new(config) } + + describe 'validations' do + context 'when entry config value is correct' do + let(:config) { { image: 'ruby:2.2' } } + + describe '#value' do + it 'returns key value' do + expect(entry.value).to eq(image: 'ruby:2.2') + end + end + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when entry value is not correct' do + context 'incorrect config value type' do + let(:config) { ['incorrect'] } + + describe '#errors' do + it 'saves errors' do + expect(entry.errors) + .to include 'hidden job config should be a hash' + end + end + end + + context 'when config is empty' do + let(:config) { {} } + + describe '#valid' do + it 'is invalid' do + expect(entry).not_to be_valid + end + end + end + end + end + + describe '#leaf?' do + it 'is a leaf' do + expect(entry).to be_leaf + end + end + + describe '#relevant?' do + it 'is not a relevant entry' do + expect(entry).not_to be_relevant + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/job_spec.rb b/spec/lib/gitlab/ci/config/node/job_spec.rb new file mode 100644 index 00000000000..1484fb60dd8 --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/job_spec.rb @@ -0,0 +1,86 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Job do + let(:entry) { described_class.new(config, name: :rspec) } + + before { entry.process! } + + describe 'validations' do + context 'when entry config value is correct' do + let(:config) { { script: 'rspec' } } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when job name is empty' do + let(:entry) { described_class.new(config, name: ''.to_sym) } + + it 'reports error' do + expect(entry.errors) + .to include "job name can't be blank" + end + end + end + + context 'when entry value is not correct' do + context 'incorrect config value type' do + let(:config) { ['incorrect'] } + + describe '#errors' do + it 'reports error about a config type' do + expect(entry.errors) + .to include 'job config should be a hash' + end + end + end + + context 'when config is empty' do + let(:config) { {} } + + describe '#valid' do + it 'is invalid' do + expect(entry).not_to be_valid + end + end + end + + context 'when unknown keys detected' do + let(:config) { { unknown: true } } + + describe '#valid' do + it 'is not valid' do + expect(entry).not_to be_valid + end + end + end + end + end + + describe '#value' do + context 'when entry is correct' do + let(:config) do + { before_script: %w[ls pwd], + script: 'rspec', + after_script: %w[cleanup] } + end + + it 'returns correct value' do + expect(entry.value) + .to eq(name: :rspec, + before_script: %w[ls pwd], + script: %w[rspec], + stage: 'test', + after_script: %w[cleanup]) + end + end + end + + describe '#relevant?' do + it 'is a relevant entry' do + expect(entry).to be_relevant + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/jobs_spec.rb b/spec/lib/gitlab/ci/config/node/jobs_spec.rb new file mode 100644 index 00000000000..b8d9c70479c --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/jobs_spec.rb @@ -0,0 +1,87 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Jobs do + let(:entry) { described_class.new(config) } + + describe 'validations' do + before { entry.process! } + + context 'when entry config value is correct' do + let(:config) { { rspec: { script: 'rspec' } } } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when entry value is not correct' do + describe '#errors' do + context 'incorrect config value type' do + let(:config) { ['incorrect'] } + + it 'returns error about incorrect type' do + expect(entry.errors) + .to include 'jobs config should be a hash' + end + end + + context 'when job is unspecified' do + let(:config) { { rspec: nil } } + + it 'reports error' do + expect(entry.errors).to include "rspec config can't be blank" + end + end + + context 'when no visible jobs present' do + let(:config) { { '.hidden'.to_sym => { script: [] } } } + + it 'returns error about no visible jobs defined' do + expect(entry.errors) + .to include 'jobs config should contain at least one visible job' + end + end + end + end + end + + context 'when valid job entries processed' do + before { entry.process! } + + let(:config) do + { rspec: { script: 'rspec' }, + spinach: { script: 'spinach' }, + '.hidden'.to_sym => {} } + end + + describe '#value' do + it 'returns key value' do + expect(entry.value).to eq( + rspec: { name: :rspec, + script: %w[rspec], + stage: 'test' }, + spinach: { name: :spinach, + script: %w[spinach], + stage: 'test' }) + end + end + + describe '#descendants' do + it 'creates valid descendant nodes' do + expect(entry.descendants.count).to eq 3 + expect(entry.descendants.first(2)) + .to all(be_an_instance_of(Gitlab::Ci::Config::Node::Job)) + expect(entry.descendants.last) + .to be_an_instance_of(Gitlab::Ci::Config::Node::HiddenJob) + end + end + + describe '#value' do + it 'returns value of visible jobs only' do + expect(entry.value.keys).to eq [:rspec, :spinach] + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/null_spec.rb b/spec/lib/gitlab/ci/config/node/null_spec.rb new file mode 100644 index 00000000000..1ab5478dcfa --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/null_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Null do + let(:null) { described_class.new(nil) } + + describe '#leaf?' do + it 'is leaf node' do + expect(null).to be_leaf + end + end + + describe '#valid?' do + it 'is always valid' do + expect(null).to be_valid + end + end + + describe '#errors' do + it 'is does not contain errors' do + expect(null.errors).to be_empty + end + end + + describe '#value' do + it 'returns nil' do + expect(null.value).to eq nil + end + end + + describe '#relevant?' do + it 'is not relevant' do + expect(null.relevant?).to eq false + end + end + + describe '#specified?' do + it 'is not defined' do + expect(null.specified?).to eq false + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/stage_spec.rb b/spec/lib/gitlab/ci/config/node/stage_spec.rb new file mode 100644 index 00000000000..fb9ec70762a --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/stage_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Stage do + let(:stage) { described_class.new(config) } + + describe 'validations' do + context 'when stage config value is correct' do + let(:config) { 'build' } + + describe '#value' do + it 'returns a stage key' do + expect(stage.value).to eq config + end + end + + describe '#valid?' do + it 'is valid' do + expect(stage).to be_valid + end + end + end + + context 'when value has a wrong type' do + let(:config) { { test: true } } + + it 'reports errors about wrong type' do + expect(stage.errors) + .to include 'stage config should be a string' + end + end + end + + describe '.default' do + it 'returns default stage' do + expect(described_class.default).to eq 'test' + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/trigger_spec.rb b/spec/lib/gitlab/ci/config/node/trigger_spec.rb new file mode 100644 index 00000000000..a4a3e36754e --- /dev/null +++ b/spec/lib/gitlab/ci/config/node/trigger_spec.rb @@ -0,0 +1,56 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Node::Trigger do + let(:entry) { described_class.new(config) } + + describe 'validations' do + context 'when entry config value is valid' do + context 'when config is a branch or tag name' do + let(:config) { %w[master feature/branch] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + + describe '#value' do + it 'returns key value' do + expect(entry.value).to eq config + end + end + end + + context 'when config is a regexp' do + let(:config) { ['/^issue-.*$/'] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + + context 'when config is a special keyword' do + let(:config) { %w[tags triggers branches] } + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + end + end + + context 'when entry value is not valid' do + let(:config) { [1] } + + describe '#errors' do + it 'saves errors' do + expect(entry.errors) + .to include 'trigger config should be an array of strings or regexps' + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/config/node/undefined_spec.rb b/spec/lib/gitlab/ci/config/node/undefined_spec.rb index 0c6608d906d..2d43e1c1a9d 100644 --- a/spec/lib/gitlab/ci/config/node/undefined_spec.rb +++ b/spec/lib/gitlab/ci/config/node/undefined_spec.rb @@ -2,39 +2,31 @@ require 'spec_helper' describe Gitlab::Ci::Config::Node::Undefined do let(:undefined) { described_class.new(entry) } - let(:entry) { Class.new } - - describe '#leaf?' do - it 'is leaf node' do - expect(undefined).to be_leaf - end - end + let(:entry) { spy('Entry') } describe '#valid?' do - it 'is always valid' do - expect(undefined).to be_valid + it 'delegates method to entry' do + expect(undefined.valid).to eq entry end end describe '#errors' do - it 'is does not contain errors' do - expect(undefined.errors).to be_empty + it 'delegates method to entry' do + expect(undefined.errors).to eq entry end end describe '#value' do - before do - allow(entry).to receive(:default).and_return('some value') - end - - it 'returns default value for entry' do - expect(undefined.value).to eq 'some value' + it 'delegates method to entry' do + expect(undefined.value).to eq entry end end - describe '#undefined?' do - it 'is not a defined entry' do - expect(undefined.defined?).to be false + describe '#specified?' do + it 'is always false' do + allow(entry).to receive(:specified?).and_return(true) + + expect(undefined.specified?).to be false end end end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index ae064a878b0..8447305a316 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -151,7 +151,13 @@ describe Gitlab::GitAccess, lib: true do def self.run_permission_checks(permissions_matrix) permissions_matrix.keys.each do |role| describe "#{role} access" do - before { project.team << [user, role] } + before do + if role == :admin + user.update_attribute(:admin, true) + else + project.team << [user, role] + end + end permissions_matrix[role].each do |action, allowed| context action do @@ -165,6 +171,17 @@ describe Gitlab::GitAccess, lib: true do end permissions_matrix = { + admin: { + push_new_branch: true, + push_master: true, + push_protected_branch: true, + push_remove_protected_branch: false, + push_tag: true, + push_new_tag: true, + push_all: true, + merge_into_protected_branch: true + }, + master: { push_new_branch: true, push_master: true, @@ -217,19 +234,20 @@ describe Gitlab::GitAccess, lib: true do run_permission_checks(permissions_matrix) end - context "when 'developers can push' is turned on for the #{protected_branch_type} protected branch" do - before { create(:protected_branch, name: protected_branch_name, developers_can_push: true, project: project) } + context "when developers are allowed to push into the #{protected_branch_type} protected branch" do + before { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) } run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true })) end - context "when 'developers can merge' is turned on for the #{protected_branch_type} protected branch" do - before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, project: project) } + context "developers are allowed to merge into the #{protected_branch_type} protected branch" do + before { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) } context "when a merge request exists for the given source/target branch" do context "when the merge request is in progress" do before do - create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch) + create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', + state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch) end run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true })) @@ -242,51 +260,59 @@ describe Gitlab::GitAccess, lib: true do run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false })) end - end - context "when a merge request does not exist for the given source/target branch" do - run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false })) + context "when a merge request does not exist for the given source/target branch" do + run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false })) + end end end - context "when 'developers can merge' and 'developers can push' are turned on for the #{protected_branch_type} protected branch" do - before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, developers_can_push: true, project: project) } + context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do + before { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) } run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true })) end + + context "when no one is allowed to push to the #{protected_branch_name} protected branch" do + before { create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) } + + run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }, + master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }, + admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false })) + end end + end - describe 'deploy key permissions' do - let(:key) { create(:deploy_key) } - let(:actor) { key } + describe 'deploy key permissions' do + let(:key) { create(:deploy_key) } + let(:actor) { key } - context 'push code' do - subject { access.check('git-receive-pack') } + context 'push code' do + subject { access.check('git-receive-pack') } - context 'when project is authorized' do - before { key.projects << project } + context 'when project is authorized' do + before { key.projects << project } - it { expect(subject).not_to be_allowed } - end + it { expect(subject).not_to be_allowed } + end - context 'when unauthorized' do - context 'to public project' do - let(:project) { create(:project, :public) } + context 'when unauthorized' do + context 'to public project' do + let(:project) { create(:project, :public) } - it { expect(subject).not_to be_allowed } - end + it { expect(subject).not_to be_allowed } + end - context 'to internal project' do - let(:project) { create(:project, :internal) } + context 'to internal project' do + let(:project) { create(:project, :internal) } - it { expect(subject).not_to be_allowed } - end + it { expect(subject).not_to be_allowed } + end - context 'to private project' do - let(:project) { create(:project, :internal) } + context 'to private project' do + let(:project) { create(:project, :internal) } - it { expect(subject).not_to be_allowed } - end + it { expect(subject).not_to be_allowed } end end end diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb index aa9ec243498..5bb095366fa 100644 --- a/spec/lib/gitlab/user_access_spec.rb +++ b/spec/lib/gitlab/user_access_spec.rb @@ -44,7 +44,7 @@ describe Gitlab::UserAccess, lib: true do describe 'push to protected branch if allowed for developers' do before do - @branch = create :protected_branch, project: project, developers_can_push: true + @branch = create :protected_branch, :developers_can_push, project: project end it 'returns true if user is a master' do @@ -65,7 +65,7 @@ describe Gitlab::UserAccess, lib: true do describe 'merge to protected branch if allowed for developers' do before do - @branch = create :protected_branch, project: project, developers_can_merge: true + @branch = create :protected_branch, :developers_can_merge, project: project end it 'returns true if user is a master' do diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index cd5f40fe3d2..853f6943cef 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -170,4 +170,52 @@ describe Ability, lib: true do end end end + + describe '.issues_readable_by_user' do + context 'with an admin user' do + it 'returns all given issues' do + user = build(:user, admin: true) + issue = build(:issue) + + expect(described_class.issues_readable_by_user([issue], user)). + to eq([issue]) + end + end + + context 'with a regular user' do + it 'returns the issues readable by the user' do + user = build(:user) + issue = build(:issue) + + expect(issue).to receive(:readable_by?).with(user).and_return(true) + + expect(described_class.issues_readable_by_user([issue], user)). + to eq([issue]) + end + + it 'returns an empty Array when no issues are readable' do + user = build(:user) + issue = build(:issue) + + expect(issue).to receive(:readable_by?).with(user).and_return(false) + + expect(described_class.issues_readable_by_user([issue], user)).to eq([]) + end + end + + context 'without a regular user' do + it 'returns issues that are publicly visible' do + hidden_issue = build(:issue) + visible_issue = build(:issue) + + expect(hidden_issue).to receive(:publicly_visible?).and_return(false) + expect(visible_issue).to receive(:publicly_visible?).and_return(true) + + issues = described_class. + issues_readable_by_user([hidden_issue, visible_issue]) + + expect(issues).to eq([visible_issue]) + end + end + end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index ec1544bf815..c3392ee7440 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -13,6 +13,26 @@ describe Commit, models: true do it { is_expected.to include_module(StaticModel) } end + describe '#author' do + it 'looks up the author in a case-insensitive way' do + user = create(:user, email: commit.author_email.upcase) + expect(commit.author).to eq(user) + end + + it 'caches the author' do + user = create(:user, email: commit.author_email) + expect(RequestStore).to receive(:active?).twice.and_return(true) + expect_any_instance_of(Commit).to receive(:find_author_by_any_email).and_call_original + + expect(commit.author).to eq(user) + key = "commit_author:#{commit.author_email}" + expect(RequestStore.store[key]).to eq(user) + + expect(commit.author).to eq(user) + RequestStore.store.clear + end + end + describe '#to_reference' do it 'returns a String reference to the object' do expect(commit.to_reference).to eq commit.id diff --git a/spec/models/concerns/mentionable_spec.rb b/spec/models/concerns/mentionable_spec.rb index 5e652660e2c..549b0042038 100644 --- a/spec/models/concerns/mentionable_spec.rb +++ b/spec/models/concerns/mentionable_spec.rb @@ -68,7 +68,7 @@ describe Issue, "Mentionable" do describe '#create_cross_references!' do let(:project) { create(:project) } - let(:author) { double('author') } + let(:author) { build(:user) } let(:commit) { project.commit } let(:commit2) { project.commit } @@ -88,6 +88,10 @@ describe Issue, "Mentionable" do let(:author) { create(:author) } let(:issues) { create_list(:issue, 2, project: project, author: author) } + before do + project.team << [author, Gitlab::Access::DEVELOPER] + end + context 'before changes are persisted' do it 'ignores pre-existing references' do issue = create_issue(description: issues[0].to_reference) diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 6a897c96690..3259f795296 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -306,4 +306,257 @@ describe Issue, models: true do expect(user2.assigned_open_issues_count).to eq(1) end end + + describe '#visible_to_user?' do + context 'with a user' do + let(:user) { build(:user) } + let(:issue) { build(:issue) } + + it 'returns true when the issue is readable' do + expect(issue).to receive(:readable_by?).with(user).and_return(true) + + expect(issue.visible_to_user?(user)).to eq(true) + end + + it 'returns false when the issue is not readable' do + expect(issue).to receive(:readable_by?).with(user).and_return(false) + + expect(issue.visible_to_user?(user)).to eq(false) + end + end + + context 'without a user' do + let(:issue) { build(:issue) } + + it 'returns true when the issue is publicly visible' do + expect(issue).to receive(:publicly_visible?).and_return(true) + + expect(issue.visible_to_user?).to eq(true) + end + + it 'returns false when the issue is not publicly visible' do + expect(issue).to receive(:publicly_visible?).and_return(false) + + expect(issue.visible_to_user?).to eq(false) + end + end + end + + describe '#readable_by?' do + describe 'with a regular user that is not a team member' do + let(:user) { create(:user) } + + context 'using a public project' do + let(:project) { create(:empty_project, :public) } + + it 'returns true for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).to be_readable_by(user) + end + + it 'returns false for a confidential issue' do + issue = build(:issue, project: project, confidential: true) + + expect(issue).not_to be_readable_by(user) + end + end + + context 'using an internal project' do + let(:project) { create(:empty_project, :internal) } + + context 'using an internal user' do + it 'returns true for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).to be_readable_by(user) + end + + it 'returns false for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).not_to be_readable_by(user) + end + end + + context 'using an external user' do + before do + allow(user).to receive(:external?).and_return(true) + end + + it 'returns false for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).not_to be_readable_by(user) + end + + it 'returns false for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).not_to be_readable_by(user) + end + end + end + + context 'using a private project' do + let(:project) { create(:empty_project, :private) } + + it 'returns false for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).not_to be_readable_by(user) + end + + it 'returns false for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).not_to be_readable_by(user) + end + + context 'when the user is the project owner' do + it 'returns true for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).not_to be_readable_by(user) + end + + it 'returns true for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).not_to be_readable_by(user) + end + end + end + end + + context 'with a regular user that is a team member' do + let(:user) { create(:user) } + let(:project) { create(:empty_project, :public) } + + context 'using a public project' do + before do + project.team << [user, Gitlab::Access::DEVELOPER] + end + + it 'returns true for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).to be_readable_by(user) + end + + it 'returns true for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).to be_readable_by(user) + end + end + + context 'using an internal project' do + let(:project) { create(:empty_project, :internal) } + + before do + project.team << [user, Gitlab::Access::DEVELOPER] + end + + it 'returns true for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).to be_readable_by(user) + end + + it 'returns true for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).to be_readable_by(user) + end + end + + context 'using a private project' do + let(:project) { create(:empty_project, :private) } + + before do + project.team << [user, Gitlab::Access::DEVELOPER] + end + + it 'returns true for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).to be_readable_by(user) + end + + it 'returns true for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).to be_readable_by(user) + end + end + end + + context 'with an admin user' do + let(:project) { create(:empty_project) } + let(:user) { create(:user, admin: true) } + + it 'returns true for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).to be_readable_by(user) + end + + it 'returns true for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).to be_readable_by(user) + end + end + end + + describe '#publicly_visible?' do + context 'using a public project' do + let(:project) { create(:empty_project, :public) } + + it 'returns true for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).to be_publicly_visible + end + + it 'returns false for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).not_to be_publicly_visible + end + end + + context 'using an internal project' do + let(:project) { create(:empty_project, :internal) } + + it 'returns false for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).not_to be_publicly_visible + end + + it 'returns false for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).not_to be_publicly_visible + end + end + + context 'using a private project' do + let(:project) { create(:empty_project, :private) } + + it 'returns false for a regular issue' do + issue = build(:issue, project: project) + + expect(issue).not_to be_publicly_visible + end + + it 'returns false for a confidential issue' do + issue = build(:issue, :confidential, project: project) + + expect(issue).not_to be_publicly_visible + end + end + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 72b8a4e25bd..e365e4e98b2 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1095,46 +1095,6 @@ describe Project, models: true do end end - describe "#developers_can_push_to_protected_branch?" do - let(:project) { create(:empty_project) } - - context "when the branch matches a protected branch via direct match" do - it "returns true if 'Developers can Push' is turned on" do - create(:protected_branch, name: "production", project: project, developers_can_push: true) - - expect(project.developers_can_push_to_protected_branch?('production')).to be true - end - - it "returns false if 'Developers can Push' is turned off" do - create(:protected_branch, name: "production", project: project, developers_can_push: false) - - expect(project.developers_can_push_to_protected_branch?('production')).to be false - end - end - - context "when the branch matches a protected branch via wilcard match" do - it "returns true if 'Developers can Push' is turned on" do - create(:protected_branch, name: "production/*", project: project, developers_can_push: true) - - expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be true - end - - it "returns false if 'Developers can Push' is turned off" do - create(:protected_branch, name: "production/*", project: project, developers_can_push: false) - - expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be false - end - end - - context "when the branch does not match a protected branch" do - it "returns false" do - create(:protected_branch, name: "production/*", project: project, developers_can_push: true) - - expect(project.developers_can_push_to_protected_branch?('staging/some-branch')).to be false - end - end - end - describe '#container_registry_path_with_namespace' do let(:project) { create(:empty_project, path: 'PROJECT') } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 5bc1bd9a930..cce15538b93 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']) } @@ -381,9 +383,13 @@ describe Repository, models: true do end describe '#rm_branch' do + let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature + let(:blank_sha) { '0000000000000000000000000000000000000000' } + context 'when pre hooks were successful' do it 'should run without errors' do - allow_any_instance_of(Gitlab::Git::Hook).to receive(:trigger).and_return([true, nil]) + expect_any_instance_of(GitHooksService).to receive(:execute). + with(user, project.repository.path_to_repo, old_rev, blank_sha, 'refs/heads/feature') expect { repository.rm_branch(user, 'feature') }.not_to raise_error end @@ -418,10 +424,13 @@ describe Repository, models: true do end describe '#commit_with_hooks' do + let(:old_rev) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' } # git rev-parse feature + context 'when pre hooks were successful' do before do expect_any_instance_of(GitHooksService).to receive(:execute). - and_return(true) + with(user, repository.path_to_repo, old_rev, sample_commit.id, 'refs/heads/feature'). + and_yield.and_return(true) end it 'should run without errors' do @@ -435,6 +444,14 @@ describe Repository, models: true do repository.commit_with_hooks(user, 'feature') { sample_commit.id } end + + context "when the branch wasn't empty" do + it 'updates the head' do + expect(repository.find_branch('feature').target.id).to eq(old_rev) + repository.commit_with_hooks(user, 'feature') { sample_commit.id } + expect(repository.find_branch('feature').target.id).to eq(sample_commit.id) + end + end end context 'when pre hooks failed' do @@ -1198,17 +1215,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 "does not fail if we attempt to reference bad commit" do expect(repository.kept_around?('abc1234')).to be_falsey @@ -1236,9 +1242,4 @@ describe Repository, models: true do File.delete(path) 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 diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb index 719da27f919..e8fd697965f 100644 --- a/spec/requests/api/branches_spec.rb +++ b/spec/requests/api/branches_spec.rb @@ -112,7 +112,7 @@ describe API::API, api: true do before do project.repository.add_branch(user, protected_branch, 'master') - create(:protected_branch, project: project, name: protected_branch, developers_can_push: true, developers_can_merge: true) + create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: protected_branch) end it 'updates that a developer can push' do diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 47c0580e0f0..ffa998dffc3 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -7,6 +7,7 @@ describe GitPushService, services: true do let(:project) { create :project } before do + project.team << [user, :master] @blankrev = Gitlab::Git::BLANK_SHA @oldrev = sample_commit.parent_id @newrev = sample_commit.id @@ -172,7 +173,7 @@ describe GitPushService, services: true do describe "Push Event" do before do service = execute_service(project, user, @oldrev, @newrev, @ref ) - @event = Event.last + @event = Event.find_by_action(Event::PUSHED) @push_data = service.push_data end @@ -224,8 +225,10 @@ describe GitPushService, services: true do it "when pushing a branch for the first time" do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: false }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) + expect(project.protected_branches).not_to be_empty + expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER) + expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::MASTER) end it "when pushing a branch for the first time with default branch protection disabled" do @@ -233,8 +236,8 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - expect(project.protected_branches).not_to receive(:create) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) + expect(project.protected_branches).to be_empty end it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do @@ -242,9 +245,12 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true, developers_can_merge: false }) - execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master') + execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) + + expect(project.protected_branches).not_to be_empty + expect(project.protected_branches.last.push_access_level.access_level).to eq(Gitlab::Access::DEVELOPER) + expect(project.protected_branches.last.merge_access_level.access_level).to eq(Gitlab::Access::MASTER) end it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do @@ -252,8 +258,10 @@ describe GitPushService, services: true do expect(project).to receive(:execute_hooks) expect(project.default_branch).to eq("master") - expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: true }) execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' ) + expect(project.protected_branches).not_to be_empty + expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER) + expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::DEVELOPER) end it "when pushing new commits to existing branch" do diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index f5bf3c1e367..8ffebcac698 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -75,6 +75,17 @@ describe MergeRequests::MergeService, services: true do expect(merge_request.merge_error).to eq("error") end + + it 'aborts if there is a merge conflict' do + allow_any_instance_of(Repository).to receive(:merge).and_return(false) + allow(service).to receive(:execute_hooks) + + service.execute(merge_request) + + expect(merge_request.open?).to be_truthy + expect(merge_request.merge_commit_sha).to be_nil + expect(merge_request.merge_error).to eq("Conflicts detected during merge") + end end end end diff --git a/spec/views/projects/issues/_related_branches.html.haml_spec.rb b/spec/views/projects/issues/_related_branches.html.haml_spec.rb new file mode 100644 index 00000000000..78af61f15a7 --- /dev/null +++ b/spec/views/projects/issues/_related_branches.html.haml_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe 'projects/issues/_related_branches' do + include Devise::TestHelpers + + let(:project) { create(:project) } + let(:branch) { project.repository.find_branch('feature') } + let!(:pipeline) { create(:ci_pipeline, project: project, sha: branch.target.id, ref: 'feature') } + + before do + assign(:project, project) + assign(:related_branches, ['feature']) + + render + end + + it 'shows the related branches with their build status' do + expect(rendered).to match('feature') + expect(rendered).to have_css('.related-branch-ci-status') + end +end |