From 3253d173fbf3384360eac7f490b350ae7055d758 Mon Sep 17 00:00:00 2001 From: Jordan Running Date: Mon, 11 Jan 2016 16:59:00 -0600 Subject: Correctly save policy_name and policy_group with `knife node edit` Fixes #4364 - Add specs for Chef::Knife::NodeEditor. - Add `policy_name` and `policy_group` to properties compared in `NodeEditor#updated?`. - Add `policy_name` and `policy_group` to Hash returned by `NodeEditor#view`. - Use `Node.from_hash` in `NodeEditor#apply_updates` instead of duplicating functionality. - Add some YARD docs for Chef::Knife::NodeEditor. --- lib/chef/knife/core/node_editor.rb | 91 ++++++++----- spec/unit/knife/core/node_editor_spec.rb | 211 +++++++++++++++++++++++++++++++ 2 files changed, 267 insertions(+), 35 deletions(-) create mode 100644 spec/unit/knife/core/node_editor_spec.rb diff --git a/lib/chef/knife/core/node_editor.rb b/lib/chef/knife/core/node_editor.rb index fe14e18d9d..7b7cb72df9 100644 --- a/lib/chef/knife/core/node_editor.rb +++ b/lib/chef/knife/core/node_editor.rb @@ -1,6 +1,7 @@ # # Author:: Daniel DeLeo () -# Copyright:: Copyright (c) 2011 Opscode, Inc. +# Author:: Jordan Running () +# Copyright:: Copyright (c) 2011-2016 Chef Software, Inc. # License:: Apache License, Version 2.0 # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -18,81 +19,101 @@ require 'chef/json_compat' require 'chef/node' -require 'tempfile' class Chef class Knife class NodeEditor + attr_reader :node, :ui, :config + private :node, :ui, :config - attr_reader :node - attr_reader :ui - attr_reader :config - + # @param node [Chef::Node] + # @param ui [Chef::Knife::UI] + # @param config [Hash] def initialize(node, ui, config) @node, @ui, @config = node, ui, config end + # Opens the node data (as JSON) in the user's editor and returns a new + # {Chef::Node} reflecting the user's changes. + # + # @return [Chef::Node] def edit_node abort "You specified the --disable_editing option, nothing to edit" if config[:disable_editing] assert_editor_set! - updated_node_data = @ui.edit_data(view) + updated_node_data = ui.edit_data(view) apply_updates(updated_node_data) @updated_node end + # Returns an array of the names of properties that have been changed or + # +false+ if none were changed. + # + # @return [Array] if any properties have been changed. + # @return [false] if no properties have been changed. def updated? + return false if @updated_node.nil? + pristine_copy = Chef::JSONCompat.parse(Chef::JSONCompat.to_json(node)) updated_copy = Chef::JSONCompat.parse(Chef::JSONCompat.to_json(@updated_node)) - unless pristine_copy == updated_copy - updated_properties = %w{name normal chef_environment run_list default override automatic}.reject do |key| - pristine_copy[key] == updated_copy[key] - end + + updated_properties = %w[ + name + chef_environment + automatic + default + normal + override + policy_name + policy_group + run_list + ].reject do |key| + pristine_copy[key] == updated_copy[key] end - ( pristine_copy != updated_copy ) && updated_properties - end - private + updated_properties.any? && updated_properties + end + # @api private def view - result = {} - result["name"] = node.name - result["chef_environment"] = node.chef_environment - result["normal"] = node.normal_attrs - result["run_list"] = node.run_list + result = { + "name" => node.name, + "chef_environment" => node.chef_environment, + "normal" => node.normal_attrs, + "policy_name" => node.policy_name, + "policy_group" => node.policy_group, + "run_list" => node.run_list, + } if config[:all_attributes] result["default"] = node.default_attrs result["override"] = node.override_attrs result["automatic"] = node.automatic_attrs end + result end + # @api private def apply_updates(updated_data) if node.name and node.name != updated_data["name"] ui.warn "Changing the name of a node results in a new node being created, #{node.name} will not be modified or removed." - confirm = ui.confirm "Proceed with creation of new node" + ui.confirm "Proceed with creation of new node" end - @updated_node = Node.new.tap do |n| - n.name( updated_data["name"] ) - n.chef_environment( updated_data["chef_environment"] ) - n.run_list( updated_data["run_list"]) - n.normal_attrs = updated_data["normal"] - - if config[:all_attributes] - n.default_attrs = updated_data["default"] - n.override_attrs = updated_data["override"] - n.automatic_attrs = updated_data["automatic"] - else - n.default_attrs = node.default_attrs - n.override_attrs = node.override_attrs - n.automatic_attrs = node.automatic_attrs - end + data = updated_data.dup + + unless config[:all_attributes] + data['automatic'] = node.automatic_attrs + data['default'] = node.default_attrs + data['override'] = node.override_attrs end + + @updated_node = Node.from_hash(data) end + private + def abort(message) ui.error(message) exit 1 diff --git a/spec/unit/knife/core/node_editor_spec.rb b/spec/unit/knife/core/node_editor_spec.rb new file mode 100644 index 0000000000..52e3aeadd5 --- /dev/null +++ b/spec/unit/knife/core/node_editor_spec.rb @@ -0,0 +1,211 @@ +# +# Author:: Jordan Running () +# Copyright:: Copyright (c) 2016 Chef Software, Inc. +# License:: Apache License, Version 2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +require 'spec_helper' +require 'chef/knife/core/node_editor' + +describe Chef::Knife::NodeEditor do + let(:node_data) do + { 'name' => 'test_node', + 'chef_environment' => 'production', + 'automatic' => { 'foo' => 'bar' }, + 'default' => { 'alpha' => { 'bravo' => 'charlie', 'delta' => 'echo' } }, + 'normal' => { 'alpha' => { 'bravo' => 'hotel' }, 'tags' => [] }, + 'override' => { 'alpha' => { 'bravo' => 'foxtrot', 'delta' => 'golf' } }, + 'policy_name' => nil, + 'policy_group' => nil, + 'run_list' => %w(role[comedy] role[drama] recipe[mystery]) + } + end + + let(:node) { Chef::Node.from_hash(node_data) } + + let(:ui) { double 'ui' } + let(:base_config) { { editor: 'cat' } } + let(:config) { base_config.merge(all_attributes: false) } + + subject { described_class.new(node, ui, config) } + + describe '#view' do + it 'returns a Hash with only the name, chef_environment, normal, ' + + 'policy_name, policy_group, and run_list properties' do + expected = node_data.select do |key,| + %w[ name chef_environment normal + policy_name policy_group run_list ].include?(key) + end + + expect(subject.view).to eq(expected) + end + + context 'when config[:all_attributes] == true' do + let(:config) { base_config.merge(all_attributes: true) } + + it 'returns a Hash with all of the node\'s properties' do + expect(subject.view).to eq(node_data) + end + end + end + + describe '#apply_updates' do + context 'when the node name is changed' do + before(:each) do + allow(ui).to receive(:warn) + allow(ui).to receive(:confirm).and_return(true) + end + + it 'emits a warning and prompts for confirmation' do + data = subject.view.merge('name' => 'foo_new_name_node') + updated_node = subject.apply_updates(data) + + expect(ui).to have_received(:warn) + .with 'Changing the name of a node results in a new node being ' + + 'created, test_node will not be modified or removed.' + + expect(ui).to have_received(:confirm) + .with('Proceed with creation of new node') + + expect(updated_node).to be_a(Chef::Node) + end + end + + context 'when config[:all_attributes] == false' do + let(:config) { base_config.merge(all_attributes: false) } + + let(:updated_data) do + subject.view.merge( + 'normal' => { 'alpha' => { 'bravo' => 'hotel2' }, 'tags' => [ 'xyz' ] }, + 'policy_name' => 'mypolicy', + 'policy_group' => 'prod', + 'run_list' => %w(role[drama] recipe[mystery]) + ) + end + + it 'returns a node with run_list and normal_attrs changed' do + updated_node = subject.apply_updates(updated_data) + expect(updated_node).to be_a(Chef::Node) + + # Expected to have been changed + expect(updated_node.chef_environment).to eql(updated_data['chef_environment']) + expect(updated_node.normal_attrs).to eql(updated_data['normal']) + expect(updated_node.policy_name).to eql(updated_data['policy_name']) + expect(updated_node.policy_group).to eql(updated_data['policy_group']) + expect(updated_node.run_list.map(&:to_s)).to eql(updated_data['run_list']) + + # Expected not to have changed + expect(updated_node.default_attrs).to eql(node.default_attrs) + expect(updated_node.override_attrs).to eql(node.override_attrs) + expect(updated_node.automatic_attrs).to eql(node.automatic_attrs) + end + end + + context 'when config[:all_attributes] == true' do + let(:config) { base_config.merge(all_attributes: true) } + + let(:updated_data) do + subject.view.merge( + 'default' => { 'alpha' => { 'bravo' => 'charlie2', 'delta' => 'echo2' } }, + 'normal' => { 'alpha' => { 'bravo' => 'hotel2' }, 'tags' => [ 'xyz' ] }, + 'override' => { 'alpha' => { 'bravo' => 'foxtrot2', 'delta' => 'golf2' } }, + 'policy_name' => 'mypolicy', + 'policy_group' => 'prod', + 'run_list' => %w(role[drama] recipe[mystery]) + ) + end + + it 'returns a node with all editable properties changed' do + updated_node = subject.apply_updates(updated_data) + expect(updated_node).to be_a(Chef::Node) + + expect(updated_node.chef_environment).to eql(updated_data['chef_environment']) + expect(updated_node.automatic_attrs).to eql(updated_data['automatic']) + expect(updated_node.normal_attrs).to eql(updated_data['normal']) + expect(updated_node.default_attrs).to eql(updated_data['default']) + expect(updated_node.override_attrs).to eql(updated_data['override']) + expect(updated_node.policy_name).to eql(updated_data['policy_name']) + expect(updated_node.policy_group).to eql(updated_data['policy_group']) + expect(updated_node.run_list.map(&:to_s)).to eql(updated_data['run_list']) + end + end + end + + describe '#updated?' do + context 'before the node has been edited' do + it 'returns false' do + expect(subject.updated?).to be false + end + end + + context 'after the node has been edited' do + context 'and changes were made' do + let(:updated_data) do + subject.view.merge( + 'default' => { 'alpha' => { 'bravo' => 'charlie2', 'delta' => 'echo2' } }, + 'normal' => { 'alpha' => { 'bravo' => 'hotel2' }, 'tags' => [ 'xyz' ] }, + 'override' => { 'alpha' => { 'bravo' => 'foxtrot2', 'delta' => 'golf2' } }, + 'policy_name' => 'mypolicy', + 'policy_group' => 'prod', + 'run_list' => %w(role[drama] recipe[mystery]) + ) + end + + context 'and changes affect only editable properties' do + before(:each) do + allow(ui).to receive(:edit_data) + .with(subject.view) + .and_return(updated_data) + + subject.edit_node + end + + it 'returns an array of the changed property names' do + expect(subject.updated?).to eql %w[ normal policy_name policy_group run_list ] + end + end + + context 'and the changes include non-editable properties' do + before(:each) do + data = updated_data.merge('bad_property' => 'bad_value') + + allow(ui).to receive(:edit_data) + .with(subject.view) + .and_return(data) + + subject.edit_node + end + + it 'returns an array of property names that doesn\'t include ' + + 'the non-editable properties' do + expect(subject.updated?).to eql %w[ normal policy_name policy_group run_list ] + end + end + end + + context 'and changes were not made' do + before(:each) do + allow(ui).to receive(:edit_data) + .with(subject.view) + .and_return(subject.view.dup) + + subject.edit_node + end + + it { is_expected.not_to be_updated } + end + end + end +end -- cgit v1.2.1