From 6a915d6f2d462a376d8cecc062dd58e520339b5e Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 2 May 2017 22:52:14 +0200 Subject: Limit `update_tracked_fields` to write to database once/hour Every time a user logs in or out, the Trackable attributes are written to the database. This is causing a lot of load on the database, for data that isn't really critical. So to avoid the database being hammered, add a Gitlab::ExclusiveLease before writing trackable attributes to the database. This lease expires after an hour, so only when the attributes were written more than an hour ago, they can be written again. Otherwise they are ignored. --- app/models/user.rb | 10 ++++++++++ .../unreleased/tc-cache-trackable-attributes.yml | 4 ++++ spec/models/user_spec.rb | 19 +++++++++++++++++++ 3 files changed, 33 insertions(+) create mode 100644 changelogs/unreleased/tc-cache-trackable-attributes.yml diff --git a/app/models/user.rb b/app/models/user.rb index accaa91b805..05f636c020a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -40,6 +40,16 @@ class User < ActiveRecord::Base devise :lockable, :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable + # Limit trackable fields to update at most once every hour + alias_method :devise_update_tracked_fields!, :update_tracked_fields! + + def update_tracked_fields!(request) + lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i) + return unless lease.try_obtain + + devise_update_tracked_fields!(request) + end + attr_accessor :force_random_password # Virtual attribute for authenticating by either username or email diff --git a/changelogs/unreleased/tc-cache-trackable-attributes.yml b/changelogs/unreleased/tc-cache-trackable-attributes.yml new file mode 100644 index 00000000000..4a2cf50893a --- /dev/null +++ b/changelogs/unreleased/tc-cache-trackable-attributes.yml @@ -0,0 +1,4 @@ +--- +title: "Limit User's trackable attributes, like `current_sign_in_at`, to update at most once/hour" +merge_request: 11053 +author: diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 63e71f5ff2f..0b59916342e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -344,6 +344,25 @@ describe User, models: true do end end + describe '#update_tracked_fields!', :redis do + let(:request) { OpenStruct.new(remote_ip: "127.0.0.1") } + let(:user) { create(:user) } + + it 'writes trackable attributes' do + expect do + user.update_tracked_fields!(request) + end.to change { user.reload.current_sign_in_at } + end + + it 'does not write trackable attributes when called a second time within the hour' do + user.update_tracked_fields!(request) + + expect do + user.update_tracked_fields!(request) + end.not_to change { user.current_sign_in_at } + end + end + shared_context 'user keys' do let(:user) { create(:user) } let!(:key) { create(:key, user: user) } -- cgit v1.2.1 From 3531ea096f730b8533df259ac2f6cbed738965ed Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Fri, 5 May 2017 09:29:03 +0200 Subject: Devise can assign trackable fields, but only allow writes once/hour Not assigning the trackable fields seems to cause strange side-effects. --- app/models/user.rb | 9 +++++---- spec/features/groups/members/sorting_spec.rb | 4 ++-- spec/models/user_spec.rb | 12 +++++++++++- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 05f636c020a..0b358fd4b19 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -40,14 +40,15 @@ class User < ActiveRecord::Base devise :lockable, :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable - # Limit trackable fields to update at most once every hour - alias_method :devise_update_tracked_fields!, :update_tracked_fields! - + # Override Devise::Models::Trackable#update_tracked_fields! + # to limit database writes to at most once every hour def update_tracked_fields!(request) + update_tracked_fields(request) + lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i) return unless lease.try_obtain - devise_update_tracked_fields!(request) + save(validate: false) end attr_accessor :force_random_password diff --git a/spec/features/groups/members/sorting_spec.rb b/spec/features/groups/members/sorting_spec.rb index 608aedd3471..902d3f789ff 100644 --- a/spec/features/groups/members/sorting_spec.rb +++ b/spec/features/groups/members/sorting_spec.rb @@ -68,7 +68,7 @@ feature 'Groups > Members > Sorting', feature: true do expect(page).to have_css('.member-sort-dropdown .dropdown-toggle-text', text: 'Name, descending') end - scenario 'sorts by recent sign in' do + scenario 'sorts by recent sign in', :redis do visit_members_list(sort: :recent_sign_in) expect(first_member).to include(owner.name) @@ -76,7 +76,7 @@ feature 'Groups > Members > Sorting', feature: true do expect(page).to have_css('.member-sort-dropdown .dropdown-toggle-text', text: 'Recent sign in') end - scenario 'sorts by oldest sign in' do + scenario 'sorts by oldest sign in', :redis do visit_members_list(sort: :oldest_sign_in) expect(first_member).to include(developer.name) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0b59916342e..c7ddd17872b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -359,7 +359,17 @@ describe User, models: true do expect do user.update_tracked_fields!(request) - end.not_to change { user.current_sign_in_at } + end.not_to change { user.reload.current_sign_in_at } + end + + it 'writes trackable attributes for a different user' do + user2 = create(:user) + + user.update_tracked_fields!(request) + + expect do + user2.update_tracked_fields!(request) + end.to change { user2.reload.current_sign_in_at } end end -- cgit v1.2.1 From 4ae411ff40352ec21bd10f859b7f3f9f85a7aee6 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Fri, 17 Mar 2017 19:14:27 -0300 Subject: Preloads head pipeline for each merge request --- app/controllers/concerns/issuable_collections.rb | 2 +- app/models/merge_request.rb | 8 ++------ app/services/ci/create_pipeline_service.rb | 8 +++++++- changelogs/unreleased/issue_27168_2.yml | 4 ++++ .../20170427205316_add_head_pipeline_id_to_merge_requests.rb | 7 +++++++ db/schema.rb | 2 ++ spec/lib/gitlab/import_export/safe_model_attributes.yml | 1 + spec/services/ci/create_pipeline_service_spec.rb | 12 ++++++++++++ 8 files changed, 36 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/issue_27168_2.yml create mode 100644 db/migrate/20170427205316_add_head_pipeline_id_to_merge_requests.rb diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index 6df2c068745..650ec1e326a 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -47,7 +47,7 @@ module IssuableCollections end def merge_requests_collection - merge_requests_finder.execute.preload(:source_project, :target_project, :author, :assignee, :labels, :milestone, :merge_request_diff, target_project: :namespace) + merge_requests_finder.execute.preload(:source_project, :target_project, :author, :assignee, :labels, :milestone, :merge_request_diff, :head_pipeline, target_project: :namespace) end def issues_finder diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 35231bab12e..3a2cff6ac21 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -13,6 +13,8 @@ class MergeRequest < ActiveRecord::Base has_one :merge_request_diff, -> { order('merge_request_diffs.id DESC') } + belongs_to :head_pipeline, foreign_key: "head_pipeline_id", class_name: "Ci::Pipeline" + has_many :events, as: :target, dependent: :destroy has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues', dependent: :delete_all @@ -829,12 +831,6 @@ class MergeRequest < ActiveRecord::Base diverged_commits_count > 0 end - def head_pipeline - return unless diff_head_sha && source_project - - @head_pipeline ||= source_project.pipeline_for(source_branch, diff_head_sha) - end - def all_pipelines return Ci::Pipeline.none unless source_project diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index ccdda08d885..26b744ae3c4 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -47,7 +47,7 @@ module Ci end Ci::Pipeline.transaction do - pipeline.save + update_merge_requests_head_pipeline if pipeline.save Ci::CreatePipelineBuildsService .new(project, current_user) @@ -118,6 +118,12 @@ module Ci origin_sha && origin_sha != Gitlab::Git::BLANK_SHA end + def update_merge_requests_head_pipeline + merge_requests = MergeRequest.where(source_branch: @pipeline.ref, source_project: @pipeline.project) + + merge_requests.update_all(head_pipeline_id: @pipeline.id) if merge_requests.any? + end + def error(message, save: false) pipeline.errors.add(:base, message) pipeline.drop if save diff --git a/changelogs/unreleased/issue_27168_2.yml b/changelogs/unreleased/issue_27168_2.yml new file mode 100644 index 00000000000..c91a95323d5 --- /dev/null +++ b/changelogs/unreleased/issue_27168_2.yml @@ -0,0 +1,4 @@ +--- +title: Preloads head pipeline for each merge request +merge_request: +author: diff --git a/db/migrate/20170427205316_add_head_pipeline_id_to_merge_requests.rb b/db/migrate/20170427205316_add_head_pipeline_id_to_merge_requests.rb new file mode 100644 index 00000000000..8fc6e380a77 --- /dev/null +++ b/db/migrate/20170427205316_add_head_pipeline_id_to_merge_requests.rb @@ -0,0 +1,7 @@ +class AddHeadPipelineIdToMergeRequests < ActiveRecord::Migration + DOWNTIME = false + + def change + add_column :merge_requests, :head_pipeline_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 722e776c27d..d5b1d3bb0d8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -123,6 +123,7 @@ ActiveRecord::Schema.define(version: 20170506185517) do t.integer "cached_markdown_version" t.boolean "clientside_sentry_enabled", default: false, null: false t.string "clientside_sentry_dsn" + t.string "default_artifacts_expire_in", default: "0", null: false end create_table "audit_events", force: :cascade do |t| @@ -690,6 +691,7 @@ ActiveRecord::Schema.define(version: 20170506185517) do t.integer "cached_markdown_version" t.datetime "last_edited_at" t.integer "last_edited_by_id" + t.integer "head_pipeline_id" end add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 3af2a172e6d..d2ceb1cf9ae 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -158,6 +158,7 @@ MergeRequest: - time_estimate - last_edited_at - last_edited_by_id +- head_pipeline_id MergeRequestDiff: - id - state diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index fa5014cee07..f2ed66b5e8e 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -34,6 +34,18 @@ describe Ci::CreatePipelineService, services: true do it { expect(pipeline).to have_attributes(status: 'pending') } it { expect(pipeline.builds.first).to be_kind_of(Ci::Build) } + it 'updates head pipeline of each merge request' do + merge_request_1 = create(:merge_request, source_branch: 'master', target_branch: "branch_1", source_project: project) + merge_request_2 = create(:merge_request, source_branch: 'master', target_branch: "branch_2", source_project: project) + merge_request_3 = create(:merge_request, source_branch: 'other_branch', target_branch: "branch_2", source_project: project) + + head_pipeline = pipeline + + expect(merge_request_1.reload.head_pipeline).to eq(head_pipeline) + expect(merge_request_2.reload.head_pipeline).to eq(head_pipeline) + expect(merge_request_3.reload.head_pipeline).to be_nil + end + context 'auto-cancel enabled' do before do project.update(auto_cancel_pending_pipelines: 'enabled') -- cgit v1.2.1 From 85f0b3a984048d1e5d146a233612a0bc96f12b5c Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Wed, 22 Mar 2017 12:20:44 -0300 Subject: Add merge requests association to pipeline --- app/models/ci/pipeline.rb | 1 + spec/workers/pipeline_metrics_worker_spec.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index db994b861e5..481f9c8dac8 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -17,6 +17,7 @@ module Ci has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id has_many :builds, foreign_key: :commit_id has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id + has_many :merge_requests, foreign_key: "head_pipeline_id" has_many :pending_builds, -> { pending }, foreign_key: :commit_id, class_name: 'Ci::Build' has_many :retryable_builds, -> { latest.failed_or_canceled }, foreign_key: :commit_id, class_name: 'Ci::Build' diff --git a/spec/workers/pipeline_metrics_worker_spec.rb b/spec/workers/pipeline_metrics_worker_spec.rb index 5dbc0da95c2..dbf2b7251c6 100644 --- a/spec/workers/pipeline_metrics_worker_spec.rb +++ b/spec/workers/pipeline_metrics_worker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe PipelineMetricsWorker do let(:project) { create(:project, :repository) } - let!(:merge_request) { create(:merge_request, source_project: project, source_branch: pipeline.ref) } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: pipeline.ref, head_pipeline_id: pipeline.id) } let(:pipeline) do create(:ci_empty_pipeline, -- cgit v1.2.1 From 24824cbb4c6d2c5ebd08dea03007f65c5df763da Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Wed, 22 Mar 2017 16:36:14 -0300 Subject: Fix Specs --- changelogs/unreleased/issue_27168_2.yml | 2 +- features/steps/project/merge_requests.rb | 1 + .../projects/merge_requests_controller_spec.rb | 3 ++- spec/features/cycle_analytics_spec.rb | 5 ++++- spec/features/issuables/issuable_list_spec.rb | 3 +++ .../merge_immediately_with_pipeline_spec.rb | 10 ++++++---- .../merge_requests/merge_when_pipeline_succeeds_spec.rb | 5 ++++- .../features/merge_requests/mini_pipeline_graph_spec.rb | 2 +- .../only_allow_merge_if_build_succeeds_spec.rb | 2 ++ spec/features/merge_requests/widget_spec.rb | 17 +++++++++++++---- spec/lib/gitlab/cycle_analytics/events_spec.rb | 4 ++++ spec/lib/gitlab/import_export/all_models.yml | 1 + spec/models/ci/pipeline_spec.rb | 2 +- spec/models/cycle_analytics/test_spec.rb | 1 + spec/models/merge_request_spec.rb | 16 +++++++--------- spec/requests/projects/cycle_analytics_events_spec.rb | 1 + .../merge_when_pipeline_succeeds_service_spec.rb | 6 ++++++ spec/services/merge_requests/update_service_spec.rb | 4 +++- 18 files changed, 61 insertions(+), 24 deletions(-) diff --git a/changelogs/unreleased/issue_27168_2.yml b/changelogs/unreleased/issue_27168_2.yml index c91a95323d5..c67692493e0 100644 --- a/changelogs/unreleased/issue_27168_2.yml +++ b/changelogs/unreleased/issue_27168_2.yml @@ -1,4 +1,4 @@ --- -title: Preloads head pipeline for each merge request +title: Preloads head pipeline for merge request collection merge_request: author: diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 4b7d6cd840b..8bd2065ac34 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -544,6 +544,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps project = merge_request.source_project project.enable_ci pipeline = create :ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch + merge_request.update(head_pipeline: pipeline) create :ci_build, pipeline: pipeline end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 0483c6b7879..bc13a8123a5 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -354,7 +354,8 @@ describe Projects::MergeRequestsController do end before do - create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) + pipeline = create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) + merge_request.update(head_pipeline: pipeline) end it 'returns :merge_when_pipeline_succeeds' do diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index df2714f91ff..57826fe872b 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -9,10 +9,13 @@ feature 'Cycle Analytics', feature: true, js: true do let(:mr) { create_merge_request_closing_issue(issue, commit_message: "References #{issue.to_reference}") } let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha) } + before { mr.update(head_pipeline: pipeline) } + context 'as an allowed user' do context 'when project is new' do before do - project.team << [user, :master] + project.add_master(user) + mr.update(head_pipeline_id: pipeline.id) login_as(user) visit namespace_project_cycle_analytics_path(project.namespace, project) wait_for_ajax diff --git a/spec/features/issuables/issuable_list_spec.rb b/spec/features/issuables/issuable_list_spec.rb index f3ec80bb149..414838fa22e 100644 --- a/spec/features/issuables/issuable_list_spec.rb +++ b/spec/features/issuables/issuable_list_spec.rb @@ -52,6 +52,9 @@ describe 'issuable list', feature: true do create(:issue, project: project, author: user) else create(:merge_request, source_project: project, source_branch: generate(:branch)) + source_branch = FFaker::Name.name + pipeline = create(:ci_empty_pipeline, project: project, ref: source_branch, status: %w(running failed success).sample, sha: 'any') + create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: source_branch, head_pipeline: pipeline) end 2.times do diff --git a/spec/features/merge_requests/merge_immediately_with_pipeline_spec.rb b/spec/features/merge_requests/merge_immediately_with_pipeline_spec.rb index 497240803d4..4f9a69baad8 100644 --- a/spec/features/merge_requests/merge_immediately_with_pipeline_spec.rb +++ b/spec/features/merge_requests/merge_immediately_with_pipeline_spec.rb @@ -4,16 +4,18 @@ feature 'Merge immediately', :feature, :js do let(:user) { create(:user) } let(:project) { create(:project, :public) } - let(:merge_request) do + let!(:merge_request) do create(:merge_request_with_diffs, source_project: project, author: user, - title: 'Bug NS-04') + title: 'Bug NS-04', + head_pipeline: pipeline, + source_branch: pipeline.ref) end let(:pipeline) do create(:ci_pipeline, project: project, - sha: merge_request.diff_head_sha, - ref: merge_request.source_branch) + ref: 'master', + sha: project.repository.commit('master').id) end before { project.team << [user, :master] } diff --git a/spec/features/merge_requests/merge_when_pipeline_succeeds_spec.rb b/spec/features/merge_requests/merge_when_pipeline_succeeds_spec.rb index cd540ca113a..9367789c7d4 100644 --- a/spec/features/merge_requests/merge_when_pipeline_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_pipeline_succeeds_spec.rb @@ -16,7 +16,10 @@ feature 'Merge When Pipeline Succeeds', :feature, :js do ref: merge_request.source_branch) end - before { project.team << [user, :master] } + before do + project.add_master(user) + merge_request.update(head_pipeline_id: pipeline.id) + end context 'when there is active pipeline for merge request' do background do diff --git a/spec/features/merge_requests/mini_pipeline_graph_spec.rb b/spec/features/merge_requests/mini_pipeline_graph_spec.rb index 449a60c1d05..5b2798af32f 100644 --- a/spec/features/merge_requests/mini_pipeline_graph_spec.rb +++ b/spec/features/merge_requests/mini_pipeline_graph_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' feature 'Mini Pipeline Graph', :js, :feature do let(:user) { create(:user) } let(:project) { create(:project, :public) } - let(:merge_request) { create(:merge_request, source_project: project) } + let(:merge_request) { create(:merge_request, source_project: project, head_pipeline: pipeline) } let(:pipeline) { create(:ci_empty_pipeline, project: project, ref: 'master', status: 'running', sha: project.commit.id) } let(:build) { create(:ci_build, pipeline: pipeline, stage: 'test', commands: 'test') } diff --git a/spec/features/merge_requests/only_allow_merge_if_build_succeeds_spec.rb b/spec/features/merge_requests/only_allow_merge_if_build_succeeds_spec.rb index 4a590e3bf68..981989b0095 100644 --- a/spec/features/merge_requests/only_allow_merge_if_build_succeeds_spec.rb +++ b/spec/features/merge_requests/only_allow_merge_if_build_succeeds_spec.rb @@ -27,6 +27,8 @@ feature 'Only allow merge requests to be merged if the pipeline succeeds', featu status: status) end + before { merge_request.update(head_pipeline: pipeline) } + context 'when merge requests can only be merged if the pipeline succeeds' do before do project.update_attribute(:only_allow_merge_if_pipeline_succeeds, true) diff --git a/spec/features/merge_requests/widget_spec.rb b/spec/features/merge_requests/widget_spec.rb index d918181a238..02193dbfc8d 100644 --- a/spec/features/merge_requests/widget_spec.rb +++ b/spec/features/merge_requests/widget_spec.rb @@ -89,6 +89,8 @@ describe 'Merge request', :feature, :js do statuses: [commit_status]) create(:ci_build, :pending, pipeline: pipeline) + merge_request.update(head_pipeline_id: pipeline.id) + visit namespace_project_merge_request_path(project.namespace, project, merge_request) end @@ -101,10 +103,15 @@ describe 'Merge request', :feature, :js do context 'when merge request is in the blocked pipeline state' do before do - create(:ci_pipeline, project: project, - sha: merge_request.diff_head_sha, - ref: merge_request.source_branch, - status: :manual) + pipeline = create( + :ci_pipeline, + project: project, + sha: merge_request.diff_head_sha, + ref: merge_request.source_branch, + status: :manual + ) + + merge_request.update(head_pipeline_id: pipeline.id) visit namespace_project_merge_request_path(project.namespace, project, @@ -129,6 +136,8 @@ describe 'Merge request', :feature, :js do statuses: [commit_status]) create(:ci_build, :pending, pipeline: pipeline) + merge_request.update(head_pipeline_id: pipeline.id) + visit namespace_project_merge_request_path(project.namespace, project, merge_request) end diff --git a/spec/lib/gitlab/cycle_analytics/events_spec.rb b/spec/lib/gitlab/cycle_analytics/events_spec.rb index 0d56bdd0ebd..8753679e233 100644 --- a/spec/lib/gitlab/cycle_analytics/events_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/events_spec.rb @@ -130,6 +130,8 @@ describe 'cycle analytics events' do end before do + merge_request.update(head_pipeline_id: pipeline.id) + create(:ci_build, pipeline: pipeline, status: :success, author: user) create(:ci_build, pipeline: pipeline, status: :success, author: user) @@ -226,6 +228,8 @@ describe 'cycle analytics events' do end before do + merge_request.update(head_pipeline_id: pipeline.id) + create(:ci_build, pipeline: pipeline, status: :success, author: user) create(:ci_build, pipeline: pipeline, status: :success, author: user) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 688e731bf15..63797c0fc0e 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -85,6 +85,7 @@ merge_requests: - merge_requests_closing_issues - metrics - timelogs +- head_pipeline merge_request_diff: - merge_request pipelines: diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 208c8cb1c3d..86ed37b50aa 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1044,8 +1044,8 @@ describe Ci::Pipeline, models: true do let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: 'a288a022a53a5a944fae87bcec6efc87b7061808') } it "returns merge requests whose `diff_head_sha` matches the pipeline's SHA" do - merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { 'a288a022a53a5a944fae87bcec6efc87b7061808' } + merge_request = create(:merge_request, source_project: project, head_pipeline: pipeline, , source_branch: pipeline.ref) expect(pipeline.merge_requests).to eq([merge_request]) end diff --git a/spec/models/cycle_analytics/test_spec.rb b/spec/models/cycle_analytics/test_spec.rb index c2ba012a0e6..d0b919efcf9 100644 --- a/spec/models/cycle_analytics/test_spec.rb +++ b/spec/models/cycle_analytics/test_spec.rb @@ -14,6 +14,7 @@ describe 'CycleAnalytics#test', feature: true do issue = context.create(:issue, project: context.project) merge_request = context.create_merge_request_closing_issue(issue) pipeline = context.create(:ci_pipeline, ref: merge_request.source_branch, sha: merge_request.diff_head_sha, project: context.project) + merge_request.update(head_pipeline: pipeline) { pipeline: pipeline, issue: issue } end, start_time_conditions: [["pipeline is started", -> (context, data) { data[:pipeline].run! }]], diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 6cf3dd30ead..45a5cd049a2 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -760,13 +760,9 @@ describe MergeRequest, models: true do describe '#head_pipeline' do describe 'when the source project exists' do it 'returns the latest pipeline' do - pipeline = double(:ci_pipeline, ref: 'master') - - allow(subject).to receive(:diff_head_sha).and_return('123abc') - - expect(subject.source_project).to receive(:pipeline_for). - with('master', '123abc'). - and_return(pipeline) + sha = "123abc" + pipeline = create(:ci_empty_pipeline, project: subject.source_project, ref: 'master', status: 'running', sha: sha) + subject.update(head_pipeline: pipeline) expect(subject.head_pipeline).to eq(pipeline) end @@ -1504,11 +1500,13 @@ describe MergeRequest, models: true do describe '#mergeable_with_slash_command?' do def create_pipeline(status) - create(:ci_pipeline_with_one_job, + pipeline = create(:ci_pipeline_with_one_job, project: project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha, status: status) + + merge_request.update(head_pipeline: pipeline) end let(:project) { create(:project, :public, :repository, only_allow_merge_if_pipeline_succeeds: true) } @@ -1594,7 +1592,7 @@ describe MergeRequest, models: true do context 'with running pipeline' do before do - create_pipeline(:running) + merge_request.update(head_pipeline: create_pipeline(:running)) end it 'is mergeable' do diff --git a/spec/requests/projects/cycle_analytics_events_spec.rb b/spec/requests/projects/cycle_analytics_events_spec.rb index c5a45949841..d92daa345b3 100644 --- a/spec/requests/projects/cycle_analytics_events_spec.rb +++ b/spec/requests/projects/cycle_analytics_events_spec.rb @@ -122,6 +122,7 @@ describe 'cycle analytics events', api: true do mr = create_merge_request_closing_issue(issue, commit_message: "References #{issue.to_reference}") pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha) + mr.update(head_pipeline_id: pipeline.id) pipeline.run create(:ci_build, pipeline: pipeline, status: :success, author: user) diff --git a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb index 769b3193275..b0304b3b73f 100644 --- a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb @@ -82,6 +82,10 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do sha: merge_request_head, status: 'success') end + before do + mr_merge_if_green_enabled.update(head_pipeline: triggering_pipeline) + end + it "merges all merge requests with merge when the pipeline succeeds enabled" do expect(MergeWorker).to receive(:perform_async) service.trigger(triggering_pipeline) @@ -124,6 +128,8 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do sha: mr_conflict.diff_head_sha, status: 'success') end + before { mr_conflict.update(head_pipeline_id: conflict_pipeline.id) } + it 'does not merge the merge request' do expect(MergeWorker).not_to receive(:perform_async) diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 31487c0f794..e5071c2f447 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -174,11 +174,13 @@ describe MergeRequests::UpdateService, services: true do context 'with active pipeline' do before do service_mock = double - create(:ci_pipeline_with_one_job, + pipeline = create(:ci_pipeline_with_one_job, project: project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha) + merge_request.update(head_pipeline_id: pipeline.id) + expect(MergeRequests::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user). and_return(service_mock) expect(service_mock).to receive(:execute).with(merge_request) -- cgit v1.2.1 From 1bf2dacf2002fabf3b7bd364031d9020e5d0b624 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Mon, 27 Mar 2017 19:19:28 -0300 Subject: Populate merge requests head_pipeline_id --- ...547_add_head_pipeline_for_each_merge_request.rb | 23 ++++++++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb diff --git a/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb b/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb new file mode 100644 index 00000000000..0e139c28402 --- /dev/null +++ b/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb @@ -0,0 +1,23 @@ +class AddHeadPipelineForEachMergeRequest < ActiveRecord::Migration + DOWNTIME = false + + class Pipeline < ActiveRecord::Base + self.table_name = "ci_pipelines" + + def self.last_per_branch + select('ref, MAX(id) as head_id, project_id').group(:ref).group(:project_id) + end + end + + class MergeRequest < ActiveRecord::Base; end + + def up + Pipeline.last_per_branch.each do |pipeline| + mrs = MergeRequest.where(source_branch: pipeline.ref, source_project_id: pipeline.project_id) + mrs.update_all(head_pipeline_id: pipeline.head_id) + end + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index d5b1d3bb0d8..d0e10f00ffd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -114,6 +114,7 @@ ActiveRecord::Schema.define(version: 20170506185517) do t.string "plantuml_url" t.boolean "plantuml_enabled" t.integer "terminal_max_session_time", default: 0, null: false + t.string "default_artifacts_expire_in", default: "0", null: false t.integer "unique_ips_limit_per_user" t.integer "unique_ips_limit_time_window" t.boolean "unique_ips_limit_enabled", default: false, null: false @@ -123,7 +124,6 @@ ActiveRecord::Schema.define(version: 20170506185517) do t.integer "cached_markdown_version" t.boolean "clientside_sentry_enabled", default: false, null: false t.string "clientside_sentry_dsn" - t.string "default_artifacts_expire_in", default: "0", null: false end create_table "audit_events", force: :cascade do |t| -- cgit v1.2.1 From d9bebd89dfcf7e4b163b271eea3d7a5c3e99fb5d Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 28 Mar 2017 17:04:14 -0300 Subject: Fix specs 2 --- app/models/ci/pipeline.rb | 11 +++------ ...547_add_head_pipeline_for_each_merge_request.rb | 26 ++++++++++++---------- .../projects/merge_requests_controller_spec.rb | 5 ++++- spec/features/cycle_analytics_spec.rb | 13 ++++++----- spec/features/merge_requests/widget_spec.rb | 6 ++--- spec/lib/gitlab/cycle_analytics/events_spec.rb | 4 ++-- spec/models/ci/pipeline_spec.rb | 2 +- spec/models/merge_request_spec.rb | 4 +++- .../merge_when_pipeline_succeeds_service_spec.rb | 2 +- .../services/merge_requests/update_service_spec.rb | 2 +- spec/workers/pipeline_metrics_worker_spec.rb | 2 +- 11 files changed, 41 insertions(+), 36 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 481f9c8dac8..81c30b0e077 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -17,6 +17,9 @@ module Ci has_many :statuses, class_name: 'CommitStatus', foreign_key: :commit_id has_many :builds, foreign_key: :commit_id has_many :trigger_requests, dependent: :destroy, foreign_key: :commit_id + + # Merge requests for which the current pipeline is running against + # the merge request's latest commit. has_many :merge_requests, foreign_key: "head_pipeline_id" has_many :pending_builds, -> { pending }, foreign_key: :commit_id, class_name: 'Ci::Build' @@ -382,14 +385,6 @@ module Ci project.execute_services(data, :pipeline_hooks) end - # Merge requests for which the current pipeline is running against - # the merge request's latest commit. - def merge_requests - @merge_requests ||= project.merge_requests - .where(source_branch: self.ref) - .select { |merge_request| merge_request.head_pipeline.try(:id) == self.id } - end - # All the merge requests for which the current pipeline runs/ran against def all_merge_requests @all_merge_requests ||= project.merge_requests.where(source_branch: ref) diff --git a/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb b/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb index 0e139c28402..bc3850c0c23 100644 --- a/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb +++ b/db/post_migrate/20170428170547_add_head_pipeline_for_each_merge_request.rb @@ -1,21 +1,23 @@ class AddHeadPipelineForEachMergeRequest < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + DOWNTIME = false - class Pipeline < ActiveRecord::Base - self.table_name = "ci_pipelines" + def up + disable_statement_timeout - def self.last_per_branch - select('ref, MAX(id) as head_id, project_id').group(:ref).group(:project_id) - end - end + pipelines = Arel::Table.new(:ci_pipelines) + merge_requests = Arel::Table.new(:merge_requests) - class MergeRequest < ActiveRecord::Base; end + head_id = pipelines. + project(Arel::Nodes::NamedFunction.new('max', [pipelines[:id]])). + from(pipelines). + where(pipelines[:ref].eq(merge_requests[:source_branch])). + where(pipelines[:project_id].eq(merge_requests[:source_project_id])) - def up - Pipeline.last_per_branch.each do |pipeline| - mrs = MergeRequest.where(source_branch: pipeline.ref, source_project_id: pipeline.project_id) - mrs.update_all(head_pipeline_id: pipeline.head_id) - end + sub_query = Arel::Nodes::SqlLiteral.new(Arel::Nodes::Grouping.new(head_id).to_sql) + + update_column_in_batches(:merge_requests, :head_pipeline_id, sub_query) end def down diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index bc13a8123a5..d9dae999a87 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -1200,7 +1200,10 @@ describe Projects::MergeRequestsController do let(:status) { pipeline.detailed_status(double('user')) } - before { get_pipeline_status } + before do + merge_request.update(head_pipeline: pipeline) + get_pipeline_status + end it 'return a detailed head_pipeline status in json' do expect(response).to have_http_status(:ok) diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index 57826fe872b..5553687707d 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -9,14 +9,17 @@ feature 'Cycle Analytics', feature: true, js: true do let(:mr) { create_merge_request_closing_issue(issue, commit_message: "References #{issue.to_reference}") } let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha) } - before { mr.update(head_pipeline: pipeline) } + before do + allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) + end context 'as an allowed user' do context 'when project is new' do before do project.add_master(user) - mr.update(head_pipeline_id: pipeline.id) + login_as(user) + visit namespace_project_cycle_analytics_path(project.namespace, project) wait_for_ajax end @@ -33,8 +36,8 @@ feature 'Cycle Analytics', feature: true, js: true do context "when there's cycle analytics data" do before do - project.team << [user, :master] - + mr.update(head_pipeline: pipeline) + project.add_master(user) create_cycle deploy_master @@ -87,7 +90,7 @@ feature 'Cycle Analytics', feature: true, js: true do context "as a guest" do before do - project.team << [guest, :guest] + project.add_guest(guest) allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) create_cycle diff --git a/spec/features/merge_requests/widget_spec.rb b/spec/features/merge_requests/widget_spec.rb index 02193dbfc8d..b1255ac0bd5 100644 --- a/spec/features/merge_requests/widget_spec.rb +++ b/spec/features/merge_requests/widget_spec.rb @@ -89,7 +89,7 @@ describe 'Merge request', :feature, :js do statuses: [commit_status]) create(:ci_build, :pending, pipeline: pipeline) - merge_request.update(head_pipeline_id: pipeline.id) + merge_request.update(head_pipeline: pipeline) visit namespace_project_merge_request_path(project.namespace, project, merge_request) end @@ -111,7 +111,7 @@ describe 'Merge request', :feature, :js do status: :manual ) - merge_request.update(head_pipeline_id: pipeline.id) + merge_request.update(head_pipeline: pipeline) visit namespace_project_merge_request_path(project.namespace, project, @@ -136,7 +136,7 @@ describe 'Merge request', :feature, :js do statuses: [commit_status]) create(:ci_build, :pending, pipeline: pipeline) - merge_request.update(head_pipeline_id: pipeline.id) + merge_request.update(head_pipeline: pipeline) visit namespace_project_merge_request_path(project.namespace, project, merge_request) end diff --git a/spec/lib/gitlab/cycle_analytics/events_spec.rb b/spec/lib/gitlab/cycle_analytics/events_spec.rb index 8753679e233..3610a0354e8 100644 --- a/spec/lib/gitlab/cycle_analytics/events_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/events_spec.rb @@ -130,7 +130,7 @@ describe 'cycle analytics events' do end before do - merge_request.update(head_pipeline_id: pipeline.id) + merge_request.update(head_pipeline: pipeline) create(:ci_build, pipeline: pipeline, status: :success, author: user) create(:ci_build, pipeline: pipeline, status: :success, author: user) @@ -228,7 +228,7 @@ describe 'cycle analytics events' do end before do - merge_request.update(head_pipeline_id: pipeline.id) + merge_request.update(head_pipeline: pipeline) create(:ci_build, pipeline: pipeline, status: :success, author: user) create(:ci_build, pipeline: pipeline, status: :success, author: user) diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 86ed37b50aa..06e990a0574 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1045,7 +1045,7 @@ describe Ci::Pipeline, models: true do it "returns merge requests whose `diff_head_sha` matches the pipeline's SHA" do allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { 'a288a022a53a5a944fae87bcec6efc87b7061808' } - merge_request = create(:merge_request, source_project: project, head_pipeline: pipeline, , source_branch: pipeline.ref) + merge_request = create(:merge_request, source_project: project, head_pipeline: pipeline, source_branch: pipeline.ref) expect(pipeline.merge_requests).to eq([merge_request]) end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 45a5cd049a2..1db1640f5e5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1507,6 +1507,8 @@ describe MergeRequest, models: true do status: status) merge_request.update(head_pipeline: pipeline) + + pipeline end let(:project) { create(:project, :public, :repository, only_allow_merge_if_pipeline_succeeds: true) } @@ -1592,7 +1594,7 @@ describe MergeRequest, models: true do context 'with running pipeline' do before do - merge_request.update(head_pipeline: create_pipeline(:running)) + create_pipeline(:running) end it 'is mergeable' do diff --git a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb index b0304b3b73f..3ef5135e6a3 100644 --- a/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb +++ b/spec/services/merge_requests/merge_when_pipeline_succeeds_service_spec.rb @@ -128,7 +128,7 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do sha: mr_conflict.diff_head_sha, status: 'success') end - before { mr_conflict.update(head_pipeline_id: conflict_pipeline.id) } + before { mr_conflict.update(head_pipeline: conflict_pipeline) } it 'does not merge the merge request' do expect(MergeWorker).not_to receive(:perform_async) diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index e5071c2f447..07f5440cc36 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -179,7 +179,7 @@ describe MergeRequests::UpdateService, services: true do ref: merge_request.source_branch, sha: merge_request.diff_head_sha) - merge_request.update(head_pipeline_id: pipeline.id) + merge_request.update(head_pipeline: pipeline) expect(MergeRequests::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user). and_return(service_mock) diff --git a/spec/workers/pipeline_metrics_worker_spec.rb b/spec/workers/pipeline_metrics_worker_spec.rb index dbf2b7251c6..ef71125c0b6 100644 --- a/spec/workers/pipeline_metrics_worker_spec.rb +++ b/spec/workers/pipeline_metrics_worker_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe PipelineMetricsWorker do let(:project) { create(:project, :repository) } - let!(:merge_request) { create(:merge_request, source_project: project, source_branch: pipeline.ref, head_pipeline_id: pipeline.id) } + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: pipeline.ref, head_pipeline: pipeline) } let(:pipeline) do create(:ci_empty_pipeline, -- cgit v1.2.1 From 52a6db10f727be9af95af4ff8b443f76d771f9c5 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 8 May 2017 15:53:54 +0100 Subject: Adds left connector class to the rendered graph --- .../pipelines/components/graph/graph_component.vue | 24 ++++++++- .../components/graph/stage_column_component.vue | 23 +++++++- .../javascripts/vue_shared/ci_action_icons.js | 4 ++ .../pipelines/graph/graph_component_spec.js | 62 ++++++++++++++++++++++ .../javascripts/vue_shared/ci_action_icons_spec.js | 5 ++ 5 files changed, 114 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/pipelines/components/graph/graph_component.vue b/app/assets/javascripts/pipelines/components/graph/graph_component.vue index a84161ef5e7..1f1b99ff401 100644 --- a/app/assets/javascripts/pipelines/components/graph/graph_component.vue +++ b/app/assets/javascripts/pipelines/components/graph/graph_component.vue @@ -64,6 +64,24 @@ capitalizeStageName(name) { return name.charAt(0).toUpperCase() + name.slice(1); }, + + isFirstColumn(index) { + return index === 0; + }, + + stageConnectorClass(index, stage) { + let className; + + // If it's the first stage column and only has one job + if (index === 0 && stage.groups.length === 1) { + className = 'no-margin'; + } else if (index > 0) { + // If it is not the first column + className = 'left-margin'; + } + + return className; + }, }, }; @@ -82,10 +100,12 @@ v-if="!isLoading" class="stage-column-list"> + :key="stage.name" + :stage-connector-class="stageConnectorClass(index, stage)" + :is-first-column="isFirstColumn(index)"/> diff --git a/app/assets/javascripts/pipelines/components/graph/stage_column_component.vue b/app/assets/javascripts/pipelines/components/graph/stage_column_component.vue index b7da185e280..9b1bbb0906f 100644 --- a/app/assets/javascripts/pipelines/components/graph/stage_column_component.vue +++ b/app/assets/javascripts/pipelines/components/graph/stage_column_component.vue @@ -13,6 +13,18 @@ export default { type: Array, required: true, }, + + isFirstColumn: { + type: Boolean, + required: false, + default: false, + }, + + stageConnectorClass: { + type: String, + required: false, + default: '', + }, }, components: { @@ -28,20 +40,27 @@ export default { jobId(job) { return `ci-badge-${job.name}`; }, + + buildConnnectorClass(index) { + return index === 0 && !this.isFirstColumn ? 'left-connector' : ''; + }, }, };