From 403e00a4aac81a0321a5e3c4be127105a4803882 Mon Sep 17 00:00:00 2001 From: Marc Bollinger Date: Sat, 25 Feb 2017 11:43:10 -0800 Subject: [PATCH 1/8] Create/Update have different tolerances --- lib/broadside/ecs/ecs_deploy.rb | 2 +- lib/broadside/target.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/broadside/ecs/ecs_deploy.rb b/lib/broadside/ecs/ecs_deploy.rb index 1a655d5..a7f79f7 100644 --- a/lib/broadside/ecs/ecs_deploy.rb +++ b/lib/broadside/ecs/ecs_deploy.rb @@ -152,7 +152,7 @@ def update_service(options = {}) desired_count: scale, service: family, task_definition: task_definition_arn - }.deep_merge(@target.service_config || {})) + }.deep_merge(@target.update_safe_service_config || {})) unless update_service_response.successful? raise EcsError, "Failed to update service:\n#{update_service_response.pretty_inspect}" diff --git a/lib/broadside/target.rb b/lib/broadside/target.rb index cf8050e..95d2df3 100644 --- a/lib/broadside/target.rb +++ b/lib/broadside/target.rb @@ -93,5 +93,16 @@ def to_h Cluster: @cluster } end + + def update_safe_service_config + attributes_disallowed_for_updates = %i( + role + load_balancers + ) + + service_config.delete_if do |k, _| + attributes_disallowed_for_updates.contains?(k) + end + end end end From 160f91a669d075b392e3016905231fcfd7a8c444 Mon Sep 17 00:00:00 2001 From: Marc Bollinger Date: Sat, 25 Feb 2017 12:12:37 -0800 Subject: [PATCH 2/8] That's why we have specs --- lib/broadside/target.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/broadside/target.rb b/lib/broadside/target.rb index 95d2df3..a23f6ac 100644 --- a/lib/broadside/target.rb +++ b/lib/broadside/target.rb @@ -101,7 +101,7 @@ def update_safe_service_config ) service_config.delete_if do |k, _| - attributes_disallowed_for_updates.contains?(k) + attributes_disallowed_for_updates.include?(k) end end end From eea15685a5d02ffec43ee33fc07705f54186579b Mon Sep 17 00:00:00 2001 From: Marc Bollinger Date: Sun, 26 Feb 2017 18:59:13 -0800 Subject: [PATCH 3/8] Style updates for create/update filter --- lib/broadside/target.rb | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/broadside/target.rb b/lib/broadside/target.rb index a23f6ac..63128a2 100644 --- a/lib/broadside/target.rb +++ b/lib/broadside/target.rb @@ -44,6 +44,14 @@ class Target record.errors.add(attr, 'is not an array of strings') unless val.is_a?(Array) && val.all? { |v| v.is_a?(String) } end + CREATE_ONLY_SERVICE_ATTRIBUTES = %i( + client_token + load_balancers + placement_constraints + placement_strategy + role + ).freeze + def initialize(name, options = {}) @name = name @@ -94,15 +102,8 @@ def to_h } end - def update_safe_service_config - attributes_disallowed_for_updates = %i( - role - load_balancers - ) - - service_config.delete_if do |k, _| - attributes_disallowed_for_updates.include?(k) - end + def service_config_for_update + service_config.except(CREATE_ONLY_SERVICE_ATTRIBUTES) end end end From 113410507e1bc4cb61c142c34b6cd6577b3b0be2 Mon Sep 17 00:00:00 2001 From: Marc Bollinger Date: Sun, 26 Feb 2017 19:21:55 -0800 Subject: [PATCH 4/8] Service config isn't always present --- lib/broadside/ecs/ecs_deploy.rb | 2 +- lib/broadside/target.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/broadside/ecs/ecs_deploy.rb b/lib/broadside/ecs/ecs_deploy.rb index a7f79f7..e6f068f 100644 --- a/lib/broadside/ecs/ecs_deploy.rb +++ b/lib/broadside/ecs/ecs_deploy.rb @@ -152,7 +152,7 @@ def update_service(options = {}) desired_count: scale, service: family, task_definition: task_definition_arn - }.deep_merge(@target.update_safe_service_config || {})) + }.deep_merge(@target.service_config_for_update || {})) unless update_service_response.successful? raise EcsError, "Failed to update service:\n#{update_service_response.pretty_inspect}" diff --git a/lib/broadside/target.rb b/lib/broadside/target.rb index 63128a2..f4776f6 100644 --- a/lib/broadside/target.rb +++ b/lib/broadside/target.rb @@ -103,7 +103,7 @@ def to_h end def service_config_for_update - service_config.except(CREATE_ONLY_SERVICE_ATTRIBUTES) + service_config.try(:except, CREATE_ONLY_SERVICE_ATTRIBUTES) end end end From 36a5b2445fa4dac1102fd30e2e5e2702f620d263 Mon Sep 17 00:00:00 2001 From: Marc Bollinger Date: Tue, 28 Feb 2017 15:49:19 -0800 Subject: [PATCH 5/8] Huh. specs found a bug. --- lib/broadside/target.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/broadside/target.rb b/lib/broadside/target.rb index f4776f6..652fa6f 100644 --- a/lib/broadside/target.rb +++ b/lib/broadside/target.rb @@ -103,7 +103,7 @@ def to_h end def service_config_for_update - service_config.try(:except, CREATE_ONLY_SERVICE_ATTRIBUTES) + service_config.try(:except, *CREATE_ONLY_SERVICE_ATTRIBUTES) end end end From bc3bb2dee0a7d1dd374d7c6651c21bc4b29d6121 Mon Sep 17 00:00:00 2001 From: Marc Bollinger Date: Tue, 28 Feb 2017 15:53:09 -0800 Subject: [PATCH 6/8] Deploy spec --- spec/broadside/ecs/ecs_deploy_spec.rb | 39 ++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/spec/broadside/ecs/ecs_deploy_spec.rb b/spec/broadside/ecs/ecs_deploy_spec.rb index c77da03..75efb39 100644 --- a/spec/broadside/ecs/ecs_deploy_spec.rb +++ b/spec/broadside/ecs/ecs_deploy_spec.rb @@ -10,7 +10,7 @@ let(:desired_count) { 4 } let(:cpu) { 1 } let(:memory) { 2000 } - let(:service_config) do + let(:base_service_config) do { desired_count: desired_count, deployment_configuration: { @@ -18,6 +18,19 @@ } } end + let(:service_config_with_load_balancers) do + base_service_config.merge( + role: 'ecsServiceRole', + load_balancers: [ + { + container_name: 'test-container', + container_port: 8080, + load_balancer_name: 'test-container-elb' + } + ] + ) + end + let(:service_config) { base_service_config } describe '#bootstrap' do it 'fails without task_definition_config' do @@ -40,7 +53,7 @@ end end - context 'with an existing service' do + shared_examples 'correctly-behaving bootstrap' do include_context 'with a running service' it 'succeeds' do @@ -57,6 +70,16 @@ end end end + + context 'with an existing service' do + it_behaves_like 'correctly-behaving bootstrap' + end + + context 'with an existing task definition that has create-only parameters' do + let(:service_config) { service_config_with_load_balancers } + + it_behaves_like 'correctly-behaving bootstrap' + end end end @@ -72,7 +95,7 @@ expect { deploy.short }.to raise_error(/No task definition for/) end - context 'with an existing task definition' do + shared_examples 'correctly-behaving deploy' do include_context 'with a task_definition' it 'short deploy does not fail' do @@ -142,6 +165,16 @@ expect(api_request_methods.include?(:update_service)).to be true end end + + context 'with an existing task definition' do + it_behaves_like 'correctly-behaving deploy' + end + + context 'with an existing task definition that has create-only parameters' do + let(:service_config) { service_config_with_load_balancers } + + it_behaves_like 'correctly-behaving deploy' + end end end From 2b3264de070537c5e5627c8fd0c103cd02b58b93 Mon Sep 17 00:00:00 2001 From: Marc Bollinger Date: Tue, 28 Feb 2017 18:05:15 -0800 Subject: [PATCH 7/8] more specs --- spec/broadside/target_spec.rb | 66 +++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 18 deletions(-) diff --git a/spec/broadside/target_spec.rb b/spec/broadside/target_spec.rb index 8a2b2de..20828d7 100644 --- a/spec/broadside/target_spec.rb +++ b/spec/broadside/target_spec.rb @@ -3,23 +3,25 @@ describe Broadside::Target do include_context 'deploy configuration' - describe '#initialize' do - let(:all_possible_options) do - { - bootstrap_commands: [], - cluster: 'some-cluster', - command: %w(some command), - docker_image: 'lumoslabs/hello', - env_file: '.env.test', - predeploy_commands: [], - scale: 9000, - service_config: {}, - tag: 'latest', - task_definition_config: {} - } - end - let(:target) { described_class.new(test_target_name, all_possible_options) } + let(:base_possible_options) do + { + bootstrap_commands: [], + cluster: 'some-cluster', + command: %w(some command), + docker_image: 'lumoslabs/hello', + env_file: '.env.test', + predeploy_commands: [], + scale: 9000, + service_config: {}, + tag: 'latest', + task_definition_config: {} + } + end + let(:all_possible_options) { base_possible_options } + let(:all_possible_options_with_create_only_service_options) { base_possible_options } + let(:target) { described_class.new(test_target_name, all_possible_options) } + describe '#initialize' do it 'should initialize without erroring using all possible options' do expect { target }.to_not raise_error end @@ -46,7 +48,7 @@ it_behaves_like 'valid_configuration?', true, env_files: nil it_behaves_like 'valid_configuration?', true, env_files: 'file' - it_behaves_like 'valid_configuration?', true, env_files: ['file', 'file2'] + it_behaves_like 'valid_configuration?', true, env_files: %w(file file2) it_behaves_like 'valid_configuration?', true, command: nil it_behaves_like 'valid_configuration?', true, command: %w(do something) @@ -76,7 +78,7 @@ let(:expected_env_vars) do [ { 'name' => 'TEST_KEY1', 'value' => 'TEST_VALUE1' }, - { 'name' => 'TEST_KEY2', 'value' => 'TEST_VALUE2'} + { 'name' => 'TEST_KEY2', 'value' => 'TEST_VALUE2' } ] end @@ -96,4 +98,32 @@ it_behaves_like 'successfully loaded env_files' end end + + describe '#service_config_for_update' do + context 'with no service config' do + it 'does not raise an error' do + expect { target }.to_not raise_error + end + end + + shared_examples 'accessor for update-safe service config parameters' do + it 'does not raise an error' do + expect { target }.to_not raise_error + end + + it 'returns the basic serivce config parameters' do + expect(target.service_config).to eq(base_possible_options[:service_config]) + end + end + + context 'with a normal service config' do + it_behaves_like 'accessor for update-safe service config parameters' + end + + context 'with a service config containing create-only parameters' do + let(:all_possible_options) { all_possible_options_with_create_only_service_options } + + it_behaves_like 'accessor for update-safe service config parameters' + end + end end From e6d7d271811367262dd2855e71d7b88e44a9bec0 Mon Sep 17 00:00:00 2001 From: Marc Bollinger Date: Tue, 28 Feb 2017 18:06:47 -0800 Subject: [PATCH 8/8] Bump version --- lib/broadside/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/broadside/version.rb b/lib/broadside/version.rb index aea81e9..913ed39 100644 --- a/lib/broadside/version.rb +++ b/lib/broadside/version.rb @@ -1,3 +1,3 @@ module Broadside - VERSION = '3.0.2'.freeze + VERSION = '3.0.5'.freeze end