New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get rid of default predeploy_commands.rb #59
Changes from 9 commits
8c175d0
7085d75
ed154f1
b8db3b7
72af1ce
ad01133
e0de1f6
ff4d61c
d34e981
471ba9d
c917024
67a50b3
b52da6e
0e63131
58cc4a5
23fbe5d
d70654a
8a73eae
d2a1cb8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,6 @@ | |
require 'broadside/command' | ||
require 'broadside/target' | ||
require 'broadside/deploy' | ||
require 'broadside/predeploy_commands' | ||
require 'broadside/ecs/ecs_deploy' | ||
require 'broadside/ecs/ecs_manager' | ||
require 'broadside/version' | ||
|
@@ -25,21 +24,15 @@ def self.configure | |
end | ||
|
||
def self.load_config(config_file) | ||
begin | ||
load USER_CONFIG_FILE if File.exists?(USER_CONFIG_FILE) | ||
rescue LoadError => e | ||
error "Encountered an error loading system configuration file '#{USER_CONFIG_FILE}' !" | ||
raise e | ||
end | ||
raise ArgumentError, "#{config_file} does not exist" unless File.exist?(config_file) | ||
|
||
config.config_file = config_file | ||
begin | ||
config.config_file = config_file | ||
load config_file | ||
rescue LoadError => e | ||
error "Encountered an error loading required configuration file '#{config_file}' !" | ||
raise e | ||
[config_file, USER_CONFIG_FILE].each { |file| load file if File.exist?(file) } | ||
rescue LoadError | ||
error "Encountered an error loading broadside Configuration" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols. |
||
raise | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, does this just reraise the same error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea |
||
end | ||
|
||
raise ArgumentError, config.errors.full_messages unless config.valid? | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,7 @@ def deploy_rollback(options) | |
end | ||
|
||
def status(options) | ||
target = Broadside.config.target_from_name!(options[:target]) | ||
target = Broadside.config.get_target_by_name!(options[:target]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lint/UselessAssignment: Useless assignment to variable - target. |
||
|
||
info "Getting status information about #{target.family}" | ||
ips = EcsManager.get_running_instance_ips(target.cluster, target.family) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,15 +25,17 @@ class Configuration | |
validates_each(:aws) do |record, _, val| | ||
[:region, :credentials].each { |v| record.errors.add("aws.#{v}") unless val.public_send(v) } | ||
end | ||
validates_each :ssh, allow_nil: true do |record, attr, val| | ||
record.errors.add(attr, 'is not a hash') unless val.is_a?(Hash) | ||
record.errors.add(attr, 'must contain a user') unless val[:user] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for the user requirement (someone may have configured a default user in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would the command then be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point. i'll address the |
||
end | ||
|
||
def initialize | ||
@logger = ::Logger.new(STDOUT) | ||
@logger.level = ::Logger::DEBUG | ||
@logger.datetime_format = '%Y-%m-%d_%H:%M:%S' | ||
@timeout = 600 | ||
@type = 'ecs' | ||
@ssh = {} | ||
@targets = {} | ||
end | ||
|
||
def aws | ||
|
@@ -44,29 +46,31 @@ def ecs | |
@ecs ||= EcsConfig.new | ||
end | ||
|
||
def ssh_cmd(ip, options = { tty: false }) | ||
def ssh_cmd(ip, options = {}) | ||
raise MissingVariableError, 'ssh not configured' unless @ssh | ||
|
||
cmd = 'ssh -o StrictHostKeyChecking=no' | ||
cmd << ' -t -t' if options[:tty] | ||
cmd << " -i #{@ssh[:keyfile]}" if @ssh[:keyfile] | ||
if (proxy = @ssh[:proxy]) | ||
raise ArgumentError, "Bad proxy host/port: #{proxy[:host]}/#{proxy[:port]}" unless proxy[:host] && proxy[:port] | ||
raise ArgumentError, "Bad proxy: #{proxy[:host]}/#{proxy[:port]}" unless proxy[:host] && proxy[:port] | ||
cmd << " -o ProxyCommand=\"ssh #{proxy[:host]} nc #{ip} #{proxy[:port]}\"" | ||
end | ||
cmd << " #{@ssh[:user]}@#{ip}" | ||
cmd | ||
end | ||
|
||
# Transform deploy target configs to Target objects | ||
def targets=(_targets) | ||
raise ArgumentError, ":targets must be a hash" unless _targets.is_a?(Hash) | ||
# transform deploy target configs to target objects | ||
|
||
@targets = _targets.inject({}) do |h, (target_name, config)| | ||
h[target_name] = Target.new(target_name, config) | ||
h | ||
h.merge(target_name => Target.new(target_name, config)) | ||
end | ||
end | ||
|
||
def target_from_name!(name) | ||
@targets.fetch(name) { |k| raise Error, "Deploy target '#{name}' does not exist!" } | ||
def get_target_by_name!(name) | ||
@targets.fetch(name) { |k| raise ArgumentError, "Deploy target '#{name}' does not exist!" } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lint/UnusedBlockArgument: Unused block argument - k. You can omit the argument if you don't care about it. |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,17 +63,16 @@ def get_running_instance_ips(cluster, family, task_arns = nil) | |
end | ||
|
||
def get_task_arns(cluster, family, filter = {}) | ||
opts = { | ||
options = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🆒 |
||
cluster: cluster, | ||
family: family, | ||
# Strange AWS restriction requires absence of family if service_name specified | ||
family: filter[:service_name] ? nil : family, | ||
desired_status: filter[:desired_status], | ||
service_name: filter[:service_name], | ||
started_by: filter[:started_by] | ||
} | ||
# strange AWS restriction requires absence of family if service_name specified | ||
opts[:family] = nil if opts[:service_name] | ||
opts.delete_if { |_k, v| v.nil? } | ||
all_results(:list_tasks, :task_arns, opts) | ||
}.reject { |_, v| v.nil? } | ||
|
||
all_results(:list_tasks, :task_arns, options) | ||
end | ||
|
||
def get_task_definition_arns(family) | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,13 @@ | ||
require 'spec_helper' | ||
|
||
describe Broadside::Target do | ||
include_context 'deploy configuration' | ||
|
||
let(:target_name) { 'tarbaby_target' } | ||
include_context 'deploy configuration' | ||
|
||
shared_examples 'valid_configuration?' do |succeeds, config_hash| | ||
#let(:command) { %w(do something) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style/LeadingCommentSpace: Missing space after #. |
||
let(:valid_options) { { scale: 100 } } | ||
let(:target) { described_class.new(target_name, valid_options.merge(config_hash) )} | ||
let(:target) { described_class.new(test_target_name, valid_options.merge(config_hash) )} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style/SpaceInsideParens: Space inside parentheses detected. |
||
|
||
it 'validates target configuration' do | ||
if succeeds | ||
|
@@ -30,24 +30,21 @@ | |
it_behaves_like 'valid_configuration?', true, env_files: ['file', 'file2'] | ||
|
||
it_behaves_like 'valid_configuration?', true, command: nil | ||
it_behaves_like 'valid_configuration?', true, command: ['bundle', 'exec', 'resque:work'] | ||
it_behaves_like 'valid_configuration?', false, command: 'bundle exec rails s' | ||
it_behaves_like 'valid_configuration?', true, command: %w(do something) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW i am super annoyed that you can't use |
||
it_behaves_like 'valid_configuration?', false, command: 'do something' | ||
|
||
it_behaves_like 'valid_configuration?', false, not_a_param: 'foo' | ||
|
||
it_behaves_like 'valid_configuration?', true, predeploy_commands: nil | ||
it_behaves_like 'valid_configuration?', false, predeploy_commands: Broadside::PredeployCommands::RAKE_DB_MIGRATE | ||
it_behaves_like 'valid_configuration?', false, predeploy_commands: 'bundle exec rake db:migrate' | ||
it_behaves_like 'valid_configuration?', true, predeploy_commands: [Broadside::PredeployCommands::RAKE_DB_MIGRATE] | ||
it_behaves_like 'valid_configuration?', true, predeploy_commands: [ | ||
Broadside::PredeployCommands::RAKE_DB_MIGRATE, | ||
['bundle', 'exec', 'rake' 'assets:precompile'] | ||
] | ||
it_behaves_like 'valid_configuration?', false, predeploy_commands: %w(do something) | ||
it_behaves_like 'valid_configuration?', true, predeploy_commands: [%w(do something)] | ||
it_behaves_like 'valid_configuration?', true, predeploy_commands: [%w(do something), %w(other command)] | ||
end | ||
|
||
describe '#ecs_env_vars' do | ||
let(:valid_options) { { scale: 1, env_files: env_files } } | ||
let(:target) { described_class.new(target_name, valid_options) } | ||
let(:target) { described_class.new(test_target_name, valid_options) } | ||
let(:dot_env_file) { File.join(FIXTURES_PATH, '.env.rspec') } | ||
|
||
shared_examples 'successfully loaded env_files' do | ||
it 'loads environment variables from a file' do | ||
|
@@ -59,7 +56,7 @@ | |
let(:env_files) { dot_env_file } | ||
let(:expected_env_vars) do | ||
[ | ||
{ 'name' => 'TEST_KEY1', 'value' => 'TEST_VALUE1'}, | ||
{ 'name' => 'TEST_KEY1', 'value' => 'TEST_VALUE1' }, | ||
{ 'name' => 'TEST_KEY2', 'value' => 'TEST_VALUE2'} | ||
] | ||
end | ||
|
@@ -72,8 +69,8 @@ | |
let(:expected_env_vars) do | ||
[ | ||
{ 'name' => 'TEST_KEY1', 'value' => 'TEST_VALUE1' }, | ||
{ 'name' => 'TEST_KEY2', 'value' => 'TEST_VALUE_OVERRIDE'}, | ||
{ 'name' => 'TEST_KEY3', 'value' => 'TEST_VALUE3'} | ||
{ 'name' => 'TEST_KEY2', 'value' => 'TEST_VALUE_OVERRIDE' }, | ||
{ 'name' => 'TEST_KEY3', 'value' => 'TEST_VALUE3' } | ||
] | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config_file should be loaded last, as it has precedence. also, could we add debug statements before each load? Like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh dip great catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(i now officially feel guilty for deleting the spec that checks load order - when i was unifying all the spec contexts that particular spec caused some difficulties and i thought these lines would force a call order but i just checked and they do not. 😞 )