diff options
| -rw-r--r-- | changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml | 4 | ||||
| -rw-r--r-- | lib/gitlab/ci/cron_parser.rb | 19 | ||||
| -rw-r--r-- | spec/features/triggers_spec.rb | 18 | ||||
| -rw-r--r-- | spec/lib/gitlab/ci/cron_parser_spec.rb | 88 | ||||
| -rw-r--r-- | spec/models/ci/trigger_schedule_spec.rb | 32 |
5 files changed, 150 insertions, 11 deletions
diff --git a/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml b/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml new file mode 100644 index 00000000000..b0c33ab3fa4 --- /dev/null +++ b/changelogs/unreleased/31274-creating-schedule-trigger--causes-http-500-when-accessing-settings-ci_cd.yml @@ -0,0 +1,4 @@ +--- +title: Fix error on CI/CD Settings page related to invalid pipeline trigger +merge_request: 10948 +author: dosuken123 diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index a3cc350ef22..dad8c3cdf5b 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -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 diff --git a/spec/features/triggers_spec.rb b/spec/features/triggers_spec.rb index 81fa2de1cc3..783f330221c 100644 --- a/spec/features/triggers_spec.rb +++ b/spec/features/triggers_spec.rb @@ -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 diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 0864bc7258d..809fda11879 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -60,14 +60,60 @@ describe Gitlab::Ci::CronParser do end end - context 'when cron_timezone is US/Pacific' do - let(:cron) { '0 0 * * *' } - let(:cron_timezone) { 'US/Pacific' } + context 'when cron_timezone is TZInfo format' do + before do + allow(Time).to receive(:zone) + .and_return(ActiveSupport::TimeZone['UTC']) + end - it_behaves_like "returns time in the future" + 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 * * *' } + 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(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 - 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) + 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 - end + 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 diff --git a/spec/models/ci/trigger_schedule_spec.rb b/spec/models/ci/trigger_schedule_spec.rb index 75d21541cee..92447564d7c 100644 --- a/spec/models/ci/trigger_schedule_spec.rb +++ b/spec/models/ci/trigger_schedule_spec.rb @@ -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 |