Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes error handling when an invalid flag is passed. #151

Merged
merged 2 commits into from
May 19, 2018
Merged

Fixes error handling when an invalid flag is passed. #151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df26f70
Fixes error handling when an invalid flag is passed.
May 18, 2018
c888573
Merge branch 'master' into JM/fix_invalid_flag_handling
jonsmorrow May 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions components/chef-cli/i18n/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,11 @@ errors:
CHEFVAL009: |
File extension '%1' is unsupported. Currently recipes must be specified with a `.rb` extension.

CHEFVAL010: |
The flag '%1' does not exist.

Available flags are:
%2
# General errors/unknown errors are handled with CHEFINT
CHEFINT001: |
An unexpected error has occurred:
Expand Down
21 changes: 15 additions & 6 deletions components/chef-cli/lib/chef-cli/command/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ def run_with_default_options(params = [])
else
run(params)
end
rescue OptionParser::InvalidOption => e
# Using nil here is a bit gross but it prevents usage from printing.
raise OptionValidationError.new("CHEFVAL010", nil,
e.message.split(":")[1].strip, # only want the flag
format_flags.lines[1..-1].join # remove 'FLAGS:' header
)
end

# This is normally overridden by the command implementations, but
Expand Down Expand Up @@ -153,8 +159,11 @@ def show_help
end

def show_help_flags
UI::Terminal.output ""
UI::Terminal.output "FLAGS:"
UI::Terminal.output "\n#{format_flags}"
end

def format_flags
flag_text = "FLAGS:\n"
justify_length = 0
options.each_value do |spec|
justify_length = [justify_length, spec[:long].length + 4].max
Expand All @@ -168,16 +177,16 @@ def show_help_flags
short = "#{short}, "
end
flags = "#{short}#{flag_spec[:long]}"
UI::Terminal.write(" #{flags.ljust(justify_length)} ")
flag_text << " #{flags.ljust(justify_length)} "
ml_padding = " " * (justify_length + 8)
first = true
flag_spec[:description].split("\n").each do |d|
UI::Terminal.write(ml_padding) unless first
flag_text << ml_padding unless first
first = false
UI::Terminal.write(d)
UI::Terminal.write("\n")
flag_text << "#{d}\n"
end
end
flag_text
end

def show_help_subcommands
Expand Down
6 changes: 6 additions & 0 deletions components/chef-run/i18n/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,12 @@ errors:
CHEFVAL009: |
File extension '%1' is unsupported. Currently recipes must be specified with a `.rb` extension.

CHEFVAL010: |
The flag '%1' does not exist.

Available flags are:
%2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This \ gets pretty long (especially with upcoming ssh flags), making it likely someone has to scroll up to see what they did wrong.
Can we do something like:

The flag provided does not exist. 

Available flags are: 
  %2
You gave: 
  %1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the format for missing commands.

Copy link
Member

@marcparadise marcparadise May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may need a separate PR, though I think the size of the dynamic portion isn't so big there

Copy link
Member

@marcparadise marcparadise May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finishing the thought: . I'm +1 on landing this as it is for now - I'm planning to take a final pass through all of en.yml and check spelling, grammar, verboseness, word wrapping v term size, consistency, etc and will make tweaks as needed then.


# General errors/unknown errors are handled with CHEFINT
CHEFINT001: |
An unexpected error has occurred:
Expand Down
29 changes: 20 additions & 9 deletions components/chef-run/lib/chef-run/cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ def perform_run
run_multi_target(initial_status_msg, target_hosts, local_policy_path)
end
end
rescue OptionParser::InvalidOption => e
# Using nil here is a bit gross but it prevents usage from printing.
ove = OptionValidationError.new("CHEFVAL010", nil,
e.message.split(":")[1].strip, # only want the flag
format_flags.lines[1..-1].join # remove 'FLAGS:' header
)
handle_perform_error(ove)
rescue => e
handle_perform_error(e)
ensure
Expand Down Expand Up @@ -475,13 +482,17 @@ def capture_exception_backtrace(e)
end

def show_help
UI::Terminal.output banner
show_help_flags
UI::Terminal.output format_help
end

