Commit 2fbcaaaf authored by Dosuken shinya's avatar Dosuken shinya Committed by Rémy Coutable

Fix lazy error handling of cron parser

parent b4176bba
---
title: Fix error on CI/CD Settings page related to invalid pipeline trigger
merge_request: 10948
author: dosuken123
......@@ -6,7 +6,7 @@ module Gitlab
def initialize(cron, cron_timezone = 'UTC')
@cron = cron
@cron_timezone = cron_timezone
@cron_timezone = ActiveSupport::TimeZone.find_tzinfo(cron_timezone).name
end
def next_time_from(time)
......@@ -24,8 +24,23 @@ module Gitlab
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)
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
# noop
end
......
......@@ -104,6 +104,24 @@ feature 'Triggers', feature: true, js: true do
expect(page).to have_content 'The form contains the following errors'
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
context 'disabling schedule' do
......
......@@ -60,14 +60,60 @@ describe Gitlab::Ci::CronParser do
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
let(:cron) { '0 0 * * *' }
let(:cron) { '* 0 * * *' }
let(:cron_timezone) { 'US/Pacific' }
it_behaves_like "returns time in the future"
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
......@@ -76,9 +122,21 @@ describe Gitlab::Ci::CronParser do
let(:cron) { 'invalid_cron' }
let(:cron_timezone) { 'invalid_cron_timezone' }
it 'returns nil' do
is_expected.to be_nil
it { is_expected.to be_nil }
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
......@@ -96,6 +154,12 @@ describe Gitlab::Ci::CronParser do
it { is_expected.to eq(false) }
end
context 'when cron syntax is quoted' do
let(:cron) { "'0 * * * *'" }
it { is_expected.to eq(false) }
end
end
describe '#cron_timezone_valid?' do
......@@ -112,5 +176,11 @@ describe Gitlab::Ci::CronParser do
it { is_expected.to eq(false) }
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
......@@ -73,4 +73,36 @@ describe Ci::TriggerSchedule, models: true do
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
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