Merge remote-tracking branch 'upstream/master' into 25680-CI_ENVIRONMENT_URL

* upstream/master: (39 commits)
  Resolve "Improve Container Registry description"
  Add username parameter to gravatar URL
  Fix replying to a commit discussion displayed in the context of an MR
  Add fog-aliyun as backup storage provider
  Add missing specs
  Make sure protected can't be null; Test protected!
  Update session cookie key name to be unique to instance in development
  Just mention which GitLab version is required
  Fix data inconsistency issue for old artifacts by moving them to a currently used path
  Fix N+1 queries for non-members in comment threads
  Fix rubocop in spec/helpers/diff_helper_spec.rb
  Merge two items into one in the doc
  Only remove FK if it exists
  Maintain notes avatar at smaller breakpoint
  Fix pipeline schedule value name in documentation
  Add test for Project#protected_for?
  Update diff discussion position per discussion instead of per note
  Display Shared Runner status in Admin Dashboard
  Make sure we're loading the fresh variables
  Now secret_variables_for would return the variables
  ...

Gemfile

@@ -97,6 +97,7 @@ gem 'fog-google', '~> 0.5'
 gem 'fog-local', '~> 0.3'
 gem 'fog-openstack', '~> 0.1'
 gem 'fog-rackspace', '~> 0.1.1'
+gem 'fog-aliyun', '~> 0.1.0'
 
 # for Google storage
 gem 'google-api-client', '~> 0.8.6'

Gemfile.lock

@@ -213,6 +213,11 @@ GEM
     flowdock (0.7.1)
       httparty (~> 0.7)
       multi_json
+    fog-aliyun (0.1.0)
+      fog-core (~> 1.27)
+      fog-json (~> 1.0)
+      ipaddress (~> 0.8)
+      xml-simple (~> 1.1)
     fog-aws (0.13.0)
       fog-core (~> 1.38)
       fog-json (~> 1.0)
@@ -913,6 +918,7 @@ DEPENDENCIES
   flay (~> 2.8.0)
   flipper (~> 0.10.2)
   flipper-active_record (~> 0.10.2)
+  fog-aliyun (~> 0.1.0)
   fog-aws (~> 0.9)
   fog-core (~> 1.44)
   fog-google (~> 0.5)

app/assets/javascripts/diff_notes/components/jump_to_discussion.js

