From 9f933953896d4a1ca7ee40ce3fef4ead4b73ab65 Mon Sep 17 00:00:00 2001
From: "Z.J. van de Weg" <git@zjvandeweg.nl>
Date: Wed, 10 May 2017 10:04:25 +0200
Subject: [PATCH] Do not schedule pipelines if the user can't
When the owner of a pipelines schedule was either blocked or was removed
from the project, the pipeline schedular would still schedule the
pipeline.
This would than fail however, given the user had no access to the
project and it contents. However, a better way to handle it would be to
not schedule it at all. Furthermore, from now on, such schedules will be
deactivated so the schedule worker can ignore it on the next runs.
---
app/models/ci/pipeline_schedule.rb | 8 +++
app/workers/pipeline_schedule_worker.rb | 8 ++-
.../unreleased/zj-pipeline-schedule-owner.yml | 4 ++
doc/ci/pipeline_schedules.md | 4 ++
spec/workers/pipeline_schedule_worker_spec.rb | 51 ++++++++++++-------
5 files changed, 55 insertions(+), 20 deletions(-)
create mode 100644 changelogs/unreleased/zj-pipeline-schedule-owner.yml
diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb
index 6d7cc83971e..cf6e53c4ca4 100644
--- a/app/models/ci/pipeline_schedule.rb
+++ b/app/models/ci/pipeline_schedule.rb
@@ -28,10 +28,18 @@ module Ci
!active?
end
+ def deactivate!
+ update_attribute(:active, false)
+ end
+
def importing_or_inactive?
importing? || inactive?
end
+ def runnable_by_owner?
+ Ability.allowed?(owner, :create_pipeline, project)
+ end
+
def set_next_run_at
self.next_run_at = Gitlab::Ci::CronParser.new(cron, cron_timezone).next_time_from(Time.now)
end
diff --git a/app/workers/pipeline_schedule_worker.rb b/app/workers/pipeline_schedule_worker.rb
index a449a765f7b..7eb0e84acb2 100644
--- a/app/workers/pipeline_schedule_worker.rb
+++ b/app/workers/pipeline_schedule_worker.rb
@@ -3,8 +3,14 @@ class PipelineScheduleWorker
include CronjobQueue
def perform
- Ci::PipelineSchedule.active.where("next_run_at < ?", Time.now).find_each do |schedule|
+ Ci::PipelineSchedule.active.where("next_run_at < ?", Time.now)
+ .preload(:owner, :project).find_each do |schedule|
begin
+ unless schedule.runnable_by_owner?
+ schedule.deactivate!
+ next
+ end
+
Ci::CreatePipelineService.new(schedule.project,
schedule.owner,
ref: schedule.ref)
diff --git a/changelogs/unreleased/zj-pipeline-schedule-owner.yml b/changelogs/unreleased/zj-pipeline-schedule-owner.yml
new file mode 100644
index 00000000000..be704e173ab
--- /dev/null
+++ b/changelogs/unreleased/zj-pipeline-schedule-owner.yml
@@ -0,0 +1,4 @@
+---
+title: Add foreign key for pipeline schedule owner
+merge_request: 11233
+author:
diff --git a/doc/ci/pipeline_schedules.md b/doc/ci/pipeline_schedules.md
index 0a9b0e7173f..73451da6c0c 100644
--- a/doc/ci/pipeline_schedules.md
+++ b/doc/ci/pipeline_schedules.md
@@ -35,6 +35,10 @@ To change the Sidekiq worker's frequency, you have to edit the `trigger_schedule
value in your `gitlab.rb` and restart GitLab. The Sidekiq worker's configuration
on GiLab.com is able to be looked up at [here](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/config/gitlab.yml.example#L185).
- Cron notation is parsed by [Rufus-Scheduler](https://github.com/jmettraux/rufus-scheduler).
+- When the owner of the schedule does not have the ability to create pipelines
+anymore, due to e.g. being blocked or removed from the project, the schedule is
+deactivated. Another user can take ownership and activate it, so the schedule is
+run again.
[ce-10533]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10533
[ce-10853]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10853
diff --git a/spec/workers/pipeline_schedule_worker_spec.rb b/spec/workers/pipeline_schedule_worker_spec.rb
index 91d5a16993f..9c650354d72 100644
--- a/spec/workers/pipeline_schedule_worker_spec.rb
+++ b/spec/workers/pipeline_schedule_worker_spec.rb
@@ -11,40 +11,53 @@ describe PipelineScheduleWorker do
end
before do
- project.add_master(user)
-
stub_ci_pipeline_to_return_yaml_file
- end
- context 'when there is a scheduled pipeline within next_run_at' do
- let(:next_run_at) { 2.days.ago }
+ pipeline_schedule.update_column(:next_run_at, 1.day.ago)
+ end
+ context 'when the schedule is runnable by the user' do
before do
- pipeline_schedule.update_column(:next_run_at, next_run_at)
+ project.add_master(user)
end
- it 'creates a new pipeline' do
- expect { subject }.to change { project.pipelines.count }.by(1)
- end
+ context 'when there is a scheduled pipeline within next_run_at' do
+ it 'creates a new pipeline' do
+ expect { subject }.to change { project.pipelines.count }.by(1)
+ end
- it 'updates the next_run_at field' do
- subject
+ it 'updates the next_run_at field' do
+ subject
+
+ expect(pipeline_schedule.reload.next_run_at).to be > Time.now
+ end
- expect(pipeline_schedule.reload.next_run_at).to be > Time.now
+ it 'sets the schedule on the pipeline' do
+ subject
+
+ expect(project.pipelines.last.pipeline_schedule).to eq(pipeline_schedule)
+ end
end
- it 'sets the schedule on the pipeline' do
- subject
- expect(project.pipelines.last.pipeline_schedule).to eq(pipeline_schedule)
+ context 'inactive schedule' do
+ before do
+ pipeline_schedule.deactivate!
+ end
+
+ it 'does not creates a new pipeline' do
+ expect { subject }.not_to change { project.pipelines.count }
+ end
end
end
- context 'inactive schedule' do
- before do
- pipeline_schedule.update(active: false)
+ context 'when the schedule is not runnable by the user' do
+ it 'deactivates the schedule' do
+ subject
+
+ expect(pipeline_schedule.reload.active).to be_falsy
end
- it 'does not creates a new pipeline' do
+ it 'does not schedule a pipeline' do
expect { subject }.not_to change { project.pipelines.count }
end
end
--
GitLab