Use (relaxed) ISO8601 durations and add more tests

README.md

@@ -123,7 +123,7 @@ You can add additional variables using the `--bindings=BINDINGS` option. For exa
 
 ### Customizing behaviour with annotations
 
-- `kubernetes-deploy.shopify.io/timeout-override-seconds` (any resource): Override the tool's hard timeout for one specific resource. Value must be a positive number of seconds (digits only).
+- `kubernetes-deploy.shopify.io/timeout-override` (any resource): Override the tool's hard timeout for one specific resource. Both full ISO8601 durations and the time portion of ISO8601 durations are valid. Value must be between 1 second and 24 hours. Example values: 45s / 3m / 1h / PT0.25H
 
 ### Running tasks at the beginning of a deploy
 

lib/kubernetes-deploy.rb

@@ -8,6 +8,7 @@
 require 'active_support/core_ext/string/strip'
 require 'active_support/core_ext/hash/keys'
 require 'active_support/core_ext/array/conversions'
+require 'active_support/duration'
 require 'colorized_string'
 
 require 'kubernetes-deploy/version'
@@ -17,6 +18,7 @@
 require 'kubernetes-deploy/statsd'
 require 'kubernetes-deploy/concurrency'
 require 'kubernetes-deploy/bindings_parser'
+require 'kubernetes-deploy/duration_parser'
 
 module KubernetesDeploy
   KubernetesDeploy::StatsD.build

lib/kubernetes-deploy/duration_parser.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+module KubernetesDeploy
+  ##
+  # This class is a less strict extension of ActiveSupport::Duration::ISO8601Parser.
+  # In addition to full ISO8601 durations, it can parse unprefixed ISO8601 time components (e.g. '1H').
+  # It is also case-insensitive.
+  # For example, this class considers the values "1H", "1h" and "PT1H" to be valid and equivalent.
+
+  class DurationParser
+    class ParsingError < ArgumentError; end
+
+    def initialize(value)
+      @iso8601_str = value.to_s.strip.upcase
+    end
+
+    def parse!
+      if @iso8601_str.blank?
+        raise ParsingError, "Cannot parse blank value"
+      end
+
+      parser = ActiveSupport::Duration::ISO8601Parser.new(@iso8601_str)
+      parser.mode = :time unless @iso8601_str.start_with?("P")
+      parts = parser.parse!
+      ActiveSupport::Duration.new(calculate_total_seconds(parts), parts)
+    rescue ActiveSupport::Duration::ISO8601Parser::ParsingError => e
+      raise ParsingError, e.message
+    end
+
+    private
+
+    # https://github.com/rails/rails/blob/19c450d5d99275924254b2041b6ad470fdaa1f93/activesupport/lib/active_support/duration.rb#L79-L83
+    def calculate_total_seconds(parts)
+      parts.inject(0) do |total, (part, value)|
+        total + value * ActiveSupport::Duration::PARTS_IN_SECONDS[part]
+      end
+    end
+  end
+end

lib/kubernetes-deploy/kubernetes_resource.rb

@@ -22,7 +22,7 @@ class KubernetesResource
       If you have reason to believe it will succeed, retry the deploy to continue to monitor the rollout.
       MSG
 
-    TIMEOUT_OVERRIDE_ANNOTATION = "kubernetes-deploy.shopify.io/timeout-override-seconds"
+    TIMEOUT_OVERRIDE_ANNOTATION = "kubernetes-deploy.shopify.io/timeout-override"
 
     def self.build(namespace:, context:, definition:, logger:)
       opts = { namespace: namespace, context: context, definition: definition, logger: logger }
@@ -41,12 +41,15 @@ def self.timeout
     end
 
     def timeout
-      return timeout_override.to_i if timeout_override.present?
+      return timeout_override if timeout_override.present?
       self.class.timeout
     end
 
     def timeout_override
-      @definition.dig("metadata", "annotations", TIMEOUT_OVERRIDE_ANNOTATION)
+      return @timeout_override if defined?(@timeout_override)
+      @timeout_override = DurationParser.new(timeout_annotation).parse!.to_i
+    rescue DurationParser::ParsingError
+      @timeout_override = nil
     end
 
     def pretty_timeout_type
@@ -71,7 +74,7 @@ def initialize(namespace:, context:, definition:, logger:)
 
     def validate_definition
       @validation_errors = []
-      validate_annotations
+      validate_timeout_annotation
 
       command = ["create", "-f", file_path, "--dry-run", "--output=name"]
       _, err, st = kubectl.run(*command, log_failure: false)
@@ -290,10 +293,21 @@ def to_s
 
     private
 
-    def validate_annotations
-      return unless timeout_override.present?
-      return if timeout_override.strip.match(/\A\d+\z/) && timeout_override.to_i > 0
-      @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation must contain digits only and must be > 0"
+    def validate_timeout_annotation
+      return if timeout_annotation.nil?
+
+      override = DurationParser.new(timeout_annotation).parse!
+      if override <= 0
+        @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: Value must be greater than 0"
+      elsif override > 24.hours
+        @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: Value must be less than 24h"
+      end
+    rescue DurationParser::ParsingError => e
+      @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: #{e}"
+    end
+
+    def timeout_annotation
+      @definition.dig("metadata", "annotations", TIMEOUT_OVERRIDE_ANNOTATION)
     end
 
     def file

test/fixtures/hello-cloud/unmanaged-pod.yml.erb

@@ -3,7 +3,7 @@ kind: Pod
 metadata:
   name: unmanaged-pod-<%= deployment_id %>
   annotations:
-    kubernetes-deploy.shopify.io/timeout-override-seconds: "42"
+    kubernetes-deploy.shopify.io/timeout-override: "42s"
   labels:
     type: unmanaged-pod
     name: unmanaged-pod-<%= deployment_id %>

test/unit/kubernetes-deploy/duration_parser_test.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+require 'test_helper'
+
+class DurationParserTest < KubernetesDeploy::TestCase
+  def test_parses_correct_iso_durations_with_prefixes
+    assert_equal 300, new_parser("PT300S").parse!
+    assert_equal 300, new_parser("PT5M").parse!
+    assert_equal 900, new_parser("PT0.25H").parse!
+    assert_equal 110839937, new_parser("P3Y6M4DT12H30M5S").parse!
+  end
+
+  def test_parses_correct_iso_durations_without_prefixes
+    assert_equal 300, new_parser("300S").parse!
+    assert_equal 300, new_parser("5M").parse!
+    assert_equal 900, new_parser("0.25H").parse!
+    assert_equal(-60, new_parser("-1M").parse!)
+  end
+
+  def test_parse_is_case_insensitive
+    assert_equal 30, new_parser("30S").parse!
+    assert_equal 30, new_parser("30s").parse!
+  end
+
+  def test_parse_raises_expected_error_for_blank_values
+    ["", "   ", nil].each do |blank_value|
+      assert_raises_message(KubernetesDeploy::DurationParser::ParsingError, "Cannot parse blank value") do
+        new_parser(blank_value).parse!
+      end
+    end
+  end
+
+  def test_extra_whitespace_is_stripped_from_values
+    assert_equal 30, new_parser("  30S    ").parse!
+  end
+
+  def test_parse_raises_expected_error_when_value_is_invalid
+    assert_raises_message(KubernetesDeploy::DurationParser::ParsingError, 'Invalid ISO 8601 duration: "FOO"') do
+      new_parser("foo").parse!
+    end
+  end
+
+  private
+
+  def new_parser(value)
+    KubernetesDeploy::DurationParser.new(value)
+  end
+end