@@ -16,6 +16,13 @@ const JumpToDiscussion = Vue.extend({
     };
   },
   computed: {
+    buttonText: function () {
+      if (this.discussionId) {
+        return 'Jump to next unresolved discussion';
+      } else {
+        return 'Jump to first unresolved discussion';
+      }
+    },
     allResolved: function () {
       return this.unresolvedDiscussionCount === 0;
     },

app/assets/stylesheets/framework/timeline.scss

@@ -4,7 +4,7 @@
   padding: 0;
 
   &::before {
-    @include notes-media('max', $screen-xs-max) {
+    @include notes-media('max', $screen-xs-min) {
       background: none;
     }
   }
@@ -30,7 +30,7 @@
   .timeline-entry-inner {
     position: relative;
 
-    @include notes-media('max', $screen-xs-max) {
+    @include notes-media('max', $screen-xs-min) {
       .timeline-icon {
         display: none;
       }

app/controllers/projects/variables_controller.rb

@@ -42,6 +42,7 @@ class Projects::VariablesController < Projects::ApplicationController
   private
 
   def project_params
-    params.require(:variable).permit([:id, :key, :value, :_destroy])
+    params.require(:variable)
+      .permit([:id, :key, :value, :protected, :_destroy])
   end
 end

app/helpers/diff_helper.rb

@@ -66,12 +66,12 @@ module DiffHelper
 
     discussions_left = discussions_right = nil
 
-    if left && (left.unchanged? || left.removed?)
+    if left && (left.unchanged? || left.discussable?)
       line_code = diff_file.line_code(left)
       discussions_left = @grouped_diff_discussions[line_code]
     end
 
-    if right && right.added?
+    if right&.discussable?
       line_code = diff_file.line_code(right)
       discussions_right = @grouped_diff_discussions[line_code]
     end

app/helpers/notes_helper.rb

@@ -50,7 +50,7 @@ module NotesHelper
   def link_to_reply_discussion(discussion, line_type = nil)
     return unless current_user
 
-    data = { discussion_id: discussion.id, line_type: line_type }
+    data = { discussion_id: discussion.reply_id, line_type: line_type }
 
     button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
                            data: data, title: 'Add a reply'

app/models/application_setting.rb

@@ -13,13 +13,13 @@ class ApplicationSetting < ActiveRecord::Base
                             [\r\n]          # any number of newline characters
                           }x
 
-  serialize :restricted_visibility_levels
-  serialize :import_sources
-  serialize :disabled_oauth_sign_in_sources, Array
-  serialize :domain_whitelist, Array
-  serialize :domain_blacklist, Array
-  serialize :repository_storages
-  serialize :sidekiq_throttling_queues, Array
+  serialize :restricted_visibility_levels # rubocop:disable Cop/ActiverecordSerialize
+  serialize :import_sources # rubocop:disable Cop/ActiverecordSerialize
+  serialize :disabled_oauth_sign_in_sources, Array # rubocop:disable Cop/ActiverecordSerialize
+  serialize :domain_whitelist, Array # rubocop:disable Cop/ActiverecordSerialize
+  serialize :domain_blacklist, Array # rubocop:disable Cop/ActiverecordSerialize
+  serialize :repository_storages # rubocop:disable Cop/ActiverecordSerialize
+  serialize :sidekiq_throttling_queues, Array # rubocop:disable Cop/ActiverecordSerialize
 
   cache_markdown_field :sign_in_text
   cache_markdown_field :help_page_text

app/models/audit_event.rb

 class AuditEvent < ActiveRecord::Base
-  serialize :details, Hash
+  serialize :details, Hash # rubocop:disable Cop/ActiverecordSerialize
 
   belongs_to :user, foreign_key: :author_id
 

app/models/ci/build.rb

@@ -25,8 +25,8 @@ module Ci
           project.environments.create(name: expanded_environment_name)
     end
 
-    serialize :options
-    serialize :yaml_variables, Gitlab::Serializer::Ci::Variables
+    serialize :options # rubocop:disable Cop/ActiverecordSerialize
+    serialize :yaml_variables, Gitlab::Serializer::Ci::Variables # rubocop:disable Cop/ActiverecordSerialize
 
     delegate :name, to: :project, prefix: true
 
@@ -208,7 +208,7 @@ module Ci
       variables += project.deployment_variables if has_environment?
       variables += yaml_variables
       variables += user_variables
-      variables += project.secret_variables
+      variables += project.secret_variables_for(ref).map(&:to_runner_variable)
       variables += trigger_request.user_variables if trigger_request
       variables
     end
@@ -270,38 +270,6 @@ module Ci
       Time.now - updated_at > 15.minutes.to_i
     end
 
-    ##
-    # Deprecated
-    #
-    # This contains a hotfix for CI build data integrity, see #4246
-    #
-    # This method is used by `ArtifactUploader` to create a store_dir.
-    # Warning: Uploader uses it after AND before file has been stored.
-    #
-    # This method returns old path to artifacts only if it already exists.
-    #
-    def artifacts_path
-      # We need the project even if it's soft deleted, because whenever
-      # we're really deleting the project, we'll also delete the builds,
-      # and in order to delete the builds, we need to know where to find
-      # the artifacts, which is depending on the data of the project.
-      # We need to retain the project in this case.
-      the_project = project || unscoped_project
-
-      old = File.join(created_at.utc.strftime('%Y_%m'),
-                      the_project.ci_id.to_s,
-                      id.to_s)
-
-      old_store = File.join(ArtifactUploader.artifacts_path, old)
-      return old if the_project.ci_id && File.directory?(old_store)
-
-      File.join(
-        created_at.utc.strftime('%Y_%m'),
-        the_project.id.to_s,
-        id.to_s
-      )
-    end
-
     def valid_token?(token)
       self.token && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.token)
     end

app/models/ci/trigger_request.rb

@@ -6,7 +6,7 @@ module Ci
     belongs_to :pipeline, foreign_key: :commit_id
     has_many :builds
 
-    serialize :variables
+    serialize :variables # rubocop:disable Cop/ActiverecordSerialize
 
     def user_variables
       return [] unless variables

app/models/ci/variable.rb

@@ -12,11 +12,16 @@ module Ci
                 message: "can contain only letters, digits and '_'." }
 
     scope :order_key_asc, -> { reorder(key: :asc) }
+    scope :unprotected, -> { where(protected: false) }
 
     attr_encrypted :value,
        mode: :per_attribute_iv_and_salt,
        insecure_mode: true,
        key: Gitlab::Application.secrets.db_key_base,
        algorithm: 'aes-256-cbc'
+
+    def to_runner_variable
+      { key: key, value: value, public: false }
+    end
   end
 end

app/models/diff_discussion.rb

@@ -10,6 +10,7 @@ class DiffDiscussion < Discussion
 
   delegate  :position,
             :original_position,
+            :change_position,
 
             to: :first_note
 

app/models/diff_note.rb

@@ -6,9 +6,9 @@ class DiffNote < Note
 
   NOTEABLE_TYPES = %w(MergeRequest Commit).freeze
 
-  serialize :original_position, Gitlab::Diff::Position
-  serialize :position, Gitlab::Diff::Position
-  serialize :change_position, Gitlab::Diff::Position
+  serialize :original_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize
+  serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize
+  serialize :change_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize
 
   validates :original_position, presence: true
   validates :position, presence: true
@@ -95,13 +95,21 @@ class DiffNote < Note
 
     return if active?
 
-    Notes::DiffPositionUpdateService.new(
-      self.project,
-      nil,
+    tracer = Gitlab::Diff::PositionTracer.new(
+      project: self.project,
       old_diff_refs: self.position.diff_refs,
-      new_diff_refs: noteable.diff_refs,
+      new_diff_refs: self.noteable.diff_refs,
       paths: self.position.paths
-    ).execute(self)
+    )
+
+    result = tracer.trace(self.position)
+    return unless result
+
+    if result[:outdated]
+      self.change_position = result[:position]
+    else
+      self.position = result[:position]
+    end
   end
 
   def verify_supported

app/models/discussion.rb

@@ -85,6 +85,12 @@ class Discussion
     first_note.discussion_id(context_noteable)
   end
 
+  def reply_id
+    # To reply to this discussion, we need the actual discussion_id from the database,
+    # not the potentially overwritten one based on the noteable.
+    first_note.discussion_id
+  end
+
   alias_method :to_param, :id
 
   def diff_discussion?

app/models/event.rb

@@ -26,7 +26,7 @@ class Event < ActiveRecord::Base
   belongs_to :target, polymorphic: true
 
   # For Hash only
-  serialize :data
+  serialize :data # rubocop:disable Cop/ActiverecordSerialize
 
   # Callbacks
   after_create :reset_project_activity

app/models/hooks/web_hook_log.rb

 class WebHookLog < ActiveRecord::Base
   belongs_to :web_hook
 
-  serialize :request_headers, Hash
-  serialize :request_data, Hash
-  serialize :response_headers, Hash
+  serialize :request_headers, Hash # rubocop:disable Cop/ActiverecordSerialize
+  serialize :request_data, Hash # rubocop:disable Cop/ActiverecordSerialize
+  serialize :response_headers, Hash # rubocop:disable Cop/ActiverecordSerialize
 
   validates :web_hook, presence: true
 

app/models/legacy_diff_note.rb

@@ -7,7 +7,7 @@
 class LegacyDiffNote < Note
   include NoteOnDiff
 
-  serialize :st_diff
+  serialize :st_diff # rubocop:disable Cop/ActiverecordSerialize
 
   validates :line_code, presence: true, line_code: true
 

app/models/merge_request.rb

@@ -21,7 +21,7 @@ class MergeRequest < ActiveRecord::Base
 
   belongs_to :assignee, class_name: "User"
 
-  serialize :merge_params, Hash
+  serialize :merge_params, Hash # rubocop:disable Cop/ActiverecordSerialize
 
   after_create :ensure_merge_request_diff, unless: :importing?
   after_update :reload_diff_if_branch_changed
@@ -421,7 +421,7 @@ class MergeRequest < ActiveRecord::Base
     MergeRequests::MergeRequestDiffCacheService.new.execute(self)
     new_diff_refs = self.diff_refs
 
-    update_diff_notes_positions(
+    update_diff_discussion_positions(
       old_diff_refs: old_diff_refs,
       new_diff_refs: new_diff_refs,
       current_user: current_user
@@ -853,19 +853,18 @@ class MergeRequest < ActiveRecord::Base
     diff_refs && diff_refs.complete?
   end
 
-  def update_diff_notes_positions(old_diff_refs:, new_diff_refs:, current_user: nil)
+  def update_diff_discussion_positions(old_diff_refs:, new_diff_refs:, current_user: nil)
     return unless has_complete_diff_refs?
     return if new_diff_refs == old_diff_refs
 
-    active_diff_notes = self.notes.new_diff_notes.select do |note|
-      note.active?(old_diff_refs)
+    active_diff_discussions = self.notes.new_diff_notes.discussions.select do |discussion|
+      discussion.active?(old_diff_refs)
     end
+    return if active_diff_discussions.empty?
 
-    return if active_diff_notes.empty?
+    paths = active_diff_discussions.flat_map { |n| n.diff_file.paths }.uniq
 
-    paths = active_diff_notes.flat_map { |n| n.diff_file.paths }.uniq
-
-    service = Notes::DiffPositionUpdateService.new(
+    service = Discussions::UpdateDiffPositionService.new(
       self.project,
       current_user,
       old_diff_refs: old_diff_refs,
@@ -873,11 +872,8 @@ class MergeRequest < ActiveRecord::Base
       paths: paths
     )
 
-    transaction do
-      active_diff_notes.each do |note|
-        service.execute(note)
-        Gitlab::Timeless.timeless(note, &:save)
-      end
+    active_diff_discussions.each do |discussion|
+      service.execute(discussion)
     end
   end
 

app/models/merge_request_diff.rb

@@ -11,8 +11,8 @@ class MergeRequestDiff < ActiveRecord::Base
 
   belongs_to :merge_request
 
-  serialize :st_commits
-  serialize :st_diffs
+  serialize :st_commits # rubocop:disable Cop/ActiverecordSerialize
+  serialize :st_diffs # rubocop:disable Cop/ActiverecordSerialize
 
   state_machine :state, initial: :empty do
     state :collected

app/models/personal_access_token.rb

@@ -3,7 +3,7 @@ class PersonalAccessToken < ActiveRecord::Base
   include TokenAuthenticatable
   add_authentication_token_field :token
 
-  serialize :scopes, Array
+  serialize :scopes, Array # rubocop:disable Cop/ActiverecordSerialize
 
   belongs_to :user
 

app/models/project.rb

@@ -1245,12 +1245,19 @@ class Project < ActiveRecord::Base
     variables
   end
 
-  def secret_variables
-    variables.map do |variable|
-      { key: variable.key, value: variable.value, public: false }
+  def secret_variables_for(ref)
+    if protected_for?(ref)
+      variables
+    else
+      variables.unprotected
     end
   end
 
+  def protected_for?(ref)
+    ProtectedBranch.protected?(self, ref) ||
+      ProtectedTag.protected?(self, ref)
+  end
+
   def deployment_variables
     return [] unless deployment_service
 

app/models/project_import_data.rb

@@ -10,7 +10,7 @@ class ProjectImportData < ActiveRecord::Base
                  insecure_mode: true,
                  algorithm: 'aes-256-cbc'
 
-  serialize :data, JSON
+  serialize :data, JSON # rubocop:disable Cop/ActiverecordSerialize
 
   validates :project, presence: true
 

app/models/project_team.rb

@@ -167,7 +167,7 @@ class ProjectTeam
       access = RequestStore.store[key]
     end
 
-    # Lookup only the IDs we need
+    # Look up only the IDs we need
     user_ids = user_ids - access.keys
 
     return access if user_ids.empty?
@@ -178,6 +178,13 @@ class ProjectTeam
       maximum(:access_level)
 
     access.merge!(users_access)
+
+    missing_user_ids = user_ids - users_access.keys
+
+    missing_user_ids.each do |user_id|
+      access[user_id] = Gitlab::Access::NO_ACCESS
+    end
+
     access
   end
 

app/models/sent_notification.rb

 class SentNotification < ActiveRecord::Base
-  serialize :position, Gitlab::Diff::Position
+  serialize :position, Gitlab::Diff::Position # rubocop:disable Cop/ActiverecordSerialize
 
   belongs_to :project
   belongs_to :noteable, polymorphic: true

app/models/service.rb

@@ -2,7 +2,7 @@
 # and implement a set of methods
 class Service < ActiveRecord::Base
   include Sortable
-  serialize :properties, JSON
+  serialize :properties, JSON # rubocop:disable Cop/ActiverecordSerialize
 
   default_value_for :active, false
   default_value_for :push_events, true

app/models/user.rb

@@ -40,7 +40,7 @@ class User < ActiveRecord::Base
          otp_secret_encryption_key: Gitlab::Application.secrets.otp_key_base
 
   devise :two_factor_backupable, otp_number_of_backup_codes: 10
-  serialize :otp_backup_codes, JSON
+  serialize :otp_backup_codes, JSON # rubocop:disable Cop/ActiverecordSerialize
 
   devise :lockable, :recoverable, :rememberable, :trackable,
     :validatable, :omniauthable, :confirmable, :registerable
@@ -781,7 +781,7 @@ class User < ActiveRecord::Base
   def avatar_url(size: nil, scale: 2, **args)
     # We use avatar_path instead of overriding avatar_url because of carrierwave.
     # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11001/diffs#note_28659864
-    avatar_path(args) || GravatarService.new.execute(email, size, scale)
+    avatar_path(args) || GravatarService.new.execute(email, size, scale, username: username)
   end
 
   def all_emails

app/services/discussions/update_diff_position_service.rb

+module Discussions
+  class UpdateDiffPositionService < BaseService
+    def execute(discussion)
+      result = tracer.trace(discussion.position)
+      return unless result
+
+      position = result[:position]
+      outdated = result[:outdated]
+
+      discussion.notes.each do |note|
+        if outdated
+          note.change_position = position
+        else
+          note.position = position
+          note.change_position = nil
+        end
+      end
+
+      Note.transaction do
+        discussion.notes.each do |note|
+          Gitlab::Timeless.timeless(note, &:save)
+        end
+
+        if outdated && current_user
+          SystemNoteService.diff_discussion_outdated(discussion, project, current_user, position)
+        end
+      end
+    end
+
+    private
+
+    def tracer
+      @tracer ||= Gitlab::Diff::PositionTracer.new(
+        project: project,
+        old_diff_refs: params[:old_diff_refs],
+        new_diff_refs: params[:new_diff_refs],
+        paths: params[:paths]
+      )
+    end
+  end
+end

app/services/gravatar_service.rb

 class GravatarService
   include Gitlab::CurrentSettings
 
-  def execute(email, size = nil, scale = 2)
-    if current_application_settings.gravatar_enabled? && email.present?
-      size = 40 if size.nil? || size <= 0
+  def execute(email, size = nil, scale = 2, username: nil)
+    return unless current_application_settings.gravatar_enabled?
 
-      sprintf gravatar_url,
-        hash: Digest::MD5.hexdigest(email.strip.downcase),
-        size: size * scale,
-        email: email.strip
-    end
+    identifier = email.presence || username.presence
+    return unless identifier
+
+    hash = Digest::MD5.hexdigest(identifier.strip.downcase)
+    size = 40 unless size && size > 0
+
+    sprintf gravatar_url,
+      hash: hash,
+      size: size * scale,
+      email: ERB::Util.url_encode(email&.strip || ''),
+      username: ERB::Util.url_encode(username&.strip || '')
   end
 
   def gitlab_config

app/services/notes/diff_position_update_service.rb

-module Notes
-  class DiffPositionUpdateService < BaseService
-    def execute(note)
-      results = tracer.trace(note.position)
-      return unless results
-
-      position = results[:position]
-      outdated = results[:outdated]
-
-      if outdated
-        note.change_position = position
-
-        if note.persisted? && current_user
-          SystemNoteService.diff_discussion_outdated(note.to_discussion, project, current_user, position)
-        end
-      else
-        note.position = position
-        note.change_position = nil
-      end
-    end
-
-    private
-
-    def tracer
-      @tracer ||= Gitlab::Diff::PositionTracer.new(
-        project: project,
-        old_diff_refs: params[:old_diff_refs],
-        new_diff_refs: params[:new_diff_refs],
-        paths: params[:paths]
-      )
-    end
-  end
-end

app/uploaders/artifact_uploader.rb

 class ArtifactUploader < GitlabUploader
   storage :file
 
-  attr_accessor :build, :field
+  attr_reader :job, :field
 
-  def self.artifacts_path
+  def self.local_artifacts_store
     Gitlab.config.artifacts.path
   end
 
   def self.artifacts_upload_path
-    File.join(self.artifacts_path, 'tmp/uploads/')
+    File.join(self.local_artifacts_store, 'tmp/uploads/')
   end
 
-  def self.artifacts_cache_path
-    File.join(self.artifacts_path, 'tmp/cache/')
-  end
-
-  def initialize(build, field)
-    @build, @field = build, field
+  def initialize(job, field)
+    @job, @field = job, field
   end
 
   def store_dir
-    File.join(self.class.artifacts_path, @build.artifacts_path)
+    default_local_path
   end
 
   def cache_dir
-    File.join(self.class.artifacts_cache_path, @build.artifacts_path)
+    File.join(self.class.local_artifacts_store, 'tmp/cache')
+  end
+
+  private
+
+  def default_local_path
+    File.join(self.class.local_artifacts_store, default_path)
   end
 
-  def filename
-    file.try(:filename)
+  def default_path
+    File.join(job.created_at.utc.strftime('%Y_%m'), job.project_id.to_s, job.id.to_s)
   end
 end

app/uploaders/gitlab_uploader.rb

@@ -10,7 +10,11 @@ class GitlabUploader < CarrierWave::Uploader::Base
   delegate :base_dir, to: :class
 
   def file_storage?
-    self.class.storage == CarrierWave::Storage::File
+    storage.is_a?(CarrierWave::Storage::File)
+  end
+
+  def file_cache_storage?
+    cache_storage.is_a?(CarrierWave::Storage::File)
   end
 
   # Reduce disk IO

app/views/admin/dashboard/index.html.haml

@@ -79,6 +79,12 @@
           = gitlab_pages
           %span.light.pull-right
             = boolean_to_icon gitlab_pages_enabled
+        - gitlab_shared_runners = 'Shared Runners'
+        - gitlab_shared_runners_enabled = Gitlab.config.gitlab_ci.shared_runners_enabled
+        %p{ "aria-label" => "#{gitlab_shared_runners}: status " + (gitlab_shared_runners_enabled ? "on" : "off") }
+          = gitlab_shared_runners
+          %span.light.pull-right
+            = boolean_to_icon gitlab_shared_runners_enabled
 
       .col-md-4
         %h4

app/views/discussions/_jump_to_next.html.haml

@@ -3,7 +3,7 @@
   %jump-to-discussion{ "inline-template" => true, ":discussion-id" => "'#{discussion.try(:id)}'" }
     .btn-group{ role: "group", "v-show" => "!allResolved", "v-if" => "showButton" }
       %button.btn.btn-default.discussion-next-btn.has-tooltip{ "@click" => "jumpToNextUnresolvedDiscussion",
-        title: "Jump to next unresolved discussion",
-        "aria-label" => "Jump to next unresolved discussion",
+        ":title" => "buttonText",
+        ":aria-label" => "buttonText",
         data: { container: "body" } }
         = custom_icon("next_discussion")

app/views/projects/diffs/_line.html.haml

@@ -3,7 +3,7 @@
 - discussions = local_assigns.fetch(:discussions, nil)
 - type = line.type
 - line_code = diff_file.line_code(line)
-- if discussions && !line.meta?
+- if discussions && line.discussable?
   - line_discussions = discussions[line_code]
 %tr.line_holder{ class: type, id: (line_code unless plain) }
   - case type

app/views/projects/pipelines_settings/_show.html.haml

@@ -42,7 +42,7 @@
           = f.label :build_timeout_in_minutes, 'Timeout', class: 'label-light'
           = f.number_field :build_timeout_in_minutes, class: 'form-control', min: '0'
           %p.help-block
-            Per job in minutes. If a job passes this threshold, it will be marked as failed.
+            Per job in minutes. If a job passes this threshold, it will be marked as failed
             = link_to icon('question-circle'), help_page_path('user/project/pipelines/settings', anchor: 'timeout'), target: '_blank'
 
         %hr

app/views/projects/registry/repositories/index.html.haml

 - page_title "Container Registry"
 
-%hr
-
-%ul.content-list
-  %li.light.prepend-top-default
+.row.prepend-top-default.append-bottom-default
+  .col-lg-3
+    %h4.prepend-top-0
+      = page_title
     %p
-      A 'container image' is a snapshot of a container.
-      You can host your container images with GitLab.
-      %br
-      To start using container images hosted on GitLab you first need to login:
-      %pre
-        %code
+      With the Docker Container Registry integrated into GitLab, every project
+      can have its own space to store its Docker images.
+    %p.append-bottom-0
+      = succeed '.' do
+        Learn more about
+        = link_to 'Container Registry', help_page_path('user/project/container_registry'), target: '_blank'
+
+  .col-lg-9
+    .panel.panel-default
+      .panel-heading
+        %h4.panel-title
+          How to use the Container Registry
+      .panel-body
+        %p
+          First log in to GitLab’s Container Registry using your GitLab username
+          and password. If you have
+          = link_to '2FA enabled', help_page_path('user/profile/account/two_factor_authentication'), target: '_blank'
+          you need to use a
+          = succeed ':' do
+            = link_to 'personal access token', help_page_path('user/profile/account/two_factor_authentication', anchor: 'personal-access-tokens'), target: '_blank'
+        %pre
           docker login #{Gitlab.config.registry.host_port}
-      %br
-      Then you are free to create and upload a container image with build and push commands:
-      %pre
-        docker build -t #{escape_once(@project.container_registry_url)}/image .
         %br
-        docker push #{escape_once(@project.container_registry_url)}/image
+        %p
+          Once you log in, you’re free to create and upload a container image
+          using the common
+          %code build
+          and
+          %code push
+          commands:
+        %pre
+          :plain
+            docker build -t #{escape_once(@project.container_registry_url)} .
+            docker push #{escape_once(@project.container_registry_url)}
 
-  - if @images.blank?
-    .nothing-here-block No container image repositories in Container Registry for this project.
+        %hr
+        %h5.prepend-top-default
+          Use different image names
+        %p.light
+          GitLab supports up to 3 levels of image names. The following
+          examples of images are valid for your project:
+        %pre
+          :plain
+            #{escape_once(@project.container_registry_url)}:tag
+            #{escape_once(@project.container_registry_url)}/optional-image-name:tag
+            #{escape_once(@project.container_registry_url)}/optional-name/optional-image-name:tag
 
-  - else
-    = render partial: 'image', collection: @images
+    - if @images.blank?
+      %p.settings-message.text-center.append-bottom-default
+        No container images stored for this project. Add one by following the
+        instructions above.
+    - else
+      = render partial: 'image', collection: @images

app/views/projects/variables/_content.html.haml

 %h4.prepend-top-0
-  Secret Variables
+  Secret variables
+  = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'secret-variables'), target: '_blank'
 %p
-  These variables will be set to environment by the runner.
+  These variables will be set to environment by the runner, and could be protected by exposing only to protected branches or tags.
 %p
   So you can use them for passwords, secret keys or whatever you want.
 %p

app/views/projects/variables/_form.html.haml

@@ -7,4 +7,13 @@
   .form-group
     = f.label :value, "Value", class: "label-light"
     = f.text_area :value, class: "form-control", placeholder: "PROJECT_VARIABLE"
+  .form-group
+    .checkbox
+      = f.label :protected do
+        = f.check_box :protected
+        %strong Protected
+      .help-block
+        This variable will be passed only to pipelines running on protected branches and tags
+        = link_to icon('question-circle'), help_page_path('ci/variables/README', anchor: 'protected-secret-variables'), target: '_blank'
+
   = f.submit btn_text, class: "btn btn-save"

app/views/projects/variables/_table.html.haml

 .table-responsive.variables-table
   %table.table
     %colgroup
+      %col
       %col
       %col
       %col{ width: 100 }
     %thead
       %th Key
       %th Value
+      %th Protected
       %th
     %tbody
       - @project.variables.order_key_asc.each do |variable|
@@ -14,6 +16,7 @@
           %tr
             %td.variable-key= variable.key
             %td.variable-value{ "data-value" => variable.value }******
+            %td.variable-protected= Gitlab::Utils.boolean_to_yes_no(variable.protected)
             %td.variable-menu
               = link_to namespace_project_variable_path(@project.namespace, @project, variable), class: "btn btn-transparent btn-variable-edit" do
                 %span.sr-only

changelogs/unreleased/24196-protected-variables.yml

+---
+title: Add protected variables which would only be passed to protected branches or
+  protected tags
+merge_request: 11688
+author:

changelogs/unreleased/30651-improve-container-registry-description.yml

+---
+title: Add changelog for improved Registry description
+merge_request: 11816
+author:

changelogs/unreleased/31602-display-whether-shared-runner-is-enabled-in-the-admin-dashboard.yml

+---
+title: Display Shared Runner status in Admin Dashboard
+merge_request: 11783
+author: Ivan Chernov

changelogs/unreleased/31644-make-cookie-sessions-unique.yml

+---
+title: Update session cookie key name to be unique to instance in development
+merge_request:
+author:

changelogs/unreleased/aliyun-backup-provider.yml

+---
+title: Add Aliyun OSS as the backup storage provider
+merge_request: 9721
+author: Yuanfei Zhu

changelogs/unreleased/bugfix-v3-deploy_keys-can_push.yml

+---
+title: "Fixed handling of the `can_push` attribute in the v3 deploy_keys api"
+merge_request: 11607
+author: Richard Clamp

changelogs/unreleased/dm-comment-on-mr-commit-discussion.yml

+---
+title: Fix replying to a commit discussion displayed in the context of an MR
+merge_request:
+author:

changelogs/unreleased/dm-fix-jump-button.yml

+---
+title: Fix title of discussion jump button at top of page
+merge_request:
+author:

changelogs/unreleased/dm-gravatar-username.yml

+---
+title: Add username parameter to gravatar URL
+merge_request:
+author:

changelogs/unreleased/fix-n-plus-one-queries-for-user-access.yml

+---
+title: Fix N+1 queries for non-members in comment threads
+merge_request:
+author:

changelogs/unreleased/fix_diff_line_comments.yml

+---
+title: 'Fix: A diff comment on a change at last line of a file shows as two comments
+  in discussion'
+merge_request:
+author:

changelogs/unreleased/migrate-artifacts-to-a-new-path.yml

+---
+title: Migrate artifacts to a new path
+merge_request:
+author:

changelogs/unreleased/zj-drop-fk-if-exists.yml

+---
+title: Remove foreigh key on ci_trigger_schedules only if it exists
+merge_request:
+author:

config/gitlab.yml.example

@@ -169,7 +169,7 @@ production: &base
   ## Gravatar
   ## For Libravatar see: http://doc.gitlab.com/ce/customization/libravatar.html
   gravatar:
-    # gravatar urls: possible placeholders: %{hash} %{size} %{email}
+    # gravatar urls: possible placeholders: %{hash} %{size} %{email} %{username}
     # plain_url: "http://..."     # default: http://www.gravatar.com/avatar/%{hash}?s=%{size}&d=identicon
     # ssl_url:   "https://..."    # default: https://secure.gravatar.com/avatar/%{hash}?s=%{size}&d=identicon
 

config/initializers/session_store.rb

@@ -10,6 +10,12 @@ rescue
   Settings.gitlab['session_expire_delay'] ||= 10080
 end
 
+cookie_key = if Rails.env.development?
+               "_gitlab_session_#{Digest::SHA256.hexdigest(Rails.root.to_s)}"
+             else
+               "_gitlab_session"
+             end
+
 if Rails.env.test?
   Gitlab::Application.config.session_store :cookie_store, key: "_gitlab_session"
 else
@@ -19,7 +25,7 @@ else
   Gitlab::Application.config.session_store(
     :redis_store, # Using the cookie_store would enable session replay attacks.
     servers: redis_config,
-    key: '_gitlab_session',
+    key: cookie_key,
     secure: Gitlab.config.gitlab.https,
     httponly: true,
     expires_in: Settings.gitlab['session_expire_delay'] * 60,

db/migrate/20170425112628_remove_foreigh_key_ci_trigger_schedules.rb

@@ -4,10 +4,20 @@ class RemoveForeighKeyCiTriggerSchedules < ActiveRecord::Migration
   DOWNTIME = false
 
   def up
-    remove_foreign_key :ci_trigger_schedules, column: :trigger_id
+    if fk_on_trigger_schedules?
+      remove_foreign_key :ci_trigger_schedules, column: :trigger_id
+    end
   end
 
   def down
     # no op, the foreign key should not have been here
   end
+
+  private
+
+  # Not made more generic and lifted to the helpers as Rails 5 will provide
+  # such an API
+  def fk_on_trigger_schedules?
+    connection.foreign_keys(:ci_trigger_schedules).include?("ci_triggers")
+  end
 end

db/migrate/20170524161101_add_protected_to_ci_variables.rb

+class AddProtectedToCiVariables < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:ci_variables, :protected, :boolean, default: false)
+  end
+
+  def down
+    remove_column(:ci_variables, :protected)
+  end
+end

db/post_migrate/20170523083112_migrate_old_artifacts.rb

+class MigrateOldArtifacts < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  # This uses special heuristic to find potential candidates for data migration
+  # Read more about this here: https://gitlab.com/gitlab-org/gitlab-ce/issues/32036#note_30422345
+  
+  def up
+    builds_with_artifacts.find_each do |build|
+      build.migrate_artifacts!
+    end
+  end
+
+  def down
+  end
+
+  private
+
+  def builds_with_artifacts
+    Build.with_artifacts
+      .joins('JOIN projects ON projects.id = ci_builds.project_id')
+      .where('ci_builds.id < ?', min_id)
+      .where('projects.ci_id IS NOT NULL')
+      .select('id', 'created_at', 'project_id', 'projects.ci_id AS ci_id')
+  end
+
+  def min_id
+    Build.joins('JOIN projects ON projects.id = ci_builds.project_id')
+      .where('projects.ci_id IS NULL')
+      .pluck('coalesce(min(ci_builds.id), 0)')
+      .first
+  end
+
+  class Build < ActiveRecord::Base
+    self.table_name = 'ci_builds'
+
+    scope :with_artifacts, -> { where.not(artifacts_file: [nil, '']) }
+
+    def migrate_artifacts!
+      return unless File.exist?(source_artifacts_path)
+      return if File.exist?(target_artifacts_path)
+
+      ensure_target_path
+
+      FileUtils.move(source_artifacts_path, target_artifacts_path)
+    end
+
+    private
+
+    def source_artifacts_path
+      @source_artifacts_path ||= 
+        File.join(Gitlab.config.artifacts.path,
+          created_at.utc.strftime('%Y_%m'),
+          ci_id.to_s, id.to_s)
+    end
+
+    def target_artifacts_path
+      @target_artifacts_path ||= 
+        File.join(Gitlab.config.artifacts.path,
+          created_at.utc.strftime('%Y_%m'),
+          project_id.to_s, id.to_s)
+    end
+
+    def ensure_target_path
+      directory = File.dirname(target_artifacts_path)
+      FileUtils.mkdir_p(directory) unless Dir.exist?(directory)
+    end
+  end
+end

db/schema.rb

@@ -356,6 +356,7 @@ ActiveRecord::Schema.define(version: 20170525174156) do
     t.string "encrypted_value_salt"
     t.string "encrypted_value_iv"
     t.integer "project_id", null: false
+    t.boolean "protected", default: false, null: false
   end
 
   add_index "ci_variables", ["project_id"], name: "index_ci_variables_on_project_id", using: :btree
@@ -1492,4 +1493,4 @@ ActiveRecord::Schema.define(version: 20170525174156) do
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
   add_foreign_key "web_hook_logs", "web_hooks", on_delete: :cascade
-end
\ No newline at end of file
+end

doc/api/build_variables.md

@@ -61,11 +61,12 @@ Create a new build variable.
 POST /projects/:id/variables
 ```
 
-| Attribute | Type    | required | Description           |
-|-----------|---------|----------|-----------------------|
-| `id`      | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user   |
-| `key`     | string  | yes      | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed |
-| `value`   | string  | yes      | The `value` of a variable |
+| Attribute   | Type    | required | Description           |
+|-------------|---------|----------|-----------------------|
+| `id`        | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user   |
+| `key`       | string  | yes      | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed |
+| `value`     | string  | yes      | The `value` of a variable |
+| `protected` | boolean | no       | Whether the variable is protected |
 
 ```
 curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/variables" --form "key=NEW_VARIABLE" --form "value=new value"
@@ -74,7 +75,8 @@ curl --request POST --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitl
 ```json
 {
     "key": "NEW_VARIABLE",
-    "value": "new value"
+    "value": "new value",
+    "protected": false
 }
 ```
 
@@ -86,11 +88,12 @@ Update a project's build variable.
 PUT /projects/:id/variables/:key
 ```
 
-| Attribute | Type    | required | Description             |
-|-----------|---------|----------|-------------------------|
-| `id`      | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user     |
-| `key`     | string  | yes      | The `key` of a variable   |
-| `value`   | string  | yes      | The `value` of a variable |
+| Attribute   | Type    | required | Description             |
+|-------------|---------|----------|-------------------------|
+| `id`        | integer/string | yes      | The ID of a project or [urlencoded NAMESPACE/PROJECT_NAME of the project](README.md#namespaced-path-encoding) owned by the authenticated user     |
+| `key`       | string  | yes      | The `key` of a variable   |
+| `value`     | string  | yes      | The `value` of a variable |
+| `protected` | boolean | no       | Whether the variable is protected |
 
 ```
 curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/projects/1/variables/NEW_VARIABLE" --form "value=updated value"
@@ -99,7 +102,8 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitla
 ```json
 {
     "key": "NEW_VARIABLE",
-    "value": "updated value"
+    "value": "updated value",
+    "protected": true
 }
 ```
 

doc/ci/variables/README.md

@@ -10,7 +10,7 @@ The variables can be overwritten and they take precedence over each other in
 this order:
 
 1. [Trigger variables][triggers] (take precedence over all)
-1. [Secret variables](#secret-variables)
+1. [Secret variables](#secret-variables) or [protected secret variables](#protected-secret-variables)
 1. YAML-defined [job-level variables](../yaml/README.md#job-variables)
 1. YAML-defined [global variables](../yaml/README.md#variables)
 1. [Deployment variables](#deployment-variables)
@@ -154,9 +154,25 @@ storing things like passwords, secret keys and credentials.
 
 Secret variables can be added by going to your project's
 **Settings ➔ Pipelines**, then finding the section called
-**Secret Variables**.
+**Secret variables**.
 
-Once you set them, they will be available for all subsequent jobs.
+Once you set them, they will be available for all subsequent pipelines.
+
+## Protected secret variables
+
+>**Notes:**
+This feature requires GitLab 9.3 or higher.
+
+Secret variables could be protected. Whenever a secret variable is
+protected, it would only be securely passed to pipelines running on the
+[protected branches] or [protected tags]. The other pipelines would not get any
+protected variables.
+
+Protected variables can be added by going to your project's
+**Settings ➔ Pipelines**, then finding the section called
+**Secret variables**, and check *Protected*.
+
+Once you set them, they will be available for all subsequent pipelines.
 
 ## Deployment variables
 
@@ -386,3 +402,5 @@ export CI_REGISTRY_PASSWORD="longalfanumstring"
 [runner]: https://docs.gitlab.com/runner/
 [triggered]: ../triggers/README.md
 [triggers]: ../triggers/README.md#pass-job-variables-to-a-trigger
+[protected branches]: ../../user/project/protected_branches.md
+[protected tags]: ../../user/project/protected_tags.md

doc/customization/libravatar.md

@@ -16,7 +16,7 @@ the configuration options as follows:
 ```yml
   gravatar:
     enabled: true
-    # gravatar URLs: possible placeholders: %{hash} %{size} %{email}
+    # gravatar URLs: possible placeholders: %{hash} %{size} %{email} %{username}
     plain_url: "http://cdn.libravatar.org/avatar/%{hash}?s=%{size}&d=identicon"
 ```
 
@@ -25,7 +25,7 @@ the configuration options as follows:
 ```yml
   gravatar:
     enabled: true
-    # gravatar URLs: possible placeholders: %{hash} %{size} %{email}
+    # gravatar URLs: possible placeholders: %{hash} %{size} %{email} %{username}
     ssl_url: "https://seccdn.libravatar.org/avatar/%{hash}?s=%{size}&d=identicon"
 ```
 

doc/development/README.md

@@ -50,6 +50,7 @@
 - [Adding database indexes](adding_database_indexes.md)
 - [Post Deployment Migrations](post_deployment_migrations.md)
 - [Foreign Keys & Associations](foreign_keys.md)
+- [Serializing Data](serializing_data.md)
 
 ## i18n
 

doc/development/serializing_data.md

+# Serializing Data
+
+**Summary:** don't store serialized data in the database, use separate columns
+and/or tables instead.
+
+Rails makes it possible to store serialized data in JSON, YAML or other formats.
+Such a field can be defined as follows:
+
+```ruby
+class Issue < ActiveRecord::Model
+  serialize :custom_fields
+end
+```
+
+While it may be tempting to store serialized data in the database there are many
+problems with this. This document will outline these problems and provide an
+alternative.
+
+## Serialized Data Is Less Powerful
+
+When using a relational database you have the ability to query individual
+fields, change the schema, index data and so forth. When you use serialized data
+all of that becomes either very difficult or downright impossible. While
+PostgreSQL does offer the ability to query JSON fields it is mostly meant for
+very specialized use cases, and not for more general use. If you use YAML in
+turn there's no way to query the data at all.
+
+## Waste Of Space
+
+Storing serialized data such as JSON or YAML will end up wasting a lot of space.
+This is because these formats often include additional characters (e.g. double
+quotes or newlines) besides the data that you are storing.
+
+## Difficult To Manage
+
+There comes a time where you will need to add a new field to the serialized
+data, or change an existing one. Using serialized data this becomes difficult
+and very time consuming as the only way of doing so is to re-write all the
+stored values. To do so you would have to:
+
+1. Retrieve the data
+1. Parse it into a Ruby structure
+1. Mutate it
+1. Serialize it back to a String
+1. Store it in the database
+
+On the other hand, if one were to use regular columns adding a column would be
+as easy as this:
+
+```sql
+ALTER TABLE table_name ADD COLUMN column_name type;
+```
+
+Such a query would take very little to no time and would immediately apply to
+all rows, without having to re-write large JSON or YAML structures.
+
+Finally, there comes a time when the JSON or YAML structure is no longer
+sufficient and you need to migrate away from it. When storing only a few rows
+this may not be a problem, but when storing millions of rows such a migration
+can easily take hours or even days to complete.
+
+## Relational Databases Are Not Document Stores
+
+When storing data as JSON or YAML you're essentially using your database as if
+it were a document store (e.g. MongoDB), except you're not using any of the
+powerful features provided by a typical RDBMS _nor_ are you using any of the
+features provided by a typical document store (e.g. the ability to index fields
+of documents with variable fields). In other words, it's a waste.
+
+## Consistent Fields
+
+One argument sometimes made in favour of serialized data is having to store
+widely varying fields and values. Sometimes this is truly the case, and then
+perhaps it might make sense to use serialized data. However, in 99% of the cases
+the fields and types stored tend to be the same for every row. Even if there is
+a slight difference you can still use separate columns and just not set the ones
+you don't need.
+
+## The Solution
+
+The solution is very simple: just use separate columns and/or separate tables.
+This will allow you to use all the features provided by your database, it will
+make it easier to manage and migrate the data, you'll conserve space, you can
+index the data efficiently and so forth.

doc/raketasks/backup_restore.md

@@ -133,7 +133,7 @@ It uses the [Fog library](http://fog.io/) to perform the upload.
 In the example below we use Amazon S3 for storage, but Fog also lets you use
 [other storage providers](http://fog.io/storage/). GitLab
 [imports cloud drivers](https://gitlab.com/gitlab-org/gitlab-ce/blob/30f5b9a5b711b46f1065baf755e413ceced5646b/Gemfile#L88)
-for AWS, Google, OpenStack Swift and Rackspace as well. A local driver is
+for AWS, Google, OpenStack Swift, Rackspace and Aliyun as well. A local driver is
 [also available](#uploading-to-locally-mounted-shares).
 
 For omnibus packages, add the following to `/etc/gitlab/gitlab.rb`:

doc/user/project/container_registry.md

@@ -95,8 +95,6 @@ and click **Registry** in the project menu.
 This view will show you all tags in your project and will easily allow you to
 delete them.
 
-![Container Registry panel](img/container_registry_panel.png)
-
 ## Build and push images using GitLab CI
 
 > **Note:**

doc/user/project/pipelines/schedules.md

@@ -53,7 +53,7 @@ Sidekiq, which runs according to its interval. For example, if you set a
 schedule to create a pipeline every minute (`* * * * *`) and the Sidekiq worker
 runs on 00:00 and 12:00 every day (`0 */12 * * *`), only 2 pipelines will be
 created per day. To change the Sidekiq worker's frequency, you have to edit the
-`trigger_schedule_worker_cron` value in your `gitlab.rb` and restart GitLab.
+`pipeline_schedule_worker_cron` value in your `gitlab.rb` and restart GitLab.
 For GitLab.com, you can check the [dedicated settings page][settings]. If you
 don't have admin access to the server, ask your administrator.
 

lib/api/entities.rb

@@ -675,6 +675,7 @@ module API
 
     class Variable < Grape::Entity
       expose :key, :value
+      expose :protected?, as: :protected
     end
 
     class Pipeline < PipelineBasic

lib/api/helpers.rb

@@ -311,6 +311,16 @@ module API
       end
     end
 
+    def present_artifacts!(artifacts_file)
+      return not_found! unless artifacts_file.exists?
+  
+      if artifacts_file.file_storage?
+        present_file!(artifacts_file.path, artifacts_file.filename)
+      else
+        redirect_to(artifacts_file.url)
+      end
+    end
+
     private
 
     def private_token

lib/api/jobs.rb

@@ -224,16 +224,6 @@ module API
         find_build(id) || not_found!
       end
 
-      def present_artifacts!(artifacts_file)
-        if !artifacts_file.file_storage?
-          redirect_to(build.artifacts_file.url)
-        elsif artifacts_file.exists?
-          present_file!(artifacts_file.path, artifacts_file.filename)
-        else
-          not_found!
-        end
-      end
-
       def filter_builds(builds, scope)
         return builds if scope.nil? || scope.empty?
 

lib/api/runner.rb

@@ -241,16 +241,7 @@ module API
       get '/:id/artifacts' do
         job = authenticate_job!
 
-        artifacts_file = job.artifacts_file
-        unless artifacts_file.file_storage?
-          return redirect_to job.artifacts_file.url
-        end
-
-        unless artifacts_file.exists?
-          not_found!
-        end
-
-        present_file!(artifacts_file.path, artifacts_file.filename)
+        present_artifacts!(job.artifacts_file)
       end
     end
   end

lib/api/v3/builds.rb

@@ -225,16 +225,6 @@ module API
           find_build(id) || not_found!
         end
 
-        def present_artifacts!(artifacts_file)
-          if !artifacts_file.file_storage?
-            redirect_to(build.artifacts_file.url)
-          elsif artifacts_file.exists?
-            present_file!(artifacts_file.path, artifacts_file.filename)
-          else
-            not_found!
-          end
-        end
-
         def filter_builds(builds, scope)
           return builds if scope.nil? || scope.empty?
 

lib/api/v3/deploy_keys.rb

@@ -41,6 +41,7 @@ module API
           params do
             requires :key, type: String, desc: 'The new deploy key'
             requires :title, type: String, desc: 'The name of the deploy key'
+            optional :can_push, type: Boolean, desc: "Can deploy key push to the project's repository"
           end
           post ":id/#{path}" do
             params[:key].strip!

lib/api/variables.rb

@@ -42,6 +42,7 @@ module API
       params do
         requires :key, type: String, desc: 'The key of the variable'
         requires :value, type: String, desc: 'The value of the variable'
+        optional :protected, type: String, desc: 'Whether the variable is protected'
       end
       post ':id/variables' do
         variable = user_project.variables.create(declared(params, include_parent_namespaces: false).to_h)
@@ -59,13 +60,14 @@ module API
       params do
         optional :key, type: String, desc: 'The key of the variable'
         optional :value, type: String, desc: 'The value of the variable'
+        optional :protected, type: String, desc: 'Whether the variable is protected'
       end
       put ':id/variables/:key' do
         variable = user_project.variables.find_by(key: params[:key])
 
         return not_found!('Variable') unless variable
 
-        if variable.update(value: params[:value])
+        if variable.update(declared_params(include_missing: false).except(:key))
           present variable, with: Entities::Variable
         else
           render_validation_error!(variable)

lib/backup/artifacts.rb

@@ -3,7 +3,7 @@ require 'backup/files'
 module Backup
   class Artifacts < Files
     def initialize
-      super('artifacts', ArtifactUploader.artifacts_path)
+      super('artifacts', ArtifactUploader.local_artifacts_store)
     end
 
     def create_files_dir

lib/ci/api/builds.rb

@@ -187,14 +187,14 @@ module Ci
           build = authenticate_build!
           artifacts_file = build.artifacts_file
 
-          unless artifacts_file.file_storage?
-            return redirect_to build.artifacts_file.url
-          end
-
           unless artifacts_file.exists?
             not_found!
           end
 
+          unless artifacts_file.file_storage?
+            return redirect_to build.artifacts_file.url
+          end
+
           present_file!(artifacts_file.path, artifacts_file.filename)
         end
 

lib/gitlab/diff/line.rb

@@ -59,6 +59,10 @@ module Gitlab
         type == 'match'
       end
 
+      def discussable?
+        !['match', 'new-nonewline', 'old-nonewline'].include?(type)
+      end
+
       def as_json(opts = nil)
         {
           type: type,

lib/gitlab/utils.rb

@@ -21,5 +21,13 @@ module Gitlab
 
       nil
     end
+
+    def boolean_to_yes_no(bool)
+      if bool
+        'Yes'
+      else
+        'No'
+      end
+    end
   end
 end

rubocop/cop/activerecord_serialize.rb

+module RuboCop
+  module Cop
+    # Cop that prevents the use of `serialize` in ActiveRecord models.
+    class ActiverecordSerialize < RuboCop::Cop::Cop
+      MSG = 'Do not store serialized data in the database, use separate columns and/or tables instead'.freeze
+
+      def on_send(node)
+        return unless in_models?(node)
+
+        add_offense(node, :selector) if node.children[1] == :serialize
+      end
+
+      def models_path
+        File.join(Dir.pwd, 'app', 'models')
+      end
+
+      def in_models?(node)
+        path = node.location.expression.source_buffer.name
+
+        path.start_with?(models_path)
+      end
+    end
+  end
+end

rubocop/rubocop.rb

 require_relative 'cop/custom_error_class'
 require_relative 'cop/gem_fetcher'
+require_relative 'cop/activerecord_serialize'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_column_with_default_to_large_table'
 require_relative 'cop/migration/add_concurrent_foreign_key'

spec/factories/ci/variables.rb

@@ -3,6 +3,10 @@ FactoryGirl.define do
     sequence(:key) { |n| "VARIABLE_#{n}" }
     value 'VARIABLE_VALUE'
 
+    trait(:protected) do
+      protected true
+    end
+
     project factory: :empty_project
   end
 end

spec/features/container_registry_spec.rb

@@ -19,7 +19,7 @@ describe "Container Registry" do
     scenario 'user visits container registry main page' do
       visit_container_registry
 
-      expect(page).to have_content 'No container image repositories'
+      expect(page).to have_content 'No container images'
     end
   end
 

spec/features/merge_requests/discussion_spec.rb

@@ -5,7 +5,7 @@ feature 'Merge Request Discussions', feature: true do
     login_as :admin
   end
 
-  context "Diff discussions" do
+  describe "Diff discussions" do
     let(:merge_request) { create(:merge_request, importing: true) }
     let(:project) { merge_request.source_project }
     let!(:old_merge_request_diff) { merge_request.merge_request_diffs.create(diff_refs: outdated_diff_refs) }
@@ -48,4 +48,43 @@ feature 'Merge Request Discussions', feature: true do
       end
     end
   end
+
+  describe 'Commit comments displayed in MR context', :js do
+    let(:merge_request) { create(:merge_request) }
+    let(:project) { merge_request.project }
+
+    shared_examples 'a functional discussion' do
+      let(:discussion_id) { note.discussion_id(merge_request) }
+
+      it 'is displayed' do
+        expect(page).to have_css(".discussion[data-discussion-id='#{discussion_id}']")
+      end
+
+      it 'can be replied to' do
+        within(".discussion[data-discussion-id='#{discussion_id}']") do
+          click_button 'Reply...'
+          fill_in 'note[note]', with: 'Test!'
+          click_button 'Comment'
+
+          expect(page).to have_css('.note', count: 2)
+        end
+      end
+    end
+
+    before(:each) do
+      visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+    end
+
+    context 'a regular commit comment' do
+      let(:note) { create(:note_on_commit, project: project) }
+
+      it_behaves_like 'a functional discussion'
+    end
+
+    context 'a commit diff comment' do
+      let(:note) { create(:diff_note_on_commit, project: project) }
+
+      it_behaves_like 'a functional discussion'
+    end
+  end
 end

spec/features/variables_spec.rb

@@ -19,7 +19,7 @@ describe 'Project variables', js: true do
     end
   end
 
-  it 'adds new variable' do
+  it 'adds new secret variable' do
     fill_in('variable_key', with: 'key')
     fill_in('variable_value', with: 'key value')
     click_button('Add new variable')
@@ -27,6 +27,7 @@ describe 'Project variables', js: true do
     expect(page).to have_content('Variables were successfully updated.')
     page.within('.variables-table') do
       expect(page).to have_content('key')
+      expect(page).to have_content('No')
     end
   end
 
@@ -41,6 +42,19 @@ describe 'Project variables', js: true do
     end
   end
 
+  it 'adds new protected variable' do
+    fill_in('variable_key', with: 'key')
+    fill_in('variable_value', with: 'value')
+    check('Protected')
+    click_button('Add new variable')
+
+    expect(page).to have_content('Variables were successfully updated.')
+    page.within('.variables-table') do
+      expect(page).to have_content('key')
+      expect(page).to have_content('Yes')
+    end
+  end
+
   it 'reveals and hides new variable' do
     fill_in('variable_key', with: 'key')
     fill_in('variable_value', with: 'key value')
@@ -85,7 +99,7 @@ describe 'Project variables', js: true do
     click_button('Save variable')
 
     expect(page).to have_content('Variable was successfully updated.')
-    expect(project.variables.first.value).to eq('key value')
+    expect(project.variables(true).first.value).to eq('key value')
   end
 
   it 'edits variable with empty value' do
@@ -98,6 +112,34 @@ describe 'Project variables', js: true do
     click_button('Save variable')
 
     expect(page).to have_content('Variable was successfully updated.')
-    expect(project.variables.first.value).to eq('')
+    expect(project.variables(true).first.value).to eq('')
+  end
+
+  it 'edits variable to be protected' do
+    page.within('.variables-table') do
+      find('.btn-variable-edit').click
+    end
+
+    expect(page).to have_content('Update variable')
+    check('Protected')
+    click_button('Save variable')
+
+    expect(page).to have_content('Variable was successfully updated.')
+    expect(project.variables(true).first).to be_protected
+  end
+
+  it 'edits variable to be unprotected' do
+    project.variables.first.update(protected: true)
+
+    page.within('.variables-table') do
+      find('.btn-variable-edit').click
+    end
+
+    expect(page).to have_content('Update variable')
+    uncheck('Protected')
+    click_button('Save variable')
+
+    expect(page).to have_content('Variable was successfully updated.')
+    expect(project.variables(true).first).not_to be_protected
   end
 end

spec/helpers/diff_helper_spec.rb

@@ -129,6 +129,33 @@ describe DiffHelper do
     end
   end
 
+  describe '#parallel_diff_discussions' do
+    let(:discussion) { { 'abc_3_3' => 'comment' } }
+    let(:diff_file) { double(line_code: 'abc_3_3') }
+
+    before do
+      helper.instance_variable_set(:@grouped_diff_discussions, discussion)
+    end
+
+    it 'does not put comments on nonewline lines' do
+      left = Gitlab::Diff::Line.new('\\nonewline', 'old-nonewline', 3, 3, 3)
+      right = Gitlab::Diff::Line.new('\\nonewline', 'new-nonewline', 3, 3, 3)
+
+      result = helper.parallel_diff_discussions(left, right, diff_file)
+
+      expect(result).to eq([nil, nil])
+    end
+
+    it 'puts comments on added lines' do
+      left = Gitlab::Diff::Line.new('\\nonewline', 'old-nonewline', 3, 3, 3)
+      right = Gitlab::Diff::Line.new('new line', 'add', 3, 3, 3)
+
+      result = helper.parallel_diff_discussions(left, right, diff_file)
+
+      expect(result).to eq([nil, 'comment'])
+    end
+  end
+
   describe "#diff_match_line" do
     let(:old_pos) { 40 }
     let(:new_pos) { 50 }

spec/lib/gitlab/utils_spec.rb

+require 'spec_helper'
+
 describe Gitlab::Utils, lib: true do
-  delegate :to_boolean, to: :described_class
+  delegate :to_boolean, :boolean_to_yes_no, to: :described_class
 
   describe '.to_boolean' do
     it 'accepts booleans' do
@@ -30,4 +32,11 @@ describe Gitlab::Utils, lib: true do
       expect(to_boolean(nil)).to be_nil
     end
   end
+
+  describe '.boolean_to_yes_no' do
+    it 'converts booleans to Yes or No' do
+      expect(boolean_to_yes_no(true)).to eq('Yes')
+      expect(boolean_to_yes_no(false)).to eq('No')
+    end
+  end
 end

spec/migrations/migrate_old_artifacts_spec.rb

+# encoding: utf-8
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20170523083112_migrate_old_artifacts.rb')
+
+describe MigrateOldArtifacts do
+  let(:migration) { described_class.new }
+  let!(:directory) { Dir.mktmpdir }
+
+  before do
+    allow(Gitlab.config.artifacts).to receive(:path).and_return(directory)
+  end
+  
+  after do
+    FileUtils.remove_entry_secure(directory)
+  end
+
+  context 'with migratable data' do
+    let(:project1) { create(:empty_project, ci_id: 2) }
+    let(:project2) { create(:empty_project, ci_id: 3) }
+    let(:project3) { create(:empty_project) }
+
+    let(:pipeline1) { create(:ci_empty_pipeline, project: project1) }
+    let(:pipeline2) { create(:ci_empty_pipeline, project: project2) }
+    let(:pipeline3) { create(:ci_empty_pipeline, project: project3) }
+
+    let!(:build_with_legacy_artifacts) { create(:ci_build, pipeline: pipeline1) }
+    let!(:build_without_artifacts) { create(:ci_build, pipeline: pipeline1) }
+    let!(:build2) { create(:ci_build, :artifacts, pipeline: pipeline2) }
+    let!(:build3) { create(:ci_build, :artifacts, pipeline: pipeline3) }
+
+    before do
+      store_artifacts_in_legacy_path(build_with_legacy_artifacts)
+    end
+
+    it "legacy artifacts are not accessible" do
+      expect(build_with_legacy_artifacts.artifacts?).to be_falsey
+    end
+
+    it "legacy artifacts are set" do
+      expect(build_with_legacy_artifacts.artifacts_file_identifier).not_to be_nil
+    end
+
+    describe '#min_id' do
+      subject { migration.send(:min_id) }
+
+      it 'returns the newest build for which ci_id is not defined' do
+        is_expected.to eq(build3.id)
+      end
+    end
+
+    describe '#builds_with_artifacts' do
+      subject { migration.send(:builds_with_artifacts).map(&:id) }
+
+      it 'returns a list of builds that has artifacts and could be migrated' do
+        is_expected.to contain_exactly(build_with_legacy_artifacts.id, build2.id)
+      end
+    end
+
+    describe '#up' do
+      context 'when migrating artifacts' do
+        before do
+          migration.up
+        end
+
+        it 'all files do have artifacts' do
+          Ci::Build.with_artifacts do |build|
+            expect(build).to have_artifacts
+          end
+        end
+
+        it 'artifacts are no longer present on legacy path' do
+          expect(File.exist?(legacy_path(build_with_legacy_artifacts))).to eq(false)
+        end
+      end
+
+      context 'when there are aritfacts in old and new directory' do
+        before do
+          store_artifacts_in_legacy_path(build2)
+
+          migration.up
+        end
+
+        it 'does not move old files' do
+          expect(File.exist?(legacy_path(build2))).to eq(true)
+        end
+      end
+    end
+
+    private
+
+    def store_artifacts_in_legacy_path(build)
+      FileUtils.mkdir_p(legacy_path(build))
+
+      FileUtils.copy(
+        Rails.root.join('spec/fixtures/ci_build_artifacts.zip'),
+        File.join(legacy_path(build), "ci_build_artifacts.zip"))
+  
+      FileUtils.copy(
+        Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'),
+        File.join(legacy_path(build), "ci_build_artifacts_metadata.gz"))
+
+      build.update_columns(
+        artifacts_file: 'ci_build_artifacts.zip',
+        artifacts_metadata: 'ci_build_artifacts_metadata.gz')
+
+      build.reload
+    end
+
+    def legacy_path(build)
+      File.join(directory,
+        build.created_at.utc.strftime('%Y_%m'),
+        build.project.ci_id.to_s,
+        build.id.to_s)
+    end
+  end
+end

spec/models/ci/build_spec.rb

@@ -1329,16 +1329,49 @@ describe Ci::Build, :models do
       it { is_expected.to include(tag_variable) }
     end
 
-    context 'when secure variable is defined' do
-      let(:secure_variable) do
+    context 'when secret variable is defined' do
+      let(:secret_variable) do
         { key: 'SECRET_KEY', value: 'secret_value', public: false }
       end
 
       before do
-        build.project.variables << Ci::Variable.new(key: 'SECRET_KEY', value: 'secret_value')
+        create(:ci_variable,
+               secret_variable.slice(:key, :value).merge(project: project))
       end
 
-      it { is_expected.to include(secure_variable) }
+      it { is_expected.to include(secret_variable) }
+    end
+
+    context 'when protected variable is defined' do
+      let(:protected_variable) do
+        { key: 'PROTECTED_KEY', value: 'protected_value', public: false }
+      end
+
+      before do
+        create(:ci_variable,
+               :protected,
+               protected_variable.slice(:key, :value).merge(project: project))
+      end
+
+      context 'when the branch is protected' do
+        before do
+          create(:protected_branch, project: build.project, name: build.ref)
+        end
+
+        it { is_expected.to include(protected_variable) }
+      end
+
+      context 'when the tag is protected' do
+        before do
+          create(:protected_tag, project: build.project, name: build.ref)
+        end
+
+        it { is_expected.to include(protected_variable) }
+      end
+
+      context 'when the ref is not protected' do
+        it { is_expected.not_to include(protected_variable) }
+      end
     end
 
     context 'when build is for triggers' do
@@ -1460,15 +1493,30 @@ describe Ci::Build, :models do
     end
 
     context 'returns variables in valid order' do
+      let(:build_pre_var) { { key: 'build', value: 'value' } }
+      let(:project_pre_var) { { key: 'project', value: 'value' } }
+      let(:pipeline_pre_var) { { key: 'pipeline', value: 'value' } }
+      let(:build_yaml_var) { { key: 'yaml', value: 'value' } }
+
       before do
-        allow(build).to receive(:predefined_variables) { ['predefined'] }
-        allow(project).to receive(:predefined_variables) { ['project'] }
-        allow(pipeline).to receive(:predefined_variables) { ['pipeline'] }
-        allow(build).to receive(:yaml_variables) { ['yaml'] }
-        allow(project).to receive(:secret_variables) { ['secret'] }
+        allow(build).to receive(:predefined_variables) { [build_pre_var] }
+        allow(project).to receive(:predefined_variables) { [project_pre_var] }
+        allow(pipeline).to receive(:predefined_variables) { [pipeline_pre_var] }
+        allow(build).to receive(:yaml_variables) { [build_yaml_var] }
+
+        allow(project).to receive(:secret_variables_for).with(build.ref) do
+          [create(:ci_variable, key: 'secret', value: 'value')]
+        end
       end
 
-      it { is_expected.to eq(%w[predefined project pipeline yaml secret]) }
+      it do
+        is_expected.to eq(
+          [build_pre_var,
+           project_pre_var,
+           pipeline_pre_var,
+           build_yaml_var,
+           { key: 'secret', value: 'value', public: false }])
+      end
     end
   end
 

spec/models/ci/variable_spec.rb

@@ -12,11 +12,33 @@ describe Ci::Variable, models: true do
   it { is_expected.not_to allow_value('foo bar').for(:key) }
   it { is_expected.not_to allow_value('foo/bar').for(:key) }
 
-  before :each do
-    subject.value = secret_value
+  describe '.unprotected' do
+    subject { described_class.unprotected }
+
+    context 'when variable is protected' do
+      before do
+        create(:ci_variable, :protected)
+      end
+
+      it 'returns nothing' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when variable is not protected' do
+      let(:variable) { create(:ci_variable, protected: false) }
+
+      it 'returns the variable' do
+        is_expected.to contain_exactly(variable)
+      end
+    end
   end
 
   describe '#value' do
+    before do
+      subject.value = secret_value
+    end
+
     it 'stores the encrypted value' do
       expect(subject.encrypted_value).not_to be_nil
     end
@@ -36,4 +58,11 @@ describe Ci::Variable, models: true do
         to raise_error(OpenSSL::Cipher::CipherError, 'bad decrypt')
     end
   end
+
+  describe '#to_runner_variable' do
+    it 'returns a hash for the runner' do
+      expect(subject.to_runner_variable)
+        .to eq(key: subject.key, value: subject.value, public: false)
+    end
+  end
 end

spec/models/diff_note_spec.rb

@@ -160,12 +160,6 @@ describe DiffNote, models: true do
       context "when noteable is a commit" do
         let(:diff_note) { create(:diff_note_on_commit, project: project, position: position) }
 
-        it "doesn't use the DiffPositionUpdateService" do
-          expect(Notes::DiffPositionUpdateService).not_to receive(:new)
-
-          diff_note
-        end
-
         it "doesn't update the position" do
           diff_note
 
@@ -178,12 +172,6 @@ describe DiffNote, models: true do
         let(:diff_note) { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
 
         context "when the note is active" do
-          it "doesn't use the DiffPositionUpdateService" do
-            expect(Notes::DiffPositionUpdateService).not_to receive(:new)
-
-            diff_note
-          end
-
           it "doesn't update the position" do
             diff_note
 
@@ -197,18 +185,11 @@ describe DiffNote, models: true do
             allow(merge_request).to receive(:diff_refs).and_return(commit.diff_refs)
           end
 
-          it "uses the DiffPositionUpdateService" do
-            service = instance_double("Notes::DiffPositionUpdateService")
-            expect(Notes::DiffPositionUpdateService).to receive(:new).with(
-              project,
-              nil,
-              old_diff_refs: position.diff_refs,
-              new_diff_refs: commit.diff_refs,
-              paths: [path]
-            ).and_return(service)
-            expect(service).to receive(:execute)
-
+          it "updates the position" do
             diff_note
+
+            expect(diff_note.original_position).to eq(position)
+            expect(diff_note.position).not_to eq(position)
           end
         end
       end

spec/models/merge_request_spec.rb

@@ -1178,7 +1178,7 @@ describe MergeRequest, models: true do
   end
 
   describe "#reload_diff" do
-    let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) }
+    let(:discussion) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject).to_discussion }
 
     let(:commit) { subject.project.commit(sample_commit.id) }
 
@@ -1197,7 +1197,7 @@ describe MergeRequest, models: true do
       subject.reload_diff
     end
 
-    it "updates diff note positions" do
+    it "updates diff discussion positions" do
       old_diff_refs = subject.diff_refs
 
       # Update merge_request_diff so that #diff_refs will return commit.diff_refs
@@ -1211,15 +1211,15 @@ describe MergeRequest, models: true do
         subject.merge_request_diff(true)
       end
 
-      expect(Notes::DiffPositionUpdateService).to receive(:new).with(
+      expect(Discussions::UpdateDiffPositionService).to receive(:new).with(
         subject.project,
         subject.author,
         old_diff_refs: old_diff_refs,
         new_diff_refs: commit.diff_refs,
-        paths: note.position.paths
+        paths: discussion.position.paths
       ).and_call_original
 
-      expect_any_instance_of(Notes::DiffPositionUpdateService).to receive(:execute).with(note)
+      expect_any_instance_of(Discussions::UpdateDiffPositionService).to receive(:execute).with(discussion).and_call_original
       expect_any_instance_of(DiffNote).to receive(:save).once
 
       subject.reload_diff(subject.author)

spec/models/project_spec.rb

@@ -1749,6 +1749,90 @@ describe Project, models: true do
     end
   end
 
+  describe '#secret_variables_for' do
+    let(:project) { create(:empty_project) }
+
+    let!(:secret_variable) do
+      create(:ci_variable, value: 'secret', project: project)
+    end
+
+    let!(:protected_variable) do
+      create(:ci_variable, :protected, value: 'protected', project: project)
+    end
+
+    subject { project.secret_variables_for('ref') }
+
+    shared_examples 'ref is protected' do
+      it 'contains all the variables' do
+        is_expected.to contain_exactly(secret_variable, protected_variable)
+      end
+    end
+
+    context 'when the ref is not protected' do
+      before do
+        stub_application_setting(
+          default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+      end
+
+      it 'contains only the secret variables' do
+        is_expected.to contain_exactly(secret_variable)
+      end
+    end
+
+    context 'when the ref is a protected branch' do
+      before do
+        create(:protected_branch, name: 'ref', project: project)
+      end
+
+      it_behaves_like 'ref is protected'
+    end
+
+    context 'when the ref is a protected tag' do
+      before do
+        create(:protected_tag, name: 'ref', project: project)
+      end
+
+      it_behaves_like 'ref is protected'
+    end
+  end
+
+  describe '#protected_for?' do
+    let(:project) { create(:empty_project) }
+
+    subject { project.protected_for?('ref') }
+
+    context 'when the ref is not protected' do
+      before do
+        stub_application_setting(
+          default_branch_protection: Gitlab::Access::PROTECTION_NONE)
+      end
+
+      it 'returns false' do
+        is_expected.to be_falsey
+      end
+    end
+
+    context 'when the ref is a protected branch' do
+      before do
+        create(:protected_branch, name: 'ref', project: project)
+      end
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+
+    context 'when the ref is a protected tag' do
+      before do
+        create(:protected_tag, name: 'ref', project: project)
+      end
+
+      it 'returns true' do
+        is_expected.to be_truthy
+      end
+    end
+  end
+
   describe '#update_project_statistics' do
     let(:project) { create(:empty_project) }
 

spec/models/project_team_spec.rb

@@ -327,69 +327,114 @@ describe ProjectTeam, models: true do
     end
   end
 
-  shared_examples_for "#max_member_access_for_users" do |enable_request_store|
-    describe "#max_member_access_for_users" do
+  shared_examples 'max member access for users' do
+    let(:project) { create(:project) }
+    let(:group) { create(:group) }
+    let(:second_group) { create(:group) }
+
+    let(:master) { create(:user) }
+    let(:reporter) { create(:user) }
+    let(:guest) { create(:user) }
+
+    let(:promoted_guest) { create(:user) }
+
+    let(:group_developer) { create(:user) }
+    let(:second_developer) { create(:user) }
+
+    let(:user_without_access) { create(:user) }
+    let(:second_user_without_access) { create(:user) }
+
+    let(:users) do
+      [master, reporter, promoted_guest, guest, group_developer, second_developer, user_without_access].map(&:id)
+    end
+
+    let(:expected) do
+      {
+        master.id => Gitlab::Access::MASTER,
+        reporter.id => Gitlab::Access::REPORTER,
+        promoted_guest.id => Gitlab::Access::DEVELOPER,
+        guest.id => Gitlab::Access::GUEST,
+        group_developer.id => Gitlab::Access::DEVELOPER,
+        second_developer.id => Gitlab::Access::MASTER,
+        user_without_access.id => Gitlab::Access::NO_ACCESS
+      }
+    end
+
+    before do
+      project.add_master(master)
+      project.add_reporter(reporter)
+      project.add_guest(promoted_guest)
+      project.add_guest(guest)
+
+      project.project_group_links.create(
+        group: group,
+        group_access: Gitlab::Access::DEVELOPER
+      )
+
+      group.add_master(promoted_guest)
+      group.add_developer(group_developer)
+      group.add_developer(second_developer)
+
+      project.project_group_links.create(
+        group: second_group,
+        group_access: Gitlab::Access::MASTER
+      )
+
+      second_group.add_master(second_developer)
+    end
+
+    it 'returns correct roles for different users' do
+      expect(project.team.max_member_access_for_user_ids(users)).to eq(expected)
+    end
+  end
+
+  describe '#max_member_access_for_user_ids' do
+    context 'with RequestStore enabled' do
       before do
-        RequestStore.begin! if enable_request_store
+        RequestStore.begin!
       end
 
       after do
-        if enable_request_store
-          RequestStore.end!
-          RequestStore.clear!
-        end
+        RequestStore.end!
+        RequestStore.clear!
       end
 
-      it 'returns correct roles for different users' do
-        master = create(:user)
-        reporter = create(:user)
-        promoted_guest = create(:user)
-        guest = create(:user)
-        project = create(:empty_project)
+      include_examples 'max member access for users'
 
-        project.add_master(master)
-        project.add_reporter(reporter)
-        project.add_guest(promoted_guest)
-        project.add_guest(guest)
+      def access_levels(users)
+        project.team.max_member_access_for_user_ids(users)
+      end
+
+      it 'does not perform extra queries when asked for users who have already been found' do
+        access_levels(users)
+
+        expect { access_levels(users) }.not_to exceed_query_limit(0)
 
-        group = create(:group)
-        group_developer = create(:user)
-        second_developer = create(:user)
-        project.project_group_links.create(
-          group: group,
-          group_access: Gitlab::Access::DEVELOPER)
-
-        group.add_master(promoted_guest)
-        group.add_developer(group_developer)
-        group.add_developer(second_developer)
-
-        second_group = create(:group)
-        project.project_group_links.create(
-          group: second_group,
-          group_access: Gitlab::Access::MASTER)
-        second_group.add_master(second_developer)
-
-        users = [master, reporter, promoted_guest, guest, group_developer, second_developer].map(&:id)
-
-        expected = {
-          master.id => Gitlab::Access::MASTER,
-          reporter.id => Gitlab::Access::REPORTER,
-          promoted_guest.id => Gitlab::Access::DEVELOPER,
-          guest.id => Gitlab::Access::GUEST,
-          group_developer.id => Gitlab::Access::DEVELOPER,
-          second_developer.id => Gitlab::Access::MASTER
-        }
-
-        expect(project.team.max_member_access_for_user_ids(users)).to eq(expected)
+        expect(access_levels(users)).to eq(expected)
       end
-    end
-  end
 
-  describe '#max_member_access_for_users with RequestStore' do
-    it_behaves_like "#max_member_access_for_users", true
-  end
+      it 'only requests the extra users when uncached users are passed' do
+        new_user = create(:user)
+        second_new_user = create(:user)
+        all_users = users + [new_user.id, second_new_user.id]
+
+        expected_all = expected.merge(new_user.id => Gitlab::Access::NO_ACCESS,
+                                      second_new_user.id => Gitlab::Access::NO_ACCESS)
+
+        access_levels(users)
 
-  describe '#max_member_access_for_users without RequestStore' do
-    it_behaves_like "#max_member_access_for_users", false
+        queries = ActiveRecord::QueryRecorder.new { access_levels(all_users) }
+
+        expect(queries.count).to eq(1)
+        expect(queries.log_message).to match(/\W#{new_user.id}\W/)
+        expect(queries.log_message).to match(/\W#{second_new_user.id}\W/)
+        expect(queries.log_message).not_to match(/\W#{promoted_guest.id}\W/)
+        expect(access_levels(all_users)).to eq(expected_all)
+      end
+    end
+
+    context 'with RequestStore disabled' do
+      include_examples 'max member access for users'
+    end
   end
 end

spec/requests/api/v3/deploy_keys_spec.rb

@@ -105,6 +105,15 @@ describe API::V3::DeployKeys do
 
         expect(response).to have_http_status(201)
       end
+
+      it 'accepts can_push parameter' do
+        key_attrs = attributes_for :write_access_key
+
+        post v3_api("/projects/#{project.id}/#{path}", admin), key_attrs
+
+        expect(response).to have_http_status(201)
+        expect(json_response['can_push']).to eq(true)
+      end
     end
 
     describe "DELETE /projects/:id/#{path}/:key_id" do

spec/requests/api/variables_spec.rb

@@ -42,6 +42,7 @@ describe API::Variables do
 
         expect(response).to have_http_status(200)
         expect(json_response['value']).to eq(variable.value)
+        expect(json_response['protected']).to eq(variable.protected?)
       end
 
       it 'responds with 404 Not Found if requesting non-existing variable' do
@@ -72,12 +73,13 @@ describe API::Variables do
     context 'authorized user with proper permissions' do
       it 'creates variable' do
         expect do
-          post api("/projects/#{project.id}/variables", user), key: 'TEST_VARIABLE_2', value: 'VALUE_2'
+          post api("/projects/#{project.id}/variables", user), key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true
         end.to change{project.variables.count}.by(1)
 
         expect(response).to have_http_status(201)
         expect(json_response['key']).to eq('TEST_VARIABLE_2')
         expect(json_response['value']).to eq('VALUE_2')
+        expect(json_response['protected']).to be_truthy
       end
 
       it 'does not allow to duplicate variable key' do
@@ -112,13 +114,14 @@ describe API::Variables do
         initial_variable = project.variables.first
         value_before = initial_variable.value
 
-        put api("/projects/#{project.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP'
+        put api("/projects/#{project.id}/variables/#{variable.key}", user), value: 'VALUE_1_UP', protected: true
 
         updated_variable = project.variables.first
 
         expect(response).to have_http_status(200)
         expect(value_before).to eq(variable.value)
         expect(updated_variable.value).to eq('VALUE_1_UP')
+        expect(updated_variable).to be_protected
       end
 
       it 'responds with 404 Not Found if requesting non-existing variable' do

spec/rubocop/cop/activerecord_serialize_spec.rb

+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/activerecord_serialize'
+
+describe RuboCop::Cop::ActiverecordSerialize do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'inside the app/models directory' do
+    it 'registers an offense when serialize is used' do
+      allow(cop).to receive(:in_models?).and_return(true)
+
+      inspect_source(cop, 'serialize :foo')
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+      end
+    end
+  end
+
+  context 'outside the app/models directory' do
+    it 'does nothing' do
+      allow(cop).to receive(:in_models?).and_return(false)
+
+      inspect_source(cop, 'serialize :foo')
+
+      expect(cop.offenses).to be_empty
+    end
+  end
+end

spec/services/notes/diff_position_update_service_spec.rb → spec/services/discussions/update_diff_position_service_spec.rb

 require 'spec_helper'
 
-describe Notes::DiffPositionUpdateService, services: true do
+describe Discussions::UpdateDiffPositionService, services: true do
   let(:project) { create(:project, :repository) }
   let(:current_user) { project.owner }
   let(:create_commit) { project.commit("913c66a37b4a45b9769037c55c2d238bd0942d2e") }
@@ -138,7 +138,7 @@ describe Notes::DiffPositionUpdateService, services: true do
   # .. ..
 
   describe "#execute" do
-    let(:note) { create(:diff_note_on_merge_request, project: project, position: old_position) }
+    let(:discussion) { create(:diff_note_on_merge_request, project: project, position: old_position).to_discussion }
 
     let(:old_position) do
       Gitlab::Diff::Position.new(
@@ -154,11 +154,11 @@ describe Notes::DiffPositionUpdateService, services: true do
       let(:line) { 16 }
 
       it "updates the position" do
-        subject.execute(note)
+        subject.execute(discussion)
 
-        expect(note.original_position).to eq(old_position)
-        expect(note.position).not_to eq(old_position)
-        expect(note.position.new_line).to eq(22)
+        expect(discussion.original_position).to eq(old_position)
+        expect(discussion.position).not_to eq(old_position)
+        expect(discussion.position.new_line).to eq(22)
       end
     end
 
@@ -166,27 +166,27 @@ describe Notes::DiffPositionUpdateService, services: true do
       let(:line) { 9 }
 
       it "doesn't update the position" do
-        subject.execute(note)
+        subject.execute(discussion)
 
-        expect(note.original_position).to eq(old_position)
-        expect(note.position).to eq(old_position)
+        expect(discussion.original_position).to eq(old_position)
+        expect(discussion.position).to eq(old_position)
       end
 
       it 'sets the change position' do
-        subject.execute(note)
+        subject.execute(discussion)
 
-        change_position = note.change_position
+        change_position = discussion.change_position
         expect(change_position.start_sha).to eq(old_diff_refs.head_sha)
         expect(change_position.head_sha).to eq(new_diff_refs.head_sha)
         expect(change_position.old_line).to eq(9)
         expect(change_position.new_line).to be_nil
       end
 
-      it 'creates a system note' do
+      it 'creates a system discussion' do
         expect(SystemNoteService).to receive(:diff_discussion_outdated).with(
-          note.to_discussion, project, current_user, instance_of(Gitlab::Diff::Position))
+          discussion, project, current_user, instance_of(Gitlab::Diff::Position))
 
-        subject.execute(note)
+        subject.execute(discussion)
       end
     end
   end

spec/services/gravatar_service_spec.rb

+require 'spec_helper'
+
+describe GravatarService, service: true do
+  describe '#execute' do
+    let(:url) { 'http://example.com/avatar?hash=%{hash}&size=%{size}&email=%{email}&username=%{username}' }
+
+    before do
+      allow(Gitlab.config.gravatar).to receive(:plain_url).and_return(url)
+    end
+
+    it 'replaces the placeholders' do
+      avatar_url = described_class.new.execute('user@example.com', 100, 2, username: 'user')
+
+      expect(avatar_url).to include("hash=#{Digest::MD5.hexdigest('user@example.com')}")
+      expect(avatar_url).to include("size=200")
+      expect(avatar_url).to include("email=user%40example.com")
+      expect(avatar_url).to include("username=user")
+    end
+  end
+end

spec/uploaders/artifact_uploader_spec.rb

+require 'rails_helper'
+
+describe ArtifactUploader do
+  let(:job) { create(:ci_build) }
+  let(:uploader) { described_class.new(job, :artifacts_file) }
+  let(:path) { Gitlab.config.artifacts.path }
+
+  describe '.local_artifacts_store' do
+    subject { described_class.local_artifacts_store }
+
+    it "delegate to artifacts path" do
+      expect(Gitlab.config.artifacts).to receive(:path)
+
+      subject
+    end
+  end
+
+  describe '.artifacts_upload_path' do
+    subject { described_class.artifacts_upload_path }
+    
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('tmp/uploads/') }
+  end
+
+  describe '#store_dir' do
+    subject { uploader.store_dir }
+    
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with("#{job.project_id}/#{job.id}") }
+  end
+
+  describe '#cache_dir' do
+    subject { uploader.cache_dir }
+    
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('tmp/cache') }
+  end
+end