From a7a1ac749e163439feb06b734b24afb00f5ff481 Mon Sep 17 00:00:00 2001
From: Avielle Wolfe <awolfe@gitlab.com>
Date: Tue, 5 Sep 2023 14:41:29 +0000
Subject: [PATCH] Update gitlab-chronic-duration to 0.11
Updates the gitlab-chronic-duration gem. Uses a new
`update_chronic_duration` feature flag to test that the new
functionality in the gem does not break any existing features
---
Gemfile | 2 +-
Gemfile.checksum | 2 +-
Gemfile.lock | 4 +-
app/models/ci/build.rb | 8 +++-
.../concerns/chronic_duration_attribute.rb | 9 +++-
app/models/container_expiration_policy.rb | 6 ++-
.../cleanup_tags_base_service.rb | 4 +-
app/validators/duration_validator.rb | 2 +-
.../development/update_chronic_duration.yml | 8 ++++
lib/gitlab/ci/build/duration_parser.rb | 2 +-
lib/gitlab/ci/config/entry/job.rb | 8 +++-
.../config/entry/legacy_validation_helpers.rb | 10 ++--
lib/gitlab/time_tracking_formatter.rb | 5 +-
.../gitlab/ci/build/duration_parser_spec.rb | 6 +--
spec/lib/gitlab/ci/config/entry/job_spec.rb | 12 +++++
.../gitlab/time_tracking_formatter_spec.rb | 46 +++++++++++++++++++
spec/models/ci/build_spec.rb | 36 +++++++++++++++
.../chronic_duration_attribute_spec.rb | 30 ++++++++++++
.../cleanup_tags_service_shared_examples.rb | 11 +++++
19 files changed, 192 insertions(+), 19 deletions(-)
create mode 100644 config/feature_flags/development/update_chronic_duration.yml
diff --git a/Gemfile b/Gemfile
index 6d6ce7eddaccb..f770046c29829 100644
--- a/Gemfile
+++ b/Gemfile
@@ -324,7 +324,7 @@ gem 'fast_blank', '~> 1.0.1'
# Parse time & duration
gem 'gitlab-chronic', '~> 0.10.5'
-gem 'gitlab_chronic_duration', '~> 0.10.6.2'
+gem 'gitlab_chronic_duration', '~> 0.11'
gem 'rack-proxy', '~> 0.7.6'
diff --git a/Gemfile.checksum b/Gemfile.checksum
index 9f4f03fe63b56..cf898be05b74d 100644
--- a/Gemfile.checksum
+++ b/Gemfile.checksum
@@ -217,7 +217,7 @@
{"name":"gitlab-markup","version":"1.9.0","platform":"ruby","checksum":"7eda045a08ec2d110084252fa13a8c9eac8bdac0e302035ca7db4b82bcbd7ed4"},
{"name":"gitlab-net-dns","version":"0.9.2","platform":"ruby","checksum":"f726d978479d43810819f12a45c0906d775a07e34df111bbe693fffbbef3059d"},
{"name":"gitlab-styles","version":"10.1.0","platform":"ruby","checksum":"f42745f5397d042fe24cf2d0eb56c995b37f9f43d8fb79b834d197a1cafdc84a"},
-{"name":"gitlab_chronic_duration","version":"0.10.6.2","platform":"ruby","checksum":"6dda4cfe7dca9b958f163ac8835c3d9cc70cf8df8cbb89bb2fbf9ba4375105fb"},
+{"name":"gitlab_chronic_duration","version":"0.11.0","platform":"ruby","checksum":"c2fd201724a9031ff0af23d07a30231cebefbf83c3e682daae452cda5f514ba6"},
{"name":"gitlab_omniauth-ldap","version":"2.2.0","platform":"ruby","checksum":"bb4d20acb3b123ed654a8f6a47d3fac673ece7ed0b6992edb92dca14bad2838c"},
{"name":"gitlab_quality-test_tooling","version":"0.9.3","platform":"ruby","checksum":"9751f3504b717499588bd0fa5517de9b6756e8b9548777ea0283b889694580f0"},
{"name":"globalid","version":"1.1.0","platform":"ruby","checksum":"b337e1746f0c8cb0a6c918234b03a1ddeb4966206ce288fbb57779f59b2d154f"},
diff --git a/Gemfile.lock b/Gemfile.lock
index 01f62db25c41f..25437eb009c70 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -676,7 +676,7 @@ GEM
rubocop-performance (~> 1.15)
rubocop-rails (~> 2.17)
rubocop-rspec (~> 2.22)
- gitlab_chronic_duration (0.10.6.2)
+ gitlab_chronic_duration (0.11.0)
numerizer (~> 0.2)
gitlab_omniauth-ldap (2.2.0)
net-ldap (~> 0.16)
@@ -1832,7 +1832,7 @@ DEPENDENCIES
gitlab-sidekiq-fetcher!
gitlab-styles (~> 10.1.0)
gitlab-utils!
- gitlab_chronic_duration (~> 0.10.6.2)
+ gitlab_chronic_duration (~> 0.11)
gitlab_omniauth-ldap (~> 2.2.0)
gitlab_quality-test_tooling (~> 0.9.3)
gon (~> 6.4.0)
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 107f3c5987a68..2b954e399ae51 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -414,7 +414,9 @@ def schedulable?
end
def options_scheduled_at
- ChronicDuration.parse(options[:start_in])&.seconds&.from_now
+ ChronicDuration.parse(
+ options[:start_in], use_complete_matcher: Feature.enabled?(:update_chronic_duration)
+ )&.seconds&.from_now
end
def action?
@@ -745,7 +747,9 @@ def artifacts_expire_in
def artifacts_expire_in=(value)
self.artifacts_expire_at =
if value
- ChronicDuration.parse(value)&.seconds&.from_now
+ ChronicDuration.parse(
+ value, use_complete_matcher: Feature.enabled?(:update_chronic_duration)
+ )&.seconds&.from_now
end
end
diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb
index af4905115b1d1..944ad252cc650 100644
--- a/app/models/concerns/chronic_duration_attribute.rb
+++ b/app/models/concerns/chronic_duration_attribute.rb
@@ -17,7 +17,14 @@ def chronic_duration_attr_writer(virtual_attribute, source_attribute, parameters
chronic_duration_attributes[virtual_attribute] = value.presence || parameters[:default].presence.to_s
begin
- new_value = value.present? ? ChronicDuration.parse(value).to_i : parameters[:default].presence
+ new_value = if value.present?
+ ChronicDuration.parse(
+ value, use_complete_matcher: Feature.enabled?(:update_chronic_duration)
+ ).to_i
+ else
+ parameters[:default].presence
+ end
+
assign_attributes(source_attribute => new_value)
rescue ChronicDuration::DurationParseError
# ignore error as it will be caught by validation
diff --git a/app/models/container_expiration_policy.rb b/app/models/container_expiration_policy.rb
index aecb47f7a030a..857b7eccf70ae 100644
--- a/app/models/container_expiration_policy.rb
+++ b/app/models/container_expiration_policy.rb
@@ -80,7 +80,11 @@ def self.older_than_options
end
def set_next_run_at
- self.next_run_at = Time.zone.now + ChronicDuration.parse(cadence).seconds
+ cadence_seconds = ChronicDuration.parse(
+ cadence, use_complete_matcher: Feature.enabled?(:update_chronic_duration)
+ ).seconds
+
+ self.next_run_at = Time.zone.now + cadence_seconds
end
def disable!
diff --git a/app/services/projects/container_repository/cleanup_tags_base_service.rb b/app/services/projects/container_repository/cleanup_tags_base_service.rb
index 45557d0350297..185895698aff9 100644
--- a/app/services/projects/container_repository/cleanup_tags_base_service.rb
+++ b/app/services/projects/container_repository/cleanup_tags_base_service.rb
@@ -100,7 +100,9 @@ def keep_n_as_integer
def older_than_in_seconds
strong_memoize(:older_than_in_seconds) do
- ChronicDuration.parse(older_than).seconds
+ ChronicDuration.parse(
+ older_than, use_complete_matcher: Feature.enabled?(:update_chronic_duration)
+ ).seconds
end
end
end
diff --git a/app/validators/duration_validator.rb b/app/validators/duration_validator.rb
index defd28d7d3b32..5d7eba0912267 100644
--- a/app/validators/duration_validator.rb
+++ b/app/validators/duration_validator.rb
@@ -12,7 +12,7 @@
#
class DurationValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
- ChronicDuration.parse(value)
+ ChronicDuration.parse(value, use_complete_matcher: Feature.enabled?(:update_chronic_duration))
rescue ChronicDuration::DurationParseError
if options[:message]
record.errors.add(:base, options[:message])
diff --git a/config/feature_flags/development/update_chronic_duration.yml b/config/feature_flags/development/update_chronic_duration.yml
new file mode 100644
index 0000000000000..4af6529961987
--- /dev/null
+++ b/config/feature_flags/development/update_chronic_duration.yml
@@ -0,0 +1,8 @@
+---
+name: update_chronic_duration
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130531
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/423696
+milestone: '16.4'
+type: development
+group: group::pipeline authoring
+default_enabled: false
diff --git a/lib/gitlab/ci/build/duration_parser.rb b/lib/gitlab/ci/build/duration_parser.rb
index 9385dccd5f328..bc365fe4e9f86 100644
--- a/lib/gitlab/ci/build/duration_parser.rb
+++ b/lib/gitlab/ci/build/duration_parser.rb
@@ -41,7 +41,7 @@ def safe_parse
def parse
return if never?
- ChronicDuration.parse(value)
+ ChronicDuration.parse(value, use_complete_matcher: Feature.enabled?(:update_chronic_duration))
end
def validation_cache
diff --git a/lib/gitlab/ci/config/entry/job.rb b/lib/gitlab/ci/config/entry/job.rb
index bc0c8016934e1..bf38e2aedafc9 100644
--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -160,7 +160,7 @@ def value
retry: retry_defined? ? retry_value : nil,
parallel: has_parallel? ? parallel_value : nil,
interruptible: interruptible_defined? ? interruptible_value : nil,
- timeout: has_timeout? ? ChronicDuration.parse(timeout.to_s) : nil,
+ timeout: parsed_timeout,
artifacts: artifacts_value,
release: release_value,
after_script: after_script_value,
@@ -174,6 +174,12 @@ def value
).compact
end
+ def parsed_timeout
+ return unless has_timeout?
+
+ ChronicDuration.parse(timeout.to_s, use_complete_matcher: Feature.enabled?(:update_chronic_duration))
+ end
+
def ignored?
allow_failure_defined? ? static_allow_failure : manual_action?
end
diff --git a/lib/gitlab/config/entry/legacy_validation_helpers.rb b/lib/gitlab/config/entry/legacy_validation_helpers.rb
index 415f6f77214c1..9363c60e6db67 100644
--- a/lib/gitlab/config/entry/legacy_validation_helpers.rb
+++ b/lib/gitlab/config/entry/legacy_validation_helpers.rb
@@ -12,7 +12,7 @@ def validate_duration(value, parser = nil)
if parser && parser.respond_to?(:validate_duration)
parser.validate_duration(value)
else
- ChronicDuration.parse(value)
+ ChronicDuration.parse(value, use_complete_matcher: Feature.enabled?(:update_chronic_duration))
end
rescue ChronicDuration::DurationParseError
false
@@ -24,8 +24,12 @@ def validate_duration_limit(value, limit, parser = nil)
if parser && parser.respond_to?(:validate_duration_limit)
parser.validate_duration_limit(value, limit)
else
- ChronicDuration.parse(value).second.from_now <
- ChronicDuration.parse(limit).second.from_now
+ ChronicDuration.parse(
+ value, use_complete_matcher: Feature.enabled?(:update_chronic_duration)
+ ).second.from_now <
+ ChronicDuration.parse(
+ limit, use_complete_matcher: Feature.enabled?(:update_chronic_duration)
+ ).second.from_now
end
rescue ChronicDuration::DurationParseError
false
diff --git a/lib/gitlab/time_tracking_formatter.rb b/lib/gitlab/time_tracking_formatter.rb
index 7094db14c5d5e..a8f85e204263a 100644
--- a/lib/gitlab/time_tracking_formatter.rb
+++ b/lib/gitlab/time_tracking_formatter.rb
@@ -15,7 +15,10 @@ def parse(string, keep_zero: false)
begin
ChronicDuration.parse(
string,
- CUSTOM_DAY_AND_MONTH_LENGTH.merge(default_unit: 'hours', keep_zero: keep_zero))
+ CUSTOM_DAY_AND_MONTH_LENGTH.merge(
+ default_unit: 'hours', keep_zero: keep_zero,
+ use_complete_matcher: Feature.enabled?(:update_chronic_duration)
+ ))
rescue StandardError
nil
end
diff --git a/spec/lib/gitlab/ci/build/duration_parser_spec.rb b/spec/lib/gitlab/ci/build/duration_parser_spec.rb
index 7f5ff1eb0ee2f..bc905aa0a3522 100644
--- a/spec/lib/gitlab/ci/build/duration_parser_spec.rb
+++ b/spec/lib/gitlab/ci/build/duration_parser_spec.rb
@@ -25,8 +25,8 @@
it { is_expected.to be_truthy }
it 'caches data' do
- expect(ChronicDuration).to receive(:parse).with(value).once.and_call_original
- expect(ChronicDuration).to receive(:parse).with(other_value).once.and_call_original
+ expect(ChronicDuration).to receive(:parse).with(value, use_complete_matcher: true).once.and_call_original
+ expect(ChronicDuration).to receive(:parse).with(other_value, use_complete_matcher: true).once.and_call_original
2.times do
expect(described_class.validate_duration(value)).to eq(86400)
@@ -41,7 +41,7 @@
it { is_expected.to be_falsy }
it 'caches data' do
- expect(ChronicDuration).to receive(:parse).with(value).once.and_call_original
+ expect(ChronicDuration).to receive(:parse).with(value, use_complete_matcher: true).once.and_call_original
2.times do
expect(described_class.validate_duration(value)).to be_falsey
diff --git a/spec/lib/gitlab/ci/config/entry/job_spec.rb b/spec/lib/gitlab/ci/config/entry/job_spec.rb
index 1a78d92987151..df58877f1205e 100644
--- a/spec/lib/gitlab/ci/config/entry/job_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/job_spec.rb
@@ -426,6 +426,18 @@
expect(entry.errors).to be_empty
expect(entry.timeout).to eq('1m 1s')
end
+
+ context 'when update_chronic_duration is disabled' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'returns correct timeout' do
+ expect(entry).to be_valid
+ expect(entry.errors).to be_empty
+ expect(entry.timeout).to eq('1m 1s')
+ end
+ end
end
context 'when it is a release' do
diff --git a/spec/lib/gitlab/time_tracking_formatter_spec.rb b/spec/lib/gitlab/time_tracking_formatter_spec.rb
index aa755d64a7a24..978f6dfd2d920 100644
--- a/spec/lib/gitlab/time_tracking_formatter_spec.rb
+++ b/spec/lib/gitlab/time_tracking_formatter_spec.rb
@@ -12,12 +12,28 @@
let(:duration_string) { '3h 20m' }
it { expect(subject).to eq(12_000) }
+
+ context 'when update_chronic_duration is false' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it { expect(subject).to eq(12_000) }
+ end
end
context 'negative durations' do
let(:duration_string) { '-3h 20m' }
it { expect(subject).to eq(-12_000) }
+
+ context 'when update_chronic_duration is false' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it { expect(subject).to eq(-12_000) }
+ end
end
context 'durations with months' do
@@ -26,6 +42,16 @@
it 'uses our custom conversions' do
expect(subject).to eq(576_000)
end
+
+ context 'when update_chronic_duration is false' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'uses our custom conversions' do
+ expect(subject).to eq(576_000)
+ end
+ end
end
context 'when the duration is zero' do
@@ -35,6 +61,16 @@
it 'returns nil' do
expect(subject).to be_nil
end
+
+ context 'when update_chronic_duration is false' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'returns nil' do
+ expect(subject).to be_nil
+ end
+ end
end
context 'when keep_zero is true' do
@@ -43,6 +79,16 @@
it 'returns zero' do
expect(subject).to eq(0)
end
+
+ context 'when update_chronic_duration is false' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'returns zero' do
+ expect(subject).to eq(0)
+ end
+ end
end
end
end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index b95dec7195ffa..15f31d98d9805 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -735,6 +735,18 @@
is_expected.to eq(1.day.since)
end
end
+
+ context 'when update_chronic_duration is disabled' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'returns date after 1 day' do
+ freeze_time do
+ is_expected.to eq(1.day.since)
+ end
+ end
+ end
end
context 'when start_in is 1 week' do
@@ -745,6 +757,18 @@
is_expected.to eq(1.week.since)
end
end
+
+ context 'when update_chronic_duration is disabled' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'returns date after 1 week' do
+ freeze_time do
+ is_expected.to eq(1.week.since)
+ end
+ end
+ end
end
end
@@ -1075,6 +1099,18 @@
is_expected.to be_nil
end
+
+ context 'when update_chronic_duration is disabled' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'assigns a valid duration' do
+ build.artifacts_expire_in = '7 days'
+
+ is_expected.to be_within(10).of(7.days.to_i)
+ end
+ end
end
describe '#commit' do
diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb
index 61b86455840bb..fe4a352390950 100644
--- a/spec/models/concerns/chronic_duration_attribute_spec.rb
+++ b/spec/models/concerns/chronic_duration_attribute_spec.rb
@@ -39,6 +39,16 @@
expect(subject.valid?).to be_truthy
end
+ context 'when update_chronic_duration is disabled' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'parses chronic duration input' do
+ expect(subject.send(source_field)).to eq(600)
+ end
+ end
+
context 'when negative input is used' do
before do
subject.send("#{source_field}=", 3600)
@@ -72,6 +82,16 @@
it 'passes validation' do
expect(subject.valid?).to be_truthy
end
+
+ context 'when update_chronic_duration is disabled' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'writes default value' do
+ expect(subject.send(source_field)).to eq(default_value)
+ end
+ end
end
context 'when nil input is used' do
@@ -90,6 +110,16 @@
it "doesn't raise exception" do
expect { subject.send("#{virtual_field}=", nil) }.not_to raise_error
end
+
+ context 'when update_chronic_duration is disabled' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it 'writes default value' do
+ expect(subject.send(source_field)).to eq(default_value)
+ end
+ end
end
end
diff --git a/spec/support/shared_examples/projects/container_repository/cleanup_tags_service_shared_examples.rb b/spec/support/shared_examples/projects/container_repository/cleanup_tags_service_shared_examples.rb
index f9f8435c211ca..d55c99b9eb147 100644
--- a/spec/support/shared_examples/projects/container_repository/cleanup_tags_service_shared_examples.rb
+++ b/spec/support/shared_examples/projects/container_repository/cleanup_tags_service_shared_examples.rb
@@ -153,6 +153,17 @@
service_response_extra: service_response_extra,
supports_caching: supports_caching,
delete_expectations: delete_expectations
+
+ context 'when update_chronic_duration is disabled' do
+ before do
+ stub_feature_flags(update_chronic_duration: false)
+ end
+
+ it_behaves_like 'removing the expected tags',
+ service_response_extra: service_response_extra,
+ supports_caching: supports_caching,
+ delete_expectations: delete_expectations
+ end
end
RSpec.shared_examples 'when combining all parameters' do
--
GitLab