From 0a8ee26b7ed037c30354b73c8dcb2b914000e0c9 Mon Sep 17 00:00:00 2001 From: "Marc A. Paradise" Date: Fri, 7 Jun 2019 09:38:16 -0400 Subject: use mixlib-cli's deprecation mechanism Deprecation logic is now available in `mixlib-cli` via `deprecated_option`. Let's use that instead of keeping our custom deprecation behavior. Signed-off-by: Marc A. Paradise --- Gemfile.lock | 35 ++++---- chef.gemspec | 2 +- lib/chef/knife/bootstrap.rb | 167 +++++++++++++++----------------------- spec/unit/knife/bootstrap_spec.rb | 101 ----------------------- 4 files changed, 84 insertions(+), 221 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 75ebc48476..a8e5daaf16 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,17 +1,17 @@ GIT remote: https://github.com/chef/chefstyle.git - revision: c9f82052de99484c69fdc368a0c239d03eb4e6d2 + revision: 80dc0f94064ece591fcb8c437aa1d60e2881ba10 branch: master specs: - chefstyle (0.12.1) + chefstyle (0.12.3) rubocop (= 0.62.0) GIT remote: https://github.com/chef/ohai.git - revision: 3734d441498e51dd44cefc21d842232cd0b5a47d + revision: f04de72cd0a1b8636ca60c238930c4ed60187516 branch: master specs: - ohai (15.0.36) + ohai (15.0.39) chef-config (>= 12.8, < 16) ffi (~> 1.9) ffi-yajl (~> 2.2) @@ -44,7 +44,7 @@ PATH license-acceptance (~> 1.0, >= 1.0.5) mixlib-archive (>= 0.4, < 2.0) mixlib-authentication (~> 2.1) - mixlib-cli (>= 1.7, < 3.0) + mixlib-cli (>= 2.1.1, < 3.0) mixlib-log (>= 2.0.3, < 4.0) mixlib-shellout (>= 2.4, < 4.0) net-sftp (~> 2.1, >= 2.1.2) @@ -75,7 +75,7 @@ PATH license-acceptance (~> 1.0, >= 1.0.5) mixlib-archive (>= 0.4, < 2.0) mixlib-authentication (~> 2.1) - mixlib-cli (>= 1.7, < 3.0) + mixlib-cli (>= 2.1.1, < 3.0) mixlib-log (>= 2.0.3, < 4.0) mixlib-shellout (>= 2.4, < 4.0) net-sftp (~> 2.1, >= 2.1.2) @@ -169,7 +169,7 @@ GEM ffi (>= 1.0.1) gyoku (1.3.1) builder (>= 2.1.2) - hashdiff (0.3.9) + hashdiff (0.4.0) hashie (3.6.0) highline (1.7.10) htmlentities (4.3.4) @@ -220,12 +220,12 @@ GEM mixlib-archive (1.0.1-universal-mingw32) mixlib-log mixlib-authentication (2.1.1) - mixlib-cli (2.0.6) + mixlib-cli (2.1.1) mixlib-config (3.0.1) tomlrb mixlib-log (3.0.1) - mixlib-shellout (2.4.4) - mixlib-shellout (2.4.4-universal-mingw32) + mixlib-shellout (3.0.4) + mixlib-shellout (3.0.4-universal-mingw32) win32-process (~> 0.8.2) wmi-lite (~> 1.0) multi_json (1.13.1) @@ -264,7 +264,7 @@ GEM pry-stack_explorer (0.4.9.3) binding_of_caller (>= 0.7) pry (>= 0.9.11) - public_suffix (3.0.3) + public_suffix (3.1.0) rack (2.0.7) rainbow (3.0.0) rake (12.3.2) @@ -296,8 +296,9 @@ GEM rainbow (>= 2.2.2, < 4.0) ruby-progressbar (~> 1.7) unicode-display_width (~> 1.4.0) - ruby-prof (0.17.0) - ruby-progressbar (1.10.0) + ruby-prof (0.18.0) + ruby-prof (0.18.0-x64-mingw32) + ruby-progressbar (1.10.1) ruby-shadow (2.5.0) rubyntlm (0.6.2) rubyzip (1.2.3) @@ -322,7 +323,7 @@ GEM tins (~> 1.0) thor (0.20.3) timers (4.3.0) - tins (1.20.2) + tins (1.20.3) tomlrb (1.2.8) train-core (2.1.7) json (>= 1.8, < 3.0) @@ -357,10 +358,10 @@ GEM unicode-display_width (1.4.1) unicode_utils (1.4.0) uuidtools (2.1.5) - webmock (3.5.1) + webmock (3.6.0) addressable (>= 2.3.6) crack (>= 0.3.2) - hashdiff + hashdiff (>= 0.4.0, < 2.0.0) win32-api (1.5.3-universal-mingw32) win32-certstore (0.3.0) ffi @@ -437,4 +438,4 @@ DEPENDENCIES yard BUNDLED WITH - 1.17.3 + 1.17.2 diff --git a/chef.gemspec b/chef.gemspec index 4524da6d9a..a89cf4ceb2 100644 --- a/chef.gemspec +++ b/chef.gemspec @@ -19,7 +19,7 @@ Gem::Specification.new do |s| s.add_dependency "train-core", "~> 2.0", ">= 2.0.12" s.add_dependency "license-acceptance", "~> 1.0", ">= 1.0.5" - s.add_dependency "mixlib-cli", ">= 1.7", "< 3.0" + s.add_dependency "mixlib-cli", ">= 2.1.1", "< 3.0" s.add_dependency "mixlib-log", ">= 2.0.3", "< 4.0" s.add_dependency "mixlib-authentication", "~> 2.1" s.add_dependency "mixlib-shellout", ">= 2.4", "< 4.0" diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb index f60928375e..19384ba768 100644 --- a/lib/chef/knife/bootstrap.rb +++ b/lib/chef/knife/bootstrap.rb @@ -347,67 +347,70 @@ class Chef Chef::Config[:knife][:bootstrap_vault_item] } - DEPRECATED_FLAGS = { - # 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"], - host_key_verify: - [:ssh_verify_host_key, "--[no-]host-key-verify"], - prerelease: - [:channel, "--prerelease", "current"], - ssh_user: - [:connection_user, "--ssh-user USERNAME"], - ssh_password: - [:connection_password, "--ssh-password PASSWORD"], - ssh_port: - [:connection_port, "--ssh-port"], - ssl_peer_fingerprint: - [:winrm_ssl_peer_fingerprint, "--ssl-peer-fingerprint FINGERPRINT"], - winrm_user: - [:connection_user, "--winrm-user USERNAME", nil, "-x USERNAME"], - winrm_password: - [:connection_password, "--winrm-password PASSWORD"], - winrm_port: - [:connection_port, "--winrm-port"], - winrm_authentication_protocol: - [:winrm_auth_method, "--winrm-authentication-protocol PROTOCOL"], - winrm_session_timeout: - [:session_timeout, "--winrm-session-timeout MINUTES"], - winrm_ssl_verify_mode: - [:winrm_no_verify_cert, "--winrm-ssl-verify-mode MODE"], - winrm_transport: - [:winrm_ssl, "--winrm-transport TRANSPORT"], - }.freeze - - DEPRECATED_FLAGS.each do |deprecated_key, 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_key} #{replacement_value}" - end - - 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 + # Deprecated options. These must be declared after + # regular options because they refer to the replacement + # option definitions implicitly. + deprecated_option :auth_timeout, + replacement: :max_wait, + long: "--max-wait SECONDS" + + deprecated_option :forward_agent, + replacement: :ssh_forward_agent, + boolean: true, long: "--forward-agent" + + deprecated_option :host_key_verify, + replacement: :ssh_verify_host_key, + boolean: true, long: "--[no-]host-key-verify", + value_mapper: Proc.new { |verify| verify ? "always" : "never" } + + deprecated_option :prerelease, + replacement: :channel, + long: "--prerelease", + boolean: true, value_mapper: Proc.new { "current" } + + deprecated_option :ssh_user, + replacement: :connection_user, + long: "--ssh-user USERNAME" + + deprecated_option :ssh_password, + replacement: :connection_password, + long: "--ssh-password PASSWORD" + + deprecated_option :ssh_port, + replacement: :connection_port, + long: "--ssh-port PASSWORD" + + deprecated_option :ssl_peer_fingerprint, + replacement: :winrm_ssl_peer_fingerprint, + long: "--ssl-peer-fingerprint FINGERPRINT" + + deprecated_option :winrm_user, + replacement: :connection_user, + long: "--winrm-user USERNAME", short: "-x USERNAME" + + deprecated_option :winrm_password, + replacement: :connection_password, + long: "--winrm-password PASSWORD" + + deprecated_option :winrm_port, + replacement: :connection_port, + long: "--winrm-port PORT" + + deprecated_option :winrm_authentication_protocol, + replacement: :winrm_auth_method, + long: "--winrm-authentication-protocol PROTOCOL" + + deprecated_option :winrm_session_timeout, + replacement: :session_timeout, + long: "--winrm-session-timeout MINUTES" + + deprecated_option :winrm_ssl_verify_mode, + replacement: :winrm_no_verify_cert, + long: "--winrm-ssl-verify-mode MODE" + + deprecated_option :winrm_transport, replacement: :winrm_ssl, + long: "--winrm-transport TRANSPORT", + value_mapper: Proc.new { |value| value == "ssl" } attr_reader :connection @@ -548,7 +551,6 @@ class Chef def run check_license - verify_deprecated_flags! plugin_setup! validate_name_args! @@ -693,7 +695,7 @@ class Chef Validatorless bootstrap over unsecure winrm channels could expose your key to network sniffing. Please use a 'winrm_auth_method' other than 'plaintext', - or enable ssl on #{server_name} then use the --ssl flag + or enable ssl on #{server_name} then use the ---winrm-ssl flag to connect. EOM @@ -703,45 +705,6 @@ 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, - # or to the mapped value in case of changing flag type. - # If a deprecated flag and its corresponding replacement - # are both used, exit - def verify_deprecated_flags! - DEPRECATED_FLAGS.each do |deprecated_key, 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] - 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_key} and #{dep_long_key}. - - Please use one or the other, but note that - #{deprecated_long} is deprecated. - EOM - exit 1 - else - config[new_key] = replacement_value || config[deprecated_key] - unless Chef::Config[:silence_deprecation_warnings] == true - ui.warn "You provided #{dep_long_key}. #{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 b04269e3df..bf2c7c4902 100644 --- a/spec/unit/knife/bootstrap_spec.rb +++ b/spec/unit/knife/bootstrap_spec.rb @@ -1657,7 +1657,6 @@ describe Chef::Knife::Bootstrap do describe "#run" do it "performs the steps we expect to run a bootstrap" do expect(knife).to receive(:check_license) - 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 @@ -1679,106 +1678,6 @@ describe Chef::Knife::Bootstrap do end end - describe "#verify_deprecated_flags!" do - before do - Chef::Config[:silence_deprecation_warnings] = false - end - - context "when a deprecated CLI flag is given on the CLI" do - 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 - - 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 - - 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 - - context "when a deprecated boolean CLI flag is given on the CLI, and its non-boolean replacement is used" do - let(:bootstrap_cli_options) { %w{--prerelease} } - it "correctly maps the old boolean value to the new value" do - expect(knife.ui).to receive(:warn) - knife.verify_deprecated_flags! - expect(knife.config[:channel]).to eq "current" - end - end - end - describe "#register_client" do let(:vault_handler_mock) { double("ChefVaultHandler") } let(:client_builder_mock) { double("ClientBuilder") } -- cgit v1.2.1