Commit d88b9ac7 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch...

Merge branch '31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd' into 'master'

Fix lazy error handling of cron parser

Closes #31274

See merge request !10948
parents b4176bba 2fbcaaaf
---
title: Fix error on CI/CD Settings page related to invalid pipeline trigger
merge_request: 10948
author: dosuken123
...@@ -6,7 +6,7 @@ module Gitlab ...@@ -6,7 +6,7 @@ module Gitlab
def initialize(cron, cron_timezone = 'UTC') def initialize(cron, cron_timezone = 'UTC')
@cron = cron @cron = cron
@cron_timezone = cron_timezone @cron_timezone = ActiveSupport::TimeZone.find_tzinfo(cron_timezone).name
end end
def next_time_from(time) def next_time_from(time)
...@@ -24,8 +24,23 @@ module Gitlab ...@@ -24,8 +24,23 @@ module Gitlab
private private
# NOTE:
# cron_timezone can only accept timezones listed in TZInfo::Timezone.
# Aliases of Timezones from ActiveSupport::TimeZone are NOT accepted,
# because Rufus::Scheduler only supports TZInfo::Timezone.
#
# For example, those codes have the same effect.
# Time.zone = 'Pacific Time (US & Canada)' (ActiveSupport::TimeZone)
# Time.zone = 'America/Los_Angeles' (TZInfo::Timezone)
#
# However, try_parse_cron only accepts the latter format.
# try_parse_cron('* * * * *', 'Pacific Time (US & Canada)') -> Doesn't work
# try_parse_cron('* * * * *', 'America/Los_Angeles') -> Works
# If you want to know more, please take a look
# https://github.com/rails/rails/blob/master/activesupport/lib/active_support/values/time_zone.rb
def try_parse_cron(cron, cron_timezone) def try_parse_cron(cron, cron_timezone)
Rufus::Scheduler.parse("#{cron} #{cron_timezone}") cron_line = Rufus::Scheduler.parse("#{cron} #{cron_timezone}")
cron_line if cron_line.is_a?(Rufus::Scheduler::CronLine)
rescue rescue
# noop # noop
end end
......
...@@ -104,6 +104,24 @@ feature 'Triggers', feature: true, js: true do ...@@ -104,6 +104,24 @@ feature 'Triggers', feature: true, js: true do
expect(page).to have_content 'The form contains the following errors' expect(page).to have_content 'The form contains the following errors'
end end
context 'when GitLab time_zone is ActiveSupport::TimeZone format' do
before do
allow(Time).to receive(:zone)
.and_return(ActiveSupport::TimeZone['Eastern Time (US & Canada)'])
end
scenario 'do fill form with valid data and save' do
find('#trigger_trigger_schedule_attributes_active').click
fill_in 'trigger_trigger_schedule_attributes_cron', with: '1 * * * *'
fill_in 'trigger_trigger_schedule_attributes_cron_timezone', with: 'UTC'
fill_in 'trigger_trigger_schedule_attributes_ref', with: 'master'
click_button 'Save trigger'
expect(page.find('.flash-notice'))
.to have_content 'Trigger was successfully updated.'
end
end
end end
context 'disabling schedule' do context 'disabling schedule' do
......
...@@ -60,14 +60,60 @@ describe Gitlab::Ci::CronParser do ...@@ -60,14 +60,60 @@ describe Gitlab::Ci::CronParser do
end end
end end
context 'when cron_timezone is TZInfo format' do
before do
allow(Time).to receive(:zone)
.and_return(ActiveSupport::TimeZone['UTC'])
end
let(:hour_in_utc) do
ActiveSupport::TimeZone[cron_timezone]
.now.change(hour: 0).in_time_zone('UTC').hour
end
context 'when cron_timezone is US/Pacific' do context 'when cron_timezone is US/Pacific' do
let(:cron) { '0 0 * * *' } let(:cron) { '* 0 * * *' }
let(:cron_timezone) { 'US/Pacific' } let(:cron_timezone) { 'US/Pacific' }
it_behaves_like "returns time in the future" it_behaves_like "returns time in the future"
it 'converts time in server time zone' do it 'converts time in server time zone' do
expect(subject.hour).to eq((Time.zone.now.in_time_zone(cron_timezone).utc_offset / 60 / 60).abs) expect(subject.hour).to eq(hour_in_utc)
end
end
end
context 'when cron_timezone is ActiveSupport::TimeZone format' do
before do
allow(Time).to receive(:zone)
.and_return(ActiveSupport::TimeZone['UTC'])
end
let(:hour_in_utc) do
ActiveSupport::TimeZone[cron_timezone]
.now.change(hour: 0).in_time_zone('UTC').hour
end
context 'when cron_timezone is Berlin' do
let(:cron) { '* 0 * * *' }
let(:cron_timezone) { 'Berlin' }
it_behaves_like "returns time in the future"
it 'converts time in server time zone' do
expect(subject.hour).to eq(hour_in_utc)
end
end
context 'when cron_timezone is Eastern Time (US & Canada)' do
let(:cron) { '* 0 * * *' }
let(:cron_timezone) { 'Eastern Time (US & Canada)' }
it_behaves_like "returns time in the future"
it 'converts time in server time zone' do
expect(subject.hour).to eq(hour_in_utc)
end
end end
end end
end end
...@@ -76,9 +122,21 @@ describe Gitlab::Ci::CronParser do ...@@ -76,9 +122,21 @@ describe Gitlab::Ci::CronParser do
let(:cron) { 'invalid_cron' } let(:cron) { 'invalid_cron' }
let(:cron_timezone) { 'invalid_cron_timezone' } let(:cron_timezone) { 'invalid_cron_timezone' }
it 'returns nil' do it { is_expected.to be_nil }
is_expected.to be_nil
end end
context 'when cron syntax is quoted' do
let(:cron) { "'0 * * * *'" }
let(:cron_timezone) { 'UTC' }
it { expect(subject).to be_nil }
end
context 'when cron syntax is rufus-scheduler syntax' do
let(:cron) { 'every 3h' }
let(:cron_timezone) { 'UTC' }
it { expect(subject).to be_nil }
end end
end end
...@@ -96,6 +154,12 @@ describe Gitlab::Ci::CronParser do ...@@ -96,6 +154,12 @@ describe Gitlab::Ci::CronParser do
it { is_expected.to eq(false) } it { is_expected.to eq(false) }
end end
context 'when cron syntax is quoted' do
let(:cron) { "'0 * * * *'" }
it { is_expected.to eq(false) }
end
end end
describe '#cron_timezone_valid?' do describe '#cron_timezone_valid?' do
...@@ -112,5 +176,11 @@ describe Gitlab::Ci::CronParser do ...@@ -112,5 +176,11 @@ describe Gitlab::Ci::CronParser do
it { is_expected.to eq(false) } it { is_expected.to eq(false) }
end end
context 'when cron_timezone is ActiveSupport::TimeZone format' do
let(:cron_timezone) { 'Eastern Time (US & Canada)' }
it { is_expected.to eq(true) }
end
end end
end end
...@@ -73,4 +73,36 @@ describe Ci::TriggerSchedule, models: true do ...@@ -73,4 +73,36 @@ describe Ci::TriggerSchedule, models: true do
end end
end end
end end
describe '#real_next_run' do
subject do
Ci::TriggerSchedule.last.real_next_run(worker_cron: worker_cron,
worker_time_zone: worker_time_zone)
end
context 'when GitLab time_zone is UTC' do
before do
allow(Time).to receive(:zone)
.and_return(ActiveSupport::TimeZone[worker_time_zone])
end
let(:worker_time_zone) { 'UTC' }
context 'when cron_timezone is Eastern Time (US & Canada)' do
before do
create(:ci_trigger_schedule, :nightly,
cron_timezone: 'Eastern Time (US & Canada)')
end
let(:worker_cron) { '0 1 2 3 *' }
it 'returns the next time worker executes' do
expect(subject.min).to eq(0)
expect(subject.hour).to eq(1)
expect(subject.day).to eq(2)
expect(subject.month).to eq(3)
end
end
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment