Chef-15: Adding short argument's deprecation check (#8626)

- Handles "-x" that was used to set "winrm-user" and is now Deprecated.
- Using symmetrical "USERNAME" and "PASSWORD" verbiages while displaying deprecated warnings.
- Added test cases
- Ensured Chefstyle
- Fixes MSYS-1046

Signed-off-by: Nimesh-Msys <nimesh.patni@msystechnologies.com>

2 files changed, 116 insertions, 27 deletions

diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb
index 0826a0f38c..f5e67dcfaf 100644
--- a/lib/chef/knife/bootstrap.rb
+++ b/lib/chef/knife/bootstrap.rb
@@ -348,10 +348,11 @@ class Chef
         }
 
       DEPRECATED_FLAGS = {
-        # deprecated_key: [new_key, deprecated_long]
+        # deprecated_key: [new_key, deprecated_long, replacement_value, deprecated_short]
         # optional third element: replacement_value - if converting from bool
         #   (--bool-option) to valued flag (--new-option VALUE)
         #   this will be the value that is assigned the new flag when the old flag is used.
+        # optional fourth element: deprecated_short - if the short form of flag has also been deprecated.
         auth_timeout: [:max_wait, "--max-wait SECONDS" ],
         forward_agent:
           [:ssh_forward_agent, "--forward-agent"],
@@ -360,7 +361,7 @@ class Chef
         prerelease:
           [:channel, "--prerelease", "current"],
         ssh_user:
-          [:connection_user, "--ssh-user USER"],
+          [:connection_user, "--ssh-user USERNAME"],
         ssh_password:
           [:connection_password, "--ssh-password PASSWORD"],
         ssh_port:
@@ -368,9 +369,9 @@ class Chef
         ssl_peer_fingerprint:
           [:winrm_ssl_peer_fingerprint, "--ssl-peer-fingerprint FINGERPRINT"],
         winrm_user:
-          [:connection_user, "--winrm-user USER"],
+          [:connection_user, "--winrm-user USERNAME", nil, "-x USERNAME"],
         winrm_password:
-          [:connection_password, "--winrm-password"],
+          [:connection_password, "--winrm-password PASSWORD"],
         winrm_port:
           [:connection_port, "--winrm-port"],
         winrm_authentication_protocol:
@@ -384,18 +385,28 @@ class Chef
       }.freeze
 
       DEPRECATED_FLAGS.each do |deprecated_key, deprecation_entry|
-        new_key, deprecated_long, replacement_value = deprecation_entry
+        new_key, deprecated_long, replacement_value, deprecated_short = deprecation_entry
         new_long = options[new_key][:long]
+        new_long_key = new_long.split(" ").first
+        if new_short = options[new_key][:short]
+          new_long += "(or #{new_short})"
+          new_long_key += "(or #{new_short.split(" ").first})"
+        end
         new_long_desc = if replacement_value.nil?
                           new_long
                         else
-                          "#{new_long.split(" ").first} #{replacement_value}"
+                          "#{new_long_key} #{replacement_value}"
                         end
-        option(deprecated_key, long: deprecated_long,
-                               description: "This flag is deprecated. Please use '#{new_long_desc}' instead.",
-                               boolean: options[new_key][:boolean] || !replacement_value.nil?,
-                               # Put deprecated options at the end of the options list
-                               on: :tail)
+
+        opt = { long: deprecated_long,
+                description: "This flag is deprecated. Please use '#{new_long_desc}' instead.",
+                boolean: options[new_key][:boolean] || !replacement_value.nil?,
+               # Put deprecated options at the end of the options list
+                on: :tail }
+
+        opt[:short] = deprecated_short if deprecated_short
+
+        option(deprecated_key, opt)
       end
 
       attr_reader :connection
@@ -699,13 +710,23 @@ class Chef
       # are both used, exit
       def verify_deprecated_flags!
         DEPRECATED_FLAGS.each do |deprecated_key, deprecation_entry|
-          new_key, deprecated_long, replacement_value = deprecation_entry
+          new_key, deprecated_long, replacement_value, deprecated_short = deprecation_entry
+
+          dep_long_key = deprecated_long.split(" ").first
+          if deprecated_short
+            dep_long_key += "(or #{deprecated_short.split(" ").first})"
+          end
+
           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
+              new_long = options[new_key][:long]
+              new_long_key = new_long.split(" ").first
+              if new_short = options[new_key][:short]
+                new_long += "(or #{new_short})"
+                new_long_key += "(or #{new_short.split(" ").first})"
+              end
               ui.error <<~EOM
-                You provided both #{new_long} and #{deprecated_long}.
+                You provided both #{new_long_key} and #{dep_long_key}.
 
                 Please use one or the other, but note that
                 #{deprecated_long} is deprecated.
@@ -714,7 +735,7 @@ class Chef
             else
               config[new_key] = replacement_value || config[deprecated_key]
               unless Chef::Config[:silence_deprecation_warnings] == true
-                ui.warn "You provided #{deprecated_long.split(" ").first}. #{options[deprecated_key][:description]}"
+                ui.warn "You provided #{dep_long_key}. #{options[deprecated_key][:description]}"
               end
             end
           end
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index f92af74284..b04269e3df 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -1,6 +1,7 @@
 #
 # Author:: Ian Meyer (<ianmmeyer@gmail.com>)
 # Copyright:: Copyright 2010-2016, Ian Meyer
