Fix incorrect deprecation warningsmp/bootstrap-deprecation-fixes

Fixed incorrect option mapping where deprecated flags
would have a 'long' taken from the replacement flag.

Updated the behavior in case of conflicting flags
(for example --ssh-user and --connection-user) to avoid
surprising results by failing with an error instead
of trying to resolve intent.

Renamed things a bit for readability/consistency.

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

2 files changed, 43 insertions, 49 deletions

diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb
index d46bf455e9..e879c2e822 100644
--- a/lib/chef/knife/bootstrap.rb
+++ b/lib/chef/knife/bootstrap.rb
@@ -335,11 +335,8 @@ class Chef
           Chef::Config[:knife][:bootstrap_vault_item]
         }
 
-      # OPTIONAL: This can be exposed as an class method on Knife
-      # subclasses instead - that would let us move deprecation handling
-      # up into the base clase.
       DEPRECATED_FLAGS = {
-        # old_key: [:new_key, old_long, new_long]
+        # deprecated_key: [new_key, deprecated_long]
         auth_timeout: [:max_wait, "--max-wait SECONDS"],
         host_key_verify: [:ssh_verify_host_key,
                           "--[no-]host-key-verify",
@@ -370,14 +367,12 @@ class Chef
                                         ],
       }.freeze
 
-      DEPRECATED_FLAGS.each do |flag, new_flag_config|
-        new_flag, old_long = new_flag_config
-        new_long = options[new_flag][:long]
-        new_flag_name = new_long.split(" ").first
-
-        option(flag, long: new_long,
-               description: "#{old_long} is deprecated. Use #{new_long} instead.",
-               boolean: options[new_flag][:boolean])
+      DEPRECATED_FLAGS.each do |deprecated_key, deprecation_entry|
+        new_key, deprecated_long = deprecation_entry
+        new_long = options[new_key][:long]
+        option(deprecated_key, long: deprecated_long,
+                               description: "#{deprecated_long} is deprecated. Use #{new_long} instead.",
+                               boolean: options[new_key][:boolean])
       end
 
       attr_accessor :client_builder
@@ -503,37 +498,8 @@ class Chef
         Erubis::Eruby.new(template).evaluate(bootstrap_context)
       end
 
-      # Check deprecated flags are used; map them to their new keys,
-      # and print a warning. Will not map a value to a new key if the
-      # CLI flag for that new key has also been specified.
-      # If both old and new flags are specified, this will warn
-      # and take the new flag value.
-      # This can be moved up to the base knife class if it's agreeable.
-      def warn_and_map_deprecated_flags
-        DEPRECATED_FLAGS.each do |old_key, new_flag_config|
-          new_key, = new_flag_config
-          if config.key?(old_key) && config_source(old_key) == :cli
-            # TODO - do we want the same warnings for knife config keys
-            #        in absence of CLI keys?
-            if config.key?(new_key) && config_source(new_key) == :cli
-              new_key_name = "--#{new_key.to_s.tr("_", "-")}"
-              old_key_name = "--#{old_key.to_s.tr("_", "-")}"
-              ui.warn <<~EOM
-                You provided both #{new_key_name} and #{old_key_name}.
-                Using: '#{new_key_name.split(" ").first} #{config[new_key]}' because #{old_key_name} is deprecated.
-              EOM
-            else
-              config[new_key] = config[old_key]
-              unless Chef::Config[:silence_deprecation_warnings] == true
-                ui.warn options[old_key][:description]
-              end
-            end
-          end
-        end
-      end
-
       def run
-        warn_and_map_deprecated_flags
+        verify_deprecated_flags!
 
         validate_name_args!
         validate_protocol!
@@ -661,6 +627,34 @@ class Chef
         true
       end
 
+      # If any deprecated flags are used, let the user know and
+      # update config[new-key] to the value given to the deprecated flag.
+      # If a deprecated flag and its corresponding replacement
+      # are both used, raise an error.
+      def verify_deprecated_flags!
+        DEPRECATED_FLAGS.each do |deprecated_key, deprecation_entry|
+          new_key, deprecated_long = deprecation_entry
+          if config.key?(deprecated_key) && config_source(deprecated_key) == :cli
+            if config.key?(new_key) && config_source(new_key) == :cli
+              new_long = options[new_key][:long].split(" ").first
+              deprecated_long = deprecated_long.split(" ").first
+              ui.error <<~EOM
+                You provided both #{new_long} and #{deprecated_long}.
+
+                Please use one or the other, but note that
+                #{deprecated_long} is deprecated.
+              EOM
+              exit 1
+            else
+              config[new_key] = config[deprecated_key]
+              unless Chef::Config[:silence_deprecation_warnings] == true
+                ui.warn options[deprecated_key][:description]
+              end
+            end
+          end
+        end
+      end
+
       # fail if the server_name is nil
       def validate_name_args!
         if server_name.nil?
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index ce590fc9ee..5bef9c5659 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -1571,7 +1571,7 @@ describe Chef::Knife::Bootstrap do
     end
 
     it "performs the steps we expect to run a bootstrap" do
-      expect(knife).to receive(:warn_and_map_deprecated_flags).ordered
+      expect(knife).to receive(:verify_deprecated_flags!).ordered
       expect(knife).to receive(:validate_name_args!).ordered
       expect(knife).to receive(:validate_protocol!).ordered
       expect(knife).to receive(:validate_first_boot_attributes!).ordered
@@ -1593,7 +1593,7 @@ describe Chef::Knife::Bootstrap do
     end
   end
 
-  describe "#warn_and_map_deprecated_flags" do
+  describe "#verify_deprecated_flags!" do
     before do
       Chef::Config[:silence_deprecation_warnings] = false
     end
@@ -1605,7 +1605,7 @@ describe Chef::Knife::Bootstrap do
       end
       it "maps the key value to the new key and points the human to the new flag" do
         expect(knife.ui).to receive(:warn).with(/--ssh-user USER is deprecated. Use --connection-user USERNAME instead./)
-        knife.warn_and_map_deprecated_flags
+        knife.verify_deprecated_flags!
         expect(knife.config[:connection_user]).to eq "sshuser"
       end
     end
@@ -1616,10 +1616,10 @@ describe Chef::Knife::Bootstrap do
         knife.config[:connection_user] = "real-user"
         knife.merge_configs
       end
-      it "warns that both are provided and takes the non-deprecated value" do
-        expect(knife.ui).to receive(:warn).with(/You provided both --connection-user and --ssh-user.*'--connection-user real-user'/m)
-        knife.warn_and_map_deprecated_flags
-        expect(knife.config[:connection_user]).to eq "real-user"
+
+      it "informs the human that both are provided and exits" do
+        expect(knife.ui).to receive(:error).with(/You provided both --connection-user and --ssh-user.*Please use.*/m)
+        expect { knife.verify_deprecated_flags! }.to raise_error SystemExit
       end
     end
   end