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.
This commit is contained in:
parent
e261b4b851
commit
9f93395389
|
@ -28,10 +28,18 @@ module Ci
|
||||||
!active?
|
!active?
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def deactivate!
|
||||||
|
update_attribute(:active, false)
|
||||||
|
end
|
||||||
|
|
||||||
def importing_or_inactive?
|
def importing_or_inactive?
|
||||||
importing? || inactive?
|
importing? || inactive?
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def runnable_by_owner?
|
||||||
|
Ability.allowed?(owner, :create_pipeline, project)
|
||||||
|
end
|
||||||
|
|
||||||
def set_next_run_at
|
def set_next_run_at
|
||||||
self.next_run_at = Gitlab::Ci::CronParser.new(cron, cron_timezone).next_time_from(Time.now)
|
self.next_run_at = Gitlab::Ci::CronParser.new(cron, cron_timezone).next_time_from(Time.now)
|
||||||
end
|
end
|
||||||
|
|
|
@ -3,8 +3,14 @@ class PipelineScheduleWorker
|
||||||
include CronjobQueue
|
include CronjobQueue
|
||||||
|
|
||||||
def perform
|
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
|
begin
|
||||||
|
unless schedule.runnable_by_owner?
|
||||||
|
schedule.deactivate!
|
||||||
|
next
|
||||||
|
end
|
||||||
|
|
||||||
Ci::CreatePipelineService.new(schedule.project,
|
Ci::CreatePipelineService.new(schedule.project,
|
||||||
schedule.owner,
|
schedule.owner,
|
||||||
ref: schedule.ref)
|
ref: schedule.ref)
|
||||||
|
|
|
@ -0,0 +1,4 @@
|
||||||
|
---
|
||||||
|
title: Add foreign key for pipeline schedule owner
|
||||||
|
merge_request: 11233
|
||||||
|
author:
|
|
@ -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
|
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).
|
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).
|
- 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-10533]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10533
|
||||||
[ce-10853]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10853
|
[ce-10853]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10853
|
||||||
|
|
|
@ -11,40 +11,53 @@ describe PipelineScheduleWorker do
|
||||||
end
|
end
|
||||||
|
|
||||||
before do
|
before do
|
||||||
project.add_master(user)
|
|
||||||
|
|
||||||
stub_ci_pipeline_to_return_yaml_file
|
stub_ci_pipeline_to_return_yaml_file
|
||||||
|
|
||||||
|
pipeline_schedule.update_column(:next_run_at, 1.day.ago)
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when there is a scheduled pipeline within next_run_at' do
|
context 'when the schedule is runnable by the user' do
|
||||||
let(:next_run_at) { 2.days.ago }
|
|
||||||
|
|
||||||
before do
|
before do
|
||||||
pipeline_schedule.update_column(:next_run_at, next_run_at)
|
project.add_master(user)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'creates a new pipeline' do
|
context 'when there is a scheduled pipeline within next_run_at' do
|
||||||
expect { subject }.to change { project.pipelines.count }.by(1)
|
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
|
||||||
|
|
||||||
|
expect(pipeline_schedule.reload.next_run_at).to be > Time.now
|
||||||
|
end
|
||||||
|
|
||||||
|
it 'sets the schedule on the pipeline' do
|
||||||
|
subject
|
||||||
|
|
||||||
|
expect(project.pipelines.last.pipeline_schedule).to eq(pipeline_schedule)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'updates the next_run_at field' do
|
context 'inactive schedule' do
|
||||||
subject
|
before do
|
||||||
|
pipeline_schedule.deactivate!
|
||||||
|
end
|
||||||
|
|
||||||
expect(pipeline_schedule.reload.next_run_at).to be > Time.now
|
it 'does not creates a new pipeline' do
|
||||||
end
|
expect { subject }.not_to change { project.pipelines.count }
|
||||||
|
end
|
||||||
it 'sets the schedule on the pipeline' do
|
|
||||||
subject
|
|
||||||
expect(project.pipelines.last.pipeline_schedule).to eq(pipeline_schedule)
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'inactive schedule' do
|
context 'when the schedule is not runnable by the user' do
|
||||||
before do
|
it 'deactivates the schedule' do
|
||||||
pipeline_schedule.update(active: false)
|
subject
|
||||||
|
|
||||||
|
expect(pipeline_schedule.reload.active).to be_falsy
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'does not creates a new pipeline' do
|
it 'does not schedule a pipeline' do
|
||||||
expect { subject }.not_to change { project.pipelines.count }
|
expect { subject }.not_to change { project.pipelines.count }
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in New Issue