Set rather than merge, based on strategy. Also fix new rubocop errors.

knife-topo.gemspec

@@ -15,6 +15,6 @@ Gem::Specification.new do |spec|
   spec.license       = 'Apache License (2.0)'
 
   spec.files         = Dir.glob('{lib}/**/*') +
-    ['LICENSE', 'README.md', __FILE__]
+                       ['LICENSE', 'README.md', __FILE__]
   spec.require_paths = ['lib']
 end

lib/chef/knife/topo/command_helper.rb

@@ -62,7 +62,7 @@ def check_chef_env(chef_env_name)
     end
 
     def most_common(vals)
-      return if vals.length == 0
+      return if vals.empty?
       vals.group_by do |val|
         val
       end.values.max_by(&:size).first

lib/chef/knife/topo/consts.rb

@@ -1,4 +1,4 @@
 # constants
 module KnifeTopo
-  PRIORITIES = %w(default force_default normal override force_override)
+  PRIORITIES = %w(default force_default normal override force_override).freeze
 end

lib/chef/knife/topo/loader.rb

@@ -107,7 +107,7 @@ def create_topo_bag
     end
 
     def list_topo_bag
-      list = Chef::DataBag.load(topo_bag_name)
+      Chef::DataBag.load(topo_bag_name)
     rescue Net::HTTPServerException => e
       raise unless e.to_s =~ /^404/
       {}

lib/chef/knife/topo/node_update_helper.rb

@@ -25,7 +25,7 @@ module KnifeTopo
   # Node update helper for knife topo
   module NodeUpdateHelper
     # Update an existing node
-    def update_node(node_updates)
+    def update_node(node_updates, merge = false)
       config[:disable_editing] = true
 
       begin
@@ -34,7 +34,7 @@ def update_node(node_updates)
 
         env = node_updates['chef_environment']
         check_chef_env(env) unless env == node['chef_environment']
-        do_node_updates(node, node_updates)
+        do_node_updates(node, node_updates, merge)
 
       rescue Net::HTTPServerException => e
         raise unless e.to_s =~ /^404/
@@ -44,8 +44,8 @@ def update_node(node_updates)
       node
     end
 
-    def do_node_updates(node, node_updates)
-      updated = update_node_with_values(node, node_updates)
+    def do_node_updates(node, node_updates, merge = false)
+      updated = update_node_with_values(node, node_updates, merge)
       if updated
         ui.info "Updating #{updated.join(', ')} on node #{node.name}"
         node.save
@@ -56,11 +56,11 @@ def do_node_updates(node, node_updates)
     end
 
     # Update original node, return list of updated properties.
-    def update_node_with_values(node, updates)
+    def update_node_with_values(node, updates, merge = false)
       updated = []
 
       # merge the normal attributes (but not tags)
-      updated << 'normal' if update_attrs(node, updates['normal'])
+      updated << 'normal' if update_attrs(node, updates['normal'], merge)
 
       # update runlist
       updated << 'run_list' if update_runlist(node, updates['run_list'])
@@ -74,15 +74,20 @@ def update_node_with_values(node, updates)
       updated << 'tags' if update_tags(node, updates['tags'])
 
       # return false if no updates, else return array of property names
-      updated.length > 0 && updated
+      !updated.empty? && updated
     end
 
     # Update methods all return true if an actual update is made
-    def update_attrs(node, attrs)
+    def update_attrs(node, attrs, merge = false)
       return false unless attrs
-      attrs.delete('tags')
+      # keep the current tags
+      attrs['tags'] = node.normal.tags
       original = Marshal.load(Marshal.dump(node.normal))
-      node.normal = Chef::Mixin::DeepMerge.merge(node.normal, attrs)
+      node.normal = if merge
+                      Chef::Mixin::DeepMerge.merge(node.normal, attrs)
+                    else
+                      attrs
+                    end
       original != node.normal
     end
 

lib/chef/knife/topo/version.rb

@@ -1,4 +1,4 @@
 # version
 module KnifeTopo
-  VERSION = '2.0.3'
+  VERSION = '2.0.4'.freeze
 end

lib/chef/knife/topo_bootstrap.rb

@@ -47,7 +47,7 @@ class TopoBootstrap < Chef::Knife
     )
 
     # Make the base bootstrap options available on topo bootstrap
-    self.options = (Chef::Knife::Bootstrap.options).merge(TopoBootstrap.options)
+    self.options = Chef::Knife::Bootstrap.options.merge(TopoBootstrap.options)
 
     attr_accessor :msgs, :results
 
@@ -120,21 +120,21 @@ def node_bootstrap(node_data)
 
     # Report is used by create, update and bootstrap commands
     def report
-      if @topo['nodes'].length > 0
+      if @topo['nodes'].empty?
+        ui.info 'No nodes found'
+      else
         report_msg(:bootstrapped, :info, false) if @bootstrap
         report_msg(:skipped, :info, true)
         report_msg(:skipped_ssh, :info, true)
         report_msg(:existed, :info, true)
         report_msg(:failed, :warn, true) if @bootstrap
-      else
-        ui.info 'No nodes found'
       end
       ui.info("Topology: #{@topo.display_info}")
     end
 
     def report_msg(state, level, only_non_zero = true)
       nodes = @results[state]
-      return if only_non_zero && nodes.length == 0
+      return if only_non_zero && nodes.empty?
       ui.send(level, @msgs[state] %
         { num: nodes.length, list: nodes.join(', ') })
     end

lib/chef/knife/topo_cookbook_create.rb

@@ -43,7 +43,7 @@ class TopoCookbookCreate < Chef::Knife
 
     def initialize(args)
       super
-      @args  = args
+      @args = args
     end
 
     def run

lib/chef/knife/topo_cookbook_upload.rb

@@ -37,14 +37,14 @@ class TopoCookbookUpload < Chef::Knife
     )
 
     # Make called command options available
-    self.options = (Chef::Knife::CookbookUpload.options).merge(
+    self.options = Chef::Knife::CookbookUpload.options.merge(
       TopoCookbookUpload.options)
 
     include KnifeTopo::Loader
 
     def initialize(args)
       super
-      @args  = args
+      @args = args
 
       # All called commands need to accept union of options
       Chef::Knife::CookbookUpload.options = options

lib/chef/knife/topo_create.rb

@@ -50,7 +50,7 @@ class TopoCreate < KnifeTopo::TopoBootstrap
     # Make called command options available
     orig_opts = KnifeTopo::TopoCreate.options
     upload_opts = Chef::Knife::CookbookUpload.options
-    merged_opts = (KnifeTopo::TopoBootstrap.options).merge(upload_opts)
+    merged_opts = KnifeTopo::TopoBootstrap.options.merge(upload_opts)
     self.options = merged_opts.merge(orig_opts)
 
     include KnifeTopo::CommandHelper
@@ -59,7 +59,7 @@ class TopoCreate < KnifeTopo::TopoBootstrap
 
     def initialize(args)
       super
-      @args  = args
+      @args = args
     end
 
     def bootstrap_msgs
@@ -116,8 +116,9 @@ def create_or_update_topo
 
     def update_nodes
       nodes = processor.generate_nodes
+      merge = @topo.merge_attrs
       nodes.each do |node_data|
-        bootstrap_or_update_node(node_data)
+        bootstrap_or_update_node(node_data, merge)
       end
     end
 
@@ -130,16 +131,14 @@ def confirm_and_update_topo
       @topo.save
     end
 
-    def bootstrap_or_update_node(node_data)
+    def bootstrap_or_update_node(node_data, merge)
       node_name = node_data['name']
       if @bootstrap
-        update_node(node_data) unless node_bootstrap(node_data)
+        update_node(node_data, merge) unless node_bootstrap(node_data)
+      elsif update_node(node_data, merge)
+        @results[:existed] << node_name
       else
-        if update_node(node_data)
-          @results[:existed] << node_name
-        else
-          @results[:skipped] << node_name
-        end
+        @results[:skipped] << node_name
       end
     end
 

lib/chef/knife/topo_export.rb

@@ -82,7 +82,7 @@ def new_topo
       update_nodes!(topo['nodes'])
 
       # pick an topo environment based on the nodes
-      return topo if @node_names.length == 0
+      return topo if @node_names.empty?
       env = pick_env(topo['nodes'])
       topo['chef_environment'] = env if env
       topo
@@ -105,13 +105,13 @@ def empty_topology
         'tags' => [],
         'strategy' => 'via_cookbook',
         'strategy_data' => default_strategy_data,
-        'nodes' => @node_names.length == 0 ? [empty_node('node1')] : []
+        'nodes' => @node_names.empty? ? [empty_node('node1')] : []
       }
     end
 
     def default_strategy_data
       {
-        'cookbook' =>  @topo_name || 'topo1',
+        'cookbook' => @topo_name || 'topo1',
         'filename' => 'topology'
       }
     end

lib/chef/knife/topo_search.rb

@@ -41,7 +41,7 @@ class TopoSearch < Chef::Knife::Search
 
     # Make the base search options available on topo search
     orig_opts = KnifeTopo::TopoSearch.options
-    self.options = (Chef::Knife::Search.options).merge(orig_opts)
+    self.options = Chef::Knife::Search.options.merge(orig_opts)
 
     include KnifeTopo::CommandHelper
 
@@ -75,7 +75,7 @@ def constrain_query(query, topo_name)
       group_query = query && !query.start_with?('NOT') ? "(#{query})" : query
 
       # search specific topologies or all/none
-      constraint = (topo_name) ? 'topo_name:' + topo_name : 'topo_name:*'
+      constraint = topo_name ? 'topo_name:' + topo_name : 'topo_name:*'
 
       # combine the grouped query and constraint
       combine(query, group_query, constraint)

lib/chef/topo/converter/topo_v1.rb

@@ -26,7 +26,9 @@ class Chef
   module Topo
     # Convert V1 topology JSON to V2
     class TopoV1Converter < Chef::Topo::Converter
-      PRIORITIES = %w(default force_default normal override force_override)
+      PRIORITIES = %w(
+        default force_default normal override force_override
+      ).freeze
 
       register_converter('topo_v1', name)
 
