From ddc12c2af5fdac67d7d3dff5b5d34638514374dc Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 1 Jun 2018 13:08:42 +0000 Subject: [PATCH 001/210] Merge branch '46913-appearance-uploader-fields-and-description-html-are-missing-in-cached-version' into 'master' Resolve "Appearance uploader fields and description HTML are missing in cached version" Closes #46913 See merge request gitlab-org/gitlab-ce!19203 --- app/models/concerns/cacheable_attributes.rb | 38 +++-- .../concerns/cacheable_attributes_spec.rb | 133 ++++++++++++++---- 2 files changed, 134 insertions(+), 37 deletions(-) diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb index b32459fdabf..d58d7165969 100644 --- a/app/models/concerns/cacheable_attributes.rb +++ b/app/models/concerns/cacheable_attributes.rb @@ -6,15 +6,16 @@ module CacheableAttributes end class_methods do + def cache_key + "#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:#{Rails.version}".freeze + end + # Can be overriden def current_without_cache last end - def cache_key - "#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json".freeze - end - + # Can be overriden def defaults {} end @@ -24,10 +25,18 @@ module CacheableAttributes end def cached - json_attributes = Rails.cache.read(cache_key) - return nil unless json_attributes.present? + if RequestStore.active? + RequestStore[:"#{name}_cached_attributes"] ||= retrieve_from_cache + else + retrieve_from_cache + end + end + + def retrieve_from_cache + record = Rails.cache.read(cache_key) + ensure_cache_setup if record.present? - build_from_defaults(JSON.parse(json_attributes)) + record end def current @@ -35,7 +44,12 @@ module CacheableAttributes return cached_record if cached_record.present? current_without_cache.tap { |current_record| current_record&.cache! } - rescue + rescue => e + if Rails.env.production? + Rails.logger.warn("Cached record for #{name} couldn't be loaded, falling back to uncached record: #{e}") + else + raise e + end # Fall back to an uncached value if there are any problems (e.g. Redis down) current_without_cache end @@ -46,9 +60,15 @@ module CacheableAttributes # Gracefully handle when Redis is not available. For example, # omnibus may fail here during gitlab:assets:compile. end + + def ensure_cache_setup + # This is a workaround for a Rails bug that causes attribute methods not + # to be loaded when read from cache: https://github.com/rails/rails/issues/27348 + define_attribute_methods + end end def cache! - Rails.cache.write(self.class.cache_key, attributes.to_json) + Rails.cache.write(self.class.cache_key, self) end end diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb index 49e4b23ebc7..c6331c5ec15 100644 --- a/spec/models/concerns/cacheable_attributes_spec.rb +++ b/spec/models/concerns/cacheable_attributes_spec.rb @@ -22,7 +22,7 @@ describe CacheableAttributes do attr_accessor :attributes - def initialize(attrs = {}) + def initialize(attrs = {}, *) @attributes = attrs end end @@ -52,7 +52,7 @@ describe CacheableAttributes do describe '.cache_key' do it 'excludes cache attributes' do - expect(minimal_test_class.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json") + expect(minimal_test_class.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:#{Rails.version}") end end @@ -75,49 +75,117 @@ describe CacheableAttributes do context 'without any attributes given' do it 'intializes a new object with the defaults' do - expect(minimal_test_class.build_from_defaults).not_to be_persisted + expect(minimal_test_class.build_from_defaults.attributes).to eq(minimal_test_class.defaults) end end - context 'without attributes given' do + context 'with attributes given' do it 'intializes a new object with the given attributes merged into the defaults' do expect(minimal_test_class.build_from_defaults(foo: 'd').attributes[:foo]).to eq('d') end end + + describe 'edge cases on concrete implementations' do + describe '.build_from_defaults' do + context 'without any attributes given' do + it 'intializes all attributes even if they are nil' do + record = ApplicationSetting.build_from_defaults + + expect(record).not_to be_persisted + expect(record.sign_in_text).to be_nil + end + end + end + end end describe '.current', :use_clean_rails_memory_store_caching do context 'redis unavailable' do - it 'returns an uncached record' do + before do allow(minimal_test_class).to receive(:last).and_return(:last) - expect(Rails.cache).to receive(:read).and_raise(Redis::BaseError) + expect(Rails.cache).to receive(:read).with(minimal_test_class.cache_key).and_raise(Redis::BaseError) + end + + context 'in production environment' do + before do + expect(Rails.env).to receive(:production?).and_return(true) + end + + it 'returns an uncached record and logs a warning' do + expect(Rails.logger).to receive(:warn).with("Cached record for TestClass couldn't be loaded, falling back to uncached record: Redis::BaseError") - expect(minimal_test_class.current).to eq(:last) + expect(minimal_test_class.current).to eq(:last) + end + end + + context 'in other environments' do + before do + expect(Rails.env).to receive(:production?).and_return(false) + end + + it 'returns an uncached record and logs a warning' do + expect(Rails.logger).not_to receive(:warn) + + expect { minimal_test_class.current }.to raise_error(Redis::BaseError) + end end end context 'when a record is not yet present' do it 'does not cache nil object' do # when missing settings a nil object is returned, but not cached - allow(minimal_test_class).to receive(:last).twice.and_return(nil) + allow(ApplicationSetting).to receive(:current_without_cache).twice.and_return(nil) - expect(minimal_test_class.current).to be_nil - expect(Rails.cache.exist?(minimal_test_class.cache_key)).to be(false) + expect(ApplicationSetting.current).to be_nil + expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(false) end - it 'cache non-nil object' do - # when the settings are set the method returns a valid object - allow(minimal_test_class).to receive(:last).and_call_original + it 'caches non-nil object' do + create(:application_setting) - expect(minimal_test_class.current).to eq(minimal_test_class.last) - expect(Rails.cache.exist?(minimal_test_class.cache_key)).to be(true) + expect(ApplicationSetting.current).to eq(ApplicationSetting.last) + expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(true) # subsequent calls retrieve the record from the cache - last_record = minimal_test_class.last - expect(minimal_test_class).not_to receive(:last) - expect(minimal_test_class.current.attributes).to eq(last_record.attributes) + last_record = ApplicationSetting.last + expect(ApplicationSetting).not_to receive(:current_without_cache) + expect(ApplicationSetting.current.attributes).to eq(last_record.attributes) end end + + describe 'edge cases' do + describe 'caching behavior', :use_clean_rails_memory_store_caching do + it 'retrieves upload fields properly' do + ar_record = create(:appearance, :with_logo) + ar_record.cache! + + cache_record = Appearance.current + + expect(cache_record).to be_persisted + expect(cache_record.logo).to be_an(AttachmentUploader) + expect(cache_record.logo.url).to end_with('/dk.png') + end + + it 'retrieves markdown fields properly' do + ar_record = create(:appearance, description: '**Hello**') + ar_record.cache! + + cache_record = Appearance.current + + expect(cache_record.description).to eq('**Hello**') + expect(cache_record.description_html).to eq('

