Merge pull request #101 from pdrakeweb/parameter-strategy

Fix bugs in the merge parameter strategy for added and dropped parame…

docs/user-guide/stack_parameter_strategies.md

@@ -30,7 +30,7 @@ per-environment tunings in the source repository acting as both a safety net
 and documentation of existing environments. A possible solution for using this
 strategy in a safe manner is the following:
 
-When a stack update is performed, a *parameter overrides file* is checked in
+When a stack update is performed, a *parameter file* is checked in
 `cloud_formation/parameters/environment-name.yml`. This file is YAML formatted
 and takes a hash of stack parameter names and values, for example:
 ```yaml
@@ -78,8 +78,8 @@ Example:
 
 ```ruby
 class CustomStrategy
-  def parameters(current, overrides)
-    current.map do |k, v|
+  def parameters(parameters, stack_parameters, template)
+    parameters.map do |k, v|
       {
         parameter_key: k,
         parameter_value: v,

lib/moonshot/default_strategy.rb

@@ -1,13 +1,12 @@
 module Moonshot
   module ParameterStrategy
-    # Default strategy: grabbing every parameter
-    # from the parameter file.
+    # Default strategy: use parameter defined in the parameter file
     class DefaultStrategy
-      def parameters(_, overrides)
-        overrides.map do |key, _|
+      def parameters(params, _, _)
+        params.map do |key, _|
           {
             parameter_key: key,
-            parameter_value: overrides[key],
+            parameter_value: params[key],
             use_previous_value: false
           }
         end

lib/moonshot/merge_strategy.rb

@@ -3,16 +3,19 @@
 
 module Moonshot
   module ParameterStrategy
-    # Merge strategy: merging parameter values
-    # declared in the parameter file, otherwise
-    # using previous value.
+    # Merge strategy: prefer parameter values defined in the parameter file,
+    # otherwise use the previously set value on the existing stack.
     class MergeStrategy
-      def parameters(current, overrides)
-        current.map do |key, _|
-          if overrides[key]
+      def parameters(params, stack_params, template)
+        stack_keys = stack_params.keys.select do |k|
+          template.parameters.any? { |p| p.name == k }
+        end
+
+        (params.keys + stack_keys).uniq.map do |key|
+          if params[key]
             {
               parameter_key: key,
-              parameter_value: overrides[key],
+              parameter_value: params[key],
               use_previous_value: false
             }
           else

lib/moonshot/stack.rb

@@ -138,7 +138,7 @@ def resources_of_type(type)
     # update.
     def overrides
       if File.exist?(parameters_file)
-        YAML.load_file(parameters_file)
+        YAML.load_file(parameters_file) || {}
       else
         {}
       end
@@ -260,7 +260,11 @@ def update_stack
         stack_name: @name,
         template_body: template.body,
         capabilities: ['CAPABILITY_IAM'],
-        parameters: @config.parameter_strategy.parameters(parameters, overrides)
+        parameters: @config.parameter_strategy.parameters(
+          overrides,
+          parameters,
+          template
+        )
       )
       true
     rescue Aws::CloudFormation::Errors::ValidationError => e

spec/moonshot/merge_strategy_spec.rb

@@ -0,0 +1,80 @@
+describe Moonshot::ParameterStrategy::MergeStrategy do
+  subject { described_class.new }
+
+  describe '#parameters' do
+    subject do
+      super().parameters(
+        parameters,
+        stack_parameters,
+        template
+      )
+    end
+
+    let(:parameters) do
+      {
+        file_parameter_1: 'file_parameter_1',
+        file_parameter_2: 'file_parameter_2'
+      }
+    end
+
+    let(:stack_parameters) do
+      {
+        stack_parameter_1: 'stack_parameter_1',
+        stack_parameter_2: 'stack_parameter_2'
+      }
+    end
+
+    let(:template_parameters) do
+      [
+        double('template_param', name: :stack_parameter_1),
+        double('template_param', name: :stack_parameter_2)
+      ]
+    end
+
+    let(:template) do
+      template = double(Moonshot::StackTemplate)
+      allow(template).to receive(:parameters).and_return(template_parameters)
+      template
+    end
+
+    let(:actual_keys) do
+      subject.map { |p| p[:parameter_key] }
+    end
+
+    it 'includes the stack and YAML file parameters' do
+      expected_keys = (parameters.keys + stack_parameters.keys).uniq
+      expect(actual_keys).to eq(expected_keys)
+    end
+
+    context 'when the template no longer includes a stack parameter' do
+      let(:removed_element) do
+        template_parameters.pop
+      end
+
+      it 'does not include the removed parameter' do
+        removed_key = removed_element.name
+        expect(actual_keys).not_to include(removed_key)
+      end
+    end
+
+    context 'when the template adds a new parameter and it is provided' do
+      let(:added_parameter) do
+        double('template_param', name: :template_parameter_1)
+      end
+
+      let(:template_parameters) do
+        super().insert(added_parameter)
+      end
+
+      let(:parameters) do
+        parameters = super()
+        parameters[added_parameter.name] = 'template_parameter_1'
+        parameters
+      end
+
+      it 'includes the added parameter' do
+        expect(actual_keys).to include(added_parameter.name)
+      end
+    end
+  end
+end