Enable setting up a new ELB during bootstrap #76
Conversation
spec/broadside/target_spec.rb
Outdated
| load_balancers: [{ load_balancer_name: 'x' }] | ||
| }, | ||
| load_balancer_config: { | ||
| subnets: ['abc', 'xyz'] |
There was a problem hiding this comment.
Style/WordArray: Use %w or %W for an array of words.
| it_behaves_like 'valid_configuration?', false, task_definition_config: { container_definitions: %w(a b) } | ||
|
|
||
| it_behaves_like 'valid_configuration?', true, { service_config: { load_balancers: [{ load_balancer_name: 'x' }] } } | ||
| it_behaves_like 'valid_configuration?', true, { |
There was a problem hiding this comment.
Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
spec/broadside/target_spec.rb
Outdated
| it_behaves_like 'valid_configuration?', false, task_definition_config: { container_definitions: %w(a b) } | ||
| it_behaves_like 'valid_configuration?', false, task_definition_config: { container_definitions: %w(a b) } | ||
|
|
||
| it_behaves_like 'valid_configuration?', true, { service_config: { load_balancers: [{ load_balancer_name: 'x' }] } } |
There was a problem hiding this comment.
Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
|
|
||
| context 'load balancers' do | ||
| let(:elb_name) { 'my-load-balancer' } | ||
| let(:elb_config) { { name: elb_name, subnets: [ 'subnet-xyz', 'subnet-abc'] } } |
There was a problem hiding this comment.
Style/SpaceInsideBrackets: Space inside square brackets detected.
|
|
||
| expect(ecs_stub).to receive(:create_service).with(service_config_args).and_call_original | ||
|
|
||
| expect { deploy.bootstrap}.to_not raise_error |
There was a problem hiding this comment.
Style/SpaceInsideBlockBraces: Space missing inside }.
| load_balancers: [ | ||
| { | ||
| availability_zones: [{ subnet_id: "notorious-subnet", zone_name: 'zone' }], | ||
| canonical_hosted_zone_id: "ZEXAMPLE", |
There was a problem hiding this comment.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| { | ||
| load_balancers: [ | ||
| { | ||
| availability_zones: [{ subnet_id: "notorious-subnet", zone_name: 'zone' }], |
There was a problem hiding this comment.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| end | ||
|
|
||
| context 'with a load_balancer_config' do | ||
| let(:elb_config) { { subnets: [ 'subnet-xyz', 'subnet-abc'] } } |
There was a problem hiding this comment.
Style/SpaceInsideBrackets: Space inside square brackets detected.
lib/broadside/target.rb
Outdated
| validates_each(:load_balancer_config, allow_nil: true) do |record, attr, val| | ||
| record.errors.add(attr, 'is not a hash') unless val.is_a?(Hash) | ||
| record.errors.add(attr, ':load_balancer_name is set automatically') if val[:load_balancer_name] | ||
| record.errors.add(attr, ":subnets is required") unless val[:subnets] && val[:subnets].is_a?(Array) |
There was a problem hiding this comment.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
lib/broadside/ecs/ecs_manager.rb
Outdated
| end | ||
|
|
||
| def get_load_balancer_arn_by_name(name) | ||
| load_balancers = elb_client.describe_load_balancers().load_balancers |
There was a problem hiding this comment.
Style/MethodCallParentheses: Do not use parentheses for method calls with no arguments.
| EcsManager.create_service(cluster, family, @target.service_config) | ||
|
|
||
| if @target.load_balancer_config | ||
| container_port = EcsManager.get_latest_task_definition(family)[:container_definitions].first[:port_mappings].first[:container_port] |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [141/120]
|
@apurvis - @slpsys and I talked about this, we reached consensus that broadside should not concern itself with this kind of infrastructure setup. This would additionally require more IAM privileges than broadside should need. I think broadside should focus itself as a deployment tool for developers and ELB creation is more of an ops-y thing that should be separate. Appreciate the legwork you did here though. |
I took a crack at this. Right now the ELB is just named the same as the
familyand I haven't actually tested this.Definitely doesn't need to be merged before we cut the 3.0 release.
Need more guidance/understanding of what to do about internal vs. external facing default and what to do about the ports.
fixes #37