Address second round of PR review feedback

Author: jaredbrook

docs/search_expressions.md

@@ -20,7 +20,7 @@ Add `SearchExpression` and optionally `SearchAggregation` to an alarm template.
 ### Properties
 
 | Property | Required | Default | Description |
-|---|---|---|---|
+| --- | --- | --- | --- |
 | `SearchExpression` | Yes | - | A CloudWatch SEARCH() expression string. Supports `${Resource::...}` [variables](variables.md). |
 | `SearchAggregation` | No | `MAX` | Aggregation function applied to the search results. Valid values: `MAX`, `MIN`, `AVG`, `SUM`. |
 

lib/cfnguardian/stacks/resources.rb

@@ -33,7 +33,7 @@ def build_template(resources)
       def add_alarm(alarm)
         actions = alarm.alarm_action.kind_of?(Array) ? alarm.alarm_action.map{|action| Ref(action)} : [Ref(alarm.alarm_action)]
         actions.concat alarm.maintenance_groups.map {|mg| Ref(mg)} if alarm.maintenance_groups.any?
-        use_search = alarm.search_expression ? true : false
+        use_search = alarm.search_expression.is_a?(String) && !alarm.search_expression.strip.empty?
 
         @template.declare do
           CloudWatch_Alarm("#{alarm.resource_hash}#{alarm.group}#{alarm.name.gsub(/[^0-9a-zA-Z]/i, '')}#{alarm.type}"[0..255]) do

spec/search_expression_spec.rb

@@ -1,7 +1,6 @@
 require 'spec_helper'
 require 'json'
 require 'yaml'
-require 'fileutils'
 require 'tmpdir'
 require 'term/ansicolor'
 require 'cfnguardian/log'
@@ -181,31 +180,121 @@
   end
 
   describe 'Validation' do
+    def compile_config(config)
+      Dir.mktmpdir do |tmpdir|
+        fixture = File.join(tmpdir, 'test_alarms.yaml')
+        File.write(fixture, config.to_yaml)
+        compile = CfnGuardian::Compile.new(fixture, false)
+        compile.get_resources
+        compile
+      end
+    end
+
     context 'when search expression alarm has no metric_name or namespace' do
       it 'does not raise validation errors' do
-        Dir.mktmpdir do |tmpdir|
-          fixture = File.join(tmpdir, 'search_expression_alarms.yaml')
-          File.write(fixture, {
+        result = compile_config({
+          'Resources' => {
+            'AutoScalingGroup' => [{ 'Id' => 'my-app-AsgGroup-abc123' }]
+          },
+          'Templates' => {
+            'AutoScalingGroup' => {
+              'CPUUtilizationHighBase' => {
+                'SearchExpression' => "SEARCH('{AWS/EC2,AutoScalingGroupName} MetricName=\"CPUUtilization\" my-app', 'Minimum', 60)",
+                'SearchAggregation' => 'MAX'
+              },
+              'StatusCheckFailed' => false
+            }
+          }
+        })
+
+        search_alarms = result.alarms.select { |a| a.search_expression }
+        expect(search_alarms.length).to eq(1)
+        expect(search_alarms.first.search_expression).to include('SEARCH')
+      end
+    end
+
+    context 'when search expression is blank' do
+      it 'raises a validation error' do
+        expect {
+          compile_config({
             'Resources' => {
               'AutoScalingGroup' => [{ 'Id' => 'my-app-AsgGroup-abc123' }]
             },
             'Templates' => {
               'AutoScalingGroup' => {
                 'CPUUtilizationHighBase' => {
-                  'SearchExpression' => "SEARCH('{AWS/EC2,AutoScalingGroupName} MetricName=\"CPUUtilization\" my-app', 'Minimum', 60)",
+                  'SearchExpression' => '   ',
+                  'SearchAggregation' => 'MAX'
+                },
+                'StatusCheckFailed' => false
+              }
+            }
+          })
+        }.to raise_error(CfnGuardian::ValidationError, /invalid SearchExpression/)
+      end
+    end
+
+    context 'when search expression is not a string' do
+      it 'raises a validation error' do
+        expect {
+          compile_config({
+            'Resources' => {
+              'AutoScalingGroup' => [{ 'Id' => 'my-app-AsgGroup-abc123' }]
+            },
+            'Templates' => {
+              'AutoScalingGroup' => {
+                'CPUUtilizationHighBase' => {
+                  'SearchExpression' => ['not', 'a', 'string'],
                   'SearchAggregation' => 'MAX'
                 },
                 'StatusCheckFailed' => false
               }
             }
-          }.to_yaml)
-
-          compile = CfnGuardian::Compile.new(fixture, false)
-          compile.get_resources
-          search_alarms = compile.alarms.select { |a| a.search_expression }
-          expect(search_alarms.length).to eq(1)
-          expect(search_alarms.first.search_expression).to include('SEARCH')
-        end
+          })
+        }.to raise_error(CfnGuardian::ValidationError, /invalid SearchExpression/)
+      end
+    end
+
+    context 'when search aggregation is invalid' do
+      it 'raises a validation error' do
+        expect {
+          compile_config({
+            'Resources' => {
+              'AutoScalingGroup' => [{ 'Id' => 'my-app-AsgGroup-abc123' }]
+            },
+            'Templates' => {
+              'AutoScalingGroup' => {
+                'CPUUtilizationHighBase' => {
+                  'SearchExpression' => "SEARCH('{AWS/EC2,AutoScalingGroupName} MetricName=\"CPUUtilization\" my-app', 'Minimum', 60)",
+                  'SearchAggregation' => 'MEDIAN'
+                },
+                'StatusCheckFailed' => false
+              }
+            }
+          })
+        }.to raise_error(CfnGuardian::ValidationError, /invalid SearchAggregation/)
+      end
+    end
+
+    context 'when search aggregation is lowercase' do
+      it 'normalizes to uppercase' do
+        result = compile_config({
+          'Resources' => {
+            'AutoScalingGroup' => [{ 'Id' => 'my-app-AsgGroup-abc123' }]
+          },
+          'Templates' => {
+            'AutoScalingGroup' => {
+              'CPUUtilizationHighBase' => {
+                'SearchExpression' => "SEARCH('{AWS/EC2,AutoScalingGroupName} MetricName=\"CPUUtilization\" my-app', 'Minimum', 60)",
+                'SearchAggregation' => 'avg'
+              },
+              'StatusCheckFailed' => false
+            }
+          }
+        })
+
+        search_alarms = result.alarms.select { |a| a.search_expression }
+        expect(search_alarms.first.search_aggregation).to eq('AVG')
       end
     end
   end