+# Copyright:: Copyright 2010-2019, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -1684,20 +1685,87 @@ describe Chef::Knife::Bootstrap do
     end
 
     context "when a deprecated CLI flag is given on the CLI" do
-      let(:bootstrap_cli_options) { %w{--ssh-user sshuser} }
-      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(/You provided --ssh-user. This flag is deprecated. Please use '--connection-user USERNAME' instead./)
-        knife.verify_deprecated_flags!
-        expect(knife.config[:connection_user]).to eq "sshuser"
+      context "with flag containing only long argument" do
+        context "when given with no other options" do
+          let(:bootstrap_cli_options) { %w{--winrm-transport XXX} }
+          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("You provided --winrm-transport. This flag is deprecated. Please use '--winrm-ssl' instead.")
+            knife.verify_deprecated_flags!
+            expect(knife.config[:winrm_ssl]).to eq "XXX"
+          end
+        end
+        context "when given along with valid flag" do
+          let(:bootstrap_cli_options) { %w{--winrm-transport XXX --connection-user user-a} }
+          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("You provided --winrm-transport. This flag is deprecated. Please use '--winrm-ssl' instead.")
+            knife.verify_deprecated_flags!
+            expect(knife.config[:winrm_ssl]).to eq "XXX"
+            expect(knife.config[:connection_user]).to eq "user-a"
+          end
+        end
+        context "when given along with its replacement" do
+          let(:bootstrap_cli_options) { %w{--winrm-transport XXX --winrm-ssl YYY} }
+          it "informs the human that both are provided and exits" do
+            expect(knife.ui).to receive(:error).with(/You provided both --winrm-ssl and --winrm-transport.*Please use.*/m)
+            expect { knife.verify_deprecated_flags! }.to raise_error SystemExit
+          end
+        end
       end
-    end
 
-    context "when a deprecated CLI flag is given on the CLI, along with its replacement" do
-      let(:bootstrap_cli_options) { %w{--connection-user a --ssh-user b} }
+      context "with flag also contains short argument" do
+        context "when given as short arg" do
+          context "with no other options" do
+            let(:bootstrap_cli_options) { %w{-x user-a} }
+            it "maps the key value to the new key and display information with both long and short arguments" do
+              expect(knife.ui).to receive(:warn).with("You provided --winrm-user(or -x). This flag is deprecated. Please use '--connection-user USERNAME(or -U USERNAME)' instead.")
+              knife.verify_deprecated_flags!
+              expect(knife.config[:connection_user]).to eq "user-a"
+            end
+          end
+          context "along with valid flag" do
+            let(:bootstrap_cli_options) { %w{-x user-a --connection-password XXX} }
+            it "maps the key value to the new key and display information with both long and short arguments" do
+              expect(knife.ui).to receive(:warn).with("You provided --winrm-user(or -x). This flag is deprecated. Please use '--connection-user USERNAME(or -U USERNAME)' instead.")
+              knife.verify_deprecated_flags!
+              expect(knife.config[:connection_user]).to eq "user-a"
+              expect(knife.config[:connection_password]).to eq "XXX"
+            end
+          end
+          context "along with its replacement" do
+            let(:bootstrap_cli_options) { %w{-x user-a --connection-user user-b} }
+            it "informs the human that both are provided and exits" do
+              expect(knife.ui).to receive(:error).with("You provided both --connection-user(or -U) and --winrm-user(or -x).\n\nPlease use one or the other, but note that\n--winrm-user USERNAME is deprecated.\n")
+              expect { knife.verify_deprecated_flags! }.to raise_error SystemExit
+            end
+          end
+        end
 
-      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
+        context "when given as long arg" do
+          context "with no other options" do
+            let(:bootstrap_cli_options) { %w{--winrm-user user-a} }
+            it "maps the key value to the new key and display information with both long and short arguments" do
+              expect(knife.ui).to receive(:warn).with("You provided --winrm-user(or -x). This flag is deprecated. Please use '--connection-user USERNAME(or -U USERNAME)' instead.")
+              knife.verify_deprecated_flags!
+              expect(knife.config[:connection_user]).to eq "user-a"
+            end
+          end
+          context "along with valid flag" do
+            let(:bootstrap_cli_options) { %w{--winrm-user user-a --connection-password XXX} }
+            it "maps the key value to the new key and display information with both long and short arguments" do
+              expect(knife.ui).to receive(:warn).with("You provided --winrm-user(or -x). This flag is deprecated. Please use '--connection-user USERNAME(or -U USERNAME)' instead.")
+              knife.verify_deprecated_flags!
+              expect(knife.config[:connection_user]).to eq "user-a"
+              expect(knife.config[:connection_password]).to eq "XXX"
+            end
+          end
+          context "along with its replacement" do
+            let(:bootstrap_cli_options) { %w{--winrm-user user-a --connection-user user-b} }
+            it "informs the human that both are provided and exits" do
+              expect(knife.ui).to receive(:error).with("You provided both --connection-user(or -U) and --winrm-user(or -x).\n\nPlease use one or the other, but note that\n--winrm-user USERNAME is deprecated.\n")
+              expect { knife.verify_deprecated_flags! }.to raise_error SystemExit
+            end
+          end
+        end
       end
     end