From 89ea6a9931bb232bd391ff4dace6e13deb6dee0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 17:06:52 +0100 Subject: Add Callout model --- app/models/callout.rb | 3 +++ app/models/user.rb | 1 + db/migrate/20180125214301_create_callouts.rb | 19 +++++++++++++++++++ 3 files changed, 23 insertions(+) create mode 100644 app/models/callout.rb create mode 100644 db/migrate/20180125214301_create_callouts.rb diff --git a/app/models/callout.rb b/app/models/callout.rb new file mode 100644 index 00000000000..b8131beb518 --- /dev/null +++ b/app/models/callout.rb @@ -0,0 +1,3 @@ +class Callout < ActiveRecord::Base + belongs_to :user +end diff --git a/app/models/user.rb b/app/models/user.rb index 9403da98268..b54d44fe80a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -137,6 +137,7 @@ class User < ActiveRecord::Base has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent has_many :custom_attributes, class_name: 'UserCustomAttribute' + has_many :callouts # # Validations diff --git a/db/migrate/20180125214301_create_callouts.rb b/db/migrate/20180125214301_create_callouts.rb new file mode 100644 index 00000000000..5dc9638a845 --- /dev/null +++ b/db/migrate/20180125214301_create_callouts.rb @@ -0,0 +1,19 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateCallouts < ActiveRecord::Migration + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + create_table :callouts do |t| + t.string :feature_name, null: false + t.boolean :dismissed_state, null: false + t.references :user, index: true, foreign_key: { on_delete: :cascade }, null: false + + t.timestamps_with_timezone null: false + end + + add_index :callouts, :feature_name, unique: true + end +end -- cgit v1.2.1 From 0dab083702ff4364bc25ea29812a4305c58e5a4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 17:07:35 +0100 Subject: Add Callout specs --- spec/factories/callouts.rb | 8 ++++++++ spec/models/callout_spec.rb | 9 +++++++++ 2 files changed, 17 insertions(+) create mode 100644 spec/factories/callouts.rb create mode 100644 spec/models/callout_spec.rb diff --git a/spec/factories/callouts.rb b/spec/factories/callouts.rb new file mode 100644 index 00000000000..b8ea879933e --- /dev/null +++ b/spec/factories/callouts.rb @@ -0,0 +1,8 @@ +FactoryBot.define do + factory :callout do + feature_name 'test_callout' + dismissed_state false + + user + end +end diff --git a/spec/models/callout_spec.rb b/spec/models/callout_spec.rb new file mode 100644 index 00000000000..8328ce06139 --- /dev/null +++ b/spec/models/callout_spec.rb @@ -0,0 +1,9 @@ +require 'rails_helper' + +describe Callout do + let(:callout) { create(:callout) } + + describe 'relationships' do + it { is_expected.to belong_to(:user) } + end +end -- cgit v1.2.1 From c4667f87037150a408da5c133e573714807e17b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 22:13:54 +0100 Subject: Implement Callouts controller --- app/controllers/callouts_controller.rb | 27 +++++++++++++++++++++++++++ config/routes.rb | 5 +++++ 2 files changed, 32 insertions(+) create mode 100644 app/controllers/callouts_controller.rb diff --git a/app/controllers/callouts_controller.rb b/app/controllers/callouts_controller.rb new file mode 100644 index 00000000000..a7bccfcce78 --- /dev/null +++ b/app/controllers/callouts_controller.rb @@ -0,0 +1,27 @@ +class CalloutsController < ApplicationController + before_action :callout, only: [:dismiss] + + def dismiss + respond_to do |format| + format.json do + if @callout + @callout.update(dismissed_state: true) + else + Callout.create(feature_name: callout_param, dismissed_state: true, user: current_user) + end + + head :ok + end + end + end + + private + + def callout + @callout = Callout.find_by(user: current_user, feature_name: callout_param) + end + + def callout_param + params.require(:feature_name) + end +end diff --git a/config/routes.rb b/config/routes.rb index f162043dd5e..f768bcebc7e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -90,6 +90,11 @@ Rails.application.routes.draw do # Notification settings resources :notification_settings, only: [:create, :update] + # Callouts + namespace :callouts do + post :dismiss + end + draw :google_api draw :import draw :uploads -- cgit v1.2.1 From bca10385539b44b39c28586d2d795e66a5e5e907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 22:49:20 +0100 Subject: Add CalloutsController specs --- spec/controllers/callouts_controller_spec.rb | 39 ++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 spec/controllers/callouts_controller_spec.rb diff --git a/spec/controllers/callouts_controller_spec.rb b/spec/controllers/callouts_controller_spec.rb new file mode 100644 index 00000000000..bf2aae190f2 --- /dev/null +++ b/spec/controllers/callouts_controller_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' + +describe CalloutsController do + let(:user) { create(:user) } + + before do + sign_in(user) + end + + describe "POST #dismiss" do + subject { post :dismiss, feature_name: 'feature_name', format: :json } + + context 'when callout entry does not exist' do + it 'should create a callout entry with dismissed state' do + expect { subject }.to change { Callout.count }.by(1) + end + + it 'should return success' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when callout entry already exists' do + let!(:callout) { create(:callout, feature_name: 'feature_name', user: user) } + + it 'should update it with a dismissed state' do + expect { subject }.to change { callout.reload.dismissed_state }.from(false).to(true) + end + + it 'should return success' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + end + end +end -- cgit v1.2.1 From 860c7c4bd41ab6353834e8e765f5cd86d93a41c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 26 Jan 2018 23:05:28 +0100 Subject: Update database schema --- db/schema.rb | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 4e82a688725..fc12e5752b3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180115201419) do +ActiveRecord::Schema.define(version: 20180125214301) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -203,6 +203,17 @@ ActiveRecord::Schema.define(version: 20180115201419) do add_index "broadcast_messages", ["starts_at", "ends_at", "id"], name: "index_broadcast_messages_on_starts_at_and_ends_at_and_id", using: :btree + create_table "callouts", force: :cascade do |t| + t.string "feature_name", null: false + t.boolean "dismissed_state", null: false + t.integer "user_id", null: false + t.datetime_with_timezone "created_at", null: false + t.datetime_with_timezone "updated_at", null: false + end + + add_index "callouts", ["feature_name"], name: "index_callouts_on_feature_name", unique: true, using: :btree + add_index "callouts", ["user_id"], name: "index_callouts_on_user_id", using: :btree + create_table "chat_names", force: :cascade do |t| t.integer "user_id", null: false t.integer "service_id", null: false @@ -1924,6 +1935,7 @@ ActiveRecord::Schema.define(version: 20180115201419) do add_index "web_hooks", ["type"], name: "index_web_hooks_on_type", using: :btree add_foreign_key "boards", "projects", name: "fk_f15266b5f9", on_delete: :cascade + add_foreign_key "callouts", "users", on_delete: :cascade add_foreign_key "chat_teams", "namespaces", on_delete: :cascade add_foreign_key "ci_build_trace_section_names", "projects", on_delete: :cascade add_foreign_key "ci_build_trace_sections", "ci_build_trace_section_names", column: "section_name_id", name: "fk_264e112c66", on_delete: :cascade -- cgit v1.2.1 From ec6c5833b62df7ff1fe69d4b874762713b4068a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 27 Jan 2018 17:32:10 +0100 Subject: Implement CalloutsHelper --- app/helpers/callouts_helper.rb | 11 +++++++++ spec/helpers/callouts_helper_spec.rb | 45 ++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 app/helpers/callouts_helper.rb create mode 100644 spec/helpers/callouts_helper_spec.rb diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb new file mode 100644 index 00000000000..e435879ee43 --- /dev/null +++ b/app/helpers/callouts_helper.rb @@ -0,0 +1,11 @@ +module CalloutsHelper + def show_gke_cluster_integration_callout?(kube_feature_name, project) + !user_dismissed?(kube_feature_name) && project.team.master?(current_user) + end + + private + + def user_dismissed?(feature_name) + Callout.find_by(user: current_user, feature_name: feature_name).dismissed_state? + end +end diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb new file mode 100644 index 00000000000..feabbf33ef6 --- /dev/null +++ b/spec/helpers/callouts_helper_spec.rb @@ -0,0 +1,45 @@ +require "spec_helper" + +describe CalloutsHelper do + let(:user) { create(:user) } + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + describe '.show_gke_cluster_integration_callout?' do + let(:project) { create(:project) } + + subject { helper.show_gke_cluster_integration_callout?('test_name', project) } + + context 'when user has not dismissed' do + before do + allow(helper).to receive(:user_dismissed?).and_return(false) + end + + context 'when user is master' do + before do + allow(project).to receive_message_chain(:team, :master?).and_return(true) + end + + it { is_expected.to be true } + end + + context 'when user is not master' do + before do + allow(project).to receive_message_chain(:team, :master?).and_return(false) + end + + it { is_expected.to be false } + end + end + + context 'when user dismissed' do + before do + allow(helper).to receive(:user_dismissed?).and_return(true) + end + + it { is_expected.to be false } + end + end +end -- cgit v1.2.1 From 8be4f3ecf480b64284bd0d2ed31f59f332df1bae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 27 Jan 2018 18:44:14 +0100 Subject: Move Callouts route to - path --- config/routes.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index f768bcebc7e..abd626119ca 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -60,6 +60,11 @@ Rails.application.routes.draw do resources :issues, module: :boards, only: [:index, :update] end + + # Callouts + namespace :callouts do + post :dismiss + end end # Koding route @@ -90,11 +95,6 @@ Rails.application.routes.draw do # Notification settings resources :notification_settings, only: [:create, :update] - # Callouts - namespace :callouts do - post :dismiss - end - draw :google_api draw :import draw :uploads -- cgit v1.2.1 From 977996d2b396c1892a0bcdb386d63e4f78675239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 00:05:48 +0100 Subject: Add check for guest user --- app/helpers/callouts_helper.rb | 2 +- spec/helpers/callouts_helper_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index e435879ee43..a27533f30c6 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -1,6 +1,6 @@ module CalloutsHelper def show_gke_cluster_integration_callout?(kube_feature_name, project) - !user_dismissed?(kube_feature_name) && project.team.master?(current_user) + current_user && !user_dismissed?(kube_feature_name) && project.team.master?(current_user) end private diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb index feabbf33ef6..f160aafbd7b 100644 --- a/spec/helpers/callouts_helper_spec.rb +++ b/spec/helpers/callouts_helper_spec.rb @@ -41,5 +41,13 @@ describe CalloutsHelper do it { is_expected.to be false } end + + context 'when the user is not logged in' do + before do + allow(helper).to receive(:current_user).and_return(nil) + end + + it { is_expected.to be_falsey } + end end end -- cgit v1.2.1 From 2cd71eb5f5fc4ae2b1e6385977ec7ef298dc77db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 00:08:27 +0100 Subject: Add safe navigation for users without callout state --- app/helpers/callouts_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index a27533f30c6..199652b1175 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -6,6 +6,6 @@ module CalloutsHelper private def user_dismissed?(feature_name) - Callout.find_by(user: current_user, feature_name: feature_name).dismissed_state? + Callout.find_by(user: current_user, feature_name: feature_name)&.dismissed_state? end end -- cgit v1.2.1 From f2253d48a109a12f00b87f5aaaa42299860315f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 15:17:15 +0100 Subject: Show GKE cluster callout for project owner as well --- app/helpers/callouts_helper.rb | 4 +++- spec/helpers/callouts_helper_spec.rb | 16 +++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index 199652b1175..e65daa572a8 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -1,6 +1,8 @@ module CalloutsHelper def show_gke_cluster_integration_callout?(kube_feature_name, project) - current_user && !user_dismissed?(kube_feature_name) && project.team.master?(current_user) + current_user && !user_dismissed?(kube_feature_name) && + (project.team.master?(current_user) || + current_user == project.owner) end private diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb index f160aafbd7b..8dd97e22477 100644 --- a/spec/helpers/callouts_helper_spec.rb +++ b/spec/helpers/callouts_helper_spec.rb @@ -26,11 +26,21 @@ describe CalloutsHelper do end context 'when user is not master' do - before do - allow(project).to receive_message_chain(:team, :master?).and_return(false) + context 'when the user is owner' do + before do + allow(project).to receive(:owner).and_return(user) + end + + it { is_expected.to be true } end - it { is_expected.to be false } + context 'when the user is not owner' do + before do + allow(project).to receive_message_chain(:team, :master?).and_return(false) + end + + it { is_expected.to be false } + end end end -- cgit v1.2.1 From 4157cad23b5215c5b878f120099b4632ed04ce04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 15:30:49 +0100 Subject: Extract feature name into constant --- app/helpers/callouts_helper.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index e65daa572a8..153797450de 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -1,6 +1,8 @@ module CalloutsHelper - def show_gke_cluster_integration_callout?(kube_feature_name, project) - current_user && !user_dismissed?(kube_feature_name) && + GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'.freeze + + def show_gke_cluster_integration_callout?(project) + current_user && !user_dismissed?(GKE_CLUSTER_INTEGRATION) && (project.team.master?(current_user) || current_user == project.owner) end -- cgit v1.2.1 From 4cfe66ae5f2c1f83944999c640c175085008105b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Tue, 30 Jan 2018 16:59:10 +0100 Subject: Fix CalloutsHelper spec subject --- spec/helpers/callouts_helper_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb index 8dd97e22477..7ec8ca8eb0c 100644 --- a/spec/helpers/callouts_helper_spec.rb +++ b/spec/helpers/callouts_helper_spec.rb @@ -10,7 +10,7 @@ describe CalloutsHelper do describe '.show_gke_cluster_integration_callout?' do let(:project) { create(:project) } - subject { helper.show_gke_cluster_integration_callout?('test_name', project) } + subject { helper.show_gke_cluster_integration_callout?(project) } context 'when user has not dismissed' do before do -- cgit v1.2.1 From a248e7adf3c11bf9e0231570163930c953a1a8ee Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 31 Jan 2018 13:41:14 +0100 Subject: Fix flay not detecting identical code Previously the script checked only for 'Similar code', but flay will tell users when IDENTICAL code is detected too. This should create a commit will create a failing pipeline, and to check that it does this commit is pushed. Fixes #42628 --- lib/tasks/flay.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tasks/flay.rake b/lib/tasks/flay.rake index b1e012e70c5..4b4881cecb8 100644 --- a/lib/tasks/flay.rake +++ b/lib/tasks/flay.rake @@ -2,7 +2,7 @@ desc 'Code duplication analyze via flay' task :flay do output = `bundle exec flay --mass 35 app/ lib/gitlab/ 2> #{File::NULL}` - if output.include? "Similar code found" + if output.include?("Similar code found") || output.include?("IDENTICAL code found") puts output exit 1 end -- cgit v1.2.1 From 830700e2a46daa8731101c44d1eba24ce7eb6877 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 31 Jan 2018 14:18:11 +0100 Subject: Upgrade Flay to have fewer parsing errors Gem is updated from 2.8 to 2.10. [changelog][1] [1]: https://github.com/seattlerb/flay/blob/master/History.rdoc --- Gemfile | 2 +- Gemfile.lock | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 05f72b6482f..56842d62d39 100644 --- a/Gemfile +++ b/Gemfile @@ -349,7 +349,7 @@ group :development, :test do gem 'scss_lint', '~> 0.56.0', require: false gem 'haml_lint', '~> 0.26.0', require: false gem 'simplecov', '~> 0.14.0', require: false - gem 'flay', '~> 2.8.0', require: false + gem 'flay', '~> 2.10.0', require: false gem 'bundler-audit', '~> 0.5.0', require: false gem 'benchmark-ips', '~> 2.3.0', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 1a3c8f42469..ed120720242 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -211,7 +211,7 @@ GEM fast_gettext (1.4.0) ffaker (2.4.0) ffi (1.9.18) - flay (2.8.1) + flay (2.10.0) erubis (~> 2.7.0) path_expander (~> 1.0) ruby_parser (~> 3.0) @@ -588,7 +588,7 @@ GEM ast (~> 2.3) parslet (1.5.0) blankslate (~> 2.0) - path_expander (1.0.1) + path_expander (1.0.2) peek (1.0.1) concurrent-ruby (>= 0.9.0) concurrent-ruby-ext (>= 0.9.0) @@ -1037,7 +1037,7 @@ DEPENDENCIES faraday (~> 0.12) fast_blank ffaker (~> 2.4) - flay (~> 2.8.0) + flay (~> 2.10.0) flipper (~> 0.11.0) flipper-active_record (~> 0.11.0) flipper-active_support_cache_store (~> 0.11.0) -- cgit v1.2.1 From 9318f4ab2b6396782edb277aeddf45720d70b7e6 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Wed, 31 Jan 2018 14:39:11 +0100 Subject: Ignore Flay failures --- .flayignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.flayignore b/.flayignore index acac0ce14c9..87cb3507b05 100644 --- a/.flayignore +++ b/.flayignore @@ -6,3 +6,5 @@ app/models/concerns/relative_positioning.rb app/workers/stuck_merge_jobs_worker.rb lib/gitlab/redis/*.rb lib/gitlab/gitaly_client/operation_service.rb +lib/gitlab/background_migration/* +app/models/project_services/kubernetes_service.rb -- cgit v1.2.1 From 0a47d1924d6b283a174672f33cf7a0de6b281fef Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 30 Jan 2018 09:59:45 +0100 Subject: Client changes for Tag,BranchNamesContainingCommit As part of gitlab-org/gitaly#884, this commit contains the client implementation for both TagNamesContaintingCommit and BranchNamesContainingCommit. The interface in the Repository model stays the same, but the implementation on the serverside, e.g. Gitaly, uses `for-each-ref`, as opposed to `branch` or `tag` which both aren't plumbing command. The result stays the same. On the serverside, we have the opportunity to limit the number of names to return. However, this is not supported on the front end yet. My proposal to use this ability: gitlab-org/gitlab-ce#42581. For now, this ability is not used as that would change more behaviours on a feature flag which might lead to unexpected changes on page refresh for example. --- app/models/repository.rb | 4 +-- lib/gitlab/git/branch.rb | 18 ++++++++----- lib/gitlab/git/repository.rb | 42 +++++++++++++++++++++--------- lib/gitlab/git/tag.rb | 8 ++++-- lib/gitlab/gitaly_client/ref_service.rb | 26 +++++++++++++++++++ spec/models/repository_spec.rb | 45 +++++++++++++++++++++++++-------- 6 files changed, 109 insertions(+), 34 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index edfb236a91a..7f443846a82 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -721,11 +721,11 @@ class Repository end def branch_names_contains(sha) - refs_contains_sha('branch', sha) + raw_repository.branch_names_contains_sha(sha) end def tag_names_contains(sha) - refs_contains_sha('tag', sha) + raw_repository.tag_names_contains_sha(sha) end def local_branches diff --git a/lib/gitlab/git/branch.rb b/lib/gitlab/git/branch.rb index 3487e099381..2c051bd33b9 100644 --- a/lib/gitlab/git/branch.rb +++ b/lib/gitlab/git/branch.rb @@ -1,13 +1,17 @@ -# Gitaly note: JV: no RPC's here. - module Gitlab module Git class Branch < Ref - def self.find(repo, branch_name) - if branch_name.is_a?(Gitlab::Git::Branch) - branch_name - else - repo.find_branch(branch_name) + class << self + def find(repo, branch_name) + if branch_name.is_a?(Gitlab::Git::Branch) + branch_name + else + repo.find_branch(branch_name) + end + end + + def names_contains_sha(repo, sha, limit: 0) + GitalyClient::RefService.new(repo).branch_names_contains_sha(sha) end end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 68b54d28876..76b94bf9030 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1355,20 +1355,23 @@ module Gitlab raise CommandError.new(e) end - def refs_contains_sha(ref_type, sha) - args = %W(#{ref_type} --contains #{sha}) - names = run_git(args).first - - if names.respond_to?(:split) - names = names.split("\n").map(&:strip) - - names.each do |name| - name.slice! '* ' + def branch_names_contains_sha(sha) + gitaly_migrate(:branch_names_contains_sha) do |is_enabled| + if is_enabled + Gitlab::Git::Branch.names_contains_sha(self, sha) + else + refs_contains_sha(:branch, sha) end + end + end - names - else - [] + def tag_names_contains_sha(sha) + gitaly_migrate(:tag_names_contains_sha) do |is_enabled| + if is_enabled + Gitlab::Git::Tag.names_contains_sha(self, sha) + else + refs_contains_sha(:tag, sha) + end end end @@ -1446,6 +1449,21 @@ module Gitlab end end + def refs_contains_sha(ref_type, sha) + args = %W(#{ref_type} --contains #{sha}) + names = run_git(args).first + + return [] unless names.respond_to?(:split) + + names = names.split("\n").map(&:strip) + + names.each do |name| + name.slice! '* ' + end + + names + end + def shell_write_ref(ref_path, ref, old_ref) raise ArgumentError, "invalid ref_path #{ref_path.inspect}" if ref_path.include?(' ') raise ArgumentError, "invalid ref #{ref.inspect}" if ref.include?("\x00") diff --git a/lib/gitlab/git/tag.rb b/lib/gitlab/git/tag.rb index bc4e160dce9..c163d383eb0 100644 --- a/lib/gitlab/git/tag.rb +++ b/lib/gitlab/git/tag.rb @@ -1,8 +1,12 @@ -# Gitaly note: JV: no RPC's here. -# module Gitlab module Git class Tag < Ref + class << self + def names_contains_sha(repo, sha) + GitalyClient::RefService.new(repo).branch_names_contains_sha(sha) + end + end + attr_reader :object_sha def initialize(repository, name, target, target_commit, message = nil) diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index 8b9a224b700..07122da4c2c 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -145,6 +145,32 @@ module Gitlab raise Gitlab::Git::Repository::GitError, response.git_error if response.git_error.present? end + # Limit: 0 implies no limit, thus all tag names will be returned + def tag_names_containing(sha, limit: 0) + request = Gitaly::ListTagNamesContainingCommitRequest.new( + repository: @gitaly_repo, + commit_id: sha, + limit: limit + ) + + stream = GitalyClient.call(@repository.storage, :ref_service, :list_tag_names_containing_commit, request) + + stream.each_with_object([]) { |response, array| array.concat(response.tag_names) } + end + + # Limit: 0 implies no limit, thus all tag names will be returned + def branch_names_contains_sha(sha, limit: 0) + request = Gitaly::ListBranchNamesContainingCommitRequest.new( + repository: @gitaly_repo, + commit_id: sha, + limit: limit + ) + + stream = GitalyClient.call(@repository.storage, :ref_service, :list_branch_names_containing_commit, request) + + stream.each_with_object([]) { |response, array| array.concat(response.branch_names) } + end + private def consume_refs_response(response) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 1102b1c9006..07d43acd338 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -36,26 +36,49 @@ describe Repository do end describe '#branch_names_contains' do - subject { repository.branch_names_contains(sample_commit.id) } + shared_examples '#branch_names_contains' do + set(:project) { create(:project, :repository) } + let(:repository) { project.repository } - it { is_expected.to include('master') } - it { is_expected.not_to include('feature') } - it { is_expected.not_to include('fix') } + subject { repository.branch_names_contains(sample_commit.id) } - describe 'when storage is broken', :broken_storage do - it 'should raise a storage error' do - expect_to_raise_storage_error do - broken_repository.branch_names_contains(sample_commit.id) + it { is_expected.to include('master') } + it { is_expected.not_to include('feature') } + it { is_expected.not_to include('fix') } + + describe 'when storage is broken', :broken_storage do + it 'should raise a storage error' do + expect_to_raise_storage_error do + broken_repository.branch_names_contains(sample_commit.id) + end end end end + + context 'when gitaly is enabled' do + it_behaves_like '#branch_names_contains' + end + + context 'when gitaly is disabled', :skip_gitaly_mock do + it_behaves_like '#branch_names_contains' + end end describe '#tag_names_contains' do - subject { repository.tag_names_contains(sample_commit.id) } + shared_examples '#tag_names_contains' do + subject { repository.tag_names_contains(sample_commit.id) } + + it { is_expected.to include('v1.1.0') } + it { is_expected.not_to include('v1.0.0') } + end + + context 'when gitaly is enabled' do + it_behaves_like '#tag_names_contains' + end - it { is_expected.to include('v1.1.0') } - it { is_expected.not_to include('v1.0.0') } + context 'when gitaly is enabled', :skip_gitaly_mock do + it_behaves_like '#tag_names_contains' + end end describe 'tags_sorted_by' do -- cgit v1.2.1 From 73bd48de977bea1c8cd8bb3d995b984b874d4eba Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 30 Jan 2018 10:56:24 +0100 Subject: Fix encoding issues when name is not UTF-8 --- lib/gitlab/gitaly_client/ref_service.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index 07122da4c2c..22fee1ff07f 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -155,7 +155,10 @@ module Gitlab stream = GitalyClient.call(@repository.storage, :ref_service, :list_tag_names_containing_commit, request) - stream.each_with_object([]) { |response, array| array.concat(response.tag_names) } + stream.each_with_object([]) do |response, array| + encoded_names = response.tag_names.map { |t| Gitlab::Git.ref_name(t) } + array.concat(encoded_names) + end end # Limit: 0 implies no limit, thus all tag names will be returned @@ -168,7 +171,10 @@ module Gitlab stream = GitalyClient.call(@repository.storage, :ref_service, :list_branch_names_containing_commit, request) - stream.each_with_object([]) { |response, array| array.concat(response.branch_names) } + stream.each_with_object([]) do |response, array| + encoded_names = response.branch_names.map { |b| Gitlab::Git.ref_name(b) } + array.concat(encoded_names) + end end private -- cgit v1.2.1 From fd46d6ceb81eb9039b4e60c1d158848dd22ba411 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 30 Jan 2018 11:22:08 +0100 Subject: Remove intermediate methods on Branch and Tag classes --- lib/gitlab/git/branch.rb | 16 +++++----------- lib/gitlab/git/repository.rb | 4 ++-- lib/gitlab/git/tag.rb | 6 ------ lib/gitlab/gitaly_client/ref_service.rb | 19 ++++++++++--------- 4 files changed, 17 insertions(+), 28 deletions(-) diff --git a/lib/gitlab/git/branch.rb b/lib/gitlab/git/branch.rb index 2c051bd33b9..ae7e88f0503 100644 --- a/lib/gitlab/git/branch.rb +++ b/lib/gitlab/git/branch.rb @@ -1,17 +1,11 @@ module Gitlab module Git class Branch < Ref - class << self - def find(repo, branch_name) - if branch_name.is_a?(Gitlab::Git::Branch) - branch_name - else - repo.find_branch(branch_name) - end - end - - def names_contains_sha(repo, sha, limit: 0) - GitalyClient::RefService.new(repo).branch_names_contains_sha(sha) + def self.find(repo, branch_name) + if branch_name.is_a?(Gitlab::Git::Branch) + branch_name + else + repo.find_branch(branch_name) end end diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 76b94bf9030..95644dacc4e 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1358,7 +1358,7 @@ module Gitlab def branch_names_contains_sha(sha) gitaly_migrate(:branch_names_contains_sha) do |is_enabled| if is_enabled - Gitlab::Git::Branch.names_contains_sha(self, sha) + gitaly_ref_client.branch_names_contains_sha(sha) else refs_contains_sha(:branch, sha) end @@ -1368,7 +1368,7 @@ module Gitlab def tag_names_contains_sha(sha) gitaly_migrate(:tag_names_contains_sha) do |is_enabled| if is_enabled - Gitlab::Git::Tag.names_contains_sha(self, sha) + gitaly_ref_client.tag_names_contains_sha(sha) else refs_contains_sha(:tag, sha) end diff --git a/lib/gitlab/git/tag.rb b/lib/gitlab/git/tag.rb index c163d383eb0..8a8f7b051ed 100644 --- a/lib/gitlab/git/tag.rb +++ b/lib/gitlab/git/tag.rb @@ -1,12 +1,6 @@ module Gitlab module Git class Tag < Ref - class << self - def names_contains_sha(repo, sha) - GitalyClient::RefService.new(repo).branch_names_contains_sha(sha) - end - end - attr_reader :object_sha def initialize(repository, name, target, target_commit, message = nil) diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index 22fee1ff07f..ba6b577fd17 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -146,7 +146,7 @@ module Gitlab end # Limit: 0 implies no limit, thus all tag names will be returned - def tag_names_containing(sha, limit: 0) + def tag_names_contains_sha(sha, limit: 0) request = Gitaly::ListTagNamesContainingCommitRequest.new( repository: @gitaly_repo, commit_id: sha, @@ -155,10 +155,7 @@ module Gitlab stream = GitalyClient.call(@repository.storage, :ref_service, :list_tag_names_containing_commit, request) - stream.each_with_object([]) do |response, array| - encoded_names = response.tag_names.map { |t| Gitlab::Git.ref_name(t) } - array.concat(encoded_names) - end + consume_ref_contains_sha_response(stream, :tag_names) end # Limit: 0 implies no limit, thus all tag names will be returned @@ -171,10 +168,7 @@ module Gitlab stream = GitalyClient.call(@repository.storage, :ref_service, :list_branch_names_containing_commit, request) - stream.each_with_object([]) do |response, array| - encoded_names = response.branch_names.map { |b| Gitlab::Git.ref_name(b) } - array.concat(encoded_names) - end + consume_ref_contains_sha_response(stream, :branch_names) end private @@ -247,6 +241,13 @@ module Gitlab Gitlab::Git::Commit.decorate(@repository, hash) end + def consume_ref_contains_sha_response(stream, collection_name) + stream.each_with_object([]) do |response, array| + encoded_names = response.send(collection_name).map { |b| Gitlab::Git.ref_name(b) } # rubocop:disable GitlabSecurity/PublicSend + array.concat(encoded_names) + end + end + def invalid_ref!(message) raise Gitlab::Git::Repository::InvalidRef.new(message) end -- cgit v1.2.1 From cca61980d5ad9c4db65b9498fe49d936657bc0e2 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 15 Jan 2018 16:21:04 +0100 Subject: Track and act upon the number of executed queries This ensures that we have more visibility in the number of SQL queries that are executed in web requests. The current threshold is hardcoded to 100 as we will rarely (maybe once or twice) change it. In production and development we use Sentry if enabled, in the test environment we raise an error. This feature is also only enabled in production/staging when running on GitLab.com as it's not very useful to other users. --- app/controllers/admin/services_controller.rb | 5 + app/controllers/boards/issues_controller.rb | 6 + .../import/gitlab_projects_controller.rb | 5 + app/controllers/projects/commits_controller.rb | 5 + .../projects/cycle_analytics_controller.rb | 5 + app/controllers/projects/forks_controller.rb | 5 + app/controllers/projects/issues_controller.rb | 10 ++ .../merge_requests/creations_controller.rb | 5 + .../projects/merge_requests_controller.rb | 6 + app/controllers/projects/network_controller.rb | 5 + app/controllers/projects/notes_controller.rb | 5 + app/controllers/projects/pipelines_controller.rb | 6 + app/controllers/projects_controller.rb | 5 + app/controllers/registrations_controller.rb | 6 + changelogs/unreleased/query-counts.yml | 5 + config/initializers/query_limiting.rb | 9 ++ doc/development/README.md | 1 + doc/development/query_count_limits.md | 65 ++++++++++ lib/api/branches.rb | 2 + lib/api/issues.rb | 6 + lib/api/merge_requests.rb | 6 + lib/api/pipelines.rb | 2 + lib/api/projects.rb | 2 + lib/api/triggers.rb | 2 + lib/api/users.rb | 2 + lib/api/v3/branches.rb | 2 + lib/api/v3/issues.rb | 6 + lib/api/v3/merge_requests.rb | 4 + lib/api/v3/pipelines.rb | 2 + lib/api/v3/triggers.rb | 2 + lib/gitlab/query_limiting.rb | 36 ++++++ .../query_limiting/active_support_subscriber.rb | 11 ++ lib/gitlab/query_limiting/middleware.rb | 55 ++++++++ lib/gitlab/query_limiting/transaction.rb | 83 ++++++++++++ .../active_support_subscriber_spec.rb | 19 +++ spec/lib/gitlab/query_limiting/middleware_spec.rb | 72 +++++++++++ spec/lib/gitlab/query_limiting/transaction_spec.rb | 144 +++++++++++++++++++++ spec/lib/gitlab/query_limiting_spec.rb | 65 ++++++++++ 38 files changed, 682 insertions(+) create mode 100644 changelogs/unreleased/query-counts.yml create mode 100644 config/initializers/query_limiting.rb create mode 100644 doc/development/query_count_limits.md create mode 100644 lib/gitlab/query_limiting.rb create mode 100644 lib/gitlab/query_limiting/active_support_subscriber.rb create mode 100644 lib/gitlab/query_limiting/middleware.rb create mode 100644 lib/gitlab/query_limiting/transaction.rb create mode 100644 spec/lib/gitlab/query_limiting/active_support_subscriber_spec.rb create mode 100644 spec/lib/gitlab/query_limiting/middleware_spec.rb create mode 100644 spec/lib/gitlab/query_limiting/transaction_spec.rb create mode 100644 spec/lib/gitlab/query_limiting_spec.rb diff --git a/app/controllers/admin/services_controller.rb b/app/controllers/admin/services_controller.rb index 4c3d336b3af..a7025b62ad7 100644 --- a/app/controllers/admin/services_controller.rb +++ b/app/controllers/admin/services_controller.rb @@ -1,6 +1,7 @@ class Admin::ServicesController < Admin::ApplicationController include ServiceParams + before_action :whitelist_query_limiting, only: [:index] before_action :service, only: [:edit, :update] def index @@ -37,4 +38,8 @@ class Admin::ServicesController < Admin::ApplicationController def service @service ||= Service.where(id: params[:id], template: true).first end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42430') + end end diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb index f8049b20b9f..ee23ee0bcc3 100644 --- a/app/controllers/boards/issues_controller.rb +++ b/app/controllers/boards/issues_controller.rb @@ -2,6 +2,7 @@ module Boards class IssuesController < Boards::ApplicationController include BoardsResponses + before_action :whitelist_query_limiting, only: [:index, :update] before_action :authorize_read_issue, only: [:index] before_action :authorize_create_issue, only: [:create] before_action :authorize_update_issue, only: [:update] @@ -92,5 +93,10 @@ module Boards } ) end + + def whitelist_query_limiting + # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42439 + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42428') + end end end diff --git a/app/controllers/import/gitlab_projects_controller.rb b/app/controllers/import/gitlab_projects_controller.rb index 567957ba2cb..f22df992fe9 100644 --- a/app/controllers/import/gitlab_projects_controller.rb +++ b/app/controllers/import/gitlab_projects_controller.rb @@ -1,4 +1,5 @@ class Import::GitlabProjectsController < Import::BaseController + before_action :whitelist_query_limiting, only: [:create] before_action :verify_gitlab_project_import_enabled def new @@ -40,4 +41,8 @@ class Import::GitlabProjectsController < Import::BaseController :path, :namespace_id, :file ) end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42437') + end end diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 0a40c67368f..1d910e461b1 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -4,6 +4,7 @@ class Projects::CommitsController < Projects::ApplicationController include ExtractsPath include RendersCommits + before_action :whitelist_query_limiting before_action :require_non_empty_project before_action :assign_ref_vars before_action :authorize_download_code! @@ -65,4 +66,8 @@ class Projects::CommitsController < Projects::ApplicationController @commits = @commits.with_pipeline_status @commits = prepare_commits_for_rendering(@commits) end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42330') + end end diff --git a/app/controllers/projects/cycle_analytics_controller.rb b/app/controllers/projects/cycle_analytics_controller.rb index 88ac3ad046b..d1b8fd80c4e 100644 --- a/app/controllers/projects/cycle_analytics_controller.rb +++ b/app/controllers/projects/cycle_analytics_controller.rb @@ -3,6 +3,7 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController include ActionView::Helpers::TextHelper include CycleAnalyticsParams + before_action :whitelist_query_limiting, only: [:show] before_action :authorize_read_cycle_analytics! def show @@ -31,4 +32,8 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController permissions: @cycle_analytics.permissions(user: current_user) } end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42671') + end end diff --git a/app/controllers/projects/forks_controller.rb b/app/controllers/projects/forks_controller.rb index 68978f8fdd1..f43bba18d81 100644 --- a/app/controllers/projects/forks_controller.rb +++ b/app/controllers/projects/forks_controller.rb @@ -2,6 +2,7 @@ class Projects::ForksController < Projects::ApplicationController include ContinueParams # Authorize + before_action :whitelist_query_limiting, only: [:create] before_action :require_non_empty_project before_action :authorize_download_code! before_action :authenticate_user!, only: [:new, :create] @@ -54,4 +55,8 @@ class Projects::ForksController < Projects::ApplicationController render :error end end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42335') + end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 384f18b316c..515cb08f1fc 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -8,6 +8,7 @@ class Projects::IssuesController < Projects::ApplicationController prepend_before_action :authenticate_user!, only: [:new] + before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update] before_action :check_issues_available! before_action :issue, except: [:index, :new, :create, :bulk_update] before_action :set_issuables_index, only: [:index] @@ -247,4 +248,13 @@ class Projects::IssuesController < Projects::ApplicationController @finder_type = IssuesFinder super end + + def whitelist_query_limiting + # Also see the following issues: + # + # 1. https://gitlab.com/gitlab-org/gitlab-ce/issues/42423 + # 2. https://gitlab.com/gitlab-org/gitlab-ce/issues/42424 + # 3. https://gitlab.com/gitlab-org/gitlab-ce/issues/42426 + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42422') + end end diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 0df80fa700f..a5a2d54ba82 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -4,6 +4,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap include RendersCommits skip_before_action :merge_request + before_action :whitelist_query_limiting, only: [:create] before_action :authorize_create_merge_request! before_action :apply_diff_view_cookie!, only: [:diffs, :diff_for_path] before_action :build_merge_request, except: [:create] @@ -125,4 +126,8 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap @project.forked_from_project end end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42384') + end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2e8a738b6d9..8af4e379f0a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -7,6 +7,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo include IssuableCollections skip_before_action :merge_request, only: [:index, :bulk_update] + before_action :whitelist_query_limiting, only: [:assign_related_issues, :update] before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] before_action :set_issuables_index, only: [:index] before_action :authenticate_user!, only: [:assign_related_issues] @@ -339,4 +340,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo access_denied! unless access_check end + + def whitelist_query_limiting + # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42441 + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42438') + end end diff --git a/app/controllers/projects/network_controller.rb b/app/controllers/projects/network_controller.rb index fb68dd771a1..3b10a93e97f 100644 --- a/app/controllers/projects/network_controller.rb +++ b/app/controllers/projects/network_controller.rb @@ -2,6 +2,7 @@ class Projects::NetworkController < Projects::ApplicationController include ExtractsPath include ApplicationHelper + before_action :whitelist_query_limiting before_action :require_non_empty_project before_action :assign_ref_vars before_action :authorize_download_code! @@ -35,4 +36,8 @@ class Projects::NetworkController < Projects::ApplicationController @options[:extended_sha1] = params[:extended_sha1] @commit = @repo.commit(@options[:extended_sha1]) end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42333') + end end diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 5940fae8dd0..4f8978c93c3 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -2,6 +2,7 @@ class Projects::NotesController < Projects::ApplicationController include NotesActions include ToggleAwardEmoji + before_action :whitelist_query_limiting, only: [:create] before_action :authorize_read_note! before_action :authorize_create_note!, only: [:create] before_action :authorize_resolve_note!, only: [:resolve, :unresolve] @@ -79,4 +80,8 @@ class Projects::NotesController < Projects::ApplicationController access_denied! unless can?(current_user, :create_note, noteable) end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42383') + end end diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index e146d0d3cd5..78d109cf33e 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -1,4 +1,5 @@ class Projects::PipelinesController < Projects::ApplicationController + before_action :whitelist_query_limiting, only: [:create, :retry] before_action :pipeline, except: [:index, :new, :create, :charts] before_action :commit, only: [:show, :builds, :failures] before_action :authorize_read_pipeline! @@ -166,4 +167,9 @@ class Projects::PipelinesController < Projects::ApplicationController def commit @commit ||= @pipeline.commit end + + def whitelist_query_limiting + # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42343 + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42339') + end end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8158934322d..ea13a72e151 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -3,6 +3,7 @@ class ProjectsController < Projects::ApplicationController include ExtractsPath include PreviewMarkdown + before_action :whitelist_query_limiting, only: [:create] before_action :authenticate_user!, except: [:index, :show, :activity, :refs] before_action :redirect_git_extension, only: [:show] before_action :project, except: [:index, :new, :create] @@ -405,4 +406,8 @@ class ProjectsController < Projects::ApplicationController # redirect_to request.original_url.sub(%r{\.git/?\Z}, '') if params[:format] == 'git' end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42440') + end end diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index d9142311b6f..1848c806c41 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -1,6 +1,8 @@ class RegistrationsController < Devise::RegistrationsController include Recaptcha::Verify + before_action :whitelist_query_limiting, only: [:destroy] + def new redirect_to(new_user_session_path) end @@ -83,4 +85,8 @@ class RegistrationsController < Devise::RegistrationsController def devise_mapping @devise_mapping ||= Devise.mappings[:user] end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42380') + end end diff --git a/changelogs/unreleased/query-counts.yml b/changelogs/unreleased/query-counts.yml new file mode 100644 index 00000000000..e01ff8a4ad8 --- /dev/null +++ b/changelogs/unreleased/query-counts.yml @@ -0,0 +1,5 @@ +--- +title: Track and act upon the number of executed queries +merge_request: +author: +type: added diff --git a/config/initializers/query_limiting.rb b/config/initializers/query_limiting.rb new file mode 100644 index 00000000000..66864d1898e --- /dev/null +++ b/config/initializers/query_limiting.rb @@ -0,0 +1,9 @@ +if Gitlab::QueryLimiting.enable? + require_dependency 'gitlab/query_limiting/active_support_subscriber' + require_dependency 'gitlab/query_limiting/transaction' + require_dependency 'gitlab/query_limiting/middleware' + + Gitlab::Application.configure do |config| + config.middleware.use(Gitlab::QueryLimiting::Middleware) + end +end diff --git a/doc/development/README.md b/doc/development/README.md index 12cca9f84b7..45e9565f9a7 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -75,6 +75,7 @@ comments: false - [Ordering table columns](ordering_table_columns.md) - [Verifying database capabilities](verifying_database_capabilities.md) - [Database Debugging and Troubleshooting](database_debugging.md) +- [Query Count Limits](query_count_limits.md) ## Testing guides diff --git a/doc/development/query_count_limits.md b/doc/development/query_count_limits.md new file mode 100644 index 00000000000..ebb6e0c2dac --- /dev/null +++ b/doc/development/query_count_limits.md @@ -0,0 +1,65 @@ +# Query Count Limits + +Each controller or API endpoint is allowed to execute up to 100 SQL queries. In +a production environment we'll only log an error in case this threshold is +exceeded, but in a test environment we'll raise an error instead. + +## Solving Failing Tests + +When a test fails because it executes more than 100 SQL queries there are two +solutions to this problem: + +1. Reduce the number of SQL queries that are executed. +2. Whitelist the controller or API endpoint. + +You should only resort to whitelisting when an existing controller or endpoint +is to blame as in this case reducing the number of SQL queries can take a lot of +effort. Newly added controllers and endpoints are not allowed to execute more +than 100 SQL queries and no exceptions will be made for this rule. _If_ a large +number of SQL queries is necessary to perform certain work it's best to have +this work performed by Sidekiq instead of doing this directly in a web request. + +## Whitelisting + +In the event that you _have_ to whitelist a controller you'll first need to +create an issue. This issue should (preferably in the title) mention the +controller or endpoint and include the appropriate labels (`database`, +`performance`, and at least a team specific label such as `Discussion`). + +Once the issue has been created you can whitelist the code in question. For +Rails controllers it's best to create a `before_action` hook that runs as early +as possible. The called method in turn should call +`Gitlab::QueryLimiting.whitelist('issue URL here')`. For example: + +```ruby +class MyController < ApplicationController + before_action :whitelist_query_limiting, only: [:show] + + def index + # ... + end + + def show + # ... + end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/...') + end +end +``` + +By using a `before_action` you don't have to modify the controller method in +question, reducing the likelihood of merge conflicts. + +For Grape API endpoints there unfortunately is not a reliable way of running a +hook before a specific endpoint. This means that you have to add the whitelist +call directly into the endpoint like so: + +```ruby +get '/projects/:id/foo' do + Gitlab::QueryLimiting.whitelist('...') + + # ... +end +``` diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 0791a110c39..1794207e29b 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -29,6 +29,8 @@ module API use :pagination end get ':id/repository/branches' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42329') + repository = user_project.repository branches = ::Kaminari.paginate_array(repository.branches.sort_by(&:name)) merged_branch_names = repository.merged_branch_names(branches.map(&:name)) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index c99fe3ab5b3..b6c278c89d0 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -161,6 +161,8 @@ module API use :issue_params end post ':id/issues' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42320') + authorize! :create_issue, user_project # Setting created_at time only allowed for admins and project owners @@ -201,6 +203,8 @@ module API :labels, :created_at, :due_date, :confidential, :state_event end put ':id/issues/:issue_iid' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42322') + issue = user_project.issues.find_by!(iid: params.delete(:issue_iid)) authorize! :update_issue, issue @@ -234,6 +238,8 @@ module API requires :to_project_id, type: Integer, desc: 'The ID of the new project' end post ':id/issues/:issue_iid/move' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42323') + issue = user_project.issues.find_by(iid: params[:issue_iid]) not_found!('Issue') unless issue diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 420aaf1c964..719afa09295 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -152,6 +152,8 @@ module API use :optional_params end post ":id/merge_requests" do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42316') + authorize! :create_merge_request, user_project mr_params = declared_params(include_missing: false) @@ -256,6 +258,8 @@ module API at_least_one_of(*at_least_one_of_ce) end put ':id/merge_requests/:merge_request_iid' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42318') + merge_request = find_merge_request_with_access(params.delete(:merge_request_iid), :update_merge_request) mr_params = declared_params(include_missing: false) @@ -283,6 +287,8 @@ module API optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch' end put ':id/merge_requests/:merge_request_iid/merge' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42317') + merge_request = find_project_merge_request(params[:merge_request_iid]) merge_when_pipeline_succeeds = to_boolean(params[:merge_when_pipeline_succeeds]) diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 675c963bae2..d2b8b832e4e 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -42,6 +42,8 @@ module API requires :ref, type: String, desc: 'Reference' end post ':id/pipeline' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42124') + authorize! :create_pipeline, user_project new_pipeline = Ci::CreatePipelineService.new(user_project, diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 8b5e4f8edcc..5b481121a10 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -210,6 +210,8 @@ module API optional :namespace, type: String, desc: 'The ID or name of the namespace that the project will be forked into' end post ':id/fork' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42284') + fork_params = declared_params(include_missing: false) namespace_id = fork_params[:namespace] diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index dd6801664b1..b3709455bc3 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -15,6 +15,8 @@ module API optional :variables, type: Hash, desc: 'The list of variables to be injected into build' end post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42283') + # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } diff --git a/lib/api/users.rb b/lib/api/users.rb index e5de31ad51b..c7c2aa280d5 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -383,6 +383,8 @@ module API optional :hard_delete, type: Boolean, desc: "Whether to remove a user's contributions" end delete ":id" do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42279') + authenticated_as_admin! user = User.find_by(id: params[:id]) diff --git a/lib/api/v3/branches.rb b/lib/api/v3/branches.rb index b201bf77667..25176c5b38e 100644 --- a/lib/api/v3/branches.rb +++ b/lib/api/v3/branches.rb @@ -14,6 +14,8 @@ module API success ::API::Entities::Branch end get ":id/repository/branches" do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42276') + repository = user_project.repository branches = repository.branches.sort_by(&:name) merged_branch_names = repository.merged_branch_names(branches.map(&:name)) diff --git a/lib/api/v3/issues.rb b/lib/api/v3/issues.rb index cb371fdbab8..b59947d81d9 100644 --- a/lib/api/v3/issues.rb +++ b/lib/api/v3/issues.rb @@ -134,6 +134,8 @@ module API use :issue_params end post ':id/issues' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42131') + # Setting created_at time only allowed for admins and project owners unless current_user.admin? || user_project.owner == current_user params.delete(:created_at) @@ -169,6 +171,8 @@ module API :labels, :created_at, :due_date, :confidential, :state_event end put ':id/issues/:issue_id' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42132') + issue = user_project.issues.find(params.delete(:issue_id)) authorize! :update_issue, issue @@ -201,6 +205,8 @@ module API requires :to_project_id, type: Integer, desc: 'The ID of the new project' end post ':id/issues/:issue_id/move' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42133') + issue = user_project.issues.find_by(id: params[:issue_id]) not_found!('Issue') unless issue diff --git a/lib/api/v3/merge_requests.rb b/lib/api/v3/merge_requests.rb index 0a24fea52a3..ce216497996 100644 --- a/lib/api/v3/merge_requests.rb +++ b/lib/api/v3/merge_requests.rb @@ -91,6 +91,8 @@ module API use :optional_params end post ":id/merge_requests" do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42126') + authorize! :create_merge_request, user_project mr_params = declared_params(include_missing: false) @@ -167,6 +169,8 @@ module API :remove_source_branch end put path do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42127') + merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request) mr_params = declared_params(include_missing: false) diff --git a/lib/api/v3/pipelines.rb b/lib/api/v3/pipelines.rb index c48cbd2b765..6d31c12f572 100644 --- a/lib/api/v3/pipelines.rb +++ b/lib/api/v3/pipelines.rb @@ -19,6 +19,8 @@ module API desc: 'Either running, branches, or tags' end get ':id/pipelines' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42123') + authorize! :read_pipeline, user_project pipelines = PipelinesFinder.new(user_project, scope: params[:scope]).execute diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index 534911fde5c..34f07dfb486 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -16,6 +16,8 @@ module API optional :variables, type: Hash, desc: 'The list of variables to be injected into build' end post ":id/(ref/:ref/)trigger/builds", requirements: { ref: /.+/ } do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42121') + # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } diff --git a/lib/gitlab/query_limiting.rb b/lib/gitlab/query_limiting.rb new file mode 100644 index 00000000000..f64f1757144 --- /dev/null +++ b/lib/gitlab/query_limiting.rb @@ -0,0 +1,36 @@ +module Gitlab + module QueryLimiting + # Returns true if we should enable tracking of query counts. + # + # This is only enabled in production/staging if we're running on GitLab.com. + # This ensures we don't produce any errors that users can't do anything + # about themselves. + def self.enable? + Gitlab.com? || Rails.env.development? || Rails.env.test? + end + + # Allows the current request to execute any number of SQL queries. + # + # This method should _only_ be used when there's a corresponding issue to + # reduce the number of queries. + # + # The issue URL is only meant to push developers into creating an issue + # instead of blindly whitelisting offending blocks of code. + def self.whitelist(issue_url) + return unless enable_whitelist? + + unless issue_url.start_with?('https://') + raise( + ArgumentError, + 'You must provide a valid issue URL in order to whitelist a block of code' + ) + end + + Transaction&.current&.whitelisted = true + end + + def self.enable_whitelist? + Rails.env.development? || Rails.env.test? + end + end +end diff --git a/lib/gitlab/query_limiting/active_support_subscriber.rb b/lib/gitlab/query_limiting/active_support_subscriber.rb new file mode 100644 index 00000000000..66049c94ec6 --- /dev/null +++ b/lib/gitlab/query_limiting/active_support_subscriber.rb @@ -0,0 +1,11 @@ +module Gitlab + module QueryLimiting + class ActiveSupportSubscriber < ActiveSupport::Subscriber + attach_to :active_record + + def sql(*) + Transaction.current&.increment + end + end + end +end diff --git a/lib/gitlab/query_limiting/middleware.rb b/lib/gitlab/query_limiting/middleware.rb new file mode 100644 index 00000000000..949ae79a047 --- /dev/null +++ b/lib/gitlab/query_limiting/middleware.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Gitlab + module QueryLimiting + # Middleware for reporting (or raising) when a request performs more than a + # certain amount of database queries. + class Middleware + CONTROLLER_KEY = 'action_controller.instance'.freeze + ENDPOINT_KEY = 'api.endpoint'.freeze + + def initialize(app) + @app = app + end + + def call(env) + transaction, retval = Transaction.run do + @app.call(env) + end + + transaction.action = action_name(env) + transaction.act_upon_results + + retval + end + + def action_name(env) + if env[CONTROLLER_KEY] + action_for_rails(env) + elsif env[ENDPOINT_KEY] + action_for_grape(env) + end + end + + private + + def action_for_rails(env) + controller = env[CONTROLLER_KEY] + action = "#{controller.class.name}##{controller.action_name}" + + if controller.content_type == 'text/html' + action + else + "#{action} (#{controller.content_type})" + end + end + + def action_for_grape(env) + endpoint = env[ENDPOINT_KEY] + route = endpoint.route rescue nil + + "#{route.request_method} #{route.path}" if route + end + end + end +end diff --git a/lib/gitlab/query_limiting/transaction.rb b/lib/gitlab/query_limiting/transaction.rb new file mode 100644 index 00000000000..3cbafadb0d0 --- /dev/null +++ b/lib/gitlab/query_limiting/transaction.rb @@ -0,0 +1,83 @@ +module Gitlab + module QueryLimiting + class Transaction + THREAD_KEY = :__gitlab_query_counts_transaction + + attr_accessor :count, :whitelisted + + # The name of the action (e.g. `UsersController#show`) that is being + # executed. + attr_accessor :action + + # The maximum number of SQL queries that can be executed in a request. For + # the sake of keeping things simple we hardcode this value here, it's not + # supposed to be changed very often anyway. + THRESHOLD = 100 + + # Error that is raised whenever exceeding the maximum number of queries. + ThresholdExceededError = Class.new(StandardError) + + def self.current + Thread.current[THREAD_KEY] + end + + # Starts a new transaction and returns it and the blocks' return value. + # + # Example: + # + # transaction, retval = Transaction.run do + # 10 + # end + # + # retval # => 10 + def self.run + transaction = new + Thread.current[THREAD_KEY] = transaction + + [transaction, yield] + ensure + Thread.current[THREAD_KEY] = nil + end + + def initialize + @action = nil + @count = 0 + @whitelisted = false + end + + # Sends a notification based on the number of executed SQL queries. + def act_upon_results + return unless threshold_exceeded? + + error = ThresholdExceededError.new(error_message) + + if raise_error? + raise(error) + else + # Raven automatically logs to the Rails log if disabled, thus we don't + # need to manually log anything in case Sentry support is not enabled. + Raven.capture_exception(error) + end + end + + def increment + @count += 1 unless whitelisted + end + + def raise_error? + Rails.env.test? + end + + def threshold_exceeded? + count > THRESHOLD + end + + def error_message + header = 'Too many SQL queries were executed' + header += " in #{action}" if action + + "#{header}: a maximum of #{THRESHOLD} is allowed but #{count} SQL queries were executed" + end + end + end +end diff --git a/spec/lib/gitlab/query_limiting/active_support_subscriber_spec.rb b/spec/lib/gitlab/query_limiting/active_support_subscriber_spec.rb new file mode 100644 index 00000000000..b49bc5c328c --- /dev/null +++ b/spec/lib/gitlab/query_limiting/active_support_subscriber_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe Gitlab::QueryLimiting::ActiveSupportSubscriber do + describe '#sql' do + it 'increments the number of executed SQL queries' do + transaction = double(:transaction) + + allow(Gitlab::QueryLimiting::Transaction) + .to receive(:current) + .and_return(transaction) + + expect(transaction) + .to receive(:increment) + .at_least(:once) + + User.count + end + end +end diff --git a/spec/lib/gitlab/query_limiting/middleware_spec.rb b/spec/lib/gitlab/query_limiting/middleware_spec.rb new file mode 100644 index 00000000000..a04bcdecb4b --- /dev/null +++ b/spec/lib/gitlab/query_limiting/middleware_spec.rb @@ -0,0 +1,72 @@ +require 'spec_helper' + +describe Gitlab::QueryLimiting::Middleware do + describe '#call' do + it 'runs the application with query limiting in place' do + middleware = described_class.new(-> (env) { env }) + + expect_any_instance_of(Gitlab::QueryLimiting::Transaction) + .to receive(:act_upon_results) + + expect(middleware.call({ number: 10 })) + .to eq({ number: 10 }) + end + end + + describe '#action_name' do + let(:middleware) { described_class.new(-> (env) { env }) } + + context 'using a Rails request' do + it 'returns the name of the controller and action' do + env = { + described_class::CONTROLLER_KEY => double( + :controller, + action_name: 'show', + class: double(:class, name: 'UsersController'), + content_type: 'text/html' + ) + } + + expect(middleware.action_name(env)).to eq('UsersController#show') + end + + it 'includes the content type if this is not text/html' do + env = { + described_class::CONTROLLER_KEY => double( + :controller, + action_name: 'show', + class: double(:class, name: 'UsersController'), + content_type: 'application/json' + ) + } + + expect(middleware.action_name(env)) + .to eq('UsersController#show (application/json)') + end + end + + context 'using a Grape API request' do + it 'returns the name of the request method and endpoint path' do + env = { + described_class::ENDPOINT_KEY => double( + :endpoint, + route: double(:route, request_method: 'GET', path: '/foo') + ) + } + + expect(middleware.action_name(env)).to eq('GET /foo') + end + + it 'returns nil if the route can not be retrieved' do + endpoint = double(:endpoint) + env = { described_class::ENDPOINT_KEY => endpoint } + + allow(endpoint) + .to receive(:route) + .and_raise(RuntimeError) + + expect(middleware.action_name(env)).to be_nil + end + end + end +end diff --git a/spec/lib/gitlab/query_limiting/transaction_spec.rb b/spec/lib/gitlab/query_limiting/transaction_spec.rb new file mode 100644 index 00000000000..b4231fcd0fa --- /dev/null +++ b/spec/lib/gitlab/query_limiting/transaction_spec.rb @@ -0,0 +1,144 @@ +require 'spec_helper' + +describe Gitlab::QueryLimiting::Transaction do + after do + Thread.current[described_class::THREAD_KEY] = nil + end + + describe '.current' do + it 'returns nil when there is no transaction' do + expect(described_class.current).to be_nil + end + + it 'returns the transaction when present' do + Thread.current[described_class::THREAD_KEY] = described_class.new + + expect(described_class.current).to be_an_instance_of(described_class) + end + end + + describe '.run' do + it 'runs a transaction and returns it and its return value' do + trans, ret = described_class.run do + 10 + end + + expect(trans).to be_an_instance_of(described_class) + expect(ret).to eq(10) + end + + it 'removes the transaction from the current thread upon completion' do + described_class.run do + 10 + end + + expect(Thread.current[described_class::THREAD_KEY]).to be_nil + end + end + + describe '#act_upon_results' do + context 'when the query threshold is not exceeded' do + it 'does nothing' do + trans = described_class.new + + expect(trans).not_to receive(:raise) + + trans.act_upon_results + end + end + + context 'when the query threshold is exceeded' do + let(:transaction) do + trans = described_class.new + trans.count = described_class::THRESHOLD + 1 + + trans + end + + it 'raises an error when this is enabled' do + expect { transaction.act_upon_results } + .to raise_error(described_class::ThresholdExceededError) + end + + it 'reports the error in Sentry if raising an error is disabled' do + expect(transaction) + .to receive(:raise_error?) + .and_return(false) + + expect(Raven) + .to receive(:capture_exception) + .with(an_instance_of(described_class::ThresholdExceededError)) + + transaction.act_upon_results + end + end + end + + describe '#increment' do + it 'increments the number of executed queries' do + transaction = described_class.new + + expect(transaction.count).to be_zero + + transaction.increment + + expect(transaction.count).to eq(1) + end + end + + describe '#raise_error?' do + it 'returns true in a test environment' do + transaction = described_class.new + + expect(transaction.raise_error?).to eq(true) + end + + it 'returns false in a production environment' do + transaction = described_class.new + + expect(Rails.env) + .to receive(:test?) + .and_return(false) + + expect(transaction.raise_error?).to eq(false) + end + end + + describe '#threshold_exceeded?' do + it 'returns false when the threshold is not exceeded' do + transaction = described_class.new + + expect(transaction.threshold_exceeded?).to eq(false) + end + + it 'returns true when the threshold is exceeded' do + transaction = described_class.new + transaction.count = described_class::THRESHOLD + 1 + + expect(transaction.threshold_exceeded?).to eq(true) + end + end + + describe '#error_message' do + it 'returns the error message to display when the threshold is exceeded' do + transaction = described_class.new + transaction.count = max = described_class::THRESHOLD + + expect(transaction.error_message).to eq( + "Too many SQL queries were executed: a maximum of #{max} " \ + "is allowed but #{max} SQL queries were executed" + ) + end + + it 'includes the action name in the error message when present' do + transaction = described_class.new + transaction.count = max = described_class::THRESHOLD + transaction.action = 'UsersController#show' + + expect(transaction.error_message).to eq( + "Too many SQL queries were executed in UsersController#show: " \ + "a maximum of #{max} is allowed but #{max} SQL queries were executed" + ) + end + end +end diff --git a/spec/lib/gitlab/query_limiting_spec.rb b/spec/lib/gitlab/query_limiting_spec.rb new file mode 100644 index 00000000000..2eddab0b8c3 --- /dev/null +++ b/spec/lib/gitlab/query_limiting_spec.rb @@ -0,0 +1,65 @@ +require 'spec_helper' + +describe Gitlab::QueryLimiting do + describe '.enable?' do + it 'returns true in a test environment' do + expect(described_class.enable?).to eq(true) + end + + it 'returns true in a development environment' do + allow(Rails.env).to receive(:development?).and_return(true) + + expect(described_class.enable?).to eq(true) + end + + it 'returns true on GitLab.com' do + allow(Gitlab).to receive(:com?).and_return(true) + + expect(described_class.enable?).to eq(true) + end + + it 'returns true in a non GitLab.com' do + expect(Gitlab).to receive(:com?).and_return(false) + expect(Rails.env).to receive(:development?).and_return(false) + expect(Rails.env).to receive(:test?).and_return(false) + + expect(described_class.enable?).to eq(false) + end + end + + describe '.whitelist' do + it 'raises ArgumentError when an invalid issue URL is given' do + expect { described_class.whitelist('foo') } + .to raise_error(ArgumentError) + end + + context 'without a transaction' do + it 'does nothing' do + expect { described_class.whitelist('https://example.com') } + .not_to raise_error + end + end + + context 'with a transaction' do + let(:transaction) { Gitlab::QueryLimiting::Transaction.new } + + before do + allow(Gitlab::QueryLimiting::Transaction) + .to receive(:current) + .and_return(transaction) + end + + it 'does not increment the number of SQL queries executed in the block' do + before = transaction.count + + described_class.whitelist('https://example.com') + + 2.times do + User.count + end + + expect(transaction.count).to eq(before) + end + end + end +end -- cgit v1.2.1 From 39b765bdadc7987ec2962f2c63982662653fafa7 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Wed, 31 Jan 2018 15:09:58 +0000 Subject: Change SAST confidence level in .gitlab-ci.yml Signed-off-by: Dmitriy Zaporozhets --- .gitlab-ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index be18520b876..e0f7615704b 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -627,6 +627,8 @@ codequality: sast: <<: *except-docs image: registry.gitlab.com/gitlab-org/gl-sast:latest + variables: + CONFIDENCE_LEVEL: 2 before_script: [] script: - /app/bin/run . -- cgit v1.2.1 From 4ff0cfe541739ead870fc9f7708db9b19e5b718c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 1 Feb 2018 21:02:07 +0100 Subject: Add callout priority enum --- app/helpers/callouts_helper.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index 153797450de..f93df287512 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -1,6 +1,11 @@ module CalloutsHelper GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'.freeze + # Higher value = higher priority + PRIORITY = { + GKE_CLUSTER_INTEGRATION: 0 + }.freeze + def show_gke_cluster_integration_callout?(project) current_user && !user_dismissed?(GKE_CLUSTER_INTEGRATION) && (project.team.master?(current_user) || -- cgit v1.2.1 From c8502e37b226927b5e5ce8488c532300f3690283 Mon Sep 17 00:00:00 2001 From: bikebilly Date: Thu, 1 Feb 2018 23:55:50 +0100 Subject: Change "Set up CI" to "Set up CI/CD" --- app/views/projects/show.html.haml | 2 +- doc/topics/autodevops/index.md | 2 +- doc/user/project/pages/getting_started_part_two.md | 8 ++++---- doc/user/project/repository/web_editor.md | 2 +- locale/gitlab.pot | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index d3e867e124c..1181fda2ee0 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -65,7 +65,7 @@ - unless @repository.gitlab_ci_yml %li.missing = link_to add_special_file_path(@project, file_name: '.gitlab-ci.yml') do - #{ _('Set up CI') } + #{ _('Set up CI/CD') } - if koding_enabled? && @repository.koding_yml.blank? %li.missing = link_to _('Set up Koding'), add_koding_stack_path(@project) diff --git a/doc/topics/autodevops/index.md b/doc/topics/autodevops/index.md index 144cd4c26b0..0fab752afad 100644 --- a/doc/topics/autodevops/index.md +++ b/doc/topics/autodevops/index.md @@ -395,7 +395,7 @@ If you want to modify the CI/CD pipeline used by Auto DevOps, you can copy the Assuming that your project is new or it doesn't have a `.gitlab-ci.yml` file present: -1. From your project home page, either click on the "Set up CI" button, or click +1. From your project home page, either click on the "Set up CI/CD" button, or click on the plus button and (`+`), then "New file" 1. Pick `.gitlab-ci.yml` as the template type 1. Select "Auto-DevOps" from the template dropdown diff --git a/doc/user/project/pages/getting_started_part_two.md b/doc/user/project/pages/getting_started_part_two.md index 64de0463dad..4a724dd5c1b 100644 --- a/doc/user/project/pages/getting_started_part_two.md +++ b/doc/user/project/pages/getting_started_part_two.md @@ -77,7 +77,7 @@ is useful for submitting merge requests to the upstream. > > 2. Why do I need to enable Shared Runners? > -> Shared Runners will run the script set by your GitLab CI +> Shared Runners will run the script set by your GitLab CI/CD configuration file. They're enabled by default to new projects, but not to forks. @@ -88,9 +88,9 @@ click **New project**, and name it considering the [practical examples](getting_started_part_one.md#practical-examples). 1. Clone it to your local computer, add your website files to your project, add, commit and push to GitLab. -1. From the your **Project**'s page, click **Set up CI**: +1. From the your **Project**'s page, click **Set up CI/CD**: - ![setup GitLab CI](img/setup_ci.png) + ![setup GitLab CI/CD](img/setup_ci.png) 1. Choose one of the templates from the dropbox menu. Pick up the template corresponding to the SSG you're using (or plain HTML). @@ -98,7 +98,7 @@ Pick up the template corresponding to the SSG you're using (or plain HTML). ![gitlab-ci templates](img/choose_ci_template.png) Once you have both site files and `.gitlab-ci.yml` in your project's -root, GitLab CI will build your site and deploy it with Pages. +root, GitLab CI/CD will build your site and deploy it with Pages. Once the first build passes, you see your site is live by navigating to your **Project**'s **Settings** > **Pages**, where you'll find its default URL. diff --git a/doc/user/project/repository/web_editor.md b/doc/user/project/repository/web_editor.md index db0c3ed9d59..33c9a1a4d6b 100644 --- a/doc/user/project/repository/web_editor.md +++ b/doc/user/project/repository/web_editor.md @@ -45,7 +45,7 @@ has already been created, which creates a link to the license itself. ![New file button](img/web_editor_template_dropdown_buttons.png) >**Note:** -The **Set up CI** button will not appear on an empty repository. You have to at +The **Set up CI/CD** button will not appear on an empty repository. You have to at least add a file in order for the button to show up. ## Upload a file diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 94458d60e01..63eff40ac0d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -2087,7 +2087,7 @@ msgstr "" msgid "Set a password on your account to pull or push via %{protocol}." msgstr "" -msgid "Set up CI" +msgid "Set up CI/CD" msgstr "" msgid "Set up Koding" -- cgit v1.2.1 From ad987b0d4a24e1e814f7322048912018bf139e89 Mon Sep 17 00:00:00 2001 From: bikebilly Date: Fri, 2 Feb 2018 00:05:44 +0100 Subject: Add changelog --- changelogs/unreleased/42684-set-up-ci-set-up-ci-cd.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/42684-set-up-ci-set-up-ci-cd.yml diff --git a/changelogs/unreleased/42684-set-up-ci-set-up-ci-cd.yml b/changelogs/unreleased/42684-set-up-ci-set-up-ci-cd.yml new file mode 100644 index 00000000000..0ef28e2ee01 --- /dev/null +++ b/changelogs/unreleased/42684-set-up-ci-set-up-ci-cd.yml @@ -0,0 +1,5 @@ +--- +title: Rename button to enable CI/CD configuration to "Set up CI/CD" +merge_request: 16870 +author: +type: changed -- cgit v1.2.1 From 6e0efbb096eb7042ad14fa2263c036009b80a678 Mon Sep 17 00:00:00 2001 From: bikebilly Date: Fri, 2 Feb 2018 00:14:19 +0100 Subject: Change "CI configuration" in "CI/CD configuration" --- app/views/projects/show.html.haml | 2 +- locale/gitlab.pot | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index 1181fda2ee0..888d820b04e 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -47,7 +47,7 @@ - if @repository.gitlab_ci_yml %li - = link_to _('CI configuration'), ci_configuration_path(@project) + = link_to _('CI/CD configuration'), ci_configuration_path(@project) - if current_user && can_push_branch?(@project, @project.default_branch) - unless @repository.changelog diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 63eff40ac0d..075103eb0de 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -363,7 +363,7 @@ msgstr "" msgid "CI / CD" msgstr "" -msgid "CI configuration" +msgid "CI/CD configuration" msgstr "" msgid "CICD|Jobs" -- cgit v1.2.1 From 5ff88153f4b826c93819b99bb639e0d19c44187e Mon Sep 17 00:00:00 2001 From: Connor Shea Date: Thu, 1 Feb 2018 23:36:24 +0000 Subject: Update CONTRIBUTING.md with a link to the Contributing page on about.gitlab.com --- CONTRIBUTING.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b366ae6f069..ed56da0353d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,6 +50,9 @@ _This notice should stay as the first item in the CONTRIBUTING.md file._ ## Contribute to GitLab +For a first-time step-by-step guide to the contribution process, see +["Contributing to GitLab"](https://about.gitlab.com/contributing/). + Thank you for your interest in contributing to GitLab. This guide details how to contribute to GitLab in a way that is efficient for everyone. -- cgit v1.2.1 From 1fb4ed21553c3741dd96894d2489628713bf50b0 Mon Sep 17 00:00:00 2001 From: Jeff Stubler Date: Mon, 20 Mar 2017 08:34:47 -0500 Subject: Change coverage badge rounding for other CI system consistency --- .../unreleased/25327-coverage-badge-rounding.yml | 5 +++++ lib/gitlab/badge/coverage/report.rb | 2 +- lib/gitlab/badge/coverage/template.rb | 4 ++-- spec/features/projects/badges/coverage_spec.rb | 4 ++-- spec/lib/gitlab/badge/coverage/template_spec.rb | 18 ++++++++++++++---- 5 files changed, 24 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/25327-coverage-badge-rounding.yml diff --git a/changelogs/unreleased/25327-coverage-badge-rounding.yml b/changelogs/unreleased/25327-coverage-badge-rounding.yml new file mode 100644 index 00000000000..ea985689484 --- /dev/null +++ b/changelogs/unreleased/25327-coverage-badge-rounding.yml @@ -0,0 +1,5 @@ +--- +title: Show coverage to two decimal points in coverage badge +merge_request: 10083 +author: Jeff Stubler +type: changed diff --git a/lib/gitlab/badge/coverage/report.rb b/lib/gitlab/badge/coverage/report.rb index 9a0482306b7..778d78185ff 100644 --- a/lib/gitlab/badge/coverage/report.rb +++ b/lib/gitlab/badge/coverage/report.rb @@ -23,7 +23,7 @@ module Gitlab @coverage ||= raw_coverage return unless @coverage - @coverage.to_i + @coverage.to_f.round(2) end def metadata diff --git a/lib/gitlab/badge/coverage/template.rb b/lib/gitlab/badge/coverage/template.rb index fcecb1d9665..afbf9dd17e3 100644 --- a/lib/gitlab/badge/coverage/template.rb +++ b/lib/gitlab/badge/coverage/template.rb @@ -25,7 +25,7 @@ module Gitlab end def value_text - @status ? "#{@status}%" : 'unknown' + @status ? ("%.2f%%" % @status) : 'unknown' end def key_width @@ -33,7 +33,7 @@ module Gitlab end def value_width - @status ? 36 : 58 + @status ? 54 : 58 end def value_color diff --git a/spec/features/projects/badges/coverage_spec.rb b/spec/features/projects/badges/coverage_spec.rb index 821ce88a402..f51001edcd7 100644 --- a/spec/features/projects/badges/coverage_spec.rb +++ b/spec/features/projects/badges/coverage_spec.rb @@ -18,7 +18,7 @@ feature 'test coverage badge' do show_test_coverage_badge - expect_coverage_badge('95%') + expect_coverage_badge('95.00%') end scenario 'user requests coverage badge for specific job' do @@ -30,7 +30,7 @@ feature 'test coverage badge' do show_test_coverage_badge(job: 'coverage') - expect_coverage_badge('85%') + expect_coverage_badge('85.00%') end scenario 'user requests coverage badge for pipeline without coverage' do diff --git a/spec/lib/gitlab/badge/coverage/template_spec.rb b/spec/lib/gitlab/badge/coverage/template_spec.rb index 383bae6e087..d9c21a22590 100644 --- a/spec/lib/gitlab/badge/coverage/template_spec.rb +++ b/spec/lib/gitlab/badge/coverage/template_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::Badge::Coverage::Template do - let(:badge) { double(entity: 'coverage', status: 90) } + let(:badge) { double(entity: 'coverage', status: 90.00) } let(:template) { described_class.new(badge) } describe '#key_text' do @@ -13,7 +13,17 @@ describe Gitlab::Badge::Coverage::Template do describe '#value_text' do context 'when coverage is known' do it 'returns coverage percentage' do - expect(template.value_text).to eq '90%' + expect(template.value_text).to eq '90.00%' + end + end + + context 'when coverage is known to many digits' do + before do + allow(badge).to receive(:status).and_return(92.349) + end + + it 'returns rounded coverage percentage' do + expect(template.value_text).to eq '92.35%' end end @@ -37,7 +47,7 @@ describe Gitlab::Badge::Coverage::Template do describe '#value_width' do context 'when coverage is known' do it 'is narrower when coverage is known' do - expect(template.value_width).to eq 36 + expect(template.value_width).to eq 54 end end @@ -113,7 +123,7 @@ describe Gitlab::Badge::Coverage::Template do describe '#width' do context 'when coverage is known' do it 'returns the key width plus value width' do - expect(template.width).to eq 98 + expect(template.width).to eq 116 end end -- cgit v1.2.1 From ad38e120069748049786373af1c2286cf9c849eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 2 Feb 2018 03:07:33 +0100 Subject: Remove dismissed_state from Callout model --- app/controllers/callouts_controller.rb | 20 ++++++-------------- app/helpers/callouts_helper.rb | 2 +- app/models/callout.rb | 3 +++ db/migrate/20180125214301_create_callouts.rb | 3 +-- db/schema.rb | 3 +-- spec/controllers/callouts_controller_spec.rb | 4 ---- spec/factories/callouts.rb | 1 - spec/models/callout_spec.rb | 9 ++++++++- 8 files changed, 20 insertions(+), 25 deletions(-) diff --git a/app/controllers/callouts_controller.rb b/app/controllers/callouts_controller.rb index a7bccfcce78..e24172edf5d 100644 --- a/app/controllers/callouts_controller.rb +++ b/app/controllers/callouts_controller.rb @@ -1,24 +1,16 @@ class CalloutsController < ApplicationController - before_action :callout, only: [:dismiss] - def dismiss - respond_to do |format| - format.json do - if @callout - @callout.update(dismissed_state: true) - else - Callout.create(feature_name: callout_param, dismissed_state: true, user: current_user) - end - - head :ok - end + if ensure_callout + respond_to { |format| format.json { head :ok } } + else + respond_to { |format| format.json { head :bad_request } } end end private - def callout - @callout = Callout.find_by(user: current_user, feature_name: callout_param) + def ensure_callout + current_user.callouts.find_or_create_by(feature_name: callout_param) end def callout_param diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index f93df287512..3841d288f52 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -15,6 +15,6 @@ module CalloutsHelper private def user_dismissed?(feature_name) - Callout.find_by(user: current_user, feature_name: feature_name)&.dismissed_state? + current_user&.callouts&.find_by(feature_name: feature_name) end end diff --git a/app/models/callout.rb b/app/models/callout.rb index b8131beb518..c8f41a39cf3 100644 --- a/app/models/callout.rb +++ b/app/models/callout.rb @@ -1,3 +1,6 @@ class Callout < ActiveRecord::Base belongs_to :user + + validates :user, presence: true + validates :feature_name, presence: true, uniqueness: { scope: :user_id } end diff --git a/db/migrate/20180125214301_create_callouts.rb b/db/migrate/20180125214301_create_callouts.rb index 5dc9638a845..b6749b4cfe6 100644 --- a/db/migrate/20180125214301_create_callouts.rb +++ b/db/migrate/20180125214301_create_callouts.rb @@ -8,12 +8,11 @@ class CreateCallouts < ActiveRecord::Migration def change create_table :callouts do |t| t.string :feature_name, null: false - t.boolean :dismissed_state, null: false t.references :user, index: true, foreign_key: { on_delete: :cascade }, null: false t.timestamps_with_timezone null: false end - add_index :callouts, :feature_name, unique: true + add_index :callouts, [:user_id, :feature_name], unique: true end end diff --git a/db/schema.rb b/db/schema.rb index fc12e5752b3..1b837db4e8d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -205,13 +205,12 @@ ActiveRecord::Schema.define(version: 20180125214301) do create_table "callouts", force: :cascade do |t| t.string "feature_name", null: false - t.boolean "dismissed_state", null: false t.integer "user_id", null: false t.datetime_with_timezone "created_at", null: false t.datetime_with_timezone "updated_at", null: false end - add_index "callouts", ["feature_name"], name: "index_callouts_on_feature_name", unique: true, using: :btree + add_index "callouts", ["user_id", "feature_name"], name: "index_callouts_on_user_id_and_feature_name", unique: true, using: :btree add_index "callouts", ["user_id"], name: "index_callouts_on_user_id", using: :btree create_table "chat_names", force: :cascade do |t| diff --git a/spec/controllers/callouts_controller_spec.rb b/spec/controllers/callouts_controller_spec.rb index bf2aae190f2..7a183d757a1 100644 --- a/spec/controllers/callouts_controller_spec.rb +++ b/spec/controllers/callouts_controller_spec.rb @@ -25,10 +25,6 @@ describe CalloutsController do context 'when callout entry already exists' do let!(:callout) { create(:callout, feature_name: 'feature_name', user: user) } - it 'should update it with a dismissed state' do - expect { subject }.to change { callout.reload.dismissed_state }.from(false).to(true) - end - it 'should return success' do subject diff --git a/spec/factories/callouts.rb b/spec/factories/callouts.rb index b8ea879933e..f28df49b00a 100644 --- a/spec/factories/callouts.rb +++ b/spec/factories/callouts.rb @@ -1,7 +1,6 @@ FactoryBot.define do factory :callout do feature_name 'test_callout' - dismissed_state false user end diff --git a/spec/models/callout_spec.rb b/spec/models/callout_spec.rb index 8328ce06139..29299678ebc 100644 --- a/spec/models/callout_spec.rb +++ b/spec/models/callout_spec.rb @@ -1,9 +1,16 @@ require 'rails_helper' describe Callout do - let(:callout) { create(:callout) } + let!(:callout) { create(:callout) } describe 'relationships' do it { is_expected.to belong_to(:user) } end + + describe 'validations' do + it { is_expected.to validate_presence_of(:user) } + + it { is_expected.to validate_presence_of(:feature_name) } + it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id) } + end end -- cgit v1.2.1 From 8f942a0534384f73cfa8686a0c67ec6451c5c39b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 2 Feb 2018 03:39:07 +0100 Subject: Use policies instead of role checks in ClustersHelper --- app/helpers/callouts_helper.rb | 3 +-- spec/helpers/callouts_helper_spec.rb | 24 ++++++++---------------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb index 3841d288f52..168b731ae89 100644 --- a/app/helpers/callouts_helper.rb +++ b/app/helpers/callouts_helper.rb @@ -8,8 +8,7 @@ module CalloutsHelper def show_gke_cluster_integration_callout?(project) current_user && !user_dismissed?(GKE_CLUSTER_INTEGRATION) && - (project.team.master?(current_user) || - current_user == project.owner) + can?(current_user, :create_cluster, project) end private diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb index 7ec8ca8eb0c..d0a5addc231 100644 --- a/spec/helpers/callouts_helper_spec.rb +++ b/spec/helpers/callouts_helper_spec.rb @@ -17,30 +17,22 @@ describe CalloutsHelper do allow(helper).to receive(:user_dismissed?).and_return(false) end - context 'when user is master' do + context 'when user can create a cluster' do before do - allow(project).to receive_message_chain(:team, :master?).and_return(true) + allow(helper).to receive(:can?).with(anything, :create_cluster, anything) + .and_return(true) end it { is_expected.to be true } end - context 'when user is not master' do - context 'when the user is owner' do - before do - allow(project).to receive(:owner).and_return(user) - end - - it { is_expected.to be true } + context 'when user can not create a cluster' do + before do + allow(helper).to receive(:can?).with(anything, :create_cluster, anything) + .and_return(false) end - context 'when the user is not owner' do - before do - allow(project).to receive_message_chain(:team, :master?).and_return(false) - end - - it { is_expected.to be false } - end + it { is_expected.to be false } end end -- cgit v1.2.1 From 74ddc80590053b04b90c35ae3e1f46bfbd9d0d15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Mon, 29 Jan 2018 16:06:17 -0500 Subject: add the uploader context to the upload model --- app/controllers/concerns/uploads_actions.rb | 2 +- app/models/upload.rb | 18 +++++++++++--- app/uploaders/file_mover.rb | 4 ++-- app/uploaders/file_uploader.rb | 28 ++++++++++++++++------ app/uploaders/gitlab_uploader.rb | 4 ++++ app/uploaders/records_uploads.rb | 13 +++++----- .../20180129193323_add_uploads_builder_context.rb | 14 +++++++++++ lib/gitlab/gfm/uploads_rewriter.rb | 2 +- spec/factories/uploads.rb | 20 +++++++++------- spec/models/upload_spec.rb | 6 +++++ spec/uploaders/file_uploader_spec.rb | 2 +- spec/uploaders/gitlab_uploader_spec.rb | 2 +- 12 files changed, 85 insertions(+), 30 deletions(-) create mode 100644 db/migrate/20180129193323_add_uploads_builder_context.rb diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 61554029d09..7ad79a1e56c 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -70,7 +70,7 @@ module UploadsActions end def build_uploader_from_params - uploader = uploader_class.new(model, params[:secret]) + uploader = uploader_class.new(model, secret: params[:secret]) uploader.retrieve_from_store!(params[:filename]) uploader end diff --git a/app/models/upload.rb b/app/models/upload.rb index fb55fd8007b..28baee95091 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -30,7 +30,8 @@ class Upload < ActiveRecord::Base end def build_uploader - uploader_class.new(model).tap do |uploader| + uploader_class.new(model, mount_point, **uploader_context) + .tap do |uploader| uploader.upload = self uploader.retrieve_from_store!(identifier) end @@ -40,6 +41,13 @@ class Upload < ActiveRecord::Base File.exist?(absolute_path) end + def uploader_context + { + identifier: identifier, + secret: secret + }.compact + end + private def checksummable? @@ -62,11 +70,15 @@ class Upload < ActiveRecord::Base !path.start_with?('/') end + def uploader_class + Object.const_get(uploader) + end + def identifier File.basename(path) end - def uploader_class - Object.const_get(uploader) + def mount_point + super&.to_sym end end diff --git a/app/uploaders/file_mover.rb b/app/uploaders/file_mover.rb index e7af1483d23..8f56f09c9f7 100644 --- a/app/uploaders/file_mover.rb +++ b/app/uploaders/file_mover.rb @@ -49,11 +49,11 @@ class FileMover end def uploader - @uploader ||= PersonalFileUploader.new(model, secret) + @uploader ||= PersonalFileUploader.new(model, secret: secret) end def temp_file_uploader - @temp_file_uploader ||= PersonalFileUploader.new(nil, secret) + @temp_file_uploader ||= PersonalFileUploader.new(nil, secret: secret) end def revert diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 85ae9863b13..cc4e7a38165 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -62,9 +62,11 @@ class FileUploader < GitlabUploader attr_accessor :model - def initialize(model, secret = nil) + def initialize(model, mounted_as = nil, **uploader_context) + super(model, nil, **uploader_context) + @model = model - @secret = secret + apply_context!(uploader_context) end def base_dir @@ -107,12 +109,12 @@ class FileUploader < GitlabUploader self.file.filename end - # the upload does not hold the secret, but holds the path - # which contains the secret: extract it def upload=(value) - if matches = DYNAMIC_PATH_PATTERN.match(value.path) - @secret = matches[:secret] - @identifier = matches[:identifier] + unless apply_context!(value.uploader_context) + if matches = DYNAMIC_PATH_PATTERN.match(value.path) + @secret = matches[:secret] + @identifier = matches[:identifier] + end end super @@ -124,6 +126,18 @@ class FileUploader < GitlabUploader private + def apply_context!(uploader_context) + @secret, @identifier = uploader_context.values_at(:secret, :identifier) + + !!(@secret && @identifier) + end + + def build_upload + super.tap do |upload| + upload.secret = secret + end + end + def markdown_name (image_or_video? ? File.basename(filename, File.extname(filename)) : filename).gsub("]", "\\]") end diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index b12829efe73..a9e5c028b03 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -29,6 +29,10 @@ class GitlabUploader < CarrierWave::Uploader::Base delegate :base_dir, :file_storage?, to: :class + def initialize(model, mounted_as = nil, **uploader_context) + super(model, mounted_as) + end + def file_cache_storage? cache_storage.is_a?(CarrierWave::Storage::File) end diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb index dfb8dccec57..458928bc067 100644 --- a/app/uploaders/records_uploads.rb +++ b/app/uploaders/records_uploads.rb @@ -24,7 +24,7 @@ module RecordsUploads uploads.where(path: upload_path).delete_all upload.destroy! if upload - self.upload = build_upload_from_uploader(self) + self.upload = build_upload upload.save! end end @@ -39,12 +39,13 @@ module RecordsUploads Upload.order(id: :desc).where(uploader: self.class.to_s) end - def build_upload_from_uploader(uploader) + def build_upload Upload.new( - size: uploader.file.size, - path: uploader.upload_path, - model: uploader.model, - uploader: uploader.class.to_s + uploader: self.class.to_s, + size: file.size, + path: upload_path, + model: model, + mount_point: mounted_as ) end diff --git a/db/migrate/20180129193323_add_uploads_builder_context.rb b/db/migrate/20180129193323_add_uploads_builder_context.rb new file mode 100644 index 00000000000..b3909a770ca --- /dev/null +++ b/db/migrate/20180129193323_add_uploads_builder_context.rb @@ -0,0 +1,14 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddUploadsBuilderContext < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + add_column :uploads, :mount_point, :string + add_column :uploads, :secret, :string + end +end diff --git a/lib/gitlab/gfm/uploads_rewriter.rb b/lib/gitlab/gfm/uploads_rewriter.rb index 3fdc3c27f73..1b74f735679 100644 --- a/lib/gitlab/gfm/uploads_rewriter.rb +++ b/lib/gitlab/gfm/uploads_rewriter.rb @@ -46,7 +46,7 @@ module Gitlab private def find_file(project, secret, file) - uploader = FileUploader.new(project, secret) + uploader = FileUploader.new(project, secret: secret) uploader.retrieve_from_store!(file) uploader.file end diff --git a/spec/factories/uploads.rb b/spec/factories/uploads.rb index c8cfe251d27..ff3a2a76acc 100644 --- a/spec/factories/uploads.rb +++ b/spec/factories/uploads.rb @@ -3,36 +3,40 @@ FactoryBot.define do model { build(:project) } size 100.kilobytes uploader "AvatarUploader" + mount_point :avatar + secret nil # we should build a mount agnostic upload by default transient do - mounted_as :avatar - secret SecureRandom.hex + filename 'myfile.jpg' end # this needs to comply with RecordsUpload::Concern#upload_path - path { File.join("uploads/-/system", model.class.to_s.underscore, mounted_as.to_s, 'avatar.jpg') } + path { File.join("uploads/-/system", model.class.to_s.underscore, mount_point.to_s, 'avatar.jpg') } trait :personal_snippet_upload do - model { build(:personal_snippet) } - path { File.join(secret, 'myfile.jpg') } uploader "PersonalFileUploader" + path { File.join(secret, filename) } + model { build(:personal_snippet) } + secret SecureRandom.hex end trait :issuable_upload do - path { File.join(secret, 'myfile.jpg') } uploader "FileUploader" + path { File.join(secret, filename) } + secret SecureRandom.hex end trait :namespace_upload do model { build(:group) } - path { File.join(secret, 'myfile.jpg') } + path { File.join(secret, filename) } uploader "NamespaceFileUploader" + secret SecureRandom.hex end trait :attachment_upload do transient do - mounted_as :attachment + mount_point :attachment end model { build(:note) } diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 42f3d609770..0dcaa026332 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -103,4 +103,10 @@ describe Upload do expect(upload).not_to exist end end + + describe "#uploader_context" do + subject { create(:upload, :issuable_upload, secret: 'secret', filename: 'file.txt') } + + it { expect(subject.uploader_context).to match(a_hash_including(secret: 'secret', identifier: 'file.txt')) } + end end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index a72f853df75..92f7467d247 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -40,7 +40,7 @@ describe FileUploader do end describe 'initialize' do - let(:uploader) { described_class.new(double, 'secret') } + let(:uploader) { described_class.new(double, secret: 'secret') } it 'accepts a secret parameter' do expect(described_class).not_to receive(:generate_secret) diff --git a/spec/uploaders/gitlab_uploader_spec.rb b/spec/uploaders/gitlab_uploader_spec.rb index a144b39f74f..60e35dcf235 100644 --- a/spec/uploaders/gitlab_uploader_spec.rb +++ b/spec/uploaders/gitlab_uploader_spec.rb @@ -4,7 +4,7 @@ require 'carrierwave/storage/fog' describe GitlabUploader do let(:uploader_class) { Class.new(described_class) } - subject { uploader_class.new } + subject { uploader_class.new(double) } describe '#file_storage?' do context 'when file storage is used' do -- cgit v1.2.1 From 210072ba3543332f662c96ffab7346a335290af4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Tue, 30 Jan 2018 08:24:28 -0500 Subject: fix specs --- changelogs/unreleased/42547-upload-store-mount-point.yml | 5 +++++ spec/lib/file_size_validator_spec.rb | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/42547-upload-store-mount-point.yml diff --git a/changelogs/unreleased/42547-upload-store-mount-point.yml b/changelogs/unreleased/42547-upload-store-mount-point.yml new file mode 100644 index 00000000000..35ae022984e --- /dev/null +++ b/changelogs/unreleased/42547-upload-store-mount-point.yml @@ -0,0 +1,5 @@ +--- +title: Added uploader metadata to the uploads. +merge_request: 16779 +author: +type: added diff --git a/spec/lib/file_size_validator_spec.rb b/spec/lib/file_size_validator_spec.rb index c44bc1840df..ebd907ecb7f 100644 --- a/spec/lib/file_size_validator_spec.rb +++ b/spec/lib/file_size_validator_spec.rb @@ -2,8 +2,8 @@ require 'spec_helper' describe FileSizeValidator do let(:validator) { described_class.new(options) } - let(:attachment) { AttachmentUploader.new } let(:note) { create(:note) } + let(:attachment) { AttachmentUploader.new(note) } describe 'options uses an integer' do let(:options) { { maximum: 10, attributes: { attachment: attachment } } } -- cgit v1.2.1 From 2b1536407fb42c1563fb41e2ee82e686e660ce11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Tue, 30 Jan 2018 10:19:30 -0500 Subject: fix the schema.rb --- db/schema.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/db/schema.rb b/db/schema.rb index 01a2df13dd3..b09e06726f8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1751,6 +1751,8 @@ ActiveRecord::Schema.define(version: 20180201145907) do t.string "model_type" t.string "uploader", null: false t.datetime "created_at", null: false + t.string "mount_point" + t.string "secret" end add_index "uploads", ["checksum"], name: "index_uploads_on_checksum", using: :btree -- cgit v1.2.1 From ce84d1835332932e25ebdc2cfbe44ff301328a1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Tue, 30 Jan 2018 14:38:10 -0500 Subject: apply fixes from feedback --- app/models/upload.rb | 3 +-- app/uploaders/file_uploader.rb | 16 +++++++++------- spec/uploaders/file_uploader_spec.rb | 27 +++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/app/models/upload.rb b/app/models/upload.rb index 28baee95091..2024228537a 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -30,8 +30,7 @@ class Upload < ActiveRecord::Base end def build_uploader - uploader_class.new(model, mount_point, **uploader_context) - .tap do |uploader| + uploader_class.new(model, mount_point, **uploader_context).tap do |uploader| uploader.upload = self uploader.retrieve_from_store!(identifier) end diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index cc4e7a38165..2310e67cb2e 100644 --- a/app/uploaders/file_uploader.rb +++ b/app/uploaders/file_uploader.rb @@ -110,14 +110,16 @@ class FileUploader < GitlabUploader end def upload=(value) - unless apply_context!(value.uploader_context) - if matches = DYNAMIC_PATH_PATTERN.match(value.path) - @secret = matches[:secret] - @identifier = matches[:identifier] - end - end - super + + return unless value + return if apply_context!(value.uploader_context) + + # fallback to the regex based extraction + if matches = DYNAMIC_PATH_PATTERN.match(value.path) + @secret = matches[:secret] + @identifier = matches[:identifier] + end end def secret diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index 92f7467d247..a559681a079 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -54,4 +54,31 @@ describe FileUploader do expect(uploader.secret).to eq('secret') end end + + describe '#upload=' do + let(:secret) { SecureRandom.hex } + let(:upload) { create(:upload, :issuable_upload, secret: secret, filename: 'file.txt') } + + it 'handles nil' do + expect(uploader).not_to receive(:apply_context!) + + uploader.upload = nil + end + + it 'extract the uploader context from it' do + expect(uploader).to receive(:apply_context!).with(a_hash_including(secret: secret, identifier: 'file.txt')) + + uploader.upload = upload + end + + context 'uploader_context is empty' do + it 'fallbacks to regex based extraction' do + expect(upload).to receive(:uploader_context).and_return({}) + + uploader.upload = upload + expect(uploader.secret).to eq(secret) + expect(uploader.instance_variable_get(:@identifier)).to eq('file.txt') + end + end + end end -- cgit v1.2.1 From 0ba3e84fe4845d1dbc02271dfe97a51968f2bd22 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Fri, 2 Feb 2018 12:34:25 -0600 Subject: Replace $.ajax in username validator with axios --- .../javascripts/pages/sessions/new/username_validator.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/pages/sessions/new/username_validator.js b/app/assets/javascripts/pages/sessions/new/username_validator.js index bb34d5d2008..745543c22da 100644 --- a/app/assets/javascripts/pages/sessions/new/username_validator.js +++ b/app/assets/javascripts/pages/sessions/new/username_validator.js @@ -1,6 +1,9 @@ /* eslint-disable comma-dangle, consistent-return, class-methods-use-this, arrow-parens, no-param-reassign, max-len */ import _ from 'underscore'; +import axios from '~/lib/utils/axios_utils'; +import flash from '~/flash'; +import { __ } from '~/locale'; const debounceTimeoutDuration = 1000; const invalidInputClass = 'gl-field-error-outline'; @@ -77,12 +80,9 @@ export default class UsernameValidator { this.state.pending = true; this.state.available = false; this.renderState(); - return $.ajax({ - type: 'GET', - url: `${gon.relative_url_root}/users/${username}/exists`, - dataType: 'json', - success: (res) => this.setAvailabilityState(res.exists) - }); + axios.get(`${gon.relative_url_root}/users/${username}/exists`) + .then(({ data }) => this.setAvailabilityState(data.exists)) + .catch(() => flash(__('An error occurred while validating username'))); } } -- cgit v1.2.1 From 5e0edf5710a11642be261549d3b1ff0f60bba0bc Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Fri, 2 Feb 2018 17:16:10 -0200 Subject: Bypass commits title markdown on notes --- app/services/system_note_service.rb | 22 ++++++++++++--- .../osw-markdown-bypass-for-commit-messages.yml | 5 ++++ spec/services/system_note_service_spec.rb | 33 +++++++++------------- 3 files changed, 37 insertions(+), 23 deletions(-) create mode 100644 changelogs/unreleased/osw-markdown-bypass-for-commit-messages.yml diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 06b23cd7076..17f0f5906cc 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -22,8 +22,7 @@ module SystemNoteService commits_text = "#{total_count} commit".pluralize(total_count) body = "added #{commits_text}\n\n" - body << existing_commit_summary(noteable, existing_commits, oldrev) - body << new_commit_summary(new_commits).join("\n") + body << commits_list(noteable, new_commits, existing_commits, oldrev) body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})" create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count)) @@ -481,7 +480,7 @@ module SystemNoteService # Returns an Array of Strings def new_commit_summary(new_commits) new_commits.collect do |commit| - "* #{commit.short_id} - #{escape_html(commit.title)}" + content_tag('li', "#{commit.short_id} - #{commit.title}") end end @@ -604,6 +603,16 @@ module SystemNoteService "#{cross_reference_note_prefix}#{gfm_reference}" end + # Builds a list of existing and new commits according to existing_commits and + # new_commits methods. + # Returns a String wrapped in `ul` and `li` tags. + def commits_list(noteable, new_commits, existing_commits, oldrev) + existing_commit_summary = existing_commit_summary(noteable, existing_commits, oldrev) + new_commit_summary = new_commit_summary(new_commits).join + + content_tag('ul', "#{existing_commit_summary}#{new_commit_summary}".html_safe) + end + # Build a single line summarizing existing commits being added in a merge # request # @@ -640,7 +649,8 @@ module SystemNoteService branch = noteable.target_branch branch = "#{noteable.target_project_namespace}:#{branch}" if noteable.for_fork? - "* #{commit_ids} - #{commits_text} from branch `#{branch}`\n" + branch_name = content_tag('code', branch) + content_tag('li', "#{commit_ids} - #{commits_text} from branch #{branch_name}".html_safe) end def escape_html(text) @@ -661,4 +671,8 @@ module SystemNoteService start_sha: oldrev ) end + + def content_tag(*args) + ActionController::Base.helpers.content_tag(*args) + end end diff --git a/changelogs/unreleased/osw-markdown-bypass-for-commit-messages.yml b/changelogs/unreleased/osw-markdown-bypass-for-commit-messages.yml new file mode 100644 index 00000000000..b2c1cd9710a --- /dev/null +++ b/changelogs/unreleased/osw-markdown-bypass-for-commit-messages.yml @@ -0,0 +1,5 @@ +--- +title: Bypass commits title markdown on notes +merge_request: +author: +type: fixed diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index ab3aa18cf4e..5b5edc1aa0d 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -54,10 +54,11 @@ describe SystemNoteService do expect(note_lines[0]).to eq "added #{new_commits.size} commits" end - it 'adds a message line for each commit' do - new_commits.each_with_index do |commit, i| - # Skip the header - expect(HTMLEntities.new.decode(note_lines[i + 1])).to eq "* #{commit.short_id} - #{commit.title}" + it 'adds a message for each commit' do + decoded_note_content = HTMLEntities.new.decode(subject.note) + + new_commits.each do |commit| + expect(decoded_note_content).to include("
  • #{commit.short_id} - #{commit.title}
    • ") end end end @@ -69,7 +70,7 @@ describe SystemNoteService do let(:old_commits) { [noteable.commits.last] } it 'includes the existing commit' do - expect(summary_line).to eq "* #{old_commits.first.short_id} - 1 commit from branch `feature`" + expect(summary_line).to start_with("
    • #{old_commits.first.short_id} - 1 commit from branch feature") end end @@ -79,22 +80,16 @@ describe SystemNoteService do context 'with oldrev' do let(:oldrev) { noteable.commits[2].id } - it 'includes a commit range' do - expect(summary_line).to start_with "* #{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id}" - end - - it 'includes a commit count' do - expect(summary_line).to end_with " - 26 commits from branch `feature`" + it 'includes a commit range and count' do + expect(summary_line) + .to start_with("
      • #{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id} - 26 commits from branch feature") end end context 'without oldrev' do - it 'includes a commit range' do - expect(summary_line).to start_with "* #{old_commits[0].short_id}..#{old_commits[-1].short_id}" - end - - it 'includes a commit count' do - expect(summary_line).to end_with " - 26 commits from branch `feature`" + it 'includes a commit range and count' do + expect(summary_line) + .to start_with("
        • #{old_commits[0].short_id}..#{old_commits[-1].short_id} - 26 commits from branch feature") end end @@ -104,7 +99,7 @@ describe SystemNoteService do end it 'includes the project namespace' do - expect(summary_line).to end_with "`#{noteable.target_project_namespace}:feature`" + expect(summary_line).to include("#{noteable.target_project_namespace}:feature") end end end @@ -693,7 +688,7 @@ describe SystemNoteService do describe '.new_commit_summary' do it 'escapes HTML titles' do commit = double(title: '
          This is a test
          ', short_id: '12345678') - escaped = '<pre>This is a test</pre>' + escaped = '<pre>This is a test</pre>' expect(described_class.new_commit_summary([commit])).to all(match(/- #{escaped}/)) end -- cgit v1.2.1 From 25262ed2ec0caa62a5325773b0cdb3c0517b6dd1 Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Fri, 2 Feb 2018 18:00:06 -0200 Subject: Remove unnused escape_html method --- app/services/system_note_service.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 17f0f5906cc..2253d638e93 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -653,10 +653,6 @@ module SystemNoteService content_tag('li', "#{commit_ids} - #{commits_text} from branch #{branch_name}".html_safe) end - def escape_html(text) - Rack::Utils.escape_html(text) - end - def url_helpers @url_helpers ||= Gitlab::Routing.url_helpers end -- cgit v1.2.1 From 1dbcd5ec47980e8c2a38630caec6ccf93d142285 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 2 Feb 2018 22:23:26 +0100 Subject: Refactor CalloutsController --- app/controllers/callouts_controller.rb | 10 +++++++--- config/routes.rb | 4 +--- spec/controllers/callouts_controller_spec.rb | 4 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/controllers/callouts_controller.rb b/app/controllers/callouts_controller.rb index e24172edf5d..0fffad3a927 100644 --- a/app/controllers/callouts_controller.rb +++ b/app/controllers/callouts_controller.rb @@ -1,9 +1,13 @@ class CalloutsController < ApplicationController - def dismiss + def create if ensure_callout - respond_to { |format| format.json { head :ok } } + respond_to do |format| + format.json { head :ok } + end else - respond_to { |format| format.json { head :bad_request } } + respond_to do |format| + format.json { head :bad_request } + end end end diff --git a/config/routes.rb b/config/routes.rb index abd626119ca..827de9d5b40 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -62,9 +62,7 @@ Rails.application.routes.draw do end # Callouts - namespace :callouts do - post :dismiss - end + resources :callouts, only: [:create] end # Koding route diff --git a/spec/controllers/callouts_controller_spec.rb b/spec/controllers/callouts_controller_spec.rb index 7a183d757a1..d4879832ef3 100644 --- a/spec/controllers/callouts_controller_spec.rb +++ b/spec/controllers/callouts_controller_spec.rb @@ -7,8 +7,8 @@ describe CalloutsController do sign_in(user) end - describe "POST #dismiss" do - subject { post :dismiss, feature_name: 'feature_name', format: :json } + describe "POST #create" do + subject { post :create, feature_name: 'feature_name', format: :json } context 'when callout entry does not exist' do it 'should create a callout entry with dismissed state' do -- cgit v1.2.1 From 838cc0907c963a025553d639283b3bbf8140dae8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 2 Feb 2018 22:25:01 +0100 Subject: Remove timestamps from Callouts --- db/migrate/20180125214301_create_callouts.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/db/migrate/20180125214301_create_callouts.rb b/db/migrate/20180125214301_create_callouts.rb index b6749b4cfe6..bce3d808efd 100644 --- a/db/migrate/20180125214301_create_callouts.rb +++ b/db/migrate/20180125214301_create_callouts.rb @@ -9,8 +9,6 @@ class CreateCallouts < ActiveRecord::Migration create_table :callouts do |t| t.string :feature_name, null: false t.references :user, index: true, foreign_key: { on_delete: :cascade }, null: false - - t.timestamps_with_timezone null: false end add_index :callouts, [:user_id, :feature_name], unique: true -- cgit v1.2.1 From 648826721f13ee4309a11638e538d96006648b39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 2 Feb 2018 22:51:03 +0100 Subject: Rename Callout to UserCallout --- app/controllers/callouts_controller.rb | 23 ---------- app/controllers/user_callouts_controller.rb | 23 ++++++++++ app/helpers/callouts_helper.rb | 19 -------- app/helpers/user_callouts_helper.rb | 19 ++++++++ app/models/callout.rb | 6 --- app/models/user.rb | 2 +- app/models/user_callout.rb | 6 +++ config/routes.rb | 4 +- db/migrate/20180125214301_create_callouts.rb | 16 ------- db/migrate/20180125214301_create_user_callouts.rb | 16 +++++++ db/schema.rb | 20 ++++----- spec/controllers/callouts_controller_spec.rb | 35 --------------- spec/controllers/user_callouts_controller_spec.rb | 35 +++++++++++++++ spec/factories/callouts.rb | 7 --- spec/factories/user_callouts.rb | 7 +++ spec/helpers/callouts_helper_spec.rb | 55 ----------------------- spec/helpers/user_callouts_helper_spec.rb | 55 +++++++++++++++++++++++ spec/models/callout_spec.rb | 16 ------- spec/models/user_callout_spec.rb | 16 +++++++ 19 files changed, 189 insertions(+), 191 deletions(-) delete mode 100644 app/controllers/callouts_controller.rb create mode 100644 app/controllers/user_callouts_controller.rb delete mode 100644 app/helpers/callouts_helper.rb create mode 100644 app/helpers/user_callouts_helper.rb delete mode 100644 app/models/callout.rb create mode 100644 app/models/user_callout.rb delete mode 100644 db/migrate/20180125214301_create_callouts.rb create mode 100644 db/migrate/20180125214301_create_user_callouts.rb delete mode 100644 spec/controllers/callouts_controller_spec.rb create mode 100644 spec/controllers/user_callouts_controller_spec.rb delete mode 100644 spec/factories/callouts.rb create mode 100644 spec/factories/user_callouts.rb delete mode 100644 spec/helpers/callouts_helper_spec.rb create mode 100644 spec/helpers/user_callouts_helper_spec.rb delete mode 100644 spec/models/callout_spec.rb create mode 100644 spec/models/user_callout_spec.rb diff --git a/app/controllers/callouts_controller.rb b/app/controllers/callouts_controller.rb deleted file mode 100644 index 0fffad3a927..00000000000 --- a/app/controllers/callouts_controller.rb +++ /dev/null @@ -1,23 +0,0 @@ -class CalloutsController < ApplicationController - def create - if ensure_callout - respond_to do |format| - format.json { head :ok } - end - else - respond_to do |format| - format.json { head :bad_request } - end - end - end - - private - - def ensure_callout - current_user.callouts.find_or_create_by(feature_name: callout_param) - end - - def callout_param - params.require(:feature_name) - end -end diff --git a/app/controllers/user_callouts_controller.rb b/app/controllers/user_callouts_controller.rb new file mode 100644 index 00000000000..69dc444f13a --- /dev/null +++ b/app/controllers/user_callouts_controller.rb @@ -0,0 +1,23 @@ +class UserCalloutsController < ApplicationController + def create + if ensure_callout + respond_to do |format| + format.json { head :ok } + end + else + respond_to do |format| + format.json { head :bad_request } + end + end + end + + private + + def ensure_callout + current_user.callouts.find_or_create_by(feature_name: callout_param) + end + + def callout_param + params.require(:feature_name) + end +end diff --git a/app/helpers/callouts_helper.rb b/app/helpers/callouts_helper.rb deleted file mode 100644 index 168b731ae89..00000000000 --- a/app/helpers/callouts_helper.rb +++ /dev/null @@ -1,19 +0,0 @@ -module CalloutsHelper - GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'.freeze - - # Higher value = higher priority - PRIORITY = { - GKE_CLUSTER_INTEGRATION: 0 - }.freeze - - def show_gke_cluster_integration_callout?(project) - current_user && !user_dismissed?(GKE_CLUSTER_INTEGRATION) && - can?(current_user, :create_cluster, project) - end - - private - - def user_dismissed?(feature_name) - current_user&.callouts&.find_by(feature_name: feature_name) - end -end diff --git a/app/helpers/user_callouts_helper.rb b/app/helpers/user_callouts_helper.rb new file mode 100644 index 00000000000..3725545d4cc --- /dev/null +++ b/app/helpers/user_callouts_helper.rb @@ -0,0 +1,19 @@ +module UserCalloutsHelper + GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'.freeze + + # Higher value = higher priority + PRIORITY = { + GKE_CLUSTER_INTEGRATION: 0 + }.freeze + + def show_gke_cluster_integration_callout?(project) + current_user && !user_dismissed?(GKE_CLUSTER_INTEGRATION) && + can?(current_user, :create_cluster, project) + end + + private + + def user_dismissed?(feature_name) + current_user&.callouts&.find_by(feature_name: feature_name) + end +end diff --git a/app/models/callout.rb b/app/models/callout.rb deleted file mode 100644 index c8f41a39cf3..00000000000 --- a/app/models/callout.rb +++ /dev/null @@ -1,6 +0,0 @@ -class Callout < ActiveRecord::Base - belongs_to :user - - validates :user, presence: true - validates :feature_name, presence: true, uniqueness: { scope: :user_id } -end diff --git a/app/models/user.rb b/app/models/user.rb index b54d44fe80a..587ee71e812 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -137,7 +137,7 @@ class User < ActiveRecord::Base has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent has_many :custom_attributes, class_name: 'UserCustomAttribute' - has_many :callouts + has_many :callouts, class_name: 'UserCallout' # # Validations diff --git a/app/models/user_callout.rb b/app/models/user_callout.rb new file mode 100644 index 00000000000..8b31aa6fa5c --- /dev/null +++ b/app/models/user_callout.rb @@ -0,0 +1,6 @@ +class UserCallout < ActiveRecord::Base + belongs_to :user + + validates :user, presence: true + validates :feature_name, presence: true, uniqueness: { scope: :user_id } +end diff --git a/config/routes.rb b/config/routes.rb index 827de9d5b40..e72ea1881cd 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -61,8 +61,8 @@ Rails.application.routes.draw do resources :issues, module: :boards, only: [:index, :update] end - # Callouts - resources :callouts, only: [:create] + # UserCallouts + resources :user_callouts, only: [:create] end # Koding route diff --git a/db/migrate/20180125214301_create_callouts.rb b/db/migrate/20180125214301_create_callouts.rb deleted file mode 100644 index bce3d808efd..00000000000 --- a/db/migrate/20180125214301_create_callouts.rb +++ /dev/null @@ -1,16 +0,0 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class CreateCallouts < ActiveRecord::Migration - # Set this constant to true if this migration requires downtime. - DOWNTIME = false - - def change - create_table :callouts do |t| - t.string :feature_name, null: false - t.references :user, index: true, foreign_key: { on_delete: :cascade }, null: false - end - - add_index :callouts, [:user_id, :feature_name], unique: true - end -end diff --git a/db/migrate/20180125214301_create_user_callouts.rb b/db/migrate/20180125214301_create_user_callouts.rb new file mode 100644 index 00000000000..945f4181274 --- /dev/null +++ b/db/migrate/20180125214301_create_user_callouts.rb @@ -0,0 +1,16 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateUserCallouts < ActiveRecord::Migration + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def change + create_table :user_callouts do |t| + t.string :feature_name, null: false + t.references :user, index: true, foreign_key: { on_delete: :cascade }, null: false + end + + add_index :user_callouts, [:user_id, :feature_name], unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 1b837db4e8d..46e4b6d2188 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -203,16 +203,6 @@ ActiveRecord::Schema.define(version: 20180125214301) do add_index "broadcast_messages", ["starts_at", "ends_at", "id"], name: "index_broadcast_messages_on_starts_at_and_ends_at_and_id", using: :btree - create_table "callouts", force: :cascade do |t| - t.string "feature_name", null: false - t.integer "user_id", null: false - t.datetime_with_timezone "created_at", null: false - t.datetime_with_timezone "updated_at", null: false - end - - add_index "callouts", ["user_id", "feature_name"], name: "index_callouts_on_user_id_and_feature_name", unique: true, using: :btree - add_index "callouts", ["user_id"], name: "index_callouts_on_user_id", using: :btree - create_table "chat_names", force: :cascade do |t| t.integer "user_id", null: false t.integer "service_id", null: false @@ -1779,6 +1769,14 @@ ActiveRecord::Schema.define(version: 20180125214301) do add_index "user_agent_details", ["subject_id", "subject_type"], name: "index_user_agent_details_on_subject_id_and_subject_type", using: :btree + create_table "user_callouts", force: :cascade do |t| + t.string "feature_name", null: false + t.integer "user_id", null: false + end + + add_index "user_callouts", ["user_id", "feature_name"], name: "index_user_callouts_on_user_id_and_feature_name", unique: true, using: :btree + add_index "user_callouts", ["user_id"], name: "index_user_callouts_on_user_id", using: :btree + create_table "user_custom_attributes", force: :cascade do |t| t.datetime_with_timezone "created_at", null: false t.datetime_with_timezone "updated_at", null: false @@ -1934,7 +1932,6 @@ ActiveRecord::Schema.define(version: 20180125214301) do add_index "web_hooks", ["type"], name: "index_web_hooks_on_type", using: :btree add_foreign_key "boards", "projects", name: "fk_f15266b5f9", on_delete: :cascade - add_foreign_key "callouts", "users", on_delete: :cascade add_foreign_key "chat_teams", "namespaces", on_delete: :cascade add_foreign_key "ci_build_trace_section_names", "projects", on_delete: :cascade add_foreign_key "ci_build_trace_sections", "ci_build_trace_section_names", column: "section_name_id", name: "fk_264e112c66", on_delete: :cascade @@ -2051,6 +2048,7 @@ ActiveRecord::Schema.define(version: 20180125214301) do add_foreign_key "todos", "projects", name: "fk_45054f9c45", on_delete: :cascade add_foreign_key "trending_projects", "projects", on_delete: :cascade add_foreign_key "u2f_registrations", "users" + add_foreign_key "user_callouts", "users", on_delete: :cascade add_foreign_key "user_custom_attributes", "users", on_delete: :cascade add_foreign_key "user_synced_attributes_metadata", "users", on_delete: :cascade add_foreign_key "users_star_projects", "projects", name: "fk_22cd27ddfc", on_delete: :cascade diff --git a/spec/controllers/callouts_controller_spec.rb b/spec/controllers/callouts_controller_spec.rb deleted file mode 100644 index d4879832ef3..00000000000 --- a/spec/controllers/callouts_controller_spec.rb +++ /dev/null @@ -1,35 +0,0 @@ -require 'spec_helper' - -describe CalloutsController do - let(:user) { create(:user) } - - before do - sign_in(user) - end - - describe "POST #create" do - subject { post :create, feature_name: 'feature_name', format: :json } - - context 'when callout entry does not exist' do - it 'should create a callout entry with dismissed state' do - expect { subject }.to change { Callout.count }.by(1) - end - - it 'should return success' do - subject - - expect(response).to have_gitlab_http_status(:ok) - end - end - - context 'when callout entry already exists' do - let!(:callout) { create(:callout, feature_name: 'feature_name', user: user) } - - it 'should return success' do - subject - - expect(response).to have_gitlab_http_status(:ok) - end - end - end -end diff --git a/spec/controllers/user_callouts_controller_spec.rb b/spec/controllers/user_callouts_controller_spec.rb new file mode 100644 index 00000000000..ac295aa72bb --- /dev/null +++ b/spec/controllers/user_callouts_controller_spec.rb @@ -0,0 +1,35 @@ +require 'spec_helper' + +describe UserCalloutsController do + let(:user) { create(:user) } + + before do + sign_in(user) + end + + describe "POST #create" do + subject { post :create, feature_name: 'feature_name', format: :json } + + context 'when callout entry does not exist' do + it 'should create a callout entry with dismissed state' do + expect { subject }.to change { UserCallout.count }.by(1) + end + + it 'should return success' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when callout entry already exists' do + let!(:callout) { create(:user_callout, feature_name: 'feature_name', user: user) } + + it 'should return success' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + end + end +end diff --git a/spec/factories/callouts.rb b/spec/factories/callouts.rb deleted file mode 100644 index f28df49b00a..00000000000 --- a/spec/factories/callouts.rb +++ /dev/null @@ -1,7 +0,0 @@ -FactoryBot.define do - factory :callout do - feature_name 'test_callout' - - user - end -end diff --git a/spec/factories/user_callouts.rb b/spec/factories/user_callouts.rb new file mode 100644 index 00000000000..ae83d65da2e --- /dev/null +++ b/spec/factories/user_callouts.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :user_callout do + feature_name 'test_callout' + + user + end +end diff --git a/spec/helpers/callouts_helper_spec.rb b/spec/helpers/callouts_helper_spec.rb deleted file mode 100644 index d0a5addc231..00000000000 --- a/spec/helpers/callouts_helper_spec.rb +++ /dev/null @@ -1,55 +0,0 @@ -require "spec_helper" - -describe CalloutsHelper do - let(:user) { create(:user) } - - before do - allow(helper).to receive(:current_user).and_return(user) - end - - describe '.show_gke_cluster_integration_callout?' do - let(:project) { create(:project) } - - subject { helper.show_gke_cluster_integration_callout?(project) } - - context 'when user has not dismissed' do - before do - allow(helper).to receive(:user_dismissed?).and_return(false) - end - - context 'when user can create a cluster' do - before do - allow(helper).to receive(:can?).with(anything, :create_cluster, anything) - .and_return(true) - end - - it { is_expected.to be true } - end - - context 'when user can not create a cluster' do - before do - allow(helper).to receive(:can?).with(anything, :create_cluster, anything) - .and_return(false) - end - - it { is_expected.to be false } - end - end - - context 'when user dismissed' do - before do - allow(helper).to receive(:user_dismissed?).and_return(true) - end - - it { is_expected.to be false } - end - - context 'when the user is not logged in' do - before do - allow(helper).to receive(:current_user).and_return(nil) - end - - it { is_expected.to be_falsey } - end - end -end diff --git a/spec/helpers/user_callouts_helper_spec.rb b/spec/helpers/user_callouts_helper_spec.rb new file mode 100644 index 00000000000..42c2e7827b2 --- /dev/null +++ b/spec/helpers/user_callouts_helper_spec.rb @@ -0,0 +1,55 @@ +require "spec_helper" + +describe UserCalloutsHelper do + let(:user) { create(:user) } + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + describe '.show_gke_cluster_integration_callout?' do + let(:project) { create(:project) } + + subject { helper.show_gke_cluster_integration_callout?(project) } + + context 'when user has not dismissed' do + before do + allow(helper).to receive(:user_dismissed?).and_return(false) + end + + context 'when user can create a cluster' do + before do + allow(helper).to receive(:can?).with(anything, :create_cluster, anything) + .and_return(true) + end + + it { is_expected.to be true } + end + + context 'when user can not create a cluster' do + before do + allow(helper).to receive(:can?).with(anything, :create_cluster, anything) + .and_return(false) + end + + it { is_expected.to be false } + end + end + + context 'when user dismissed' do + before do + allow(helper).to receive(:user_dismissed?).and_return(true) + end + + it { is_expected.to be false } + end + + context 'when the user is not logged in' do + before do + allow(helper).to receive(:current_user).and_return(nil) + end + + it { is_expected.to be_falsey } + end + end +end diff --git a/spec/models/callout_spec.rb b/spec/models/callout_spec.rb deleted file mode 100644 index 29299678ebc..00000000000 --- a/spec/models/callout_spec.rb +++ /dev/null @@ -1,16 +0,0 @@ -require 'rails_helper' - -describe Callout do - let!(:callout) { create(:callout) } - - describe 'relationships' do - it { is_expected.to belong_to(:user) } - end - - describe 'validations' do - it { is_expected.to validate_presence_of(:user) } - - it { is_expected.to validate_presence_of(:feature_name) } - it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id) } - end -end diff --git a/spec/models/user_callout_spec.rb b/spec/models/user_callout_spec.rb new file mode 100644 index 00000000000..b39d12e48ac --- /dev/null +++ b/spec/models/user_callout_spec.rb @@ -0,0 +1,16 @@ +require 'rails_helper' + +describe UserCallout do + let!(:callout) { create(:user_callout) } + + describe 'relationships' do + it { is_expected.to belong_to(:user) } + end + + describe 'validations' do + it { is_expected.to validate_presence_of(:user) } + + it { is_expected.to validate_presence_of(:feature_name) } + it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id) } + end +end -- cgit v1.2.1 From e60bf2f256746e49eb79b6cd97dfe2147ea957b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= Date: Fri, 2 Feb 2018 18:30:03 -0300 Subject: Incorporate OperationService.UserSquash Gitaly RPC --- GITALY_SERVER_VERSION | 2 +- lib/gitlab/git/repository.rb | 63 +++++++++++++--------- lib/gitlab/gitaly_client/operation_service.rb | 26 +++++++++ spec/lib/gitlab/git/repository_spec.rb | 2 +- .../gitlab/gitaly_client/operation_service_spec.rb | 49 +++++++++++++++++ 5 files changed, 114 insertions(+), 28 deletions(-) diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 4a7076db09a..bd14e8533ef 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.77.0 +0.78.0 diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index f28624ff37a..359ad9f4521 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1222,33 +1222,13 @@ module Gitlab end def squash(user, squash_id, branch:, start_sha:, end_sha:, author:, message:) - squash_path = worktree_path(SQUASH_WORKTREE_PREFIX, squash_id) - env = git_env_for_user(user).merge( - 'GIT_AUTHOR_NAME' => author.name, - 'GIT_AUTHOR_EMAIL' => author.email - ) - diff_range = "#{start_sha}...#{end_sha}" - diff_files = run_git!( - %W(diff --name-only --diff-filter=a --binary #{diff_range}) - ).chomp - - with_worktree(squash_path, branch, sparse_checkout_files: diff_files, env: env) do - # Apply diff of the `diff_range` to the worktree - diff = run_git!(%W(diff --binary #{diff_range})) - run_git!(%w(apply --index), chdir: squash_path, env: env) do |stdin| - stdin.write(diff) + gitaly_migrate(:squash) do |is_enabled| + if is_enabled + gitaly_operation_client.user_squash(user, squash_id, branch, + start_sha, end_sha, author, message) + else + git_squash(user, squash_id, branch, start_sha, end_sha, author, message) end - - # Commit the `diff_range` diff - run_git!(%W(commit --no-verify --message #{message}), chdir: squash_path, env: env) - - # Return the squash sha. May print a warning for ambiguous refs, but - # we can ignore that with `--quiet` and just take the SHA, if present. - # HEAD here always refers to the current HEAD commit, even if there is - # another ref called HEAD. - run_git!( - %w(rev-parse --quiet --verify HEAD), chdir: squash_path, env: env - ).chomp end end @@ -2164,6 +2144,37 @@ module Gitlab end end + def git_squash(user, squash_id, branch, start_sha, end_sha, author, message) + squash_path = worktree_path(SQUASH_WORKTREE_PREFIX, squash_id) + env = git_env_for_user(user).merge( + 'GIT_AUTHOR_NAME' => author.name, + 'GIT_AUTHOR_EMAIL' => author.email + ) + diff_range = "#{start_sha}...#{end_sha}" + diff_files = run_git!( + %W(diff --name-only --diff-filter=a --binary #{diff_range}) + ).chomp + + with_worktree(squash_path, branch, sparse_checkout_files: diff_files, env: env) do + # Apply diff of the `diff_range` to the worktree + diff = run_git!(%W(diff --binary #{diff_range})) + run_git!(%w(apply --index), chdir: squash_path, env: env) do |stdin| + stdin.write(diff) + end + + # Commit the `diff_range` diff + run_git!(%W(commit --no-verify --message #{message}), chdir: squash_path, env: env) + + # Return the squash sha. May print a warning for ambiguous refs, but + # we can ignore that with `--quiet` and just take the SHA, if present. + # HEAD here always refers to the current HEAD commit, even if there is + # another ref called HEAD. + run_git!( + %w(rev-parse --quiet --verify HEAD), chdir: squash_path, env: env + ).chomp + end + 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) diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index cd2734b5a07..831cfd1e014 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -183,6 +183,32 @@ module Gitlab end end + def user_squash(user, squash_id, branch, start_sha, end_sha, author, message) + request = Gitaly::UserSquashRequest.new( + repository: @gitaly_repo, + user: Gitlab::Git::User.from_gitlab(user).to_gitaly, + squash_id: squash_id.to_s, + branch: encode_binary(branch), + start_sha: start_sha, + end_sha: end_sha, + author: Gitlab::Git::User.from_gitlab(author).to_gitaly, + commit_message: encode_binary(message) + ) + + response = GitalyClient.call( + @repository.storage, + :operation_service, + :user_squash, + request + ) + + if response.git_error.presence + raise Gitlab::Git::Repository::GitError, response.git_error + end + + response.squash_sha + end + def user_commit_files( user, branch_name, commit_message, actions, author_email, author_name, start_branch_name, start_repository) diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 96a442f782f..e768ff7735f 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -2183,7 +2183,7 @@ describe Gitlab::Git::Repository, seed_helper: true do repository.squash(user, squash_id, opts) end - context 'sparse checkout' do + context 'sparse checkout', :skip_gitaly_mock do let(:expected_files) { %w(files files/js files/js/application.js) } before do diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb index d9ec28ab02e..9fbdd73ee0e 100644 --- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb @@ -123,4 +123,53 @@ describe Gitlab::GitalyClient::OperationService do expect(subject.branch_created).to be(false) end end + + describe '#user_squash' do + let(:branch_name) { 'my-branch' } + let(:squash_id) { '1' } + let(:start_sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' } + let(:end_sha) { '54cec5282aa9f21856362fe321c800c236a61615' } + let(:commit_message) { 'Squash message' } + let(:request) do + Gitaly::UserSquashRequest.new( + repository: repository.gitaly_repository, + user: gitaly_user, + squash_id: squash_id.to_s, + branch: branch_name, + start_sha: start_sha, + end_sha: end_sha, + author: gitaly_user, + commit_message: commit_message + ) + end + let(:squash_sha) { 'f00' } + let(:response) { Gitaly::UserSquashResponse.new(squash_sha: squash_sha) } + + subject do + client.user_squash(user, squash_id, branch_name, start_sha, end_sha, user, commit_message) + end + + it 'sends a user_squash message and returns the squash sha' do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_squash).with(request, kind_of(Hash)) + .and_return(response) + + expect(subject).to eq(squash_sha) + end + + context "when git_error is present" do + let(:response) do + Gitaly::UserSquashResponse.new(git_error: "something failed") + end + + it "throws a PreReceive exception" do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_squash).with(request, kind_of(Hash)) + .and_return(response) + + expect { subject }.to raise_error( + Gitlab::Git::Repository::GitError, "something failed") + end + end + end end -- cgit v1.2.1 From 20714ee90232b5577fc99f2a773ddccf71482c08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sat, 3 Feb 2018 00:16:24 +0100 Subject: Change UserCallout feautre_name to enum --- app/controllers/user_callouts_controller.rb | 6 +++- app/helpers/user_callouts_helper.rb | 5 ---- app/models/user_callout.rb | 9 +++++- db/migrate/20180125214301_create_user_callouts.rb | 2 +- db/schema.rb | 2 +- spec/controllers/user_callouts_controller_spec.rb | 36 ++++++++++++++++------- spec/factories/user_callouts.rb | 2 +- spec/models/user_callout_spec.rb | 2 +- 8 files changed, 42 insertions(+), 22 deletions(-) diff --git a/app/controllers/user_callouts_controller.rb b/app/controllers/user_callouts_controller.rb index 69dc444f13a..aa048a9a1fc 100644 --- a/app/controllers/user_callouts_controller.rb +++ b/app/controllers/user_callouts_controller.rb @@ -1,6 +1,6 @@ class UserCalloutsController < ApplicationController def create - if ensure_callout + if check_feature_name && ensure_callout respond_to do |format| format.json { head :ok } end @@ -13,6 +13,10 @@ class UserCalloutsController < ApplicationController private + def check_feature_name + UserCallout.feature_names.keys.include?(callout_param) + end + def ensure_callout current_user.callouts.find_or_create_by(feature_name: callout_param) end diff --git a/app/helpers/user_callouts_helper.rb b/app/helpers/user_callouts_helper.rb index 3725545d4cc..a292f676331 100644 --- a/app/helpers/user_callouts_helper.rb +++ b/app/helpers/user_callouts_helper.rb @@ -1,11 +1,6 @@ module UserCalloutsHelper GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'.freeze - # Higher value = higher priority - PRIORITY = { - GKE_CLUSTER_INTEGRATION: 0 - }.freeze - def show_gke_cluster_integration_callout?(project) current_user && !user_dismissed?(GKE_CLUSTER_INTEGRATION) && can?(current_user, :create_cluster, project) diff --git a/app/models/user_callout.rb b/app/models/user_callout.rb index 8b31aa6fa5c..a7cfe5df7c0 100644 --- a/app/models/user_callout.rb +++ b/app/models/user_callout.rb @@ -1,6 +1,13 @@ class UserCallout < ActiveRecord::Base belongs_to :user + enum feature_name: { + gke_cluster_integration: 0 + } + validates :user, presence: true - validates :feature_name, presence: true, uniqueness: { scope: :user_id } + validates :feature_name, + presence: true, + uniqueness: { scope: :user_id }, + inclusion: { in: UserCallout.feature_names.keys } end diff --git a/db/migrate/20180125214301_create_user_callouts.rb b/db/migrate/20180125214301_create_user_callouts.rb index 945f4181274..856eff36ae0 100644 --- a/db/migrate/20180125214301_create_user_callouts.rb +++ b/db/migrate/20180125214301_create_user_callouts.rb @@ -7,7 +7,7 @@ class CreateUserCallouts < ActiveRecord::Migration def change create_table :user_callouts do |t| - t.string :feature_name, null: false + t.integer :feature_name, null: false t.references :user, index: true, foreign_key: { on_delete: :cascade }, null: false end diff --git a/db/schema.rb b/db/schema.rb index 6e50b7681d0..14024164359 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1770,7 +1770,7 @@ ActiveRecord::Schema.define(version: 20180202111106) do add_index "user_agent_details", ["subject_id", "subject_type"], name: "index_user_agent_details_on_subject_id_and_subject_type", using: :btree create_table "user_callouts", force: :cascade do |t| - t.string "feature_name", null: false + t.integer "feature_name", null: false t.integer "user_id", null: false end diff --git a/spec/controllers/user_callouts_controller_spec.rb b/spec/controllers/user_callouts_controller_spec.rb index ac295aa72bb..48e2ff75cac 100644 --- a/spec/controllers/user_callouts_controller_spec.rb +++ b/spec/controllers/user_callouts_controller_spec.rb @@ -8,27 +8,41 @@ describe UserCalloutsController do end describe "POST #create" do - subject { post :create, feature_name: 'feature_name', format: :json } + subject { post :create, feature_name: feature_name, format: :json } - context 'when callout entry does not exist' do - it 'should create a callout entry with dismissed state' do - expect { subject }.to change { UserCallout.count }.by(1) + context 'with valid feature name' do + let(:feature_name) { UserCallout.feature_names.keys.first } + + context 'when callout entry does not exist' do + it 'should create a callout entry with dismissed state' do + expect { subject }.to change { UserCallout.count }.by(1) + end + + it 'should return success' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end end - it 'should return success' do - subject + context 'when callout entry already exists' do + let!(:callout) { create(:user_callout, feature_name: UserCallout.feature_names.keys.first, user: user) } + + it 'should return success' do + subject - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:ok) + end end end - context 'when callout entry already exists' do - let!(:callout) { create(:user_callout, feature_name: 'feature_name', user: user) } + context 'with invalid feature name' do + let(:feature_name) { 'bogus_feature_name' } - it 'should return success' do + it 'should return bad request' do subject - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:bad_request) end end end diff --git a/spec/factories/user_callouts.rb b/spec/factories/user_callouts.rb index ae83d65da2e..528e442c14b 100644 --- a/spec/factories/user_callouts.rb +++ b/spec/factories/user_callouts.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :user_callout do - feature_name 'test_callout' + feature_name :gke_cluster_integration user end diff --git a/spec/models/user_callout_spec.rb b/spec/models/user_callout_spec.rb index b39d12e48ac..64ba17c81fe 100644 --- a/spec/models/user_callout_spec.rb +++ b/spec/models/user_callout_spec.rb @@ -11,6 +11,6 @@ describe UserCallout do it { is_expected.to validate_presence_of(:user) } it { is_expected.to validate_presence_of(:feature_name) } - it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id) } + it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id).ignoring_case_sensitivity } end end -- cgit v1.2.1 From fa7cc50f04af34bd58039d190f5844b7f5c3fe8b Mon Sep 17 00:00:00 2001 From: George Tsiolis Date: Sun, 4 Feb 2018 00:36:01 +0200 Subject: Include branch in mobile view for pipelines --- app/assets/javascripts/vue_shared/components/commit.vue | 4 ++-- app/assets/stylesheets/pages/pipelines.scss | 2 +- changelogs/unreleased/style-include-branch-in-mobile-view.yml | 5 +++++ 3 files changed, 8 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/style-include-branch-in-mobile-view.yml diff --git a/app/assets/javascripts/vue_shared/components/commit.vue b/app/assets/javascripts/vue_shared/components/commit.vue index 6d1fe7ee8ca..97789636787 100644 --- a/app/assets/javascripts/vue_shared/components/commit.vue +++ b/app/assets/javascripts/vue_shared/components/commit.vue @@ -118,7 +118,7 @@