Bogus servers should not be killable (fixes infochimps-labs#250)

* style guide corrections ('&&', not 'and'; don't use single-letter variable names)
* also fixed a thing where security groups enumeration would die if a bogus server existed that's bit me before
* reviewed other knife cluster commands and made them not tolerate bogosity either

lib/chef/knife/cluster_bootstrap.rb

@@ -62,7 +62,6 @@ def perform_execution(target)
         ensure_common_environment(target)
         # Execute across all servers in parallel
         Ironfan.parallel(target.values) {|computer| run_bootstrap(computer)}
-#         threads = target.servers.map{ |server| Thread.new(server) { |svr| run_bootstrap(svr, svr.public_hostname) } }
       end
 
       def confirm_execution(target)

lib/chef/knife/cluster_start.rb

@@ -24,7 +24,7 @@ class ClusterStart < Ironfan::Script
       import_banner_and_options(Ironfan::Script)
 
       def relevant?(server)
-        server.stopped?
+        (not bogus?) && stopped?
       end
 
       def perform_execution(target)

lib/chef/knife/cluster_stop.rb

@@ -24,7 +24,7 @@ class ClusterStop < Ironfan::Script
       import_banner_and_options(Ironfan::Script)
 
       def relevant?(server)
-        server.running?
+        (not bogus?) && server.running?
       end
 
       def perform_execution(target)

lib/chef/knife/ironfan_knife_common.rb

@@ -84,19 +84,18 @@ def confirm_execution(*args)
     #
     def get_relevant_slice( *predicate )
       full_target = get_slice( *predicate )
-      display(full_target) do |m|
-        rel = relevant?(m)
-        { :relevant? => (rel ? "[blue]#{rel}[reset]" : '-' ) }
+      display(full_target) do |mach|
+        rel = relevant?(mach)
+        { :relevant? => (rel ? "[green]#{rel}[reset]" : '-' ) }
       end
-      full_target.select{|m| relevant?(m) }
+      full_target.select{|mach| relevant?(mach) }
     end
 
     # passes target to Broker::Conductor#display, will show headings in server slice
     # tables based on the --verbose flag
     def display(target, display_style=nil, &block)
       display_style ||= (config[:verbosity] == 0 ? :default : :expanded)
-#       target.display(ui, display_style, &block)
-      broker.display(target, display_style)
+      broker.display(target, display_style, &block)
     end
 
     #

lib/ironfan/broker.rb

@@ -31,11 +31,15 @@ def discover!(cluster)
     end
 
     def display(computers,style)
-      defined_data = computers.map {|m| m.to_display(style) }
+      defined_data = computers.map do |mach|
+        hsh = mach.to_display(style)
+        hsh.merge!(yield(mach)) if block_given?
+        hsh
+      end
       if defined_data.empty?
         ui.info "Nothing to report"
       else
-        headings = defined_data.map{|r| r.keys}.flatten.uniq
+        headings = defined_data.map{|hsh| hsh.keys }.flatten.uniq
         Formatador.display_compact_table(defined_data, headings.to_a)
       end
     end

lib/ironfan/broker/computer.rb

@@ -17,11 +17,11 @@ def initialize(*args)
         super
         providers[:chef] ||=    Ironfan::Provider::ChefServer
         return unless server
-        providers[:iaas] =      server.selected_cloud.provider
-        volumes =               server.volumes.values
-        volumes +=              server.implied_volumes
-        volumes.each { |v| self.drive v.name, :volume => v }
-      rescue StandardError => err ; err.polish("#{self.class} on #{args}'") rescue nil ; raise
+        providers[:iaas]   = server.selected_cloud.provider
+        volumes            = server.volumes.values
+        volumes           += server.implied_volumes
+        volumes.each{|vol| self.drive vol.name, :volume => vol }
+      rescue StandardError => err ; err.polish("#{self.class} on '#{args.inspect}'") rescue nil ; raise
       end
 
       def name
@@ -38,11 +38,11 @@ def correlate
           res.expected_ids(self).each do |id|
             next unless res.recall? id
 
-            recalled =            res.recall id
-            recalled.users <<     self
+            recalled =  res.recall id
+            recalled.users << self
 
-            target =              res.resource_type.to_s
-            target +=             "__#{id}" if res.multiple?
+            target  = res.resource_type.to_s
+            target += "__#{id}" if res.multiple?
             self[target.to_sym] = recalled
 
             recalled.on_correlate(self)
@@ -130,14 +130,6 @@ def chef_client_script_content
         @chef_client_script_content = Ironfan.safely{ File.read(script_filename) }
       end
 
-#       def ensure_resource(type)
-#         if type.multiple?
-#           existing = resources[type.resource_id]
-#         else
-#
-#         end
-#       end
-
       #
       # Display
       #
@@ -200,7 +192,7 @@ def machine=(value)
         self[:machine] = value
       end
       def dns_name         ; machine? ? machine.dns_name : nil ; end
-      def bootstrap_distro ; (server && server.selected_cloud) ? server.selected_cloud.bootstrap_distro   : nil ; end
+      def bootstrap_distro ; (server && server.selected_cloud) ? server.selected_cloud.bootstrap_distro : nil ; end
 
       def keypair
         resources[:keypair]
@@ -225,16 +217,16 @@ def client?
         not client.nil?
       end
       def created?
-        machine? and machine.created?
+        machine? && machine.created?
       end
       def machine?
         not machine.nil?
       end
       def killable?
-        not permanent? and (node? || client? || created?)
+        (not bogus?) && (not permanent?) && (node? || client? || created?)
       end
       def launchable?
-        not bogus? and not created?
+        (not bogus?) && (not created?)
       end
       def node?
         not node.nil?
@@ -245,13 +237,13 @@ def permanent?
         [true, :true, 'true'].include? server.selected_cloud.permanent
       end
       def running?
-        machine? and machine.running?
+        machine? && machine.running?
       end
       def server?
         not server.nil?
       end
       def stopped?
-        machine? and machine.stopped?
+        machine? && machine.stopped?
       end
 
       # @return [Boolean] true if machine is likely to be reachable by ssh
@@ -295,15 +287,15 @@ def correlate
 
       def validate
         computers = self
-        values.each{|c|    c.validate }
+        values.each{|c| c.validate }
         values.map {|c| c.providers.values}.flatten.uniq.each {|p| p.validate computers }
       end
 
       def group_action(verb)
         computers = self
-        provider_keys = values.map {|c| c.chosen_providers({ :providers => :iaas })}.flatten.uniq
-        providers     = provider_keys.map { |pk| values.map { |c| c.providers[pk] } }.flatten.compact.uniq
-        providers.each { |p| p.send(verb, computers) }
+        provider_keys = values.map{|c| c.chosen_providers({ :providers => :iaas })}.flatten.uniq
+        providers     = provider_keys.map{|pk| values.map{|c| c.providers[pk] } }.flatten.compact.uniq
+        providers.each{|p| p.send(verb, computers) }
       end
 
       def prepare
@@ -318,19 +310,19 @@ def aggregate
       # Manipulation
       #
       def kill(options={})
-        Ironfan.parallel(values) {|c| c.kill(options) }
+        Ironfan.parallel(values){|cc| cc.kill(options) }
       end
       def launch
-        Ironfan.parallel(values) {|c| c.launch }
+        Ironfan.parallel(values){|cc| cc.launch }
       end
       def save(options={})
-        Ironfan.parallel(values) {|c| c.save(options) }
+        Ironfan.parallel(values){|cc| cc.save(options) }
       end
       def start
-        Ironfan.parallel(values) {|c| c.start }
+        Ironfan.parallel(values){|cc| cc.start }
       end
       def stop
-        Ironfan.parallel(values) {|c| c.stop }
+        Ironfan.parallel(values){|cc| cc.stop }
       end
 
       def environments
@@ -351,7 +343,7 @@ def create_expected!
       # Return the selection inside another Computers collection
       def select(&block)
         result = empty_copy
-        values.select(&block).each{|m| result << m}
+        values.select(&block).each{|mach| result << mach}
         result
       end
 
@@ -363,16 +355,14 @@ def empty_copy
 
       # Find all selected computers, as well as any bogus computers from discovery
       def slice(facet_name=nil, slice_indexes=nil)
-        return self if (facet_name.nil? and slice_indexes.nil?)
-        result          = empty_copy
+        return self if (facet_name.nil? && slice_indexes.nil?)
         slice_array     = build_slice_array(slice_indexes)
-        each do |m|
-          result << m if (m.bogus? or (                 # bogus computer or
-            ( m.server.facet_name == facet_name ) and   # facet match and
-              ( slice_array.include? m.server.index or  #   index match or
-                slice_indexes.nil? ) ) )                #   no indexes specified
+        select do |mach|
+          mach.bogus? or (
+            # facet match, and index match (or no indexes specified)
+            (mach.server.facet_name == facet_name) &&
+            (slice_array.include?(mach.server.index) || slice_indexes.nil?))
         end
-        result
       end
 
       def build_slice_array(slice_indexes)

lib/ironfan/provider/ec2/security_group.rb

@@ -19,6 +19,7 @@ def self.shared?()      true;   end
         def self.multiple?()    true;   end
         def self.resource_type()        :security_group;   end
         def self.expected_ids(computer)
+          return unless computer.server
           ec2 = computer.server.cloud(:ec2)
           ec2.security_groups.keys.map { |name| group_name_with_vpc(name,ec2.vpc) }.uniq
         end
@@ -73,7 +74,7 @@ def self.prepare!(computers)
           # First, deduce the list of all groups to which at least one instance belongs
           # We'll use this later to decide whether to create groups, or authorize access,
           # using a VPC security group or an EC2 security group.
-          groups_that_should_exist = computers.map { |c| expected_ids(c) }.flatten.sort.uniq
+          groups_that_should_exist = computers.map{|comp| expected_ids(comp) }.flatten.compact.sort.uniq
           groups_to_create << groups_that_should_exist
 
           computers.select { |computer| Ec2.applicable computer }.each do |computer|