Hello

') + end + end + end + + it 'uses RequestStore in addition to Rails.cache', :request_store do + # Warm up the cache + create(:application_setting).cache! + + expect(Rails.cache).to receive(:read).with(ApplicationSetting.cache_key).once.and_call_original + + 2.times { ApplicationSetting.current } + end end describe '.cached', :use_clean_rails_memory_store_caching do @@ -127,27 +195,36 @@ describe CacheableAttributes do end end - context 'when cached settings do not include the latest defaults' do + context 'when cached is warm' do before do - Rails.cache.write(minimal_test_class.cache_key, { bar: 'b', baz: 'c' }.to_json) - minimal_test_class.define_singleton_method(:defaults) do - { foo: 'a', bar: 'b', baz: 'c' } - end + # Warm up the cache + create(:appearance).cache! end - it 'includes attributes from defaults' do - expect(minimal_test_class.cached.attributes[:foo]).to eq(minimal_test_class.defaults[:foo]) + it 'retrieves the record from cache' do + expect(ActiveRecord::QueryRecorder.new { Appearance.cached }.count).to eq(0) + expect(Appearance.cached).to eq(Appearance.current_without_cache) end end end describe '#cache!', :use_clean_rails_memory_store_caching do - let(:appearance_record) { create(:appearance) } + let(:record) { create(:appearance) } it 'caches the attributes' do - appearance_record.cache! + record.cache! - expect(Rails.cache.read(Appearance.cache_key)).to eq(appearance_record.attributes.to_json) + expect(Rails.cache.read(Appearance.cache_key)).to eq(record) + end + + describe 'edge cases' do + let(:record) { create(:appearance) } + + it 'caches the attributes' do + record.cache! + + expect(Rails.cache.read(Appearance.cache_key)).to eq(record) + end end end end -- GitLab From 53e4fb1766276bed12a8e8050e3b3bfd09959ff2 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 1 Jun 2018 18:56:21 +0000 Subject: [PATCH 002/210] Merge branch 'fix-feature-memoization' into 'master' Use RequestStore to memoize Flipper features so that memoized values are cleared between requests See merge request gitlab-org/gitlab-ce!19281 --- app/services/ci/register_job_service.rb | 5 ++++- lib/feature.rb | 11 +++++++++-- spec/lib/feature_spec.rb | 24 ++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 4291631913a..925775aea0b 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -89,7 +89,10 @@ module Ci end def builds_for_group_runner - hierarchy_groups = Gitlab::GroupHierarchy.new(runner.groups).base_and_descendants + # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL` + groups = ::Group.joins(:runner_namespaces).merge(runner.runner_namespaces) + + hierarchy_groups = Gitlab::GroupHierarchy.new(groups).base_and_descendants projects = Project.where(namespace_id: hierarchy_groups) .with_group_runners_enabled .with_builds_enabled diff --git a/lib/feature.rb b/lib/feature.rb index 6474de6e56d..314ae224d90 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -63,8 +63,15 @@ class Feature end def flipper - Thread.current[:flipper] ||= - Flipper.new(flipper_adapter).tap { |flip| flip.memoize = true } + if RequestStore.active? + RequestStore[:flipper] ||= build_flipper_instance + else + @flipper ||= build_flipper_instance + end + end + + def build_flipper_instance + Flipper.new(flipper_adapter).tap { |flip| flip.memoize = true } end # This method is called from config/initializers/flipper.rb and can be used diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 10020511bf8..6eb10497428 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -64,4 +64,28 @@ describe Feature do expect(described_class.all).to eq(features.to_a) end end + + describe '.flipper' do + shared_examples 'a memoized Flipper instance' do + it 'memoizes the Flipper instance' do + expect(Flipper).to receive(:new).once.and_call_original + + 2.times do + described_class.flipper + end + end + end + + context 'when request store is inactive' do + before do + described_class.instance_variable_set(:@flipper, nil) + end + + it_behaves_like 'a memoized Flipper instance' + end + + context 'when request store is inactive', :request_store do + it_behaves_like 'a memoized Flipper instance' + end + end end -- GitLab From 60f8ee664788c2e776f312c9e5a2faa91da104e7 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 1 Jun 2018 17:59:04 +0000 Subject: [PATCH 003/210] Merge branch 'sh-bump-ruby-2.4' into 'master' Upgrade to Ruby 2.4.4 See merge request gitlab-org/gitlab-ce!19055 --- .gitlab-ci.yml | 6 +++--- .ruby-version | 2 +- app/models/clusters/platforms/kubernetes.rb | 4 ++-- app/models/clusters/providers/gcp.rb | 2 +- app/models/concerns/has_variable.rb | 2 +- app/models/pages_domain.rb | 2 +- app/models/project_import_data.rb | 2 +- app/models/remote_mirror.rb | 2 +- config/initializers/{secret_token.rb => 01_secret_token.rb} | 3 +++ config/settings.rb | 4 ++++ .../20160302152808_remove_wrong_import_url_from_projects.rb | 2 +- ...rate_kubernetes_service_to_new_clusters_architectures.rb | 2 +- doc/install/installation.md | 6 +++--- spec/initializers/secret_token_spec.rb | 2 +- spec/models/concerns/has_variable_spec.rb | 4 +++- 15 files changed, 27 insertions(+), 18 deletions(-) rename config/initializers/{secret_token.rb => 01_secret_token.rb} (95%) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index d3daab78940..1679ae378c9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1,4 +1,4 @@ -image: "dev.gitlab.org:5005/gitlab/gitlab-build-images:ruby-2.3.7-golang-1.9-git-2.17-chrome-65.0-node-8.x-yarn-1.2-postgresql-9.6" +image: "dev.gitlab.org:5005/gitlab/gitlab-build-images:ruby-2.4.4-golang-1.9-git-2.17-chrome-65.0-node-8.x-yarn-1.2-postgresql-9.6" .dedicated-runner: &dedicated-runner retry: 1 @@ -6,7 +6,7 @@ image: "dev.gitlab.org:5005/gitlab/gitlab-build-images:ruby-2.3.7-golang-1.9-git - gitlab-org .default-cache: &default-cache - key: "ruby-2.3.7-debian-stretch-with-yarn" + key: "ruby-2.4.4-debian-stretch-with-yarn" paths: - vendor/ruby - .yarn-cache/ @@ -550,7 +550,7 @@ static-analysis: script: - scripts/static-analysis cache: - key: "ruby-2.3.7-debian-stretch-with-yarn-and-rubocop" + key: "ruby-2.4.4-debian-stretch-with-yarn-and-rubocop" paths: - vendor/ruby - .yarn-cache/ diff --git a/.ruby-version b/.ruby-version index 00355e29d11..79a614418f7 100644 --- a/.ruby-version +++ b/.ruby-version @@ -1 +1 @@ -2.3.7 +2.4.4 diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb index ba6552f238f..25eac5160f1 100644 --- a/app/models/clusters/platforms/kubernetes.rb +++ b/app/models/clusters/platforms/kubernetes.rb @@ -11,12 +11,12 @@ module Clusters attr_encrypted :password, mode: :per_attribute_iv, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, algorithm: 'aes-256-cbc' attr_encrypted :token, mode: :per_attribute_iv, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, algorithm: 'aes-256-cbc' before_validation :enforce_namespace_to_lower_case diff --git a/app/models/clusters/providers/gcp.rb b/app/models/clusters/providers/gcp.rb index 7fac32466ab..eb2e42fd3fe 100644 --- a/app/models/clusters/providers/gcp.rb +++ b/app/models/clusters/providers/gcp.rb @@ -11,7 +11,7 @@ module Clusters attr_encrypted :access_token, mode: :per_attribute_iv, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, algorithm: 'aes-256-cbc' validates :gcp_project_id, diff --git a/app/models/concerns/has_variable.rb b/app/models/concerns/has_variable.rb index 8a241e4374a..c8e20c0ab81 100644 --- a/app/models/concerns/has_variable.rb +++ b/app/models/concerns/has_variable.rb @@ -13,7 +13,7 @@ module HasVariable attr_encrypted :value, mode: :per_attribute_iv_and_salt, insecure_mode: true, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, algorithm: 'aes-256-cbc' def key=(new_key) diff --git a/app/models/pages_domain.rb b/app/models/pages_domain.rb index 2e478a24778..bfea64c3759 100644 --- a/app/models/pages_domain.rb +++ b/app/models/pages_domain.rb @@ -19,7 +19,7 @@ class PagesDomain < ActiveRecord::Base attr_encrypted :key, mode: :per_attribute_iv_and_salt, insecure_mode: true, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, algorithm: 'aes-256-cbc' after_initialize :set_verification_code diff --git a/app/models/project_import_data.rb b/app/models/project_import_data.rb index 6da6632f4f2..1d7089ccfc7 100644 --- a/app/models/project_import_data.rb +++ b/app/models/project_import_data.rb @@ -3,7 +3,7 @@ require 'carrierwave/orm/activerecord' class ProjectImportData < ActiveRecord::Base belongs_to :project, inverse_of: :import_data attr_encrypted :credentials, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, marshal: true, encode: true, mode: :per_attribute_iv_and_salt, diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb index bbf8fd9c6a7..aba1f2f384f 100644 --- a/app/models/remote_mirror.rb +++ b/app/models/remote_mirror.rb @@ -5,7 +5,7 @@ class RemoteMirror < ActiveRecord::Base UNPROTECTED_BACKOFF_DELAY = 5.minutes attr_encrypted :credentials, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, marshal: true, encode: true, mode: :per_attribute_iv_and_salt, diff --git a/config/initializers/secret_token.rb b/config/initializers/01_secret_token.rb similarity index 95% rename from config/initializers/secret_token.rb rename to config/initializers/01_secret_token.rb index 750a5b34f3b..02bded43083 100644 --- a/config/initializers/secret_token.rb +++ b/config/initializers/01_secret_token.rb @@ -1,3 +1,6 @@ +# This file needs to be loaded BEFORE any initializers that attempt to +# prepend modules that require access to secrets (e.g. EE's 0_as_concern.rb). +# # Be sure to restart your server when you modify this file. require 'securerandom' diff --git a/config/settings.rb b/config/settings.rb index 69d637761ea..4aa903109ea 100644 --- a/config/settings.rb +++ b/config/settings.rb @@ -85,6 +85,10 @@ class Settings < Settingslogic File.expand_path(path, Rails.root) end + def attr_encrypted_db_key_base + Gitlab::Application.secrets.db_key_base[0..31] + end + private def base_url(config) diff --git a/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb b/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb index 611767ac7fe..95105118764 100644 --- a/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb +++ b/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb @@ -8,7 +8,7 @@ class RemoveWrongImportUrlFromProjects < ActiveRecord::Migration extend AttrEncrypted attr_accessor :credentials attr_encrypted :credentials, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, marshal: true, encode: true, :mode => :per_attribute_iv_and_salt, diff --git a/db/post_migrate/20171124104327_migrate_kubernetes_service_to_new_clusters_architectures.rb b/db/post_migrate/20171124104327_migrate_kubernetes_service_to_new_clusters_architectures.rb index 11b581e4b57..1586a7eb92f 100644 --- a/db/post_migrate/20171124104327_migrate_kubernetes_service_to_new_clusters_architectures.rb +++ b/db/post_migrate/20171124104327_migrate_kubernetes_service_to_new_clusters_architectures.rb @@ -48,7 +48,7 @@ class MigrateKubernetesServiceToNewClustersArchitectures < ActiveRecord::Migrati attr_encrypted :token, mode: :per_attribute_iv, - key: Gitlab::Application.secrets.db_key_base, + key: Settings.attr_encrypted_db_key_base, algorithm: 'aes-256-cbc' end diff --git a/doc/install/installation.md b/doc/install/installation.md index a0ae9017f71..34268c67140 100644 --- a/doc/install/installation.md +++ b/doc/install/installation.md @@ -133,9 +133,9 @@ Remove the old Ruby 1.8 if present: Download Ruby and compile it: mkdir /tmp/ruby && cd /tmp/ruby - curl --remote-name --progress https://cache.ruby-lang.org/pub/ruby/2.3/ruby-2.3.7.tar.gz - echo '540996fec64984ab6099e34d2f5820b14904f15a ruby-2.3.7.tar.gz' | shasum -c - && tar xzf ruby-2.3.7.tar.gz - cd ruby-2.3.7 + curl --remote-name --progress https://cache.ruby-lang.org/pub/ruby/2.4/ruby-2.4.4.tar.gz + echo 'ec82b0d53bd0adad9b19e6b45e44d54e9ec3f10c ruby-2.4.4.tar.gz' | shasum -c - && tar xzf ruby-2.4.4.tar.gz + cd ruby-2.4.4 ./configure --disable-install-rdoc make diff --git a/spec/initializers/secret_token_spec.rb b/spec/initializers/secret_token_spec.rb index d56e14e0e0b..c3dfd7bedbe 100644 --- a/spec/initializers/secret_token_spec.rb +++ b/spec/initializers/secret_token_spec.rb @@ -1,5 +1,5 @@ require 'spec_helper' -require_relative '../../config/initializers/secret_token' +require_relative '../../config/initializers/01_secret_token' describe 'create_tokens' do include StubENV diff --git a/spec/models/concerns/has_variable_spec.rb b/spec/models/concerns/has_variable_spec.rb index f87869a2fdc..3fbe86c5b56 100644 --- a/spec/models/concerns/has_variable_spec.rb +++ b/spec/models/concerns/has_variable_spec.rb @@ -45,8 +45,10 @@ describe HasVariable do end it 'fails to decrypt if iv is incorrect' do - subject.encrypted_value_iv = SecureRandom.hex + # attr_encrypted expects the IV to be 16 bytes and base64-encoded + subject.encrypted_value_iv = [SecureRandom.hex(8)].pack('m') subject.instance_variable_set(:@value, nil) + expect { subject.value } .to raise_error(OpenSSL::Cipher::CipherError, 'bad decrypt') end -- GitLab From 0b63e17c95b9fa4788e1f117bce97dc31080294c Mon Sep 17 00:00:00 2001 From: Mayra Cabrera Date: Fri, 1 Jun 2018 15:43:56 -0500 Subject: [PATCH 004/210] Update VERSION to 11.0.0-rc1 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 8ca9077d87b..69d3cd2a771 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -11.0.0-pre +11.0.0-rc1 -- GitLab From 1841da16abe864b3dae19636fee9e9bbe9a01b56 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 1 Jun 2018 21:24:32 +0000 Subject: [PATCH 005/210] Merge branch 'sh-add-ruby-2.4-comment' into 'master' Add comment about the need for truncating keys in Ruby 2.4 See merge request gitlab-org/gitlab-ce!19330 --- config/settings.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/config/settings.rb b/config/settings.rb index 4aa903109ea..58f38d103ea 100644 --- a/config/settings.rb +++ b/config/settings.rb @@ -85,7 +85,14 @@ class Settings < Settingslogic File.expand_path(path, Rails.root) end + # Returns a 256-bit key for attr_encrypted def attr_encrypted_db_key_base + # Ruby 2.4+ requires passing in the exact required length for OpenSSL keys + # (https://github.com/ruby/ruby/commit/ce635262f53b760284d56bb1027baebaaec175d1). + # Previous versions quietly truncated the input. + # + # The default mode for the attr_encrypted gem is to use a 256-bit key. + # We truncate the 128-byte string to 32 bytes. Gitlab::Application.secrets.db_key_base[0..31] end -- GitLab From cfcc471ee11aa12b743a83f163e03aac6a5de098 Mon Sep 17 00:00:00 2001 From: Mayra Cabrera Date: Mon, 4 Jun 2018 16:59:56 -0500 Subject: [PATCH 006/210] Revert "Merge branch 'per-project-pipeline-iid' into 'master'" This reverts commit 7b00bc5b2f51a66b8dc8e63c47b13940010dd68d, reversing changes made to 5f0a5367c8081a788f03a8e14907838481128fae. --- app/models/ci/pipeline.rb | 6 -- app/models/concerns/atomic_internal_id.rb | 16 ++--- app/models/concerns/iid_routes.rb | 9 --- app/models/deployment.rb | 1 - app/models/internal_id.rb | 2 +- app/models/issue.rb | 1 - app/models/merge_request.rb | 1 - app/models/milestone.rb | 1 - .../unreleased/per-project-pipeline-iid.yml | 5 -- ...160449_add_pipeline_iid_to_ci_pipelines.rb | 13 ---- ...9_add_index_constraints_to_pipeline_iid.rb | 15 ----- db/schema.rb | 2 - doc/ci/variables/README.md | 4 -- lib/gitlab/ci/pipeline/chain/populate.rb | 3 - .../gitlab/ci/pipeline/chain/populate_spec.rb | 65 ++++--------------- .../import_export/safe_model_attributes.yml | 1 - spec/models/ci/build_spec.rb | 1 - spec/models/ci/pipeline_spec.rb | 13 +--- spec/models/deployment_spec.rb | 1 - spec/models/issue_spec.rb | 1 - spec/models/merge_request_spec.rb | 1 - spec/models/milestone_spec.rb | 2 - .../models/atomic_internal_id_spec.rb | 29 ++------- 23 files changed, 28 insertions(+), 165 deletions(-) delete mode 100644 app/models/concerns/iid_routes.rb delete mode 100644 changelogs/unreleased/per-project-pipeline-iid.yml delete mode 100644 db/migrate/20180424160449_add_pipeline_iid_to_ci_pipelines.rb delete mode 100644 db/migrate/20180425205249_add_index_constraints_to_pipeline_iid.rb diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 5eb30f4aaa0..53af87a271a 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -7,17 +7,12 @@ module Ci include Presentable include Gitlab::OptimisticLocking include Gitlab::Utils::StrongMemoize - include AtomicInternalId belongs_to :project, inverse_of: :pipelines belongs_to :user belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline' belongs_to :pipeline_schedule, class_name: 'Ci::PipelineSchedule' - has_internal_id :iid, scope: :project, presence: false, init: ->(s) do - s&.project&.pipelines&.maximum(:iid) || s&.project&.pipelines&.count - end - has_many :stages has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id, inverse_of: :pipeline has_many :builds, foreign_key: :commit_id, inverse_of: :pipeline @@ -536,7 +531,6 @@ module Ci def predefined_variables Gitlab::Ci::Variables::Collection.new - .append(key: 'CI_PIPELINE_IID', value: iid.to_s) .append(key: 'CI_CONFIG_PATH', value: ci_yaml_file_path) .append(key: 'CI_PIPELINE_SOURCE', value: source.to_s) .append(key: 'CI_COMMIT_MESSAGE', value: git_commit_message) diff --git a/app/models/concerns/atomic_internal_id.rb b/app/models/concerns/atomic_internal_id.rb index 164c704260e..22f516a172f 100644 --- a/app/models/concerns/atomic_internal_id.rb +++ b/app/models/concerns/atomic_internal_id.rb @@ -25,13 +25,9 @@ module AtomicInternalId extend ActiveSupport::Concern module ClassMethods - def has_internal_id(column, scope:, init:, presence: true) # rubocop:disable Naming/PredicateName - before_validation :"ensure_#{scope}_#{column}!", on: :create - validates column, presence: presence - - define_method("ensure_#{scope}_#{column}!") do + def has_internal_id(column, scope:, init:) # rubocop:disable Naming/PredicateName + before_validation(on: :create) do scope_value = association(scope).reader - if read_attribute(column).blank? && scope_value scope_attrs = { scope_value.class.table_name.singularize.to_sym => scope_value } usage = self.class.table_name.to_sym @@ -39,9 +35,13 @@ module AtomicInternalId new_iid = InternalId.generate_next(self, scope_attrs, usage, init) write_attribute(column, new_iid) end - - read_attribute(column) end + + validates column, presence: true, numericality: true end end + + def to_param + iid.to_s + end end diff --git a/app/models/concerns/iid_routes.rb b/app/models/concerns/iid_routes.rb deleted file mode 100644 index 246748cf52c..00000000000 --- a/app/models/concerns/iid_routes.rb +++ /dev/null @@ -1,9 +0,0 @@ -module IidRoutes - ## - # This automagically enforces all related routes to use `iid` instead of `id` - # If you want to use `iid` for some routes and `id` for other routes, this module should not to be included, - # instead you should define `iid` or `id` explictly at each route generators. e.g. pipeline_path(project.id, pipeline.iid) - def to_param - iid.to_s - end -end diff --git a/app/models/deployment.rb b/app/models/deployment.rb index ac86e9e8de0..254764eefde 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -1,6 +1,5 @@ class Deployment < ActiveRecord::Base include AtomicInternalId - include IidRoutes belongs_to :project, required: true belongs_to :environment, required: true diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb index f50f28deffe..f7f930e86ed 100644 --- a/app/models/internal_id.rb +++ b/app/models/internal_id.rb @@ -14,7 +14,7 @@ class InternalId < ActiveRecord::Base belongs_to :project belongs_to :namespace - enum usage: { issues: 0, merge_requests: 1, deployments: 2, milestones: 3, epics: 4, ci_pipelines: 5 } + enum usage: { issues: 0, merge_requests: 1, deployments: 2, milestones: 3, epics: 4 } validates :usage, presence: true diff --git a/app/models/issue.rb b/app/models/issue.rb index d136700836d..41a290f34b4 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -2,7 +2,6 @@ require 'carrierwave/orm/activerecord' class Issue < ActiveRecord::Base include AtomicInternalId - include IidRoutes include Issuable include Noteable include Referable diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4c1628d2bdb..79fc155fd3c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1,6 +1,5 @@ class MergeRequest < ActiveRecord::Base include AtomicInternalId - include IidRoutes include Issuable include Noteable include Referable diff --git a/app/models/milestone.rb b/app/models/milestone.rb index d05dcfd083a..d14e3a4ded5 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -9,7 +9,6 @@ class Milestone < ActiveRecord::Base include CacheMarkdownField include AtomicInternalId - include IidRoutes include Sortable include Referable include StripAttribute diff --git a/changelogs/unreleased/per-project-pipeline-iid.yml b/changelogs/unreleased/per-project-pipeline-iid.yml deleted file mode 100644 index 78a513a9986..00000000000 --- a/changelogs/unreleased/per-project-pipeline-iid.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Add per-project pipeline id -merge_request: 18558 -author: -type: added diff --git a/db/migrate/20180424160449_add_pipeline_iid_to_ci_pipelines.rb b/db/migrate/20180424160449_add_pipeline_iid_to_ci_pipelines.rb deleted file mode 100644 index e8f0c91d612..00000000000 --- a/db/migrate/20180424160449_add_pipeline_iid_to_ci_pipelines.rb +++ /dev/null @@ -1,13 +0,0 @@ -class AddPipelineIidToCiPipelines < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def up - add_column :ci_pipelines, :iid, :integer - end - - def down - remove_column :ci_pipelines, :iid, :integer - end -end diff --git a/db/migrate/20180425205249_add_index_constraints_to_pipeline_iid.rb b/db/migrate/20180425205249_add_index_constraints_to_pipeline_iid.rb deleted file mode 100644 index 3fa59b44d5d..00000000000 --- a/db/migrate/20180425205249_add_index_constraints_to_pipeline_iid.rb +++ /dev/null @@ -1,15 +0,0 @@ -class AddIndexConstraintsToPipelineIid < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_concurrent_index :ci_pipelines, [:project_id, :iid], unique: true, where: 'iid IS NOT NULL' - end - - def down - remove_concurrent_index :ci_pipelines, [:project_id, :iid] - end -end diff --git a/db/schema.rb b/db/schema.rb index 0d6b44d1b92..97247387bc7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -451,12 +451,10 @@ ActiveRecord::Schema.define(version: 20180529093006) do t.integer "config_source" t.boolean "protected" t.integer "failure_reason" - t.integer "iid" end add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree add_index "ci_pipelines", ["pipeline_schedule_id"], name: "index_ci_pipelines_on_pipeline_schedule_id", using: :btree - add_index "ci_pipelines", ["project_id", "iid"], name: "index_ci_pipelines_on_project_id_and_iid", unique: true, where: "(iid IS NOT NULL)", using: :btree add_index "ci_pipelines", ["project_id", "ref", "status", "id"], name: "index_ci_pipelines_on_project_id_and_ref_and_status_and_id", using: :btree add_index "ci_pipelines", ["project_id", "sha"], name: "index_ci_pipelines_on_project_id_and_sha", using: :btree add_index "ci_pipelines", ["project_id"], name: "index_ci_pipelines_on_project_id", using: :btree diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index aa4395b01a9..f10423b92cf 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -72,7 +72,6 @@ future GitLab releases.** | **CI_RUNNER_REVISION** | all | 10.6 | GitLab Runner revision that is executing the current job | | **CI_RUNNER_EXECUTABLE_ARCH** | all | 10.6 | The OS/architecture of the GitLab Runner executable (note that this is not necessarily the same as the environment of the executor) | | **CI_PIPELINE_ID** | 8.10 | 0.5 | The unique id of the current pipeline that GitLab CI uses internally | -| **CI_PIPELINE_IID** | 11.0 | all | The unique id of the current pipeline scoped to project | | **CI_PIPELINE_TRIGGERED** | all | all | The flag to indicate that job was [triggered] | | **CI_PIPELINE_SOURCE** | 10.0 | all | Indicates how the pipeline was triggered. Possible options are: `push`, `web`, `trigger`, `schedule`, `api`, and `pipeline`. For pipelines created before GitLab 9.5, this will show as `unknown` | | **CI_PROJECT_DIR** | all | all | The full path where the repository is cloned and where the job is run | @@ -353,8 +352,6 @@ Running on runner-8a2f473d-project-1796893-concurrent-0 via runner-8a2f473d-mach ++ CI_PROJECT_URL=https://example.com/gitlab-examples/ci-debug-trace ++ export CI_PIPELINE_ID=52666 ++ CI_PIPELINE_ID=52666 -++ export CI_PIPELINE_IID=123 -++ CI_PIPELINE_IID=123 ++ export CI_RUNNER_ID=1337 ++ CI_RUNNER_ID=1337 ++ export CI_RUNNER_DESCRIPTION=shared-runners-manager-1.example.com @@ -442,7 +439,6 @@ export CI_JOB_MANUAL="true" export CI_JOB_TRIGGERED="true" export CI_JOB_TOKEN="abcde-1234ABCD5678ef" export CI_PIPELINE_ID="1000" -export CI_PIPELINE_IID="10" export CI_PROJECT_ID="34" export CI_PROJECT_DIR="/builds/gitlab-org/gitlab-ce" export CI_PROJECT_NAME="gitlab-ce" diff --git a/lib/gitlab/ci/pipeline/chain/populate.rb b/lib/gitlab/ci/pipeline/chain/populate.rb index f34c11ca3c2..69b8a8fc68f 100644 --- a/lib/gitlab/ci/pipeline/chain/populate.rb +++ b/lib/gitlab/ci/pipeline/chain/populate.rb @@ -8,9 +8,6 @@ module Gitlab PopulateError = Class.new(StandardError) def perform! - # Allocate next IID. This operation must be outside of transactions of pipeline creations. - pipeline.ensure_project_iid! - ## # Populate pipeline with block argument of CreatePipelineService#execute. # diff --git a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb index c5a4d9b4778..4d7d6951a51 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb @@ -42,10 +42,6 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do it 'correctly assigns user' do expect(pipeline.builds).to all(have_attributes(user: user)) end - - it 'has pipeline iid' do - expect(pipeline.iid).to be > 0 - end end context 'when pipeline is empty' do @@ -72,10 +68,6 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do expect(pipeline.errors.to_a) .to include 'No stages / jobs for this pipeline.' end - - it 'wastes pipeline iid' do - expect(InternalId.ci_pipelines.where(project_id: project.id).last.last_value).to be > 0 - end end context 'when pipeline has validation errors' do @@ -95,10 +87,6 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do expect(pipeline.errors.to_a) .to include 'Failed to build the pipeline!' end - - it 'wastes pipeline iid' do - expect(InternalId.ci_pipelines.where(project_id: project.id).last.last_value).to be > 0 - end end context 'when there is a seed blocks present' do @@ -123,12 +111,6 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do expect(pipeline.variables.first.key).to eq 'VAR' expect(pipeline.variables.first.value).to eq '123' end - - it 'has pipeline iid' do - step.perform! - - expect(pipeline.iid).to be > 0 - end end context 'when seeds block tries to persist some resources' do @@ -139,12 +121,6 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do it 'raises exception' do expect { step.perform! }.to raise_error(ActiveRecord::RecordNotSaved) end - - it 'wastes pipeline iid' do - expect { step.perform! }.to raise_error - - expect(InternalId.ci_pipelines.where(project_id: project.id).last.last_value).to be > 0 - end end end @@ -156,39 +132,22 @@ describe Gitlab::Ci::Pipeline::Chain::Populate do end end - context 'when variables policy is specified' do - shared_examples_for 'a correct pipeline' do - it 'populates pipeline according to used policies' do - step.perform! - - expect(pipeline.stages.size).to eq 1 - expect(pipeline.stages.first.builds.size).to eq 1 - expect(pipeline.stages.first.builds.first.name).to eq 'rspec' - end + context 'when using only/except build policies' do + let(:config) do + { rspec: { script: 'rspec', stage: 'test', only: ['master'] }, + prod: { script: 'cap prod', stage: 'deploy', only: ['tags'] } } end - context 'when using only/except build policies' do - let(:config) do - { rspec: { script: 'rspec', stage: 'test', only: ['master'] }, - prod: { script: 'cap prod', stage: 'deploy', only: ['tags'] } } - end - - let(:pipeline) do - build(:ci_pipeline, ref: 'master', config: config) - end - - it_behaves_like 'a correct pipeline' + let(:pipeline) do + build(:ci_pipeline, ref: 'master', config: config) + end - context 'when variables expression is specified' do - context 'when pipeline iid is the subject' do - let(:config) do - { rspec: { script: 'rspec', only: { variables: ["$CI_PIPELINE_IID == '1'"] } }, - prod: { script: 'cap prod', only: { variables: ["$CI_PIPELINE_IID == '1000'"] } } } - end + it 'populates pipeline according to used policies' do + step.perform! - it_behaves_like 'a correct pipeline' - end - end + expect(pipeline.stages.size).to eq 1 + expect(pipeline.stages.first.builds.size).to eq 1 + expect(pipeline.stages.first.builds.first.name).to eq 'rspec' end end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 2aebdc57f7c..74e7a45fd6c 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -242,7 +242,6 @@ Ci::Pipeline: - config_source - failure_reason - protected -- iid Ci::Stage: - id - name diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 66c9708b4cf..b5eac913639 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1559,7 +1559,6 @@ describe Ci::Build do { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true }, { key: 'CI_PROJECT_URL', value: project.web_url, public: true }, { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true }, - { key: 'CI_PIPELINE_IID', value: pipeline.iid.to_s, public: true }, { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true }, { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true }, { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true }, diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 24692ebb9a3..f5295bec65b 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -35,16 +35,6 @@ describe Ci::Pipeline, :mailer do end end - describe 'modules' do - it_behaves_like 'AtomicInternalId', validate_presence: false do - let(:internal_id_attribute) { :iid } - let(:instance) { build(:ci_pipeline) } - let(:scope) { :project } - let(:scope_attrs) { { project: instance.project } } - let(:usage) { :ci_pipelines } - end - end - describe '#source' do context 'when creating new pipeline' do let(:pipeline) do @@ -205,8 +195,7 @@ describe Ci::Pipeline, :mailer do it 'includes all predefined variables in a valid order' do keys = subject.map { |variable| variable[:key] } - expect(keys).to eq %w[CI_PIPELINE_IID - CI_CONFIG_PATH + expect(keys).to eq %w[CI_CONFIG_PATH CI_PIPELINE_SOURCE CI_COMMIT_MESSAGE CI_COMMIT_TITLE diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index e01906f4b6c..aee70bcfb29 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -20,7 +20,6 @@ describe Deployment do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:deployment) } - let(:scope) { :project } let(:scope_attrs) { { project: instance.project } } let(:usage) { :deployments } end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index e818fbeb9cf..128acf83686 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -17,7 +17,6 @@ describe Issue do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:issue) } - let(:scope) { :project } let(:scope_attrs) { { project: instance.project } } let(:usage) { :issues } end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 65cc9372cbe..9ffa91fc265 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -84,7 +84,6 @@ describe MergeRequest do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:merge_request) } - let(:scope) { :target_project } let(:scope_attrs) { { project: instance.target_project } } let(:usage) { :merge_requests } end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 204d6b47832..4bb9717d33e 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -6,7 +6,6 @@ describe Milestone do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:milestone, project: build(:project), group: nil) } - let(:scope) { :project } let(:scope_attrs) { { project: instance.project } } let(:usage) { :milestones } end @@ -16,7 +15,6 @@ describe Milestone do it_behaves_like 'AtomicInternalId' do let(:internal_id_attribute) { :iid } let(:instance) { build(:milestone, project: nil, group: build(:group)) } - let(:scope) { :group } let(:scope_attrs) { { namespace: instance.group } } let(:usage) { :milestones } end diff --git a/spec/support/shared_examples/models/atomic_internal_id_spec.rb b/spec/support/shared_examples/models/atomic_internal_id_spec.rb index 7ab1041d17c..6a6e13418a9 100644 --- a/spec/support/shared_examples/models/atomic_internal_id_spec.rb +++ b/spec/support/shared_examples/models/atomic_internal_id_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -shared_examples_for 'AtomicInternalId' do |validate_presence: true| +shared_examples_for 'AtomicInternalId' do describe '.has_internal_id' do describe 'Module inclusion' do subject { described_class } @@ -9,31 +9,14 @@ shared_examples_for 'AtomicInternalId' do |validate_presence: true| end describe 'Validation' do - before do - allow_any_instance_of(described_class).to receive(:"ensure_#{scope}_#{internal_id_attribute}!") - - instance.valid? - end - - context 'when presence validation is required' do - before do - skip unless validate_presence - end + subject { instance } - it 'validates presence' do - expect(instance.errors[internal_id_attribute]).to include("can't be blank") - end + before do + allow(InternalId).to receive(:generate_next).and_return(nil) end - context 'when presence validation is not required' do - before do - skip if validate_presence - end - - it 'does not validate presence' do - expect(instance.errors[internal_id_attribute]).to be_empty - end - end + it { is_expected.to validate_presence_of(internal_id_attribute) } + it { is_expected.to validate_numericality_of(internal_id_attribute) } end describe 'Creating an instance' do -- GitLab From 2e1f2364c864232971e009ab4d58377c46f23375 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 4 Jun 2018 19:07:18 -0500 Subject: [PATCH 007/210] Update VERSION to 11.0.0-rc2 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 69d3cd2a771..f25fb60adc9 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -11.0.0-rc1 +11.0.0-rc2 -- GitLab From 700ea84f82405867622730350f618d2191a37f81 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 5 Jun 2018 07:07:47 +0000 Subject: [PATCH 008/210] Merge branch 'osw-ignore-diff-header-when-persisting-diff-hunk' into 'master' Adjust insufficient diff hunks being persisted on NoteDiffFile See merge request gitlab-org/gitlab-ce!19399 --- ...-diff-header-when-persisting-diff-hunk.yml | 5 + lib/gitlab/diff/file.rb | 9 +- lib/gitlab/diff/line.rb | 4 + spec/lib/gitlab/diff/file_spec.rb | 113 ++++++++++-------- 4 files changed, 78 insertions(+), 53 deletions(-) create mode 100644 changelogs/unreleased/osw-ignore-diff-header-when-persisting-diff-hunk.yml diff --git a/changelogs/unreleased/osw-ignore-diff-header-when-persisting-diff-hunk.yml b/changelogs/unreleased/osw-ignore-diff-header-when-persisting-diff-hunk.yml new file mode 100644 index 00000000000..ef66deaa0ef --- /dev/null +++ b/changelogs/unreleased/osw-ignore-diff-header-when-persisting-diff-hunk.yml @@ -0,0 +1,5 @@ +--- +title: Adjust insufficient diff hunks being persisted on NoteDiffFile +merge_request: +author: +type: fixed diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 765fb0289a8..2820293ad5c 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -78,9 +78,12 @@ module Gitlab # Returns the raw diff content up to the given line index def diff_hunk(diff_line) - # Adding 2 because of the @@ diff header and Enum#take should consider - # an extra line, because we're passing an index. - raw_diff.each_line.take(diff_line.index + 2).join + diff_line_index = diff_line.index + # @@ (match) header is not kept if it's found in the top of the file, + # therefore we should keep an extra line on this scenario. + diff_line_index += 1 unless diff_lines.first.match? + + diff_lines.select { |line| line.index <= diff_line_index }.map(&:text).join("\n") end def old_sha diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 0603141e441..a1e904cfef4 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -53,6 +53,10 @@ module Gitlab %w[match new-nonewline old-nonewline].include?(type) end + def match? + type == :match + end + def discussable? !meta? end diff --git a/spec/lib/gitlab/diff/file_spec.rb b/spec/lib/gitlab/diff/file_spec.rb index 0588fe935c3..f0e83ccfc7a 100644 --- a/spec/lib/gitlab/diff/file_spec.rb +++ b/spec/lib/gitlab/diff/file_spec.rb @@ -470,56 +470,69 @@ describe Gitlab::Diff::File do end describe '#diff_hunk' do - let(:raw_diff) do - < path } -- options = { chdir: path } -+ -+ vars = { -+ "PWD" => path -+ } -+ -+ options = { -+ chdir: path -+ } - - unless File.directory?(path) - FileUtils.mkdir_p(path) -@@ -19,6 +25,7 @@ module Popen - - @cmd_output = "" - @cmd_status = 0 -+ - Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr| - @cmd_output << stdout.read - @cmd_output << stderr.read -EOS - end - - it 'returns raw diff up to given line index' do - allow(diff_file).to receive(:raw_diff) { raw_diff } - diff_line = instance_double(Gitlab::Diff::Line, index: 5) - - diff_hunk = < Date: Mon, 4 Jun 2018 21:52:44 +0000 Subject: [PATCH 009/210] Merge branch '47162-styling-of-acceptance-terms-page-is-broken' into 'master' Resolve "Styling of acceptance terms page is broken" Closes #47162 See merge request gitlab-org/gitlab-ce!19371 --- app/assets/stylesheets/framework/terms.scss | 8 +++++--- app/views/layouts/terms.html.haml | 6 +++--- app/views/users/terms/index.html.haml | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/assets/stylesheets/framework/terms.scss b/app/assets/stylesheets/framework/terms.scss index 744fd0ff796..7cda674e5c8 100644 --- a/app/assets/stylesheets/framework/terms.scss +++ b/app/assets/stylesheets/framework/terms.scss @@ -11,15 +11,15 @@ padding-top: $gl-padding; } - .panel { - .panel-heading { + .card { + .card-header { display: -webkit-flex; display: flex; align-items: center; justify-content: space-between; line-height: $line-height-base; - .title { + .card-title { display: flex; align-items: center; @@ -34,6 +34,8 @@ .navbar-collapse { padding-right: 0; + flex-grow: 0; + flex-basis: auto; .navbar-nav { margin: 0; diff --git a/app/views/layouts/terms.html.haml b/app/views/layouts/terms.html.haml index a8964b19ba1..977eb350365 100644 --- a/app/views/layouts/terms.html.haml +++ b/app/views/layouts/terms.html.haml @@ -14,9 +14,9 @@ %div{ class: "#{container_class} limit-container-width" } .content{ id: "content-body" } - .panel.panel-default - .panel-heading - .title + .card + .card-header + .card-title = brand_header_logo - logo_text = brand_header_logo_type - if logo_text.present? diff --git a/app/views/users/terms/index.html.haml b/app/views/users/terms/index.html.haml index e0fe551cf36..b9f25a71170 100644 --- a/app/views/users/terms/index.html.haml +++ b/app/views/users/terms/index.html.haml @@ -1,8 +1,8 @@ - redirect_params = { redirect: @redirect } if @redirect -.panel-content.rendered-terms +.card-body.rendered-terms = markdown_field(@term, :terms) -.row-content-block.footer-block.clearfix +.card-footer.footer-block.clearfix - if can?(current_user, :accept_terms, @term) .float-right = button_to accept_term_path(@term, redirect_params), class: 'btn btn-success prepend-left-8' do -- GitLab From aba8f6493fc3b6faa273c6a625fa6c8a5b31d035 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 29 May 2018 15:23:09 +0000 Subject: [PATCH 010/210] Merge branch '46885-group-visibility' into 'master' Resolve "Visibility section alignment is broken" Closes #46885 See merge request gitlab-org/gitlab-ce!19204 --- app/views/shared/_visibility_level.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/_visibility_level.html.haml b/app/views/shared/_visibility_level.html.haml index d67409ffe14..38c6f560dc6 100644 --- a/app/views/shared/_visibility_level.html.haml +++ b/app/views/shared/_visibility_level.html.haml @@ -1,6 +1,6 @@ - with_label = local_assigns.fetch(:with_label, true) -.form-group.visibility-level-setting +.form-group.row.visibility-level-setting - if with_label = f.label :visibility_level, class: 'col-form-label col-sm-2' do Visibility Level -- GitLab From f4f9e54ebe85e5eea4cc9cd880c87524f1471b67 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 5 Jun 2018 01:39:09 +0000 Subject: [PATCH 011/210] Merge branch 'jivl-fix-labels-not-displayed-after-selection' into 'master' Resolve: Labels are not displayed after selection Closes #47250 See merge request gitlab-org/gitlab-ce!19394 --- app/assets/javascripts/labels_select.js | 2 +- spec/javascripts/labels_select_spec.js | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/labels_select.js b/app/assets/javascripts/labels_select.js index eafdaf4a672..7d0ff53f366 100644 --- a/app/assets/javascripts/labels_select.js +++ b/app/assets/javascripts/labels_select.js @@ -426,7 +426,7 @@ export default class LabelsSelect { const tpl = _.template([ '<% _.each(labels, function(label){ %>', '?label_name[]=<%- encodeURIComponent(label.title) %>">', - '', + '', '<%- label.title %>', '', '', diff --git a/spec/javascripts/labels_select_spec.js b/spec/javascripts/labels_select_spec.js index a2b89c0aef5..386e00bfd0c 100644 --- a/spec/javascripts/labels_select_spec.js +++ b/spec/javascripts/labels_select_spec.js @@ -40,5 +40,9 @@ describe('LabelsSelect', () => { it('generated label item template has correct label styles', () => { expect($labelEl.find('span.label').attr('style')).toBe(`background-color: ${label.color}; color: ${label.text_color};`); }); + + it('generated label item has a badge class', () => { + expect($labelEl.find('span').hasClass('badge')).toEqual(true); + }); }); }); -- GitLab From dcbdfe7ac7106dca801bf08881d768f543f57390 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 5 Jun 2018 03:19:24 +0000 Subject: [PATCH 012/210] Merge branch '47115-fix-markdown-blockquote' into 'master' Resolve "Markdown blockquote is not displaying properly" Closes #47115 See merge request gitlab-org/gitlab-ce!19395 --- .../stylesheets/framework/typography.scss | 23 ++++++++++--------- .../mailers/highlighted_diff_email.scss | 1 + 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index 97b821e0cb9..9e77ea03a24 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -114,26 +114,27 @@ font-size: 0.95em; } + blockquote, .blockquote { color: $gl-grayish-blue; font-size: inherit; padding: 8px 24px; margin: 16px 0; border-left: 3px solid $white-dark; - } - .blockquote:dir(rtl) { - border-left: 0; - border-right: 3px solid $white-dark; - } + &:dir(rtl) { + border-left: 0; + border-right: 3px solid $white-dark; + } - .blockquote p { - color: $gl-grayish-blue !important; - font-size: inherit; - line-height: 1.5; + p { + color: $gl-grayish-blue !important; + font-size: inherit; + line-height: 1.5; - &:last-child { - margin: 0; + &:last-child { + margin: 0; + } } } diff --git a/app/assets/stylesheets/mailers/highlighted_diff_email.scss b/app/assets/stylesheets/mailers/highlighted_diff_email.scss index b5eda79e5ed..1835c4364d3 100644 --- a/app/assets/stylesheets/mailers/highlighted_diff_email.scss +++ b/app/assets/stylesheets/mailers/highlighted_diff_email.scss @@ -138,6 +138,7 @@ pre { margin: 0; } +blockquote, .blockquote { color: $gl-grayish-blue; padding: 0 0 0 15px; -- GitLab From 6d6ba149451de0390e27b6ad287b68fece67e15e Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 5 Jun 2018 09:15:08 +0000 Subject: [PATCH 013/210] Merge branch 'fix-jobs-log' into 'master' Fix jobs log background color Closes #47261 See merge request gitlab-org/gitlab-ce!19411 --- app/assets/stylesheets/bootstrap_migration.scss | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/app/assets/stylesheets/bootstrap_migration.scss b/app/assets/stylesheets/bootstrap_migration.scss index e24f8b1d4e8..c83144eb1ef 100644 --- a/app/assets/stylesheets/bootstrap_migration.scss +++ b/app/assets/stylesheets/bootstrap_migration.scss @@ -63,6 +63,20 @@ code { padding: 2px 4px; background-color: $red-100; border-radius: 3px; + + .code & { + background-color: inherit; + padding: unset; + } + + .build-trace & { + background-color: inherit; + padding: inherit; + } +} + +.code { + padding: 9.5px; } table { -- GitLab From d783f79bbb5a10ecb8de03a883ed8ca23bd6c0c5 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 5 Jun 2018 09:19:49 +0000 Subject: [PATCH 014/210] Merge branch 'ide-alignment-fixes' into 'master' Fixed alignment issues with IDE sidebar Closes #47244 See merge request gitlab-org/gitlab-ce!19272 --- .../ide/components/activity_bar.vue | 14 +++++++++++--- app/assets/stylesheets/pages/repo.scss | 19 ++++++++++++++++--- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/ide/components/activity_bar.vue b/app/assets/javascripts/ide/components/activity_bar.vue index 05dbc1410de..6efcad6adea 100644 --- a/app/assets/javascripts/ide/components/activity_bar.vue +++ b/app/assets/javascripts/ide/components/activity_bar.vue @@ -1,4 +1,5 @@