diff options
25 files changed, 164 insertions, 59 deletions
diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 018f61ca3a8..5b62d7fa3a7 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -83,4 +83,8 @@ position: fixed; top: $header-height; } + + &:not(.affix-top) { + min-height: 100%; + } } diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index ffafc678968..fe63728ea23 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -89,6 +89,7 @@ class CommitStatus < ActiveRecord::Base else PipelineUpdateWorker.perform_async(pipeline.id) end + ExpireJobCacheWorker.perform_async(commit_status.id) end end end diff --git a/app/views/profiles/preferences/show.html.haml b/app/views/profiles/preferences/show.html.haml index 99690e6b98a..0ff19b3eab1 100644 --- a/app/views/profiles/preferences/show.html.haml +++ b/app/views/profiles/preferences/show.html.haml @@ -37,9 +37,9 @@ = f.select :dashboard, dashboard_choices, {}, class: 'form-control' .form-group = f.label :project_view, class: 'label-light' do - Project view + Project home page content = f.select :project_view, project_view_choices, {}, class: 'form-control' .help-block - Choose what content you want to see on a project's home page. + Choose what content you want to see on a project’s home page .form-group = f.submit 'Save changes', class: 'btn btn-save' diff --git a/app/workers/expire_job_cache_worker.rb b/app/workers/expire_job_cache_worker.rb new file mode 100644 index 00000000000..08e281e7350 --- /dev/null +++ b/app/workers/expire_job_cache_worker.rb @@ -0,0 +1,35 @@ +class ExpireJobCacheWorker + include Sidekiq::Worker + include BuildQueue + + def perform(job_id) + job = CommitStatus.joins(:pipeline, :project).find_by(id: job_id) + return unless job + + pipeline = job.pipeline + project = job.project + + Gitlab::EtagCaching::Store.new.tap do |store| + store.touch(project_pipeline_path(project, pipeline)) + store.touch(project_job_path(project, job)) + end + end + + private + + def project_pipeline_path(project, pipeline) + Gitlab::Routing.url_helpers.namespace_project_pipeline_path( + project.namespace, + project, + pipeline, + format: :json) + end + + def project_job_path(project, job) + Gitlab::Routing.url_helpers.namespace_project_build_path( + project.namespace, + project, + job.id, + format: :json) + end +end diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb index 603e2f1aaea..d760f5b140f 100644 --- a/app/workers/expire_pipeline_cache_worker.rb +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -10,6 +10,7 @@ class ExpirePipelineCacheWorker store = Gitlab::EtagCaching::Store.new store.touch(project_pipelines_path(project)) + store.touch(project_pipeline_path(project, pipeline)) store.touch(commit_pipelines_path(project, pipeline.commit)) if pipeline.commit store.touch(new_merge_request_pipelines_path(project)) each_pipelines_merge_request_path(project, pipeline) do |path| @@ -28,6 +29,14 @@ class ExpirePipelineCacheWorker format: :json) end + def project_pipeline_path(project, pipeline) + Gitlab::Routing.url_helpers.namespace_project_pipeline_path( + project.namespace, + project, + pipeline, + format: :json) + end + def commit_pipelines_path(project, commit) Gitlab::Routing.url_helpers.pipelines_namespace_project_commit_path( project.namespace, diff --git a/changelogs/unreleased/zj-fix-pipeline-etag.yml b/changelogs/unreleased/zj-fix-pipeline-etag.yml new file mode 100644 index 00000000000..03ebef8c575 --- /dev/null +++ b/changelogs/unreleased/zj-fix-pipeline-etag.yml @@ -0,0 +1,4 @@ +--- +title: Fix issue where real time pipelines were not cached +merge_request: 11615 +author: diff --git a/doc/user/profile/preferences.md b/doc/user/profile/preferences.md index e5038835027..f2ad42f21fd 100644 --- a/doc/user/profile/preferences.md +++ b/doc/user/profile/preferences.md @@ -50,15 +50,15 @@ You have 6 options here that you can use for your default dashboard view: - Your groups - Your [Todos] -### Default project view +### Project home page content -The default project view settings allows you to choose what content you want to -see on a project's landing page. +The project home page content setting allows you to choose what content you want to +see on a project’s home page. You can choose between 2 options: - Show the files and the readme (default) -- Show the project's activity +- Show the project’s activity [rouge]: http://rouge.jneen.net/ "Rouge website" [todos]: ../../workflow/todos.md diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb index 8133760e619..caf36164891 100644 --- a/features/steps/project/merge_requests.rb +++ b/features/steps/project/merge_requests.rb @@ -556,8 +556,14 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps step '"Bug NS-05" has CI status' do 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) + + pipeline = + create(:ci_pipeline, + project: project, + sha: merge_request.diff_head_sha, + ref: merge_request.source_branch, + head_pipeline_of: merge_request) + create :ci_build, pipeline: pipeline end diff --git a/lib/gitlab/etag_caching/router.rb b/lib/gitlab/etag_caching/router.rb index ba31041d0c1..bdf885559c5 100644 --- a/lib/gitlab/etag_caching/router.rb +++ b/lib/gitlab/etag_caching/router.rb @@ -38,7 +38,7 @@ module Gitlab 'project_pipelines' ), Gitlab::EtagCaching::Router::Route.new( - %r(^(?!.*(#{RESERVED_WORDS})).*/pipelines/\d+\.json\z), + %r(^(?!.*(#{RESERVED_WORDS_REGEX})).*/pipelines/\d+\.json\z), 'project_pipeline' ) ].freeze diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 457e07334b9..587a5820c6f 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -357,8 +357,7 @@ describe Projects::MergeRequestsController do end before do - pipeline = create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch) - merge_request.update(head_pipeline: pipeline) + create(:ci_empty_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, head_pipeline_of: merge_request) end it 'returns :merge_when_pipeline_succeeds' do @@ -1173,13 +1172,13 @@ describe Projects::MergeRequestsController do let!(:pipeline) do create(:ci_pipeline, project: merge_request.source_project, ref: merge_request.source_branch, - sha: merge_request.diff_head_sha) + sha: merge_request.diff_head_sha, + head_pipeline_of: merge_request) end let(:status) { pipeline.detailed_status(double('user')) } before do - merge_request.update(head_pipeline: pipeline) get_pipeline_status end diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index 561fbc8e247..361c5b9a49e 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -20,6 +20,15 @@ FactoryGirl.define do end end + # Persist merge request head_pipeline_id + # on pipeline factories to avoid circular references + transient { head_pipeline_of nil } + + after(:create) do |pipeline, evaluator| + merge_request = evaluator.head_pipeline_of + merge_request&.update(head_pipeline: pipeline) + end + factory :ci_pipeline do transient { config nil } diff --git a/spec/features/cycle_analytics_spec.rb b/spec/features/cycle_analytics_spec.rb index cbeb73d9cae..1c829e91c20 100644 --- a/spec/features/cycle_analytics_spec.rb +++ b/spec/features/cycle_analytics_spec.rb @@ -7,7 +7,7 @@ feature 'Cycle Analytics', feature: true, js: true do let(:issue) { create(:issue, project: project, created_at: 2.days.ago) } let(:milestone) { create(:milestone, project: project) } 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) } + let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha, head_pipeline_of: mr) } context 'as an allowed user' do context 'when project is new' do @@ -33,7 +33,6 @@ feature 'Cycle Analytics', feature: true, js: true do context "when there's cycle analytics data" do before do allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) - mr.update(head_pipeline: pipeline) project.add_master(user) create_cycle 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 11b6f0c0a64..e08721b4724 100644 --- a/spec/features/merge_requests/merge_when_pipeline_succeeds_spec.rb +++ b/spec/features/merge_requests/merge_when_pipeline_succeeds_spec.rb @@ -13,12 +13,12 @@ feature 'Merge When Pipeline Succeeds', :feature, :js do let(:pipeline) do create(:ci_pipeline, project: project, sha: merge_request.diff_head_sha, - ref: merge_request.source_branch) + ref: merge_request.source_branch, + head_pipeline_of: merge_request) end 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 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 cdda0542c51..63a4f4d90b4 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 @@ -28,11 +28,9 @@ feature 'Only allow merge requests to be merged if the pipeline succeeds', featu project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, - status: status) + status: status, head_pipeline_of: merge_request) 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 ae799584c0f..9b182cc05b9 100644 --- a/spec/features/merge_requests/widget_spec.rb +++ b/spec/features/merge_requests/widget_spec.rb @@ -88,11 +88,10 @@ describe 'Merge request', :feature, :js do sha: merge_request.diff_head_sha, ref: merge_request.source_branch, status: 'failed', - statuses: [commit_status]) + statuses: [commit_status], + head_pipeline_of: merge_request) create(:ci_build, :pending, pipeline: pipeline) - merge_request.update(head_pipeline: pipeline) - visit namespace_project_merge_request_path(project.namespace, project, merge_request) end @@ -105,15 +104,13 @@ describe 'Merge request', :feature, :js do context 'when merge request is in the blocked pipeline state' do before do - pipeline = create( + create( :ci_pipeline, project: project, sha: merge_request.diff_head_sha, ref: merge_request.source_branch, - status: :manual - ) - - merge_request.update(head_pipeline: pipeline) + status: :manual, + head_pipeline_of: merge_request) visit namespace_project_merge_request_path(project.namespace, project, @@ -135,11 +132,10 @@ describe 'Merge request', :feature, :js do sha: merge_request.diff_head_sha, ref: merge_request.source_branch, status: 'pending', - statuses: [commit_status]) + statuses: [commit_status], + head_pipeline_of: merge_request) create(:ci_build, :pending, pipeline: pipeline) - 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 3610a0354e8..a1b3fe8509e 100644 --- a/spec/lib/gitlab/cycle_analytics/events_spec.rb +++ b/spec/lib/gitlab/cycle_analytics/events_spec.rb @@ -126,12 +126,11 @@ describe 'cycle analytics events' do create(:ci_pipeline, ref: merge_request.source_branch, sha: merge_request.diff_head_sha, - project: context.project) + project: context.project, + head_pipeline_of: merge_request) end before do - merge_request.update(head_pipeline: pipeline) - create(:ci_build, pipeline: pipeline, status: :success, author: user) create(:ci_build, pipeline: pipeline, status: :success, author: user) @@ -224,12 +223,11 @@ describe 'cycle analytics events' do create(:ci_pipeline, ref: merge_request.source_branch, sha: merge_request.diff_head_sha, - project: context.project) + project: context.project, + head_pipeline_of: merge_request) end before do - 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/lib/gitlab/etag_caching/router_spec.rb b/spec/lib/gitlab/etag_caching/router_spec.rb index 5ae4a19263c..46a238b17f4 100644 --- a/spec/lib/gitlab/etag_caching/router_spec.rb +++ b/spec/lib/gitlab/etag_caching/router_spec.rb @@ -77,6 +77,17 @@ describe Gitlab::EtagCaching::Router do expect(result).to be_blank end + it 'matches pipeline#show endpoint' do + env = build_env( + '/my-group/my-project/pipelines/2.json' + ) + + result = described_class.match(env) + + expect(result).to be_present + expect(result.name).to eq 'project_pipeline' + end + def build_env(path) { 'PATH_INFO' => path } end diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 6947affcc1e..c50b8bf7b13 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -36,6 +36,16 @@ describe CommitStatus, :models do it { is_expected.to eq(commit_status.user) } end + describe 'status state machine' do + let!(:commit_status) { create(:commit_status, :running, project: project) } + + it 'invalidates the cache after a transition' do + expect(ExpireJobCacheWorker).to receive(:perform_async).with(commit_status.id) + + commit_status.success! + end + end + describe '#started?' do subject { commit_status.started? } diff --git a/spec/models/cycle_analytics/test_spec.rb b/spec/models/cycle_analytics/test_spec.rb index d0b919efcf9..fd58bd1d6ad 100644 --- a/spec/models/cycle_analytics/test_spec.rb +++ b/spec/models/cycle_analytics/test_spec.rb @@ -13,8 +13,7 @@ describe 'CycleAnalytics#test', feature: true do data_fn: lambda do |context| 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 = context.create(:ci_pipeline, ref: merge_request.source_branch, sha: merge_request.diff_head_sha, project: context.project, head_pipeline_of: merge_request) { 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 ce870fcc1d3..0e05f719499 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -718,8 +718,7 @@ describe MergeRequest, models: true do describe '#head_pipeline' do describe 'when the source project exists' do it 'returns the latest pipeline' do - pipeline = create(:ci_empty_pipeline, project: subject.source_project, ref: 'master', status: 'running', sha: "123abc") - subject.update(head_pipeline: pipeline) + pipeline = create(:ci_empty_pipeline, project: subject.source_project, ref: 'master', status: 'running', sha: "123abc", head_pipeline_of: subject) expect(subject.head_pipeline).to eq(pipeline) end @@ -1396,9 +1395,8 @@ describe MergeRequest, models: true do project: project, ref: merge_request.source_branch, sha: merge_request.diff_head_sha, - status: status) - - merge_request.update(head_pipeline: pipeline) + status: status, + head_pipeline_of: merge_request) pipeline end diff --git a/spec/requests/projects/cycle_analytics_events_spec.rb b/spec/requests/projects/cycle_analytics_events_spec.rb index d92daa345b3..d4d3c9478a0 100644 --- a/spec/requests/projects/cycle_analytics_events_spec.rb +++ b/spec/requests/projects/cycle_analytics_events_spec.rb @@ -121,8 +121,7 @@ describe 'cycle analytics events', api: true do issue.update(milestone: milestone) 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 = create(:ci_empty_pipeline, status: 'created', project: project, ref: mr.source_branch, sha: mr.source_branch_sha, head_pipeline_of: mr) 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 3ef5135e6a3..f17db70faf6 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 @@ -79,11 +79,8 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do context 'when triggered by pipeline with valid ref and sha' do let(:triggering_pipeline) do create(:ci_pipeline, project: project, ref: merge_request_ref, - sha: merge_request_head, status: 'success') - end - - before do - mr_merge_if_green_enabled.update(head_pipeline: triggering_pipeline) + sha: merge_request_head, status: 'success', + head_pipeline_of: mr_merge_if_green_enabled) end it "merges all merge requests with merge when the pipeline succeeds enabled" do @@ -125,11 +122,10 @@ describe MergeRequests::MergeWhenPipelineSucceedsService do let(:conflict_pipeline) do create(:ci_pipeline, project: project, ref: mr_conflict.source_branch, - sha: mr_conflict.diff_head_sha, status: 'success') + sha: mr_conflict.diff_head_sha, status: 'success', + head_pipeline_of: mr_conflict) end - 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 860a7798857..d371fc68312 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -180,12 +180,13 @@ describe MergeRequests::UpdateService, services: true do context 'with active pipeline' do before do service_mock = double - pipeline = create(:ci_pipeline_with_one_job, + create( + :ci_pipeline_with_one_job, project: project, - ref: merge_request.source_branch, - sha: merge_request.diff_head_sha) - - merge_request.update(head_pipeline: pipeline) + ref: merge_request.source_branch, + sha: merge_request.diff_head_sha, + head_pipeline_of: merge_request + ) expect(MergeRequests::MergeWhenPipelineSucceedsService).to receive(:new).with(project, user). and_return(service_mock) diff --git a/spec/workers/expire_job_cache_worker_spec.rb b/spec/workers/expire_job_cache_worker_spec.rb new file mode 100644 index 00000000000..1b614342a18 --- /dev/null +++ b/spec/workers/expire_job_cache_worker_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe ExpireJobCacheWorker do + set(:pipeline) { create(:ci_empty_pipeline) } + let(:project) { pipeline.project } + subject { described_class.new } + + describe '#perform' do + context 'with a job in the pipeline' do + let(:job) { create(:ci_build, pipeline: pipeline) } + + it 'invalidates Etag caching for the job path' do + pipeline_path = "/#{project.full_path}/pipelines/#{pipeline.id}.json" + job_path = "/#{project.full_path}/builds/#{job.id}.json" + + expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipeline_path) + expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(job_path) + + subject.perform(job.id) + end + end + + context 'when there is no job in the pipeline' do + it 'does not change the etag store' do + expect(Gitlab::EtagCaching::Store).not_to receive(:new) + + subject.perform(9999) + end + end + end +end diff --git a/spec/workers/expire_pipeline_cache_worker_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb index ceba604dea2..28e5b706803 100644 --- a/spec/workers/expire_pipeline_cache_worker_spec.rb +++ b/spec/workers/expire_pipeline_cache_worker_spec.rb @@ -10,9 +10,11 @@ describe ExpirePipelineCacheWorker 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" + pipeline_path = "/#{project.full_path}/pipelines/#{pipeline.id}.json" expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipelines_path) expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(new_mr_pipelines_path) + expect_any_instance_of(Gitlab::EtagCaching::Store).to receive(:touch).with(pipeline_path) subject.perform(pipeline.id) end |