From 995588ad627e9c97c1ebb1124a8c24d2fd117313 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 2 Jul 2018 15:38:05 +0200 Subject: [PATCH 01/27] Remove orphaned routes This removes all orphaned project and namespace routes from the "routes" table. This is in as a preparation step for removing dynamic route generation, as discussed in https://gitlab.com/gitlab-org/gitlab-ce/issues/44390. --- .../20180702124358_remove_orphaned_routes.rb | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 db/migrate/20180702124358_remove_orphaned_routes.rb diff --git a/db/migrate/20180702124358_remove_orphaned_routes.rb b/db/migrate/20180702124358_remove_orphaned_routes.rb new file mode 100644 index 00000000000..6f6e289ba87 --- /dev/null +++ b/db/migrate/20180702124358_remove_orphaned_routes.rb @@ -0,0 +1,49 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class RemoveOrphanedRoutes < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class Route < ActiveRecord::Base + self.table_name = 'routes' + include EachBatch + + def self.orphaned_namespace_routes + where(source_type: 'Namespace') + .where('NOT EXISTS ( SELECT 1 FROM namespaces WHERE namespaces.id = routes.source_id )') + end + + def self.orphaned_project_routes + where(source_type: 'Project') + .where('NOT EXISTS ( SELECT 1 FROM projects WHERE projects.id = routes.source_id )') + end + end + + def up + # Some of these queries can take up to 10 seconds to run on GitLab.com, + # which is pretty close to our 15 second statement timeout. To ensure a + # smooth deployment procedure we disable the statement timeouts for this + # migration, just in case. + disable_statement_timeout + + # On GitLab.com there are around 4000 orphaned project routes, and around + # 150 orphaned namespace routes. + [ + Route.orphaned_project_routes, + Route.orphaned_namespace_routes + ].each do |relation| + relation.each_batch(of: 1_000) do |batch| + batch.delete_all + end + end + end + + def down + # There is no way to restore orphaned routes, and this doesn't make any + # sense anyway. + end +end From e3aaa56c92f97f8aff3aeb847754794e9eae4706 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 24 Jul 2018 22:30:09 +0800 Subject: [PATCH 02/27] Introduce PolicyCheckable for checking policies --- app/models/deploy_token.rb | 5 +--- app/policies/concerns/policy_checkable.rb | 36 +++++++++++++++++++++++ config/application.rb | 1 + 3 files changed, 38 insertions(+), 4 deletions(-) create mode 100644 app/policies/concerns/policy_checkable.rb diff --git a/app/models/deploy_token.rb b/app/models/deploy_token.rb index 7ab647abe93..51029950dda 100644 --- a/app/models/deploy_token.rb +++ b/app/models/deploy_token.rb @@ -1,6 +1,7 @@ class DeployToken < ActiveRecord::Base include Expirable include TokenAuthenticatable + include PolicyCheckable add_authentication_token_field :token AVAILABLE_SCOPES = %i(read_repository read_registry).freeze @@ -58,10 +59,6 @@ class DeployToken < ActiveRecord::Base write_attribute(:expires_at, value.presence || Forever.date) end - def admin? - false - end - private def ensure_at_least_one_scope diff --git a/app/policies/concerns/policy_checkable.rb b/app/policies/concerns/policy_checkable.rb new file mode 100644 index 00000000000..5aae23c18e9 --- /dev/null +++ b/app/policies/concerns/policy_checkable.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +# Include this module if we want to pass something else than the user to +# check policies. This defines several methods which the policy checker +# would call and check. +module PolicyCheckable + extend ActiveSupport::Concern + + def blocked? + false + end + + def admin? + false + end + + def external? + false + end + + def internal? + false + end + + def access_locked? + false + end + + def required_terms_not_accepted? + false + end + + def can_create_group + false + end +end diff --git a/config/application.rb b/config/application.rb index 0304f466734..80d6ab7e9fa 100644 --- a/config/application.rb +++ b/config/application.rb @@ -43,6 +43,7 @@ module Gitlab #{config.root}/app/models/members #{config.root}/app/models/project_services #{config.root}/app/workers/concerns + #{config.root}/app/policies/concerns #{config.root}/app/services/concerns #{config.root}/app/serializers/concerns #{config.root}/app/finders/concerns From 7a233b37cd1281698107f1f3236b425bf4cc5ae7 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 2 Jul 2018 17:09:49 +0200 Subject: [PATCH 03/27] Remove code for dynamically generating routes This adds a database migration that creates routes for any projects and namespaces that don't already have one. We also remove the runtime code for dynamically creating routes, as this is no longer necessary. --- app/finders/admin/projects_finder.rb | 4 +- app/models/concerns/routable.rb | 44 +----- .../concerns/storage/legacy_namespace.rb | 2 - app/models/namespace.rb | 1 - app/models/project.rb | 3 - app/serializers/pipeline_serializer.rb | 9 +- app/services/projects/transfer_service.rb | 1 - app/views/admin/projects/_projects.html.haml | 2 +- .../stop-dynamic-routable-creation.yml | 5 + .../20180702134423_generate_missing_routes.rb | 143 ++++++++++++++++++ .../projects/pipelines_controller_spec.rb | 14 +- .../generate_missing_routes_spec.rb | 84 ++++++++++ spec/models/concerns/routable_spec.rb | 33 ---- spec/models/namespace_spec.rb | 10 ++ spec/models/project_spec.rb | 4 - spec/serializers/pipeline_serializer_spec.rb | 4 +- .../projects/transfer_service_spec.rb | 6 - 17 files changed, 265 insertions(+), 104 deletions(-) create mode 100644 changelogs/unreleased/stop-dynamic-routable-creation.yml create mode 100644 db/migrate/20180702134423_generate_missing_routes.rb create mode 100644 spec/migrations/generate_missing_routes_spec.rb diff --git a/app/finders/admin/projects_finder.rb b/app/finders/admin/projects_finder.rb index 53b77f5fed9..543bf1a1415 100644 --- a/app/finders/admin/projects_finder.rb +++ b/app/finders/admin/projects_finder.rb @@ -7,7 +7,7 @@ class Admin::ProjectsFinder end def execute - items = Project.without_deleted.with_statistics + items = Project.without_deleted.with_statistics.with_route items = by_namespace_id(items) items = by_visibilty_level(items) items = by_with_push(items) @@ -16,7 +16,7 @@ class Admin::ProjectsFinder items = by_archived(items) items = by_personal(items) items = by_name(items) - items = items.includes(namespace: [:owner]) + items = items.includes(namespace: [:owner, :route]) sort(items).page(params[:page]) end diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index 0176a12a131..cb91f8fbac8 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -90,34 +90,17 @@ module Routable end def full_name - if route && route.name.present? - @full_name ||= route.name # rubocop:disable Gitlab/ModuleWithInstanceVariables - else - update_route if persisted? - - build_full_name - end + route&.name || build_full_name end - # Every time `project.namespace.becomes(Namespace)` is called for polymorphic_path, - # a new instance is instantiated, and we end up duplicating the same query to retrieve - # the route. Caching this per request ensures that even if we have multiple instances, - # we will not have to duplicate work, avoiding N+1 queries in some cases. def full_path - return uncached_full_path unless RequestStore.active? && persisted? - - RequestStore[full_path_key] ||= uncached_full_path + route&.path || build_full_path end def full_path_components full_path.split('/') end - def expires_full_path_cache - RequestStore.delete(full_path_key) if RequestStore.active? - @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables - end - def build_full_path if parent && path parent.full_path + '/' + path @@ -138,16 +121,6 @@ module Routable self.errors[:path].concat(route_path_errors) if route_path_errors end - def uncached_full_path - if route && route.path.present? - @full_path ||= route.path # rubocop:disable Gitlab/ModuleWithInstanceVariables - else - update_route if persisted? - - build_full_path - end - end - def full_name_changed? name_changed? || parent_changed? end @@ -156,10 +129,6 @@ module Routable path_changed? || parent_changed? end - def full_path_key - @full_path_key ||= "routable/full_path/#{self.class.name}/#{self.id}" - end - def build_full_name if parent && name parent.human_name + ' / ' + name @@ -168,18 +137,9 @@ module Routable end end - def update_route - return if Gitlab::Database.read_only? - - prepare_route - route.save - end - def prepare_route route || build_route(source: self) route.path = build_full_path route.name = build_full_name - @full_path = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables - @full_name = nil # rubocop:disable Gitlab/ModuleWithInstanceVariables end end diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb index f66bdd529f1..d61ad19b43d 100644 --- a/app/models/concerns/storage/legacy_namespace.rb +++ b/app/models/concerns/storage/legacy_namespace.rb @@ -11,8 +11,6 @@ module Storage Namespace.find(parent_id_was) # raise NotFound early if needed end - expires_full_path_cache - move_repositories if parent_changed? diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 7034c633268..c1dc2f55346 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -304,7 +304,6 @@ class Namespace < ActiveRecord::Base def write_projects_repository_config all_projects.find_each do |project| - project.expires_full_path_cache # we need to clear cache to validate renames correctly project.write_repository_config end end diff --git a/app/models/project.rb b/app/models/project.rb index f880d728839..2034e752635 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1233,8 +1233,6 @@ class Project < ActiveRecord::Base return true if skip_disk_validation return false unless repository_storage - expires_full_path_cache # we need to clear cache to validate renames correctly - # Check if repository with same path already exists on disk we can # skip this for the hashed storage because the path does not change if legacy_storage? && repository_with_same_path_already_exists? @@ -1613,7 +1611,6 @@ class Project < ActiveRecord::Base # When we import a project overwriting the original project, there # is a move operation. In that case we don't want to send the instructions. send_move_instructions(full_path_was) unless import_started? - expires_full_path_cache self.old_path_with_namespace = full_path_was SystemHooksService.new.execute_hooks_for(self, :rename) diff --git a/app/serializers/pipeline_serializer.rb b/app/serializers/pipeline_serializer.rb index 4a33160afa1..3205578b83e 100644 --- a/app/serializers/pipeline_serializer.rb +++ b/app/serializers/pipeline_serializer.rb @@ -11,10 +11,15 @@ class PipelineSerializer < BaseSerializer :retryable_builds, :cancelable_statuses, :trigger_requests, - :project, :manual_actions, :artifacts, - { pending_builds: :project } + { + pending_builds: :project, + project: [:route, { namespace: :route }], + artifacts: { + project: [:route, { namespace: :route }] + } + } ]) end diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index a4a66330546..c2a0c5fa7f3 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -77,7 +77,6 @@ module Projects Gitlab::PagesTransfer.new.move_project(project.path, @old_namespace.full_path, @new_namespace.full_path) project.old_path_with_namespace = @old_path - project.expires_full_path_cache write_repository_config(@new_path) diff --git a/app/views/admin/projects/_projects.html.haml b/app/views/admin/projects/_projects.html.haml index 00933d726d9..fdaacc098e0 100644 --- a/app/views/admin/projects/_projects.html.haml +++ b/app/views/admin/projects/_projects.html.haml @@ -17,7 +17,7 @@ - if project.archived %span.badge.badge-warning archived .title - = link_to [:admin, project.namespace.becomes(Namespace), project] do + = link_to(admin_namespace_project_path(project.namespace, project)) do .dash-project-avatar .avatar-container.s40 = project_icon(project, alt: '', class: 'avatar project-avatar s40') diff --git a/changelogs/unreleased/stop-dynamic-routable-creation.yml b/changelogs/unreleased/stop-dynamic-routable-creation.yml new file mode 100644 index 00000000000..8bfcb5b2d11 --- /dev/null +++ b/changelogs/unreleased/stop-dynamic-routable-creation.yml @@ -0,0 +1,5 @@ +--- +title: Stop dynamically creating project and namespace routes +merge_request: 20313 +author: +type: performance diff --git a/db/migrate/20180702134423_generate_missing_routes.rb b/db/migrate/20180702134423_generate_missing_routes.rb new file mode 100644 index 00000000000..994725f9bd1 --- /dev/null +++ b/db/migrate/20180702134423_generate_missing_routes.rb @@ -0,0 +1,143 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +# This migration generates missing routes for any projects and namespaces that +# don't already have a route. +# +# On GitLab.com this would insert 611 project routes, and 0 namespace routes. +# The exact number could vary per instance, so we take care of both just in +# case. +class GenerateMissingRoutes < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + class User < ActiveRecord::Base + self.table_name = 'users' + end + + class Route < ActiveRecord::Base + self.table_name = 'routes' + end + + module Routable + def build_full_path + if parent && path + parent.build_full_path + '/' + path + else + path + end + end + + def build_full_name + if parent && name + parent.human_name + ' / ' + name + else + name + end + end + + def human_name + build_full_name + end + + def attributes_for_insert + time = Time.zone.now + + { + # We can't use "self.class.name" here as that would include the + # migration namespace. + source_type: source_type_for_route, + source_id: id, + created_at: time, + updated_at: time, + name: build_full_name, + + # The route path might already be taken. Instead of trying to generate a + # new unique name on every conflict, we just append the row ID to the + # route path. + path: "#{build_full_path}-#{id}" + } + end + end + + class Project < ActiveRecord::Base + self.table_name = 'projects' + + include EachBatch + include GenerateMissingRoutes::Routable + + belongs_to :namespace, class_name: 'GenerateMissingRoutes::Namespace' + + has_one :route, + as: :source, + inverse_of: :source, + class_name: 'GenerateMissingRoutes::Route' + + alias_method :parent, :namespace + alias_attribute :parent_id, :namespace_id + + def self.without_routes + where( + 'NOT EXISTS ( + SELECT 1 + FROM routes + WHERE source_type = ? + AND source_id = projects.id + )', + 'Project' + ) + end + + def source_type_for_route + 'Project' + end + end + + class Namespace < ActiveRecord::Base + self.table_name = 'namespaces' + + include EachBatch + include GenerateMissingRoutes::Routable + + belongs_to :parent, class_name: 'GenerateMissingRoutes::Namespace' + belongs_to :owner, class_name: 'GenerateMissingRoutes::User' + + has_one :route, + as: :source, + inverse_of: :source, + class_name: 'GenerateMissingRoutes::Route' + + def self.without_routes + where( + 'NOT EXISTS ( + SELECT 1 + FROM routes + WHERE source_type = ? + AND source_id = namespaces.id + )', + 'Namespace' + ) + end + + def source_type_for_route + 'Namespace' + end + end + + def up + [Namespace, Project].each do |model| + model.without_routes.each_batch(of: 100) do |batch| + rows = batch.map(&:attributes_for_insert) + + Gitlab::Database.bulk_insert(:routes, rows) + end + end + end + + def down + # Removing routes we previously generated makes no sense. + end +end diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 290fcd4f8e6..d89716b1b50 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -55,10 +55,8 @@ describe Projects::PipelinesController do stub_feature_flags(ci_pipeline_persisted_stages: false) end - it 'returns JSON with serialized pipelines', :request_store do - queries = ActiveRecord::QueryRecorder.new do - get_pipelines_index_json - end + it 'returns JSON with serialized pipelines' do + get_pipelines_index_json expect(response).to have_gitlab_http_status(:ok) expect(response).to match_response_schema('pipeline') @@ -73,8 +71,14 @@ describe Projects::PipelinesController do json_response.dig('pipelines', 0, 'details', 'stages').tap do |stages| expect(stages.count).to eq 3 end + end - expect(queries.count).to be_within(5).of(30) + it 'does not execute N+1 queries' do + queries = ActiveRecord::QueryRecorder.new do + get_pipelines_index_json + end + + expect(queries.count).to be <= 36 end end diff --git a/spec/migrations/generate_missing_routes_spec.rb b/spec/migrations/generate_missing_routes_spec.rb new file mode 100644 index 00000000000..32515d353b0 --- /dev/null +++ b/spec/migrations/generate_missing_routes_spec.rb @@ -0,0 +1,84 @@ +require 'spec_helper' +require Rails.root.join('db', 'migrate', '20180702134423_generate_missing_routes.rb') + +describe GenerateMissingRoutes, :migration do + describe '#up' do + let(:namespaces) { table(:namespaces) } + let(:projects) { table(:projects) } + let(:routes) { table(:routes) } + + it 'creates routes for projects without a route' do + namespace = namespaces.create!(name: 'GitLab', path: 'gitlab') + + routes.create!( + path: 'gitlab', + source_type: 'Namespace', + source_id: namespace.id + ) + + project = projects.create!( + name: 'GitLab CE', + path: 'gitlab-ce', + namespace_id: namespace.id + ) + + described_class.new.up + + route = routes.where(source_type: 'Project').take + + expect(route.source_id).to eq(project.id) + expect(route.path).to eq("gitlab/gitlab-ce-#{project.id}") + end + + it 'creates routes for namespaces without a route' do + namespace = namespaces.create!(name: 'GitLab', path: 'gitlab') + + described_class.new.up + + route = routes.where(source_type: 'Namespace').take + + expect(route.source_id).to eq(namespace.id) + expect(route.path).to eq("gitlab-#{namespace.id}") + end + + it 'does not create routes for namespaces that already have a route' do + namespace = namespaces.create!(name: 'GitLab', path: 'gitlab') + + routes.create!( + path: 'gitlab', + source_type: 'Namespace', + source_id: namespace.id + ) + + described_class.new.up + + expect(routes.count).to eq(1) + end + + it 'does not create routes for projects that already have a route' do + namespace = namespaces.create!(name: 'GitLab', path: 'gitlab') + + routes.create!( + path: 'gitlab', + source_type: 'Namespace', + source_id: namespace.id + ) + + project = projects.create!( + name: 'GitLab CE', + path: 'gitlab-ce', + namespace_id: namespace.id + ) + + routes.create!( + path: 'gitlab/gitlab-ce', + source_type: 'Project', + source_id: project.id + ) + + described_class.new.up + + expect(routes.count).to eq(2) + end + end +end diff --git a/spec/models/concerns/routable_spec.rb b/spec/models/concerns/routable_spec.rb index ed3e28fbeca..565266321d3 100644 --- a/spec/models/concerns/routable_spec.rb +++ b/spec/models/concerns/routable_spec.rb @@ -12,16 +12,6 @@ describe Group, 'Routable' do it { is_expected.to have_many(:redirect_routes).dependent(:destroy) } end - describe 'GitLab read-only instance' do - it 'does not save route if route is not present' do - group.route.path = '' - allow(Gitlab::Database).to receive(:read_only?).and_return(true) - expect(group).to receive(:update_route).and_call_original - - expect { group.full_path }.to change { Route.count }.by(0) - end - end - describe 'Callbacks' do it 'creates route record on create' do expect(group.route.path).to eq(group.path) @@ -131,29 +121,6 @@ describe Group, 'Routable' do it { expect(group.full_path).to eq(group.path) } it { expect(nested_group.full_path).to eq("#{group.full_path}/#{nested_group.path}") } - - context 'with RequestStore active', :request_store do - it 'does not load the route table more than once' do - group.expires_full_path_cache - expect(group).to receive(:uncached_full_path).once.and_call_original - - 3.times { group.full_path } - expect(group.full_path).to eq(group.path) - end - end - end - - describe '#expires_full_path_cache' do - context 'with RequestStore active', :request_store do - it 'expires the full_path cache' do - expect(group.full_path).to eq('foo') - - group.route.update(path: 'bar', name: 'bar') - group.expires_full_path_cache - - expect(group.full_path).to eq('bar') - end - end end describe '#full_name' do diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index c1b385aaf76..f56b79119d3 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -295,6 +295,16 @@ describe Namespace do parent.update(path: 'mygroup_new') + # Routes are loaded when creating the projects, so we need to manually + # reload them for the below code to be aware of the above UPDATE. + [ + project_in_parent_group, + hashed_project_in_subgroup, + legacy_project_in_subgroup + ].each do |project| + project.route.reload + end + expect(project_rugged(project_in_parent_group).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}" expect(project_rugged(hashed_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}" expect(project_rugged(legacy_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{legacy_project_in_subgroup.path}" diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index b0ec725bf70..deaaaed424b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2942,8 +2942,6 @@ describe Project do expect(project).to receive(:expire_caches_before_rename) - expect(project).to receive(:expires_full_path_cache) - project.rename_repo end @@ -3103,8 +3101,6 @@ describe Project do expect(project).to receive(:expire_caches_before_rename) - expect(project).to receive(:expires_full_path_cache) - project.rename_repo end diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index eb4235e3ee6..cf57776346a 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -125,7 +125,7 @@ describe PipelineSerializer do it 'verifies number of queries', :request_store do recorded = ActiveRecord::QueryRecorder.new { subject } - expect(recorded.count).to be_within(2).of(27) + expect(recorded.count).to be_within(2).of(31) expect(recorded.cached_count).to eq(0) end end @@ -144,7 +144,7 @@ describe PipelineSerializer do # pipeline. With the same ref this check is cached but if refs are # different then there is an extra query per ref # https://gitlab.com/gitlab-org/gitlab-ce/issues/46368 - expect(recorded.count).to be_within(2).of(30) + expect(recorded.count).to be_within(2).of(34) expect(recorded.cached_count).to eq(0) end end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 7e85f599afb..1a85c52fc97 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -31,12 +31,6 @@ describe Projects::TransferService do transfer_project(project, user, group) end - it 'expires full_path cache' do - expect(project).to receive(:expires_full_path_cache) - - transfer_project(project, user, group) - end - it 'invalidates the user\'s personal_project_count cache' do expect(user).to receive(:invalidate_personal_projects_count) From 43914593c177b50f6c413921e6ff0eb0a2049f91 Mon Sep 17 00:00:00 2001 From: Jasper Maes Date: Thu, 26 Jul 2018 17:35:42 +0200 Subject: [PATCH 04/27] Rails5 update Gemfile.rails5.lock --- Gemfile.rails5.lock | 13 +++++++------ .../unreleased/rails5-update-gemfile-lock-2.yml | 5 +++++ 2 files changed, 12 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/rails5-update-gemfile-lock-2.yml diff --git a/Gemfile.rails5.lock b/Gemfile.rails5.lock index cb64fe554ff..b055b561b61 100644 --- a/Gemfile.rails5.lock +++ b/Gemfile.rails5.lock @@ -364,7 +364,7 @@ GEM grape-entity (0.7.1) activesupport (>= 4.0) multi_json (>= 1.3.2) - grape-path-helpers (1.0.5) + grape-path-helpers (1.0.6) activesupport (>= 4, < 5.1) grape (~> 1.0) rake (~> 12) @@ -378,7 +378,8 @@ GEM google-protobuf (~> 3.1) googleapis-common-protos-types (~> 1.0.0) googleauth (>= 0.5.1, < 0.7) - haml (4.0.7) + haml (5.0.4) + temple (>= 0.8.0) tilt haml_lint (0.26.0) haml (>= 4.0, < 5.1) @@ -400,7 +401,7 @@ GEM hipchat (1.5.2) httparty mimemagic - html-pipeline (2.8.3) + html-pipeline (2.8.4) activesupport (>= 2) nokogiri (>= 1.4) html2text (0.2.0) @@ -518,7 +519,7 @@ GEM net-ssh (5.0.1) netrc (0.11.0) nio4r (2.3.1) - nokogiri (1.8.3) + nokogiri (1.8.4) mini_portile2 (~> 2.3.0) nokogumbo (1.5.0) nokogiri @@ -821,7 +822,7 @@ GEM et-orbi (~> 1.0) rugged (0.27.2) safe_yaml (1.0.4) - sanitize (4.6.5) + sanitize (4.6.6) crass (~> 1.0.2) nokogiri (>= 1.4.4) nokogumbo (~> 1.4) @@ -915,7 +916,7 @@ GEM rack (>= 1, < 3) thor (0.19.4) thread_safe (0.3.6) - tilt (2.0.6) + tilt (2.0.8) timecop (0.8.1) timfel-krb5-auth (0.8.3) toml (0.1.2) diff --git a/changelogs/unreleased/rails5-update-gemfile-lock-2.yml b/changelogs/unreleased/rails5-update-gemfile-lock-2.yml new file mode 100644 index 00000000000..1f3e9bd2238 --- /dev/null +++ b/changelogs/unreleased/rails5-update-gemfile-lock-2.yml @@ -0,0 +1,5 @@ +--- +title: Rails5 update Gemfile.rails5.lock +merge_request: 20858 +author: Jasper Maes +type: fixed From f8f2d0f9e18a76324d85ae319a3bb8585ed107d5 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Wed, 18 Jul 2018 01:37:58 +0100 Subject: [PATCH 05/27] Create instance_statistics namespace and move convdev index and cohorts to it --- ...ersational_development_index_controller.rb | 5 --- .../application_controller.rb | 12 +++++++ .../cohorts_controller.rb | 2 +- ...ersational_development_index_controller.rb | 5 +++ .../application_settings/_usage.html.haml | 2 +- .../cohorts/_cohorts_table.html.haml | 0 .../cohorts/_usage_ping.html.haml | 0 .../cohorts/index.html.haml | 0 .../_callout.html.haml | 0 .../_card.html.haml | 0 .../_disabled.html.haml | 0 .../_no_data.html.haml | 0 .../index.html.haml} | 0 .../layouts/instance_statistics.html.haml | 6 ++++ .../layouts/nav/sidebar/_admin.html.haml | 4 +-- .../sidebar/_instance_statistics.html.haml | 33 +++++++++++++++++++ config/routes.rb | 1 + config/routes/admin.rb | 4 --- config/routes/instance_statistics.rb | 6 ++++ .../cohorts_spec.rb} | 4 +-- .../conversational_development_index_spec.rb} | 8 ++--- 21 files changed, 73 insertions(+), 19 deletions(-) delete mode 100644 app/controllers/admin/conversational_development_index_controller.rb create mode 100644 app/controllers/instance_statistics/application_controller.rb rename app/controllers/{admin => instance_statistics}/cohorts_controller.rb (75%) create mode 100644 app/controllers/instance_statistics/conversational_development_index_controller.rb rename app/views/{admin => instance_statistics}/cohorts/_cohorts_table.html.haml (100%) rename app/views/{admin => instance_statistics}/cohorts/_usage_ping.html.haml (100%) rename app/views/{admin => instance_statistics}/cohorts/index.html.haml (100%) rename app/views/{admin => instance_statistics}/conversational_development_index/_callout.html.haml (100%) rename app/views/{admin => instance_statistics}/conversational_development_index/_card.html.haml (100%) rename app/views/{admin => instance_statistics}/conversational_development_index/_disabled.html.haml (100%) rename app/views/{admin => instance_statistics}/conversational_development_index/_no_data.html.haml (100%) rename app/views/{admin/conversational_development_index/show.html.haml => instance_statistics/conversational_development_index/index.html.haml} (100%) create mode 100644 app/views/layouts/instance_statistics.html.haml create mode 100644 app/views/layouts/nav/sidebar/_instance_statistics.html.haml create mode 100644 config/routes/instance_statistics.rb rename spec/features/{admin/admin_cohorts_spec.rb => instance_statistics/cohorts_spec.rb} (75%) rename spec/features/{admin/admin_conversational_development_index_spec.rb => instance_statistics/conversational_development_index_spec.rb} (75%) diff --git a/app/controllers/admin/conversational_development_index_controller.rb b/app/controllers/admin/conversational_development_index_controller.rb deleted file mode 100644 index 921169d3e2b..00000000000 --- a/app/controllers/admin/conversational_development_index_controller.rb +++ /dev/null @@ -1,5 +0,0 @@ -class Admin::ConversationalDevelopmentIndexController < Admin::ApplicationController - def show - @metric = ConversationalDevelopmentIndex::Metric.order(:created_at).last&.present - end -end diff --git a/app/controllers/instance_statistics/application_controller.rb b/app/controllers/instance_statistics/application_controller.rb new file mode 100644 index 00000000000..fbdce7d0f0d --- /dev/null +++ b/app/controllers/instance_statistics/application_controller.rb @@ -0,0 +1,12 @@ +class InstanceStatistics::ApplicationController < ApplicationController + before_action :authenticate_user! + layout 'instance_statistics' + + def index + redirect_to instance_statistics_conversations_development_index_index_path + end + + def authenticate_user! + render_404 unless current_user.admin? + end +end diff --git a/app/controllers/admin/cohorts_controller.rb b/app/controllers/instance_statistics/cohorts_controller.rb similarity index 75% rename from app/controllers/admin/cohorts_controller.rb rename to app/controllers/instance_statistics/cohorts_controller.rb index 10d9d1b5345..77d09c198c8 100644 --- a/app/controllers/admin/cohorts_controller.rb +++ b/app/controllers/instance_statistics/cohorts_controller.rb @@ -1,4 +1,4 @@ -class Admin::CohortsController < Admin::ApplicationController +class InstanceStatistics::CohortsController < InstanceStatistics::ApplicationController def index if Gitlab::CurrentSettings.usage_ping_enabled cohorts_results = Rails.cache.fetch('cohorts', expires_in: 1.day) do diff --git a/app/controllers/instance_statistics/conversational_development_index_controller.rb b/app/controllers/instance_statistics/conversational_development_index_controller.rb new file mode 100644 index 00000000000..023ce917e13 --- /dev/null +++ b/app/controllers/instance_statistics/conversational_development_index_controller.rb @@ -0,0 +1,5 @@ +class InstanceStatistics::ConversationalDevelopmentIndexController < InstanceStatistics::ApplicationController + def index + @metric = ConversationalDevelopmentIndex::Metric.order(:created_at).last&.present + end +end diff --git a/app/views/admin/application_settings/_usage.html.haml b/app/views/admin/application_settings/_usage.html.haml index 49a3ee33a85..40b5c51ac88 100644 --- a/app/views/admin/application_settings/_usage.html.haml +++ b/app/views/admin/application_settings/_usage.html.haml @@ -23,7 +23,7 @@ periodically collect usage information. = link_to 'Learn more', help_page_path("user/admin_area/settings/usage_statistics", anchor: "usage-ping") about what information is shared with GitLab Inc. Visit - = link_to 'Cohorts', admin_cohorts_path(anchor: 'usage-ping') + = link_to 'Cohorts', instance_statistics_cohorts_path(anchor: 'usage-ping') to see the JSON payload sent. - else The usage ping is disabled, and cannot be configured through this diff --git a/app/views/admin/cohorts/_cohorts_table.html.haml b/app/views/instance_statistics/cohorts/_cohorts_table.html.haml similarity index 100% rename from app/views/admin/cohorts/_cohorts_table.html.haml rename to app/views/instance_statistics/cohorts/_cohorts_table.html.haml diff --git a/app/views/admin/cohorts/_usage_ping.html.haml b/app/views/instance_statistics/cohorts/_usage_ping.html.haml similarity index 100% rename from app/views/admin/cohorts/_usage_ping.html.haml rename to app/views/instance_statistics/cohorts/_usage_ping.html.haml diff --git a/app/views/admin/cohorts/index.html.haml b/app/views/instance_statistics/cohorts/index.html.haml similarity index 100% rename from app/views/admin/cohorts/index.html.haml rename to app/views/instance_statistics/cohorts/index.html.haml diff --git a/app/views/admin/conversational_development_index/_callout.html.haml b/app/views/instance_statistics/conversational_development_index/_callout.html.haml similarity index 100% rename from app/views/admin/conversational_development_index/_callout.html.haml rename to app/views/instance_statistics/conversational_development_index/_callout.html.haml diff --git a/app/views/admin/conversational_development_index/_card.html.haml b/app/views/instance_statistics/conversational_development_index/_card.html.haml similarity index 100% rename from app/views/admin/conversational_development_index/_card.html.haml rename to app/views/instance_statistics/conversational_development_index/_card.html.haml diff --git a/app/views/admin/conversational_development_index/_disabled.html.haml b/app/views/instance_statistics/conversational_development_index/_disabled.html.haml similarity index 100% rename from app/views/admin/conversational_development_index/_disabled.html.haml rename to app/views/instance_statistics/conversational_development_index/_disabled.html.haml diff --git a/app/views/admin/conversational_development_index/_no_data.html.haml b/app/views/instance_statistics/conversational_development_index/_no_data.html.haml similarity index 100% rename from app/views/admin/conversational_development_index/_no_data.html.haml rename to app/views/instance_statistics/conversational_development_index/_no_data.html.haml diff --git a/app/views/admin/conversational_development_index/show.html.haml b/app/views/instance_statistics/conversational_development_index/index.html.haml similarity index 100% rename from app/views/admin/conversational_development_index/show.html.haml rename to app/views/instance_statistics/conversational_development_index/index.html.haml diff --git a/app/views/layouts/instance_statistics.html.haml b/app/views/layouts/instance_statistics.html.haml new file mode 100644 index 00000000000..af39318b2ab --- /dev/null +++ b/app/views/layouts/instance_statistics.html.haml @@ -0,0 +1,6 @@ +- page_title "Instance Statistics" +- header_title "Instance Statistics", instance_statistics_root_path +- nav "instance_statistics" +- @left_sidebar = true + += render template: "layouts/application" diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml index 0047efa363d..302de31a6d4 100644 --- a/app/views/layouts/nav/sidebar/_admin.html.haml +++ b/app/views/layouts/nav/sidebar/_admin.html.haml @@ -48,11 +48,11 @@ %span = _('Gitaly Servers') = nav_link path: 'cohorts#index' do - = link_to admin_cohorts_path, title: _('Cohorts') do + = link_to instance_statistics_cohorts_path, title: _('Cohorts') do %span = _('Cohorts') = nav_link(controller: :conversational_development_index) do - = link_to admin_conversational_development_index_path, title: _('ConvDev Index') do + = link_to instance_statistics_conversational_development_index_index_path, title: _('ConvDev Index') do %span = _('ConvDev Index') diff --git a/app/views/layouts/nav/sidebar/_instance_statistics.html.haml b/app/views/layouts/nav/sidebar/_instance_statistics.html.haml new file mode 100644 index 00000000000..cb64c97f7b4 --- /dev/null +++ b/app/views/layouts/nav/sidebar/_instance_statistics.html.haml @@ -0,0 +1,33 @@ +.nav-sidebar{ class: ("sidebar-collapsed-desktop" if collapsed_sidebar?) } + .nav-sidebar-inner-scroll + .context-header + = link_to instance_statistics_root_path, title: 'Instance Statistics' do + .avatar-container.s40.settings-avatar + = sprite_icon('chart', size: 24) + .sidebar-context-title Instance Statistics + %ul.sidebar-top-level-items + = nav_link(controller: :conversational_development_index) do + = link_to instance_statistics_conversational_development_index_index_path do + .nav-icon-container + = sprite_icon('comment') + %span.nav-item-name + = _('ConvDev Index') + %ul.sidebar-sub-level-items.is-fly-out-only + = nav_link(controller: :conversational_development_index, html_options: { class: "fly-out-top-item" } ) do + = link_to instance_statistics_conversational_development_index_index_path do + %strong.fly-out-top-item-name + = _('ConvDev Index') + + = nav_link(controller: :cohorts) do + = link_to instance_statistics_cohorts_path do + .nav-icon-container + = sprite_icon('users') + %span.nav-item-name + = _('Cohorts') + %ul.sidebar-sub-level-items.is-fly-out-only + = nav_link(controller: :cohorts, html_options: { class: "fly-out-top-item" } ) do + = link_to instance_statistics_cohorts_path do + %strong.fly-out-top-item-name + = _('Cohorts') + + = render 'shared/sidebar_toggle_button' diff --git a/config/routes.rb b/config/routes.rb index e0a9139b1b4..17b969f669d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -110,6 +110,7 @@ Rails.application.routes.draw do draw :group draw :user draw :project + draw :instance_statistics root to: "root#index" diff --git a/config/routes/admin.rb b/config/routes/admin.rb index ff27ceb50dc..0ed57d78066 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -76,8 +76,6 @@ namespace :admin do resource :system_info, controller: 'system_info', only: [:show] resources :requests_profiles, only: [:index, :show], param: :name, constraints: { name: /.+\.html/ } - get 'conversational_development_index' => 'conversational_development_index#show' - resources :projects, only: [:index] scope(path: 'projects/*namespace_id', @@ -123,8 +121,6 @@ namespace :admin do end end - resources :cohorts, only: :index - resources :jobs, only: :index do collection do post :cancel_all diff --git a/config/routes/instance_statistics.rb b/config/routes/instance_statistics.rb new file mode 100644 index 00000000000..365797cb688 --- /dev/null +++ b/config/routes/instance_statistics.rb @@ -0,0 +1,6 @@ +namespace :instance_statistics do + root to: redirect("instance_statistics/conversational_development_index") + + resources :cohorts, only: :index + resources :conversational_development_index, only: :index +end diff --git a/spec/features/admin/admin_cohorts_spec.rb b/spec/features/instance_statistics/cohorts_spec.rb similarity index 75% rename from spec/features/admin/admin_cohorts_spec.rb rename to spec/features/instance_statistics/cohorts_spec.rb index 9dce9494b97..81fc5eff980 100644 --- a/spec/features/admin/admin_cohorts_spec.rb +++ b/spec/features/instance_statistics/cohorts_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -describe 'Admin cohorts page' do +describe 'Cohorts page' do before do sign_in(create(:admin)) end @@ -8,7 +8,7 @@ describe 'Admin cohorts page' do it 'See users count per month' do 2.times { create(:user) } - visit admin_cohorts_path + visit instance_statistics_cohorts_path expect(page).to have_content("#{Time.now.strftime('%b %Y')} 3 0") end diff --git a/spec/features/admin/admin_conversational_development_index_spec.rb b/spec/features/instance_statistics/conversational_development_index_spec.rb similarity index 75% rename from spec/features/admin/admin_conversational_development_index_spec.rb rename to spec/features/instance_statistics/conversational_development_index_spec.rb index 2d2c7df5364..d441a7a5af9 100644 --- a/spec/features/admin/admin_conversational_development_index_spec.rb +++ b/spec/features/instance_statistics/conversational_development_index_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe 'Admin Conversational Development Index' do +describe 'Conversational Development Index' do before do sign_in(create(:admin)) end @@ -9,7 +9,7 @@ describe 'Admin Conversational Development Index' do it 'shows empty state' do stub_application_setting(usage_ping_enabled: false) - visit admin_conversational_development_index_path + visit instance_statistics_conversational_development_index_index_path expect(page).to have_content('Usage ping is not enabled') end @@ -19,7 +19,7 @@ describe 'Admin Conversational Development Index' do it 'shows empty state' do stub_application_setting(usage_ping_enabled: true) - visit admin_conversational_development_index_path + visit instance_statistics_conversational_development_index_index_path expect(page).to have_content('Data is still calculating') end @@ -30,7 +30,7 @@ describe 'Admin Conversational Development Index' do stub_application_setting(usage_ping_enabled: true) create(:conversational_development_index_metric) - visit admin_conversational_development_index_path + visit instance_statistics_conversational_development_index_index_path expect(page).to have_content( 'Issues created per active user 1.2 You 9.3 Lead 13.3%' From 866429f7892842c2a29a902eace64efac781019b Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Wed, 18 Jul 2018 02:41:40 +0100 Subject: [PATCH 06/27] Add instance_statistics_visibility_private application setting and top nav icon --- .../application_controller.rb | 2 +- app/helpers/application_settings_helper.rb | 5 +++++ app/models/application_setting.rb | 1 + .../application_settings/_usage.html.haml | 3 +++ app/views/layouts/nav/_dashboard.html.haml | 6 +++++- ...istics_visibility_to_application_setting.rb | 18 ++++++++++++++++++ 6 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20180718005113_add_instance_statistics_visibility_to_application_setting.rb diff --git a/app/controllers/instance_statistics/application_controller.rb b/app/controllers/instance_statistics/application_controller.rb index fbdce7d0f0d..7c454e15a59 100644 --- a/app/controllers/instance_statistics/application_controller.rb +++ b/app/controllers/instance_statistics/application_controller.rb @@ -7,6 +7,6 @@ class InstanceStatistics::ApplicationController < ApplicationController end def authenticate_user! - render_404 unless current_user.admin? + render_404 if ApplicationSettingsHelper.hide_instance_statistics?(current_user) end end diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 358b896702b..b355008f087 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -15,6 +15,10 @@ module ApplicationSettingsHelper def allowed_protocols_present? Gitlab::CurrentSettings.enabled_git_access_protocol.present? end + + def hide_instance_statistics?(user = current_user) + Gitlab::CurrentSettings.instance_statistics_visibility_private? && !user.admin? + end def enabled_protocol case Gitlab::CurrentSettings.enabled_git_access_protocol @@ -247,6 +251,7 @@ module ApplicationSettingsHelper :unique_ips_limit_per_user, :unique_ips_limit_time_window, :usage_ping_enabled, + :instance_statistics_visibility_private, :user_default_external, :user_oauth_applications, :version_check_enabled, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index f770b219422..01a59039cff 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -290,6 +290,7 @@ class ApplicationSetting < ActiveRecord::Base user_default_external: false, polling_interval_multiplier: 1, usage_ping_enabled: Settings.gitlab['usage_ping_enabled'], + instance_statistics_visibility_private: false, gitaly_timeout_fast: 10, gitaly_timeout_medium: 30, gitaly_timeout_default: 55, diff --git a/app/views/admin/application_settings/_usage.html.haml b/app/views/admin/application_settings/_usage.html.haml index 40b5c51ac88..860e7765168 100644 --- a/app/views/admin/application_settings/_usage.html.haml +++ b/app/views/admin/application_settings/_usage.html.haml @@ -30,6 +30,9 @@ form. For more information, see the documentation on = succeed '.' do = link_to 'deactivating the usage ping', help_page_path('user/admin_area/settings/usage_statistics', anchor: 'deactivate-the-usage-ping') + .form-group + = f.label :instance_statistics_visibility_private, 'Instance Statistics visibility' + = f.select :instance_statistics_visibility_private, options_for_select({'All users' => false, 'Only admins' => true}, Gitlab::CurrentSettings.instance_statistics_visibility_private?), {}, class: 'form-control' = f.submit 'Save changes', class: "btn btn-success" diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index a71a4b13a7e..356e52918d9 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -70,8 +70,12 @@ = nav_link(controller: 'admin/dashboard') do = link_to admin_root_path, class: 'admin-icon qa-admin-area-link', title: _('Admin area'), aria: { label: _("Admin area") }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do = sprite_icon('admin', size: 18) + - unless hide_instance_statistics? + = nav_link(controller: :instance_statistics) do + = link_to instance_statistics_root_path, title: 'Instance statistics', aria: { label: 'Instance statistics' }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do + = sprite_icon('chart', size: 18) - if Gitlab::Sherlock.enabled? %li = link_to sherlock_transactions_path, class: 'admin-icon', title: _('Sherlock Transactions'), data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do - = icon('tachometer fw') + = icon('tachometer fw') \ No newline at end of file diff --git a/db/migrate/20180718005113_add_instance_statistics_visibility_to_application_setting.rb b/db/migrate/20180718005113_add_instance_statistics_visibility_to_application_setting.rb new file mode 100644 index 00000000000..9d5f4c15eb9 --- /dev/null +++ b/db/migrate/20180718005113_add_instance_statistics_visibility_to_application_setting.rb @@ -0,0 +1,18 @@ +class AddInstanceStatisticsVisibilityToApplicationSetting < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default(:application_settings, :instance_statistics_visibility_private, + :boolean, + default: false, + allow_null: false) + end + + def down + remove_column(:application_settings, :instance_statistics_visibility_private) + end +end From b34b8c7760c68171228af5ef478caeae8794fe08 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Thu, 19 Jul 2018 11:12:06 +0100 Subject: [PATCH 07/27] add changelog entry --- ...1416-making-instance-wide-data-tools-more-accessible.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/41416-making-instance-wide-data-tools-more-accessible.yml diff --git a/changelogs/unreleased/41416-making-instance-wide-data-tools-more-accessible.yml b/changelogs/unreleased/41416-making-instance-wide-data-tools-more-accessible.yml new file mode 100644 index 00000000000..9976e8c815d --- /dev/null +++ b/changelogs/unreleased/41416-making-instance-wide-data-tools-more-accessible.yml @@ -0,0 +1,6 @@ +--- +title: Add instance_statistics namespace, containing convdev index and cohorts instance + stats +merge_request: 20679 +author: +type: changed From 4090362ed34b459cd8ace3db8b98bc01877fe07a Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Fri, 20 Jul 2018 15:46:14 +0100 Subject: [PATCH 08/27] schema.db update --- db/schema.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/schema.rb b/db/schema.rb index 8ae0197d1b4..fbacff19591 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -168,6 +168,7 @@ ActiveRecord::Schema.define(version: 20180722103201) do t.boolean "enforce_terms", default: false t.boolean "mirror_available", default: true, null: false t.boolean "hide_third_party_offers", default: false, null: false + t.boolean "instance_statistics_visibility_private", default: false, null: false end create_table "audit_events", force: :cascade do |t| From 626d5caf0fb4fe242c24dda294457479eebe6c16 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Mon, 23 Jul 2018 14:29:54 +0100 Subject: [PATCH 09/27] Scope instance_statistics to dash --- config/routes.rb | 3 ++- config/routes/instance_statistics.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 17b969f669d..63e40a31b76 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -70,6 +70,8 @@ Rails.application.routes.draw do get 'ide' => 'ide#index' get 'ide/*vueroute' => 'ide#index', format: false + + draw :instance_statistics end # Koding route @@ -110,7 +112,6 @@ Rails.application.routes.draw do draw :group draw :user draw :project - draw :instance_statistics root to: "root#index" diff --git a/config/routes/instance_statistics.rb b/config/routes/instance_statistics.rb index 365797cb688..725549beb7e 100644 --- a/config/routes/instance_statistics.rb +++ b/config/routes/instance_statistics.rb @@ -1,5 +1,5 @@ namespace :instance_statistics do - root to: redirect("instance_statistics/conversational_development_index") + root to: redirect('-/instance_statistics/conversational_development_index') resources :cohorts, only: :index resources :conversational_development_index, only: :index From 01fd71adbd2736dcf28e56eb54301b4a01c0f81b Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Mon, 23 Jul 2018 14:30:11 +0100 Subject: [PATCH 10/27] Add read_instance_statistics global policy --- .../instance_statistics/application_controller.rb | 6 +----- app/helpers/application_settings_helper.rb | 4 ---- app/policies/global_policy.rb | 3 +++ app/views/layouts/nav/_dashboard.html.haml | 2 +- 4 files changed, 5 insertions(+), 10 deletions(-) diff --git a/app/controllers/instance_statistics/application_controller.rb b/app/controllers/instance_statistics/application_controller.rb index 7c454e15a59..d5bef4b4316 100644 --- a/app/controllers/instance_statistics/application_controller.rb +++ b/app/controllers/instance_statistics/application_controller.rb @@ -2,11 +2,7 @@ class InstanceStatistics::ApplicationController < ApplicationController before_action :authenticate_user! layout 'instance_statistics' - def index - redirect_to instance_statistics_conversations_development_index_index_path - end - def authenticate_user! - render_404 if ApplicationSettingsHelper.hide_instance_statistics?(current_user) + render_404 unless can?(current_user, :read_instance_statistics) end end diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index b355008f087..9e2346177a4 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -15,10 +15,6 @@ module ApplicationSettingsHelper def allowed_protocols_present? Gitlab::CurrentSettings.enabled_git_access_protocol.present? end - - def hide_instance_statistics?(user = current_user) - Gitlab::CurrentSettings.instance_statistics_visibility_private? && !user.admin? - end def enabled_protocol case Gitlab::CurrentSettings.enabled_git_access_protocol diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index 6e3827736b2..bf146dc375b 100644 --- a/app/policies/global_policy.rb +++ b/app/policies/global_policy.rb @@ -19,6 +19,9 @@ class GlobalPolicy < BasePolicy @user&.required_terms_not_accepted? end + condition(:private_instance_statistics, score: 0) { Gitlab::CurrentSettings.instance_statistics_visibility_private? } + rule { admin | ~private_instance_statistics }.enable :read_instance_statistics + rule { anonymous }.policy do prevent :log_in prevent :receive_notifications diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index 356e52918d9..fb7ba1527e3 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -70,7 +70,7 @@ = nav_link(controller: 'admin/dashboard') do = link_to admin_root_path, class: 'admin-icon qa-admin-area-link', title: _('Admin area'), aria: { label: _("Admin area") }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do = sprite_icon('admin', size: 18) - - unless hide_instance_statistics? + - if can?(current_user, :read_instance_statistics) = nav_link(controller: :instance_statistics) do = link_to instance_statistics_root_path, title: 'Instance statistics', aria: { label: 'Instance statistics' }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do = sprite_icon('chart', size: 18) From 1387f9ddd2b4e1cea3c93cdf9534af180d3b6d1d Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Mon, 23 Jul 2018 14:35:24 +0100 Subject: [PATCH 11/27] Correct active page state for instance statistics --- app/views/layouts/nav/_dashboard.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/layouts/nav/_dashboard.html.haml b/app/views/layouts/nav/_dashboard.html.haml index fb7ba1527e3..5913c6d4ccd 100644 --- a/app/views/layouts/nav/_dashboard.html.haml +++ b/app/views/layouts/nav/_dashboard.html.haml @@ -71,7 +71,7 @@ = link_to admin_root_path, class: 'admin-icon qa-admin-area-link', title: _('Admin area'), aria: { label: _("Admin area") }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do = sprite_icon('admin', size: 18) - if can?(current_user, :read_instance_statistics) - = nav_link(controller: :instance_statistics) do + = nav_link(controller: [:conversational_development_index, :cohorts]) do = link_to instance_statistics_root_path, title: 'Instance statistics', aria: { label: 'Instance statistics' }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do = sprite_icon('chart', size: 18) - if Gitlab::Sherlock.enabled? From 3f0e6d92055e7e1923a51684a397a2a150475899 Mon Sep 17 00:00:00 2001 From: "Jacob Vosmaer (GitLab)" Date: Fri, 27 Jul 2018 08:43:19 +0000 Subject: [PATCH 12/27] More Gitaly cleanup: fetch_ref, allow disk access blocks --- lib/gitlab/git/conflict/resolver.rb | 2 +- lib/gitlab/git/repository.rb | 30 ++++--------------- lib/gitlab/git/repository_mirroring.rb | 4 ++- .../import_export/merge_request_parser.rb | 6 +++- .../project_tree_restorer_spec.rb | 2 +- spec/models/repository_spec.rb | 6 +--- 6 files changed, 17 insertions(+), 33 deletions(-) diff --git a/lib/gitlab/git/conflict/resolver.rb b/lib/gitlab/git/conflict/resolver.rb index 0e4a973301f..6dc792c16b8 100644 --- a/lib/gitlab/git/conflict/resolver.rb +++ b/lib/gitlab/git/conflict/resolver.rb @@ -17,7 +17,7 @@ module Gitlab end rescue GRPC::FailedPrecondition => e raise Gitlab::Git::Conflict::Resolver::ConflictSideMissing.new(e.message) - rescue Rugged::ReferenceError, Rugged::OdbError, GRPC::BadStatus => e + rescue GRPC::BadStatus => e raise Gitlab::Git::CommandError.new(e) end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 0356e8efc5c..eb02c7ac8e1 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -558,7 +558,9 @@ module Gitlab if is_enabled gitaly_operation_client.user_update_branch(branch_name, user, newrev, oldrev) else - OperationService.new(user, self).update_branch(branch_name, newrev, oldrev) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + OperationService.new(user, self).update_branch(branch_name, newrev, oldrev) + end end end end @@ -832,18 +834,9 @@ module Gitlab Gitlab::Git.check_namespace!(source_repository) source_repository = RemoteRepository.new(source_repository) unless source_repository.is_a?(RemoteRepository) - message, status = GitalyClient.migrate(:fetch_ref) do |is_enabled| - if is_enabled - gitaly_fetch_ref(source_repository, source_ref: source_ref, target_ref: target_ref) - else - # When removing this code, also remove source_repository#path - # to remove deprecated method calls - local_fetch_ref(source_repository.path, source_ref: source_ref, target_ref: target_ref) - end - end - - # Make sure ref was created, and raise Rugged::ReferenceError when not - raise Rugged::ReferenceError, message if status != 0 + args = %W(fetch --no-tags -f #{GITALY_INTERNAL_URL} #{source_ref}:#{target_ref}) + message, status = run_git(args, env: source_repository.fetch_env) + raise Gitlab::Git::CommandError, message if status != 0 target_ref end @@ -1244,17 +1237,6 @@ module Gitlab gitaly_repository_client.apply_gitattributes(revision) end - def local_fetch_ref(source_path, source_ref:, target_ref:) - args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref}) - run_git(args) - end - - def gitaly_fetch_ref(source_repository, source_ref:, target_ref:) - args = %W(fetch --no-tags -f #{GITALY_INTERNAL_URL} #{source_ref}:#{target_ref}) - - run_git(args, env: source_repository.fetch_env) - end - def gitaly_delete_refs(*ref_names) gitaly_ref_client.delete_refs(refs: ref_names) if ref_names.any? end diff --git a/lib/gitlab/git/repository_mirroring.rb b/lib/gitlab/git/repository_mirroring.rb index 8835bfb2481..65eb5cc18cf 100644 --- a/lib/gitlab/git/repository_mirroring.rb +++ b/lib/gitlab/git/repository_mirroring.rb @@ -6,7 +6,9 @@ module Gitlab if is_enabled gitaly_ref_client.remote_branches(remote_name) else - rugged_remote_branches(remote_name) + Gitlab::GitalyClient::StorageSettings.allow_disk_access do + rugged_remote_branches(remote_name) + end end end end diff --git a/lib/gitlab/import_export/merge_request_parser.rb b/lib/gitlab/import_export/merge_request_parser.rb index d0527f014a7..62a1833b39c 100644 --- a/lib/gitlab/import_export/merge_request_parser.rb +++ b/lib/gitlab/import_export/merge_request_parser.rb @@ -27,7 +27,11 @@ module Gitlab # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1295 def fetch_ref - @project.repository.fetch_ref(@project.repository, source_ref: @diff_head_sha, target_ref: @merge_request.source_branch) + target_ref = Gitlab::Git::BRANCH_REF_PREFIX + @merge_request.source_branch + + unless @project.repository.fetch_source_branch!(@project.repository, @diff_head_sha, target_ref) + Rails.logger.warn("Import/Export warning: Failed to create #{target_ref} for MR: #{@merge_request.iid}") + end end def branch_exists?(branch_name) 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 bac5693c830..a88ac0a091e 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -16,7 +16,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do @shared = @project.import_export_shared allow(@shared).to receive(:export_path).and_return('spec/lib/gitlab/import_export/') - allow_any_instance_of(Repository).to receive(:fetch_ref).and_return(true) + allow_any_instance_of(Repository).to receive(:fetch_source_branch!).and_return(true) allow_any_instance_of(Gitlab::Git::Repository).to receive(:branch_exists?).and_return(false) expect_any_instance_of(Gitlab::Git::Repository).to receive(:create_branch).with('feature', 'DCBA') diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 5d64602ca56..52ec8dbe25a 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1129,16 +1129,12 @@ describe Repository do end it 'raises Rugged::ReferenceError' do - raise_reference_error = raise_error(Rugged::ReferenceError) do |err| - expect(err.cause).to be_nil - end - expect do Gitlab::Git::OperationService.new(git_user, target_project.repository.raw_repository) .with_branch('feature', start_repository: project.repository.raw_repository, &:itself) - end.to raise_reference_error + end.to raise_error(Gitlab::Git::CommandError) end end From 321d3e048377fadd01c0655ea84f0f604b0abe9f Mon Sep 17 00:00:00 2001 From: Sam Beckham Date: Fri, 27 Jul 2018 09:27:20 +0000 Subject: [PATCH 13/27] Resolve "The comparison list for the modified icons" --- app/assets/javascripts/badges/components/badge.vue | 2 +- .../ide/components/commit_sidebar/stage_button.vue | 2 +- app/assets/stylesheets/snippets.scss | 4 ++-- app/helpers/snippets_helper.rb | 2 +- app/views/layouts/header/_default.html.haml | 2 +- app/views/layouts/nav/sidebar/_group.html.haml | 2 +- app/views/layouts/nav/sidebar/_profile.html.haml | 2 +- app/views/layouts/nav/sidebar/_project.html.haml | 4 ++-- app/views/shared/snippets/_embed.html.haml | 2 +- package.json | 2 +- spec/helpers/snippets_helper_spec.rb | 4 ++-- yarn.lock | 6 +++--- 12 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/assets/javascripts/badges/components/badge.vue b/app/assets/javascripts/badges/components/badge.vue index b4bfaee1d85..155c348286c 100644 --- a/app/assets/javascripts/badges/components/badge.vue +++ b/app/assets/javascripts/badges/components/badge.vue @@ -93,7 +93,7 @@ export default {