Get rid of default predeploy_commands.rb #59

apurvis · 2017-02-02T19:13:22Z

rename target_from_name! to get_target_by_name!
trivial style changes to cut down on some lines
fix bug with missing SSH user

there are no functionality changes in here except getting rid of predeploy_commands.rb; everything else is just style.

houndci-bot · 2017-02-02T19:16:32Z

spec/broadside/target_spec.rb

    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) )}


Style/SpaceInsideParens: Space inside parentheses detected.
Style/SpaceInsideBlockBraces: Space missing inside }.

houndci-bot · 2017-02-02T19:16:33Z

spec/broadside/target_spec.rb


  shared_examples 'valid_configuration?' do |succeeds, config_hash|
+    #let(:command) { %w(do something) }


Style/LeadingCommentSpace: Missing space after #.

houndci-bot · 2017-02-02T19:16:33Z

lib/broadside/configuration.rb

-    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!" }


Lint/UnusedBlockArgument: Unused block argument - k. You can omit the argument if you don't care about it.

houndci-bot · 2017-02-02T19:16:33Z

lib/broadside.rb

-    end
-
+    raise ArgumentError, "#{config_file} does not exist" unless File.exist?(config_file)
+


Style/TrailingWhitespace: Trailing whitespace detected.

houndci-bot · 2017-02-02T19:34:57Z

lib/broadside.rb

-      raise e
+      [config_file, USER_CONFIG_FILE].each { |file| load file if File.exist?(file) }
+    rescue LoadError
+      error "Encountered an error loading broadside Configuration"


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2017-02-03T00:00:21Z

lib/broadside/command.rb

        debug "Getting status information about #{family}"
+


Style/TrailingWhitespace: Trailing whitespace detected.

houndci-bot · 2017-02-03T00:00:22Z

lib/broadside/command.rb

-        family = target.family
-        pastel = Pastel.new
-
+        target = Broadside.config.get_target_by_name!(options[:target])


Lint/UselessAssignment: Useless assignment to variable - target.

matl33t · 2017-02-03T00:05:25Z

lib/broadside.rb

-    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) }


config_file should be loaded last, as it has precedence. also, could we add debug statements before each load? Like

debug "Loading broadside configuration at #{file}"

oh dip great catch

(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. 😞 )

matl33t · 2017-02-03T00:05:59Z

lib/broadside.rb

+      [config_file, USER_CONFIG_FILE].each { |file| load file if File.exist?(file) }
+    rescue LoadError
+      error 'Encountered an error loading broadside configuration'
+      raise


oh, does this just reraise the same error?

…essed

matl33t · 2017-02-03T00:16:29Z

lib/broadside/configuration.rb

@@ -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]


no need for the user requirement (someone may have configured a default user in ~/.ssh/config)

would the command then be ssh @123.123.123.123? wouldn't you need to ditch the @?

good point. i'll address the @ with #11

matl33t · 2017-02-03T00:17:43Z

lib/broadside/ecs/ecs_manager.rb

@@ -63,17 +63,16 @@ def get_running_instance_ips(cluster, family, task_arns = nil)
      end

      def get_task_arns(cluster, family, filter = {})
-        opts = {
+        options = {


apurvis · 2017-02-03T00:18:29Z

spec/broadside/target_spec.rb

@@ -30,24 +28,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)


FWIW i am super annoyed that you can't use let(:command) { %w(do something) } in an it_behaves_like expression. oh well.

houndci-bot · 2017-02-03T00:24:48Z

lib/broadside/configuration.rb

        cmd << " -o ProxyCommand=\"ssh #{proxy[:host]} nc #{ip} #{proxy[:port]}\""
      end
-      cmd << " #{@ssh[:user]}@#{ip}"
+      cmd << " #{@ssh[:user]}@" if @ssh[:user]
+      cmd << "#{ip}"


Style/UnneededInterpolation: Prefer to_s over string interpolation.

apurvis@lumoslabs.com added 8 commits February 2, 2017 02:59

no need for variable context

8c175d0

rename get_target_by_name; put some spec stuff more where it should be

7085d75

Get rid of stupid predeploy commands constants

ed154f1

Validate ssh config

b8db3b7

less code

72af1ce

MissingVariableError

ad01133

Cleaner

e0de1f6

move raise

ff4d61c

apurvis assigned matl33t Feb 2, 2017

houndci-bot reviewed Feb 2, 2017

View reviewed changes

apurvis@lumoslabs.com added 2 commits February 2, 2017 11:26

Error message

d34e981

Remove comment

471ba9d

houndci-bot reviewed Feb 2, 2017

View reviewed changes

apurvis@lumoslabs.com added 2 commits February 2, 2017 11:37

hound

c917024

Merge master

67a50b3

apurvis mentioned this pull request Feb 3, 2017

Rebase commits from print_more_status #60

Merged

houndci-bot reviewed Feb 3, 2017

View reviewed changes

Undo merge damage

b52da6e

matl33t reviewed Feb 3, 2017

View reviewed changes

apurvis@lumoslabs.com added 3 commits February 2, 2017 16:07

Appease hound; add a basic spec for Configuration object

0e63131

Fix load order; print debug statements about which file is being proc…

58cc4a5

…essed

Use rspec .ordered feature

23fbe5d

matl33t reviewed Feb 3, 2017

View reviewed changes

apurvis commented Feb 3, 2017

View reviewed changes

Deal with the case of the missing ssh user

d70654a

houndci-bot reviewed Feb 3, 2017

View reviewed changes

hound

8a73eae

matl33t approved these changes Feb 3, 2017

View reviewed changes

Use MissingVariableError instead of ArgumentError on missing proxy

d2a1cb8

apurvis merged commit 1c8f9e0 into master Feb 3, 2017

apurvis deleted the tweaks branch February 3, 2017 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of default predeploy_commands.rb #59

Get rid of default predeploy_commands.rb #59

apurvis commented Feb 2, 2017 •

edited

houndci-bot Feb 2, 2017

houndci-bot Feb 2, 2017

houndci-bot Feb 2, 2017

houndci-bot Feb 2, 2017

houndci-bot Feb 2, 2017

houndci-bot Feb 3, 2017

houndci-bot Feb 3, 2017

matl33t Feb 3, 2017

apurvis Feb 3, 2017

apurvis Feb 3, 2017

matl33t Feb 3, 2017

apurvis Feb 3, 2017

matl33t Feb 3, 2017

apurvis Feb 3, 2017

matl33t Feb 3, 2017

matl33t Feb 3, 2017

apurvis Feb 3, 2017

houndci-bot Feb 3, 2017


		shared_examples 'valid_configuration?' do \|succeeds, config_hash\|
		#let(:command) { %w(do something) }

		end

		raise ArgumentError, "#{config_file} does not exist" unless File.exist?(config_file)

Get rid of default predeploy_commands.rb #59

Get rid of default predeploy_commands.rb #59

Conversation

apurvis commented Feb 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apurvis commented Feb 2, 2017 •

edited