From 511e761b41b81484c85e3d125f45873ce38e9201 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 3 Jan 2020 15:08:33 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- Gemfile | 2 +- Gemfile.lock | 8 +-- app/controllers/application_controller.rb | 9 +++ app/services/ci/find_exposed_artifacts_service.rb | 2 + ...p-hashed-storage-migration-on-big-instances.yml | 5 ++ changelogs/unreleased/burndown-optimisations.yml | 5 ++ .../unreleased/fix-no-artifacts-when-exposed.yml | 5 ++ changelogs/unreleased/sh-fix-ci-lint-errors.yml | 5 ++ config/initializers/correlation_id.rb | 3 - config/initializers/labkit_middleware.rb | 3 + config/initializers/tracing.rb | 2 +- ...170221_add_storage_version_index_to_projects.rb | 17 +++++ db/schema.rb | 3 +- lib/api/api.rb | 8 +++ lib/api/commit_statuses.rb | 12 ++-- lib/api/helpers/internal_helpers.rb | 4 +- lib/api/internal/base.rb | 12 ++++ lib/api/projects.rb | 2 +- lib/gitlab/application_context.rb | 50 +++++++++++++ lib/gitlab/ci/config/entry/artifacts.rb | 5 ++ lib/gitlab/middleware/correlation_id.rb | 31 -------- lib/gitlab/sidekiq_middleware.rb | 4 +- .../sidekiq_middleware/correlation_injector.rb | 14 ---- .../sidekiq_middleware/correlation_logger.rb | 15 ---- lib/gitlab/utils/lazy_attributes.rb | 45 ++++++++++++ scripts/sync-stable-branch.sh | 10 +-- spec/controllers/application_controller_spec.rb | 46 ++++++++++++ .../notes/components/discussion_notes_spec.js | 18 +++-- spec/lib/gitlab/application_context_spec.rb | 82 ++++++++++++++++++++++ spec/lib/gitlab/ci/yaml_processor_spec.rb | 13 ++++ .../import_export/project_tree_restorer_spec.rb | 5 +- spec/lib/gitlab/profiler_spec.rb | 2 +- .../correlation_injector_spec.rb | 49 ------------- .../sidekiq_middleware/correlation_logger_spec.rb | 35 --------- spec/lib/gitlab/sidekiq_middleware_spec.rb | 4 +- spec/lib/gitlab/utils/lazy_attributes_spec.rb | 70 ++++++++++++++++++ spec/requests/api/internal/base_spec.rb | 12 ++++ spec/requests/api/projects_spec.rb | 8 +++ .../ci/find_exposed_artifacts_service_spec.rb | 29 ++++++++ spec/spec_helper.rb | 6 ++ .../logging_application_context_shared_examples.rb | 24 +++++++ 41 files changed, 502 insertions(+), 182 deletions(-) create mode 100644 changelogs/unreleased/194066-add-index-to-help-hashed-storage-migration-on-big-instances.yml create mode 100644 changelogs/unreleased/burndown-optimisations.yml create mode 100644 changelogs/unreleased/fix-no-artifacts-when-exposed.yml create mode 100644 changelogs/unreleased/sh-fix-ci-lint-errors.yml delete mode 100644 config/initializers/correlation_id.rb create mode 100644 config/initializers/labkit_middleware.rb create mode 100644 db/migrate/20200102170221_add_storage_version_index_to_projects.rb create mode 100644 lib/gitlab/application_context.rb delete mode 100644 lib/gitlab/middleware/correlation_id.rb delete mode 100644 lib/gitlab/sidekiq_middleware/correlation_injector.rb delete mode 100644 lib/gitlab/sidekiq_middleware/correlation_logger.rb create mode 100644 lib/gitlab/utils/lazy_attributes.rb create mode 100644 spec/lib/gitlab/application_context_spec.rb delete mode 100644 spec/lib/gitlab/sidekiq_middleware/correlation_injector_spec.rb delete mode 100644 spec/lib/gitlab/sidekiq_middleware/correlation_logger_spec.rb create mode 100644 spec/lib/gitlab/utils/lazy_attributes_spec.rb create mode 100644 spec/support/shared_examples/logging_application_context_shared_examples.rb diff --git a/Gemfile b/Gemfile index 652cc04ecbc..0c5fffe73ea 100644 --- a/Gemfile +++ b/Gemfile @@ -301,7 +301,7 @@ gem 'sentry-raven', '~> 2.9' gem 'premailer-rails', '~> 1.10.3' # LabKit: Tracing and Correlation -gem 'gitlab-labkit', '~> 0.5' +gem 'gitlab-labkit', '0.8.0' # I18n gem 'ruby_parser', '~> 3.8', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 9ca82644a97..15ceff6e868 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -117,7 +117,7 @@ GEM activemodel (>= 5.0) brakeman (4.2.1) browser (2.5.3) - builder (3.2.3) + builder (3.2.4) bullet (6.0.2) activesupport (>= 3.0.0) uniform_notifier (~> 1.11) @@ -362,7 +362,7 @@ GEM github-markup (1.7.0) gitlab-chronic (0.10.5) numerizer (~> 0.2) - gitlab-labkit (0.7.0) + gitlab-labkit (0.8.0) actionpack (>= 5.0.0, < 6.1.0) activesupport (>= 5.0.0, < 6.1.0) grpc (~> 1.19) @@ -589,7 +589,7 @@ GEM activesupport (>= 4) railties (>= 4) request_store (~> 1.0) - loofah (2.3.1) + loofah (2.4.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) lumberjack (1.0.13) @@ -1204,7 +1204,7 @@ DEPENDENCIES gitaly (~> 1.73.0) github-markup (~> 1.7.0) gitlab-chronic (~> 0.10.5) - gitlab-labkit (~> 0.5) + gitlab-labkit (= 0.8.0) gitlab-license (~> 1.0) gitlab-markup (~> 1.7.0) gitlab-net-dns (~> 0.9.1) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f5306801c04..60b5d9b6da8 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -33,6 +33,7 @@ class ApplicationController < ActionController::Base before_action :check_impersonation_availability before_action :required_signup_info + around_action :set_current_context around_action :set_locale around_action :set_session_storage @@ -448,6 +449,14 @@ class ApplicationController < ActionController::Base request.base_url end + def set_current_context(&block) + Gitlab::ApplicationContext.with_context( + user: -> { auth_user }, + project: -> { @project }, + namespace: -> { @group }, + &block) + end + def set_locale(&block) Gitlab::I18n.with_user_locale(current_user, &block) end diff --git a/app/services/ci/find_exposed_artifacts_service.rb b/app/services/ci/find_exposed_artifacts_service.rb index 5c75af294bf..d268252577f 100644 --- a/app/services/ci/find_exposed_artifacts_service.rb +++ b/app/services/ci/find_exposed_artifacts_service.rb @@ -46,6 +46,8 @@ module Ci # it could contain many. We only need to know whether it has 1 or more # artifacts, so fetching the first 2 would be sufficient. def first_2_metadata_entries_for_artifacts_paths(job) + return [] unless job.artifacts_metadata + job.artifacts_paths .lazy .map { |path| job.artifacts_metadata_entry(path, recursive: true) } diff --git a/changelogs/unreleased/194066-add-index-to-help-hashed-storage-migration-on-big-instances.yml b/changelogs/unreleased/194066-add-index-to-help-hashed-storage-migration-on-big-instances.yml new file mode 100644 index 00000000000..ea1b4d79565 --- /dev/null +++ b/changelogs/unreleased/194066-add-index-to-help-hashed-storage-migration-on-big-instances.yml @@ -0,0 +1,5 @@ +--- +title: Add Index to help Hashed Storage migration on big instances +merge_request: 22391 +author: +type: performance diff --git a/changelogs/unreleased/burndown-optimisations.yml b/changelogs/unreleased/burndown-optimisations.yml new file mode 100644 index 00000000000..07b6375c477 --- /dev/null +++ b/changelogs/unreleased/burndown-optimisations.yml @@ -0,0 +1,5 @@ +--- +title: Performance improvements on milestone burndown chart +merge_request: 22380 +author: +type: performance diff --git a/changelogs/unreleased/fix-no-artifacts-when-exposed.yml b/changelogs/unreleased/fix-no-artifacts-when-exposed.yml new file mode 100644 index 00000000000..7ca77f520a6 --- /dev/null +++ b/changelogs/unreleased/fix-no-artifacts-when-exposed.yml @@ -0,0 +1,5 @@ +--- +title: Fix bug when trying to expose artifacts and no artifacts are produced by the job +merge_request: 22378 +author: +type: fixed diff --git a/changelogs/unreleased/sh-fix-ci-lint-errors.yml b/changelogs/unreleased/sh-fix-ci-lint-errors.yml new file mode 100644 index 00000000000..5f97b98f3d9 --- /dev/null +++ b/changelogs/unreleased/sh-fix-ci-lint-errors.yml @@ -0,0 +1,5 @@ +--- +title: Gracefully error handle CI lint errors in artifacts section +merge_request: 22388 +author: +type: fixed diff --git a/config/initializers/correlation_id.rb b/config/initializers/correlation_id.rb deleted file mode 100644 index 2a7c138dc40..00000000000 --- a/config/initializers/correlation_id.rb +++ /dev/null @@ -1,3 +0,0 @@ -# frozen_string_literal: true - -Rails.application.config.middleware.use(Gitlab::Middleware::CorrelationId) diff --git a/config/initializers/labkit_middleware.rb b/config/initializers/labkit_middleware.rb new file mode 100644 index 00000000000..ea4103f052f --- /dev/null +++ b/config/initializers/labkit_middleware.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +Rails.application.config.middleware.use(Labkit::Middleware::Rack) diff --git a/config/initializers/tracing.rb b/config/initializers/tracing.rb index 0ae57021fcf..aaf74eb4cd3 100644 --- a/config/initializers/tracing.rb +++ b/config/initializers/tracing.rb @@ -2,7 +2,7 @@ if Labkit::Tracing.enabled? Rails.application.configure do |config| - config.middleware.insert_after Gitlab::Middleware::CorrelationId, ::Labkit::Tracing::RackMiddleware + config.middleware.insert_after Labkit::Middleware::Rack, ::Labkit::Tracing::RackMiddleware end # Instrument the Sidekiq client diff --git a/db/migrate/20200102170221_add_storage_version_index_to_projects.rb b/db/migrate/20200102170221_add_storage_version_index_to_projects.rb new file mode 100644 index 00000000000..8965b5eedb9 --- /dev/null +++ b/db/migrate/20200102170221_add_storage_version_index_to_projects.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddStorageVersionIndexToProjects < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :projects, :id, where: 'storage_version < 2 or storage_version IS NULL', name: 'index_on_id_partial_with_legacy_storage' + end + + def down + remove_concurrent_index :projects, :id, where: 'storage_version < 2 or storage_version IS NULL', name: 'index_on_id_partial_with_legacy_storage' + end +end diff --git a/db/schema.rb b/db/schema.rb index 567e135fdff..30e439b08c4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_12_29_140154) do +ActiveRecord::Schema.define(version: 2020_01_02_170221) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -3349,6 +3349,7 @@ ActiveRecord::Schema.define(version: 2019_12_29_140154) do t.index ["creator_id"], name: "index_projects_on_creator_id" t.index ["description"], name: "index_projects_on_description_trigram", opclass: :gin_trgm_ops, using: :gin t.index ["id", "repository_storage", "last_repository_updated_at"], name: "idx_projects_on_repository_storage_last_repository_updated_at" + t.index ["id"], name: "index_on_id_partial_with_legacy_storage", where: "((storage_version < 2) OR (storage_version IS NULL))" t.index ["id"], name: "index_projects_on_id_partial_for_visibility", unique: true, where: "(visibility_level = ANY (ARRAY[10, 20]))" t.index ["id"], name: "index_projects_on_mirror_and_mirror_trigger_builds_both_true", where: "((mirror IS TRUE) AND (mirror_trigger_builds IS TRUE))" t.index ["last_activity_at"], name: "index_projects_on_last_activity_at" diff --git a/lib/api/api.rb b/lib/api/api.rb index 56eccb036b6..eae10738f32 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -43,6 +43,14 @@ module API header['X-Content-Type-Options'] = 'nosniff' end + before do + Gitlab::ApplicationContext.push( + user: -> { current_user }, + project: -> { @project }, + namespace: -> { @group } + ) + end + # The locale is set to the current user's locale when `current_user` is loaded after { Gitlab::I18n.use_default_locale } diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index d108c811f4b..6e26ee309f0 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -71,27 +71,27 @@ module API ref = params[:ref] ref ||= pipeline&.ref - ref ||= @project.repository.branch_names_contains(commit.sha).first + ref ||= user_project.repository.branch_names_contains(commit.sha).first not_found! 'References for commit' unless ref name = params[:name] || params[:context] || 'default' unless pipeline - pipeline = @project.ci_pipelines.create!( + pipeline = user_project.ci_pipelines.create!( source: :external, sha: commit.sha, ref: ref, user: current_user, - protected: @project.protected_for?(ref)) + protected: user_project.protected_for?(ref)) end status = GenericCommitStatus.running_or_pending.find_or_initialize_by( - project: @project, + project: user_project, pipeline: pipeline, name: name, ref: ref, user: current_user, - protected: @project.protected_for?(ref) + protected: user_project.protected_for?(ref) ) optional_attributes = @@ -117,7 +117,7 @@ module API render_api_error!('invalid state', 400) end - MergeRequest.where(source_project: @project, source_branch: ref) + MergeRequest.where(source_project: user_project, source_branch: ref) .update_all(head_pipeline_id: pipeline.id) if pipeline.latest? present status, with: Entities::CommitStatus diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 607e0784415..b719e5c0886 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -107,8 +107,10 @@ module API if params[:gl_repository] @project, @repo_type = Gitlab::GlRepository.parse(params[:gl_repository]) @redirected_path = nil - else + elsif params[:project] @project, @repo_type, @redirected_path = Gitlab::RepoPath.parse(params[:project]) + else + @project, @repo_type, @redirected_path = nil, nil, nil end end # rubocop:enable Gitlab/ModuleWithInstanceVariables diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 11f2a2ea1c0..d64de2bb465 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -6,6 +6,13 @@ module API class Base < Grape::API before { authenticate_by_gitlab_shell_token! } + before do + Gitlab::ApplicationContext.push( + user: -> { actor&.user }, + project: -> { project } + ) + end + helpers ::API::Helpers::InternalHelpers UNKNOWN_CHECK_RESULT_ERROR = 'Unknown check result'.freeze @@ -205,7 +212,12 @@ module API status 200 response = Gitlab::InternalPostReceive::Response.new + + # Try to load the project and users so we have the application context + # available for logging before we schedule any jobs. user = actor.user + project + push_options = Gitlab::PushOptions.new(params[:push_options]) response.reference_counter_decreased = Gitlab::ReferenceCounter.new(params[:gl_repository]).decrease diff --git a/lib/api/projects.rb b/lib/api/projects.rb index d1f99ea49ce..68f199cc160 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -355,7 +355,7 @@ module API post ':id/unarchive' do authorize!(:archive_project, user_project) - ::Projects::UpdateService.new(@project, current_user, archived: false).execute + ::Projects::UpdateService.new(user_project, current_user, archived: false).execute present user_project, with: Entities::Project, current_user: current_user end diff --git a/lib/gitlab/application_context.rb b/lib/gitlab/application_context.rb new file mode 100644 index 00000000000..b9190b519a0 --- /dev/null +++ b/lib/gitlab/application_context.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module Gitlab + # A GitLab-rails specific accessor for `Labkit::Logging::ApplicationContext` + class ApplicationContext + include Gitlab::Utils::LazyAttributes + + def self.with_context(args, &block) + application_context = new(**args) + Labkit::Context.with_context(application_context.to_lazy_hash, &block) + end + + def self.push(args) + application_context = new(**args) + Labkit::Context.push(application_context.to_lazy_hash) + end + + def initialize(user: nil, project: nil, namespace: nil) + @user, @project, @namespace = user, project, namespace + end + + def to_lazy_hash + { user: -> { username }, + project: -> { project_path }, + root_namespace: -> { root_namespace_path } } + end + + private + + lazy_attr_reader :user, type: User + lazy_attr_reader :project, type: Project + lazy_attr_reader :namespace, type: Namespace + + def project_path + project&.full_path + end + + def username + user&.username + end + + def root_namespace_path + if namespace + namespace.full_path_components.first + else + project&.full_path_components&.first + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index 9d8d7675234..aebc1675bec 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -54,6 +54,11 @@ module Gitlab def expose_as_present? return false unless Feature.enabled?(:ci_expose_arbitrary_artifacts_in_mr, default_enabled: true) + # This duplicates the `validates :config, type: Hash` above, + # but Validatable currently doesn't halt the validation + # chain if it encounters a validation error. + return false unless @config.is_a?(Hash) + !@config[:expose_as].nil? end end diff --git a/lib/gitlab/middleware/correlation_id.rb b/lib/gitlab/middleware/correlation_id.rb deleted file mode 100644 index fffd5da827f..00000000000 --- a/lib/gitlab/middleware/correlation_id.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -# A dumb middleware that steals correlation id -# and sets it as a global context for the request -module Gitlab - module Middleware - class CorrelationId - include ActionView::Helpers::TagHelper - - def initialize(app) - @app = app - end - - def call(env) - ::Labkit::Correlation::CorrelationId.use_id(correlation_id(env)) do - @app.call(env) - end - end - - private - - def correlation_id(env) - request(env).request_id - end - - def request(env) - ActionDispatch::Request.new(env) - end - end - end -end diff --git a/lib/gitlab/sidekiq_middleware.rb b/lib/gitlab/sidekiq_middleware.rb index c6726dcfa67..4893cbc1f45 100644 --- a/lib/gitlab/sidekiq_middleware.rb +++ b/lib/gitlab/sidekiq_middleware.rb @@ -15,7 +15,7 @@ module Gitlab chain.add Gitlab::SidekiqMiddleware::MemoryKiller if memory_killer chain.add Gitlab::SidekiqMiddleware::RequestStoreMiddleware if request_store chain.add Gitlab::SidekiqMiddleware::BatchLoader - chain.add Gitlab::SidekiqMiddleware::CorrelationLogger + chain.add Labkit::Middleware::Sidekiq::Server chain.add Gitlab::SidekiqMiddleware::InstrumentationLogger chain.add Gitlab::SidekiqStatus::ServerMiddleware end @@ -27,7 +27,7 @@ module Gitlab def self.client_configurator lambda do |chain| chain.add Gitlab::SidekiqStatus::ClientMiddleware - chain.add Gitlab::SidekiqMiddleware::CorrelationInjector + chain.add Labkit::Middleware::Sidekiq::Client end end end diff --git a/lib/gitlab/sidekiq_middleware/correlation_injector.rb b/lib/gitlab/sidekiq_middleware/correlation_injector.rb deleted file mode 100644 index 1539fd706ab..00000000000 --- a/lib/gitlab/sidekiq_middleware/correlation_injector.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module SidekiqMiddleware - class CorrelationInjector - def call(worker_class, job, queue, redis_pool) - job[Labkit::Correlation::CorrelationId::LOG_KEY] ||= - Labkit::Correlation::CorrelationId.current_or_new_id - - yield - end - end - end -end diff --git a/lib/gitlab/sidekiq_middleware/correlation_logger.rb b/lib/gitlab/sidekiq_middleware/correlation_logger.rb deleted file mode 100644 index cffc4483573..00000000000 --- a/lib/gitlab/sidekiq_middleware/correlation_logger.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module SidekiqMiddleware - class CorrelationLogger - def call(worker, job, queue) - correlation_id = job[Labkit::Correlation::CorrelationId::LOG_KEY] - - Labkit::Correlation::CorrelationId.use_id(correlation_id) do - yield - end - end - end - end -end diff --git a/lib/gitlab/utils/lazy_attributes.rb b/lib/gitlab/utils/lazy_attributes.rb new file mode 100644 index 00000000000..79f3a7dcb53 --- /dev/null +++ b/lib/gitlab/utils/lazy_attributes.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module Gitlab + module Utils + module LazyAttributes + extend ActiveSupport::Concern + include Gitlab::Utils::StrongMemoize + + class_methods do + def lazy_attr_reader(*one_or_more_names, type: nil) + names = Array.wrap(one_or_more_names) + names.each { |name| define_lazy_reader(name, type: type) } + end + + def lazy_attr_accessor(*one_or_more_names, type: nil) + names = Array.wrap(one_or_more_names) + names.each do |name| + define_lazy_reader(name, type: type) + define_lazy_writer(name) + end + end + + private + + def define_lazy_reader(name, type:) + define_method(name) do + strong_memoize("#{name}_lazy_loaded") do + value = instance_variable_get("@#{name}") + value = value.call if value.respond_to?(:call) + value = nil if type && !value.is_a?(type) + value + end + end + end + + def define_lazy_writer(name) + define_method("#{name}=") do |value| + clear_memoization("#{name}_lazy_loaded") + instance_variable_set("@#{name}", value) + end + end + end + end + end +end diff --git a/scripts/sync-stable-branch.sh b/scripts/sync-stable-branch.sh index 1eb416bf4f5..5aaec323628 100644 --- a/scripts/sync-stable-branch.sh +++ b/scripts/sync-stable-branch.sh @@ -7,31 +7,31 @@ set -e if [[ "$MERGE_TRAIN_TRIGGER_TOKEN" == '' ]] then - echo 'The variable MERGE_TRAIN_TRIGGER_TOKEN must be set to a non-empy value' + echo 'The variable MERGE_TRAIN_TRIGGER_TOKEN must be set to a non-empty value' exit 1 fi if [[ "$MERGE_TRAIN_TRIGGER_URL" == '' ]] then - echo 'The variable MERGE_TRAIN_TRIGGER_URL must be set to a non-empy value' + echo 'The variable MERGE_TRAIN_TRIGGER_URL must be set to a non-empty value' exit 1 fi if [[ "$CI_COMMIT_REF_NAME" == '' ]] then - echo 'The variable CI_COMMIT_REF_NAME must be set to a non-empy value' + echo 'The variable CI_COMMIT_REF_NAME must be set to a non-empty value' exit 1 fi if [[ "$SOURCE_PROJECT" == '' ]] then - echo 'The variable SOURCE_PROJECT must be set to a non-empy value' + echo 'The variable SOURCE_PROJECT must be set to a non-empty value' exit 1 fi if [[ "$TARGET_PROJECT" == '' ]] then - echo 'The variable TARGET_PROJECT must be set to a non-empy value' + echo 'The variable TARGET_PROJECT must be set to a non-empty value' exit 1 fi diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index e72ab16f62a..0c299dcda34 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -895,4 +895,50 @@ describe ApplicationController do end end end + + context '#set_current_context' do + controller(described_class) do + def index + Labkit::Context.with_context do |context| + render json: context.to_h + end + end + end + + let_it_be(:user) { create(:user) } + + before do + sign_in(user) + end + + it 'does not break anything when no group or project method is defined' do + get :index + + expect(response).to have_gitlab_http_status(:success) + end + + it 'sets the username in the context when signed in' do + get :index + + expect(json_response['meta.user']).to eq(user.username) + end + + it 'sets the group if it was available' do + group = build_stubbed(:group) + controller.instance_variable_set(:@group, group) + + get :index, format: :json + + expect(json_response['meta.root_namespace']).to eq(group.path) + end + + it 'sets the project if one was available' do + project = build_stubbed(:project) + controller.instance_variable_set(:@project, project) + + get :index, format: :json + + expect(json_response['meta.project']).to eq(project.full_path) + end + end end diff --git a/spec/frontend/notes/components/discussion_notes_spec.js b/spec/frontend/notes/components/discussion_notes_spec.js index 51c7f45d5b0..d259e79de84 100644 --- a/spec/frontend/notes/components/discussion_notes_spec.js +++ b/spec/frontend/notes/components/discussion_notes_spec.js @@ -6,7 +6,6 @@ import NoteableNote from '~/notes/components/noteable_note.vue'; import PlaceholderNote from '~/vue_shared/components/notes/placeholder_note.vue'; import PlaceholderSystemNote from '~/vue_shared/components/notes/placeholder_system_note.vue'; import SystemNote from '~/vue_shared/components/notes/system_note.vue'; -import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue'; import createStore from '~/notes/stores'; import { noteableDataMock, discussionMock, notesDataMock } from '../../notes/mock_data'; @@ -48,13 +47,13 @@ describe('DiscussionNotes', () => { it('renders an element for each note in the discussion', () => { createComponent(); const notesCount = discussionMock.notes.length; - const els = wrapper.findAll(TimelineEntryItem); + const els = wrapper.findAll(NoteableNote); expect(els.length).toBe(notesCount); }); it('renders one element if replies groupping is enabled', () => { createComponent({ shouldGroupReplies: true }); - const els = wrapper.findAll(TimelineEntryItem); + const els = wrapper.findAll(NoteableNote); expect(els.length).toBe(1); }); @@ -85,7 +84,7 @@ describe('DiscussionNotes', () => { ]; discussion.notes = notesData; createComponent({ discussion, shouldRenderDiffs: true }); - const notes = wrapper.findAll('.notes > li'); + const notes = wrapper.findAll('.notes > *'); expect(notes.at(0).is(PlaceholderSystemNote)).toBe(true); expect(notes.at(1).is(PlaceholderNote)).toBe(true); @@ -111,7 +110,14 @@ describe('DiscussionNotes', () => { describe('events', () => { describe('with groupped notes and replies expanded', () => { - const findNoteAtIndex = index => wrapper.find(`.note:nth-of-type(${index + 1}`); + const findNoteAtIndex = index => { + const noteComponents = [NoteableNote, SystemNote, PlaceholderNote, PlaceholderSystemNote]; + const allowedNames = noteComponents.map(c => c.name); + return wrapper + .findAll('.notes *') + .filter(w => allowedNames.includes(w.name())) + .at(index); + }; beforeEach(() => { createComponent({ shouldGroupReplies: true, isExpanded: true }); @@ -146,7 +152,7 @@ describe('DiscussionNotes', () => { let note; beforeEach(() => { createComponent(); - note = wrapper.find('.note'); + note = wrapper.find('.notes > *'); }); it('emits deleteNote when first note emits handleDeleteNote', () => { diff --git a/spec/lib/gitlab/application_context_spec.rb b/spec/lib/gitlab/application_context_spec.rb new file mode 100644 index 00000000000..1c8fcb8d737 --- /dev/null +++ b/spec/lib/gitlab/application_context_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::ApplicationContext do + describe '.with_context' do + it 'yields the block' do + expect { |b| described_class.with_context({}, &b) }.to yield_control + end + + it 'passes the expected context on to labkit' do + fake_proc = duck_type(:call) + expected_context = hash_including(user: fake_proc, project: fake_proc, root_namespace: fake_proc) + + expect(Labkit::Context).to receive(:with_context).with(expected_context) + + described_class.with_context( + user: build(:user), + project: build(:project), + namespace: build(:namespace)) {} + end + + it 'raises an error when passing invalid options' do + expect { described_class.with_context(no: 'option') {} }.to raise_error(ArgumentError) + end + end + + describe '.push' do + it 'passes the expected context on to labkit' do + fake_proc = duck_type(:call) + expected_context = hash_including(user: fake_proc, project: fake_proc, root_namespace: fake_proc) + + expect(Labkit::Context).to receive(:push).with(expected_context) + + described_class.push(user: build(:user)) + end + + it 'raises an error when passing invalid options' do + expect { described_class.push(no: 'option')}.to raise_error(ArgumentError) + end + end + + describe '#to_lazy_hash' do + let(:user) { build(:user) } + let(:project) { build(:project) } + let(:namespace) { build(:group) } + let(:subgroup) { build(:group, parent: namespace) } + + def result(context) + context.to_lazy_hash.transform_values { |v| v.call } + end + + it 'does not call the attributes until needed' do + fake_proc = double('Proc') + + expect(fake_proc).not_to receive(:call) + + described_class.new(user: fake_proc, project: fake_proc, namespace: fake_proc).to_lazy_hash + end + + it 'correctly loads the expected values when they are wrapped in a block' do + context = described_class.new(user: -> { user }, project: -> { project }, namespace: -> { subgroup }) + + expect(result(context)) + .to include(user: user.username, project: project.full_path, root_namespace: namespace.full_path) + end + + it 'correctly loads the expected values when passed directly' do + context = described_class.new(user: user, project: project, namespace: subgroup) + + expect(result(context)) + .to include(user: user.username, project: project.full_path, root_namespace: namespace.full_path) + end + + it 'falls back to a projects namespace when a project is passed but no namespace' do + context = described_class.new(project: project) + + expect(result(context)) + .to include(project: project.full_path, root_namespace: project.full_path_components.first) + end + end +end diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index f61b28b06c8..2e470d59345 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -1270,6 +1270,19 @@ module Gitlab expect(builds.first[:options][:artifacts][:when]).to eq(when_state) end end + + it "gracefully handles errors in artifacts type" do + config = <<~YAML + test: + script: + - echo "Hello world" + artifacts: + - paths: + - test/ + YAML + + expect { described_class.new(config) }.to raise_error(described_class::ValidationError) + end end describe '#environment' do diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index ec1b935ad63..dae5b028c15 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -654,13 +654,10 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do let(:user) { create(:user) } let!(:project) { create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') } let(:project_tree_restorer) { described_class.new(user: user, shared: shared, project: project) } - let(:correlation_id) { 'my-correlation-id' } before do setup_import_export_config('with_invalid_records') - # Import is running from the rake task, `correlation_id` is not assigned - expect(Labkit::Correlation::CorrelationId).to receive(:new_id).and_return(correlation_id) subject end @@ -682,7 +679,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect(import_failure.relation_index).to be_present expect(import_failure.exception_class).to eq('ActiveRecord::RecordInvalid') expect(import_failure.exception_message).to be_present - expect(import_failure.correlation_id_value).to eq('my-correlation-id') + expect(import_failure.correlation_id_value).not_to be_empty expect(import_failure.created_at).to be_present end end diff --git a/spec/lib/gitlab/profiler_spec.rb b/spec/lib/gitlab/profiler_spec.rb index c27e3ca7ace..8f6fb6eda65 100644 --- a/spec/lib/gitlab/profiler_spec.rb +++ b/spec/lib/gitlab/profiler_spec.rb @@ -84,7 +84,7 @@ describe Gitlab::Profiler do expect(severity).to eq(Logger::DEBUG) expect(message).to include('public').and include(described_class::FILTERED_STRING) expect(message).not_to include(private_token) - end.twice + end.at_least(1) # This spec could be wrapped in more blocks in the future custom_logger.debug("public #{private_token}") end diff --git a/spec/lib/gitlab/sidekiq_middleware/correlation_injector_spec.rb b/spec/lib/gitlab/sidekiq_middleware/correlation_injector_spec.rb deleted file mode 100644 index d5ed939485a..00000000000 --- a/spec/lib/gitlab/sidekiq_middleware/correlation_injector_spec.rb +++ /dev/null @@ -1,49 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::SidekiqMiddleware::CorrelationInjector do - class TestWorker - include ApplicationWorker - end - - before do |example| - Sidekiq.client_middleware do |chain| - chain.add described_class - end - end - - after do |example| - Sidekiq.client_middleware do |chain| - chain.remove described_class - end - - Sidekiq::Queues.clear_all - end - - around do |example| - Sidekiq::Testing.fake! do - example.run - end - end - - it 'injects into payload the correlation id' do - expect_next_instance_of(described_class) do |instance| - expect(instance).to receive(:call).and_call_original - end - - Labkit::Correlation::CorrelationId.use_id('new-correlation-id') do - TestWorker.perform_async(1234) - end - - expected_job_params = { - "class" => "TestWorker", - "args" => [1234], - "correlation_id" => "new-correlation-id" - } - - expect(Sidekiq::Queues.jobs_by_worker).to a_hash_including( - "TestWorker" => a_collection_containing_exactly( - a_hash_including(expected_job_params))) - end -end diff --git a/spec/lib/gitlab/sidekiq_middleware/correlation_logger_spec.rb b/spec/lib/gitlab/sidekiq_middleware/correlation_logger_spec.rb deleted file mode 100644 index 27eea963402..00000000000 --- a/spec/lib/gitlab/sidekiq_middleware/correlation_logger_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::SidekiqMiddleware::CorrelationLogger do - class TestWorker - include ApplicationWorker - end - - before do |example| - Sidekiq::Testing.server_middleware do |chain| - chain.add described_class - end - end - - after do |example| - Sidekiq::Testing.server_middleware do |chain| - chain.remove described_class - end - end - - it 'injects into payload the correlation id', :sidekiq_might_not_need_inline do - expect_any_instance_of(described_class).to receive(:call).and_call_original - - expect_any_instance_of(TestWorker).to receive(:perform).with(1234) do - expect(Labkit::Correlation::CorrelationId.current_id).to eq('new-correlation-id') - end - - Sidekiq::Client.push( - 'queue' => 'test', - 'class' => TestWorker, - 'args' => [1234], - 'correlation_id' => 'new-correlation-id') - end -end diff --git a/spec/lib/gitlab/sidekiq_middleware_spec.rb b/spec/lib/gitlab/sidekiq_middleware_spec.rb index aef472e0648..ef4a898bdb6 100644 --- a/spec/lib/gitlab/sidekiq_middleware_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware_spec.rb @@ -38,7 +38,7 @@ describe Gitlab::SidekiqMiddleware do [ Gitlab::SidekiqMiddleware::Monitor, Gitlab::SidekiqMiddleware::BatchLoader, - Gitlab::SidekiqMiddleware::CorrelationLogger, + Labkit::Middleware::Sidekiq::Server, Gitlab::SidekiqMiddleware::InstrumentationLogger, Gitlab::SidekiqStatus::ServerMiddleware, Gitlab::SidekiqMiddleware::Metrics, @@ -120,7 +120,7 @@ describe Gitlab::SidekiqMiddleware do # This test ensures that this does not happen it "invokes the chain" do expect_any_instance_of(Gitlab::SidekiqStatus::ClientMiddleware).to receive(:call).with(*middleware_expected_args).once.and_call_original - expect_any_instance_of(Gitlab::SidekiqMiddleware::CorrelationInjector).to receive(:call).with(*middleware_expected_args).once.and_call_original + expect_any_instance_of(Labkit::Middleware::Sidekiq::Client).to receive(:call).with(*middleware_expected_args).once.and_call_original expect { |b| chain.invoke(worker_class_arg, job, queue, redis_pool, &b) }.to yield_control.once end diff --git a/spec/lib/gitlab/utils/lazy_attributes_spec.rb b/spec/lib/gitlab/utils/lazy_attributes_spec.rb new file mode 100644 index 00000000000..c0005c194c4 --- /dev/null +++ b/spec/lib/gitlab/utils/lazy_attributes_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true +require 'fast_spec_helper' +require 'active_support/concern' + +describe Gitlab::Utils::LazyAttributes do + subject(:klass) do + Class.new do + include Gitlab::Utils::LazyAttributes + + lazy_attr_reader :number, type: Numeric + lazy_attr_reader :reader_1, :reader_2 + lazy_attr_accessor :incorrect_type, :string_attribute, :accessor_2, type: String + + def initialize + @number = -> { 1 } + @reader_1, @reader_2 = 'reader_1', -> { 'reader_2' } + @incorrect_type, @accessor_2 = -> { :incorrect_type }, -> { 'accessor_2' } + end + end + end + + context 'class methods' do + it { is_expected.to respond_to(:lazy_attr_reader, :lazy_attr_accessor) } + it { is_expected.not_to respond_to(:define_lazy_reader, :define_lazy_writer) } + end + + context 'instance methods' do + subject(:instance) { klass.new } + + it do + is_expected.to respond_to(:number, :reader_1, :reader_2, :incorrect_type, + :incorrect_type=, :accessor_2, :accessor_2=, + :string_attribute, :string_attribute=) + end + + context 'reading attributes' do + it 'returns the correct values for procs', :aggregate_failures do + expect(instance.number).to eq(1) + expect(instance.reader_2).to eq('reader_2') + expect(instance.accessor_2).to eq('accessor_2') + end + + it 'does not return the value if the type did not match what was specified' do + expect(instance.incorrect_type).to be_nil + end + + it 'only calls the block once even if it returned `nil`', :aggregate_failures do + expect(instance.instance_variable_get('@number')).to receive(:call).once.and_call_original + expect(instance.instance_variable_get('@accessor_2')).to receive(:call).once.and_call_original + expect(instance.instance_variable_get('@incorrect_type')).to receive(:call).once.and_call_original + + 2.times do + instance.number + instance.incorrect_type + instance.accessor_2 + end + end + end + + context 'writing attributes' do + it 'sets the correct values', :aggregate_failures do + instance.string_attribute = -> { 'updated 1' } + instance.accessor_2 = nil + + expect(instance.string_attribute).to eq('updated 1') + expect(instance.accessor_2).to be_nil + end + end + end +end diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index 4fe0d4a5983..12e6e7c7a09 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -389,6 +389,12 @@ describe API::Internal::Base do end end end + + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { user: key.user.username, project: project.full_path } } + + subject { push(key, project) } + end end context "access denied" do @@ -885,6 +891,12 @@ describe API::Internal::Base do post api('/internal/post_receive'), params: valid_params end + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { user: user.username, project: project.full_path } } + + subject { post api('/internal/post_receive'), params: valid_params } + end + context 'when there are merge_request push options' do before do valid_params[:push_options] = ['merge_request.create'] diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 9af4f484f99..975e59346b8 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1626,6 +1626,14 @@ describe API::Projects do end end end + + it_behaves_like 'storing arguments in the application context' do + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :public) } + let(:expected_params) { { user: user.username, project: project.full_path } } + + subject { get api("/projects/#{project.id}", user) } + end end describe 'GET /projects/:id/users' do diff --git a/spec/services/ci/find_exposed_artifacts_service_spec.rb b/spec/services/ci/find_exposed_artifacts_service_spec.rb index f6309822fe0..b0f190b0e7a 100644 --- a/spec/services/ci/find_exposed_artifacts_service_spec.rb +++ b/spec/services/ci/find_exposed_artifacts_service_spec.rb @@ -50,10 +50,39 @@ describe Ci::FindExposedArtifactsService do end end + shared_examples 'does not find any matches' do + it 'returns empty array' do + expect(subject).to eq [] + end + end + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } subject { described_class.new(project, user).for_pipeline(pipeline) } + context 'with jobs having no exposed artifacts' do + let!(:job) do + create_job_with_artifacts(artifacts: { + paths: ['other_artifacts_0.1.2/doc_sample.txt', 'something-else.html'] + }) + end + + it_behaves_like 'does not find any matches' + end + + context 'with jobs having no artifacts (metadata)' do + let!(:job) do + create(:ci_build, pipeline: pipeline, options: { + artifacts: { + expose_as: 'Exposed artifact', + paths: ['other_artifacts_0.1.2/doc_sample.txt', 'something-else.html'] + } + }) + end + + it_behaves_like 'does not find any matches' + end + context 'with jobs having at most 1 matching exposed artifact' do let!(:job) do create_job_with_artifacts(artifacts: { diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 1f0119108a8..6393e482904 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -245,6 +245,12 @@ RSpec.configure do |config| Rails.cache = caching_store end + config.around do |example| + # Wrap each example in it's own context to make sure the contexts don't + # leak + Labkit::Context.with_context { example.run } + end + config.around(:each, :clean_gitlab_redis_cache) do |example| redis_cache_cleanup! diff --git a/spec/support/shared_examples/logging_application_context_shared_examples.rb b/spec/support/shared_examples/logging_application_context_shared_examples.rb new file mode 100644 index 00000000000..038ede884c8 --- /dev/null +++ b/spec/support/shared_examples/logging_application_context_shared_examples.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'storing arguments in the application context' do + around do |example| + Labkit::Context.with_context { example.run } + end + + it 'places the expected params in the application context' do + # Stub the clearing of the context so we can validate it later + # The `around` block above makes sure we do clean it up later + allow(Labkit::Context).to receive(:pop) + + subject + + Labkit::Context.with_context do |context| + expect(context.to_h) + .to include(log_hash(expected_params)) + end + end + + def log_hash(hash) + hash.transform_keys! { |key| "meta.#{key}" } + end +end -- cgit v1.2.1