@@ -51,7 +53,7 @@ def convert_node(n)
       def determine_strategy
         @output['strategy'] = 'direct_to_node'
         cookbooks = @input['cookbook_attributes']
-        return unless cookbooks && cookbooks.length > 0
+        return unless cookbooks && !cookbooks.empty?
 
         cookbooks.each do |cb|
           cond = cb['conditional'] || []

spec/unit/attributes_vs_normal_spec.rb

@@ -33,8 +33,8 @@
         },
         {
           'name' => 'node2',
-          'attributes' => { 'test' => { 'anAttr' => 'value2'  } },
-          'normal' => { 'test' => { 'anotherAttr' => 'value3'  } }
+          'attributes' => { 'test' => { 'anAttr' => 'value2' } },
+          'normal' => { 'test' => { 'anotherAttr' => 'value3' } }
         }
       ]
     }

spec/unit/topo_cookbook_create_spec.rb

@@ -29,7 +29,7 @@
 
 describe KnifeTopo::TopoCookbookCreate do
   before :each do
-    Chef::Config[:node_name]  = 'christine_test'
+    Chef::Config[:node_name] = 'christine_test'
 
     @data = {
       'id' => 'topo1',

spec/unit/topo_create_bootstrap_spec.rb

@@ -29,7 +29,7 @@
 
 describe 'KnifeTopo::TopoCreate Bootstrap' do
   before :each do
-    Chef::Config[:node_name]  = 'christine_test'
+    Chef::Config[:node_name] = 'christine_test'
 
     @topobag_name = 'testsys_test'
     @topo1_name = 'topo1'
@@ -47,12 +47,14 @@
 
   it 'disables upload and only bootstraps new nodes with --bootstrap '\
     'and not --overwrite' do
-    cmd = KnifeTopo::TopoCreate.new([
-      'topo1',
-      '--bootstrap',
-      '--disable-upload',
-      "--data-bag=#{@topobag_name}"
-    ])
+    cmd = KnifeTopo::TopoCreate.new(
+      [
+        'topo1',
+        '--bootstrap',
+        '--disable-upload',
+        "--data-bag=#{@topobag_name}"
+      ]
+    )
 
     expect(cmd).to receive(:load_local_topo_or_exit).with(
       @topo1_name
@@ -73,9 +75,11 @@
     expect(cmd).not_to receive(:run_bootstrap).with(
       @merged_data['nodes'][1], anything, anything
     )
-    expect(cmd).not_to receive(:update_node).with(@merged_data['nodes'][0])
+    expect(cmd).not_to receive(:update_node).with(
+      @merged_data['nodes'][0], nil
+    )
     expect(cmd).to receive(:update_node).with(
-      @merged_data['nodes'][1]
+      @merged_data['nodes'][1], nil
     ).and_return(true)
 
     cmd.run

spec/unit/topo_create_overwrite_spec.rb

@@ -28,7 +28,7 @@
 
 describe 'KnifeTopo::TopoCreate Overwrite' do
   before :each do
-    Chef::Config[:node_name]  = 'christine_test'
+    Chef::Config[:node_name] = 'christine_test'
 
     data = UnitTestData.new
     @topobag_name = 'testsys_test'
@@ -71,8 +71,8 @@
     ).with(@merged_data['nodes'][0], anything, true).and_return(true)
     allow(cmd).to receive(
       :update_node
-    ).with(@merged_data['nodes'][0]).and_return(nil)
-    expect(cmd).to receive(:update_node).with(@merged_data['nodes'][1])
+    ).with(@merged_data['nodes'][0], nil).and_return(nil)
+    expect(cmd).to receive(:update_node).with(@merged_data['nodes'][1], nil)
 
     cmd.run
 

spec/unit/topo_create_spec.rb

@@ -31,7 +31,7 @@
 
 describe KnifeTopo::TopoCreate do
   before :each do
-    Chef::Config[:node_name]  = 'christine_test'
+    Chef::Config[:node_name] = 'christine_test'
     data = UnitTestData.new
     @merged_data = data.topo1_merged
     @topo1_data = data.topo1
@@ -82,8 +82,8 @@
       expect(@cmd).not_to receive(:run_bootstrap)
       node1 = @merged_data['nodes'][0]
       node2 = @merged_data['nodes'][1]
-      expect(@cmd).to receive(:update_node).with(node1).and_return(nil)
-      expect(@cmd).to receive(:update_node).with(node2).and_return(true)
+      expect(@cmd).to receive(:update_node).with(node1, nil).and_return(nil)
+      expect(@cmd).to receive(:update_node).with(node2, nil).and_return(true)
 
       @cmd.run
       expect(@cmd.results[:bootstrapped]).to eq([])
@@ -127,7 +127,7 @@
       expect(Chef::Node).to receive(:load).and_raise(@exception_404)
       expect(@cmd).not_to receive(:create_object)
 
-      @cmd.update_node(@topo1_data['nodes'][1])
+      @cmd.update_node(@topo1_data['nodes'][1], false)
     end
   end