From c623c41c2f917e0773a6e3f0b6c78b027ca846f7 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Thu, 20 Apr 2017 08:44:01 +0200 Subject: [PATCH] Use a better performing query to find all MRs for pipeline And add some specs. --- app/models/ci/pipeline.rb | 12 ++++------ app/workers/expire_pipeline_cache_worker.rb | 4 ++-- .../tc-realtime-every-pipeline-on-mr.yml | 4 ++++ spec/models/ci/pipeline_spec.rb | 24 +++++++++++++++++++ .../expire_pipeline_cache_worker_spec.rb | 14 ++++++++++- 5 files changed, 48 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/tc-realtime-every-pipeline-on-mr.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index bd5b0ed3cff..b5fe589706f 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -80,22 +80,22 @@ module Ci end after_transition [:created, :pending] => :running do |pipeline| - pipeline.run_after_commit { PipelineMetricsWorker.perform_async(id) } + pipeline.run_after_commit { PipelineMetricsWorker.perform_async(pipeline.id) } end after_transition any => [:success] do |pipeline| - pipeline.run_after_commit { PipelineMetricsWorker.perform_async(id) } + pipeline.run_after_commit { PipelineMetricsWorker.perform_async(pipeline.id) } end after_transition [:created, :pending, :running] => :success do |pipeline| - pipeline.run_after_commit { PipelineSuccessWorker.perform_async(id) } + pipeline.run_after_commit { PipelineSuccessWorker.perform_async(pipeline.id) } end after_transition do |pipeline, transition| next if transition.loopback? pipeline.run_after_commit do - PipelineHooksWorker.perform_async(id) + PipelineHooksWorker.perform_async(pipeline.id) ExpirePipelineCacheWorker.perform_async(pipeline.id) end end @@ -386,9 +386,7 @@ module Ci # 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) - .select { |merge_request| merge_request.all_pipelines.includes(self) } + @all_merge_requests ||= project.merge_requests.where(source_branch: ref) end def detailed_status(current_user) diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb index 65e7b091506..0210d459048 100644 --- a/app/workers/expire_pipeline_cache_worker.rb +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -2,8 +2,8 @@ class ExpirePipelineCacheWorker include Sidekiq::Worker include PipelineQueue - def perform(id) - pipeline = Ci::Pipeline.find(id) + def perform(pipeline_id) + pipeline = Ci::Pipeline.find(pipeline_id) project = pipeline.project store = Gitlab::EtagCaching::Store.new diff --git a/changelogs/unreleased/tc-realtime-every-pipeline-on-mr.yml b/changelogs/unreleased/tc-realtime-every-pipeline-on-mr.yml new file mode 100644 index 00000000000..944baae257c --- /dev/null +++ b/changelogs/unreleased/tc-realtime-every-pipeline-on-mr.yml @@ -0,0 +1,4 @@ +--- +title: Properly expire cache for all MRs of a pipeline +merge_request: 10770 +author: diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 42c0791fba1..be14fdca81c 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1037,6 +1037,30 @@ describe Ci::Pipeline, models: true do end end + describe "#all_merge_requests" do + let(:project) { create(:project, :repository) } + let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) } + + 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) + + expect(pipeline.all_merge_requests).to eq([merge_request]) + end + + it "returns merge requests whose source branch matches the pipeline's source branch" do + pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master^').id) + merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) + + expect(pipeline.all_merge_requests).to eq([merge_request]) + end + + it "doesn't return merge requests whose source branch doesn't match the pipeline's ref" do + create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master') + + expect(pipeline.all_merge_requests).to be_empty + end + end + describe '#stuck?' do before do create(:ci_build, :pending, pipeline: pipeline) diff --git a/spec/workers/expire_pipeline_cache_worker_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb index 0138bfa4359..67f01539ada 100644 --- a/spec/workers/expire_pipeline_cache_worker_spec.rb +++ b/spec/workers/expire_pipeline_cache_worker_spec.rb @@ -7,7 +7,7 @@ describe ExpirePipelineCacheWorker do subject { described_class.new } describe '#perform' do - it 'invalidate Etag caching for project pipelines path' do + it 'invalidates Etag caching for project pipelines path' do pipelines_path = "/#{project.full_path}/pipelines.json" new_mr_pipelines_path = "/#{project.full_path}/merge_requests/new.json" @@ -17,6 +17,18 @@ describe ExpirePipelineCacheWorker do subject.perform(pipeline.id) end + it 'invalidates Etag caching for merge request pipelines if pipeline runs on any commit of that source branch' do + project = create(:project, :repository) + pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master^').id) + merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref) + merge_request_pipelines_path = "/#{project.full_path}/merge_requests/#{merge_request.iid}/pipelines.json" + + allow_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch) + expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(merge_request_pipelines_path) + + subject.perform(pipeline.id) + end + it 'updates the cached status for a project' do expect(Gitlab::Cache::Ci::ProjectPipelineStatus).to receive(:update_for_pipeline). with(pipeline)