def show_help_flags
UI::Terminal.output ""
UI::Terminal.output "FLAGS:"
def format_help
help_text = banner.clone # This prevents us appending to the banner text
help_text << "\n"
help_text << format_flags
end

def format_flags
flag_text = "FLAGS:\n"
justify_length = 0
options.each_value do |spec|
justify_length = [justify_length, spec[:long].length + 4].max
Expand All @@ -495,16 +506,16 @@ def show_help_flags
short = "#{short}, "
end
flags = "#{short}#{flag_spec[:long]}"
UI::Terminal.write(" #{flags.ljust(justify_length)} ")
flag_text << " #{flags.ljust(justify_length)} "
ml_padding = " " * (justify_length + 8)
first = true
flag_spec[:description].split("\n").each do |d|
UI::Terminal.write(ml_padding) unless first
flag_text << ml_padding unless first
first = false
UI::Terminal.write(d)
UI::Terminal.write("\n")
flag_text << "#{d}\n"
end
end
flag_text
end

def usage
Expand Down
91 changes: 43 additions & 48 deletions components/chef-run/spec/unit/cli_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,54 +38,49 @@
allow(subject).to receive(:inspect).and_return("The subject instance")
end
describe "run" do
# it "t" do
# expect(subject).to receive(:parse_options).with(argv)
# subject.run
# end

# it "sets up the cli" do
# expect(subject).to receive(:setup_cli)
# expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
# end
#
# # TODO - test for Sender.new.run in thread
# it "performs the steps necessary to capture telemetry" do
# expect(telemetry).to receive(:timed_run_capture).and_yield
# expect(telemetry).to receive(:commit)
# expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
# end
#
# it "calls perform_run" do
# expect(subject).to receive(:perform_run)
# expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
# end
#
# context "perform_run raises WrappedError" do
# let(:e) { ChefRun::WrappedError.new(RuntimeError.new("Test"), "host") }
#
# it "prints the error and exits" do
# expect(subject).to receive(:perform_run).and_raise(e)
# expect(ChefRun::UI::ErrorPrinter).to receive(:show_error).with(e)
# expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(1) }
# end
# end
#
# context "perform_run raises SystemExit" do
# it "exits with same exit code" do
# expect(subject).to receive(:perform_run).and_raise(SystemExit.new(99))
# expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(99) }
# end
# end
#
# context "perform_run raises any other exception" do
# let(:e) { Exception.new("test") }
#
# it "exits with same exit code" do
# expect(subject).to receive(:perform_run).and_raise(e)
# expect(ChefRun::UI::ErrorPrinter).to receive(:dump_unexpected_error).with(e)
# expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(64) }
# end
# end
it "sets up the cli" do
expect(subject).to receive(:setup_cli)
expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
end

# TODO - test for Sender.new.run in thread
it "performs the steps necessary to capture telemetry" do
expect(telemetry).to receive(:timed_run_capture).and_yield
expect(telemetry).to receive(:commit)
expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
end

it "calls perform_run" do
expect(subject).to receive(:perform_run)
expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(0) }
end

context "perform_run raises WrappedError" do
let(:e) { ChefRun::WrappedError.new(RuntimeError.new("Test"), "host") }

it "prints the error and exits" do
expect(subject).to receive(:perform_run).and_raise(e)
expect(ChefRun::UI::ErrorPrinter).to receive(:show_error).with(e)
expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(1) }
end
end

context "perform_run raises SystemExit" do
it "exits with same exit code" do
expect(subject).to receive(:perform_run).and_raise(SystemExit.new(99))
expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(99) }
end
end

context "perform_run raises any other exception" do
let(:e) { Exception.new("test") }

it "exits with same exit code" do
expect(subject).to receive(:perform_run).and_raise(e)
expect(ChefRun::UI::ErrorPrinter).to receive(:dump_unexpected_error).with(e)
expect { subject.run }.to raise_error(SystemExit) { |e| expect(e.status).to eq(64) }
end
end
end

describe "#perform_run" do
Expand Down