wip validations

Author: sethboyles

app/actions/deployment_create.rb

@@ -219,6 +219,8 @@ def starting_state(message)
 
       def starting_process_instances(message, desired_instances)
         if message.strategy == DeploymentModel::CANARY_STRATEGY
+          # TODO: replace this with [message.max_in_flight, deployment.next_canary_step].min
+          # or remove this 'if' and make `desired_instances` do this work
           1
         else
           [message.max_in_flight, desired_instances].min

app/messages/deployment_create_message.rb

@@ -10,25 +10,21 @@ class DeploymentCreateMessage < MetadataBaseMessage
       options
     ]
 
+    ALLOWED_STEP_KEYS = [
+      :instance_weight
+    ]
+
     validates_with NoAdditionalKeysValidator
     validates :strategy,
               inclusion: { in: %w[rolling canary], message: "'%<value>s' is not a supported deployment strategy" },
               allow_nil: true
     validate :mutually_exclusive_droplet_sources
 
-    validates :options,
-              allow_nil: true,
-              hash: true
-
-    validates :canary_steps,
-              allow_nil: true,
-              array: true
+    # validates :options,
+    #           allow_nil: true,
+    #           hash: true
 
-    validates :canary_options,
-              allow_nil: true,
-              hash: true
-
-    validate :validate_canary_options
+    validate :validate_options
 
     def app_guid
       relationships&.dig(:app, :data, :guid)
@@ -46,30 +42,75 @@ def max_in_flight
       options&.dig(:max_in_flight) || 1
     end
 
-    def canary_steps
-      return [] unless canary_options.present? && canary_options.is_a?(Hash)
+    private
+
+    def mutually_exclusive_droplet_sources
+      return unless revision.present? && droplet.present?
 
-      canary_options&.dig(:steps) || []
+      errors.add(:droplet, "Cannot set both fields 'droplet' and 'revision'")
     end
 
-    def canary_options
-      return {} unless options.present? && options.is_a?(Hash)
+    def validate_options
+      return unless options.present?
 
-      options&.dig(:canary) || {}
+      unless options.is_a?(Hash)
+        errors.add(:options, 'must be an object')
+        return
+      end
+
+      validate_max_in_flight if options[:max_in_flight]
+      validate_canary if options[:canary]
     end
 
-    private
+    def validate_max_in_flight
+      max_in_flight = options[:max_in_flight]
 
-    def mutually_exclusive_droplet_sources
-      return unless revision.present? && droplet.present?
+      return unless !max_in_flight.is_a?(Integer) || max_in_flight < 1
 
-      errors.add(:droplet, "Cannot set both fields 'droplet' and 'revision'")
+      errors.add(:max_in_flight, 'must be an integer greater than 0')
+    end
+
+    def validate_canary
+      canary_options = options[:canary]
+      unless canary_options.is_a?(Hash)
+        errors.add(:'options.canary', 'must be an object')
+        return
+      end
+
+      if canary_options && strategy != 'canary'
+        errors.add(:'options.canary', 'are only valid for Canary deployments')
+        return
+      end
+
+      validate_steps if options[:canary][:steps]
     end
 
-    def validate_canary_options
-      return if canary_options.blank?
+    def validate_steps
+      steps = options[:canary][:steps]
+      if !steps.is_a?(Array) || steps.any? { |step| !step.is_a?(Hash) }
+        errors.add(:'options.canary.steps', 'must be an array of objects')
+        return
+      end
+
+      steps.each do |step|
+        disallowed_keys = step.keys - ALLOWED_STEP_KEYS
+
+        errors.add(:'options.canary.steps', "has unsupported key(s): #{disallowed_keys.join(', ')}") if disallowed_keys.present?
+      end
+
+      errors.add(:'options.canary.steps', 'missing key: "instance_weight"') if steps.any? { |step| !step.key?(:instance_weight) }
+
+      if steps.any? { |step| !step[:instance_weight].is_a?(Integer) }
+        errors.add(:'options.canary.steps.instance_weight', 'must be an Integer between 1-100 (inclusive)')
+        return
+      end
+
+      errors.add(:'options.canary.steps.instance_weight', 'must be an Integer between 1-100 (inclusive)') if steps.any? { |step| (1..100).exclude?(step[:instance_weight]) }
+
+      weights = steps.map { |step| step[:instance_weight] }
+      return unless weights.sort != weights
 
-      errors.add(:canary_options, 'are only valid for Canary deployments') unless strategy == 'canary'
+      errors.add(:'options.canary.steps.instance_weight', 'must be sorted in ascending order')
     end
   end
 end

app/messages/validators.rb

@@ -262,40 +262,38 @@ def validate(record)
       options = record.options
       return unless options.present?
 
-      if !options.is_a?(Hash)
+      unless options.is_a?(Hash)
         record.errors.add(:options, 'must be an object')
         return
       end
-      
+
       max_in_flight = options[:max_in_flight]
 
       # TODO: figure out if we should allow max_in_flight set to nil explicitly
-      if options.key?(:max_in_flight) && (!max_in_flight.is_a?(Integer) || max_in_flight < 1)
-        record.errors.add(:max_in_flight, 'must be an integer greater than 0')
-      end
+      record.errors.add(:max_in_flight, 'must be an integer greater than 0') if options.key?(:max_in_flight) && (!max_in_flight.is_a?(Integer) || max_in_flight < 1)
 
       canary_options = options[:canary]
       return if canary_options.nil?
-      
-      if !canary_options.is_a?(Hash)
-        record.errors.add(:"options.canary", 'must be an object')
+
+      unless canary_options.is_a?(Hash)
+        record.errors.add(:'options.canary', 'must be an object')
         return
       end
 
       # binding.pry
 
       strategy = record.strategy
 
-      if canary_options && strategy != "canary"
-        record.errors.add(:"options.canary", 'are only valid for Canary deployments')
+      if canary_options && strategy != 'canary'
+        record.errors.add(:'options.canary', 'are only valid for Canary deployments')
         return
       end
 
       steps = canary_options[:steps]
 
-      if steps && (!steps.is_a?(Array) || steps.any? { |step| !step.is_a?(Hash) })
-        record.errors.add(:"options.canary.steps", 'must be an array of objects')
-      end
+      return unless steps && (!steps.is_a?(Array) || steps.any? { |step| !step.is_a?(Hash) })
+
+      record.errors.add(:'options.canary.steps', 'must be an array of objects')
     end
   end
 

spec/lightweight_spec_helper.rb

@@ -2,7 +2,7 @@
 $LOAD_PATH.push(File.expand_path(File.join(__dir__, '..', 'lib')))
 
 require 'active_support/all'
-
+require 'pry'
 # So that specs using this helper don't fail with undefined constant error
 module VCAP
   module CloudController