[SHACK-145] Add handling for multi-target errors #98

marcparadise · 2018-04-30T19:52:16Z

This wraps the actions for a multi-target command
(converge in this case) in a Terminal::Job instance,
which tracks its failure state.

After all jobs are complete, this collects those that failed
and sends them over to ErrorPrinter for formatting and capture
to file, while a top level MULTI001 (basically: one or more jobs failed)
error is shown to the user with the location of the errors file.

Aside: renamed Terminal::Action to Terminal::Job to avoid confusion with ChefWorkstation::Action.

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

marcparadise · 2018-04-30T20:05:51Z

components/chef-workstation/lib/chef-workstation/command/base.rb

+      # When running multiple jobs, exceptions are captured to the
+      # job to avoid interrupting other jobs in process.  This function
+      # collects them and raises a MultiJobFailure if failure has occurred;
+      # we do *not* differentiate between one failed jobs and multiple failed jobs


The question here is, did I say "one failed jobs" to underscore the point around not differentiating, or was it just a typo... :D

Either way, you can't back down now!

jonsmorrow

Looking good so far

tyler-ball

Some minor questions and issues I saw, it also looks like it needs some more test coverage. But otherwise coming along well!

tyler-ball · 2018-04-30T20:18:25Z

components/chef-workstation/lib/chef-workstation/command/base.rb

+      # When running multiple jobs, exceptions are captured to the
+      # job to avoid interrupting other jobs in process.  This function
+      # collects them and raises a MultiJobFailure if failure has occurred;
+      # we do *not* differentiate between one failed jobs and multiple failed jobs


Either way, you can't back down now!

tyler-ball · 2018-04-30T20:20:04Z

components/chef-workstation/lib/chef-workstation/config.rb

@@ -28,6 +28,10 @@ def default_location
        File.join(WS_BASE_PATH, "config.toml")
      end

+      def error_output_path
+        File.join(File.dirname(log.location), "errors.txt")


Are we sure we want to have 3 different log files? I think I would rather have these errors show up in our normal log file

I'm looking to provide a place where they can quickly look and see only the messages related to the failures that just occurred.

Relatedly, I'd been thinking it would be nice to give something like chef show errors which does that for you.

I like that idea! We could introduce chef errors show and chef errors report (with report sending a bug report to us)

tyler-ball · 2018-04-30T20:20:28Z

components/chef-workstation/lib/chef-workstation/ui/error_printer.rb

@@ -33,13 +33,43 @@ def t
    DEFAULT_ERROR_NO = "CHEFINT001"

    def self.show_error(e)
+      # Name is misleading - it's unwrapping but also doing furtehr


tyler-ball · 2018-04-30T20:21:10Z

components/chef-workstation/lib/chef-workstation/ui/error_printer.rb

@@ -33,13 +33,43 @@ def t
    DEFAULT_ERROR_NO = "CHEFINT001"

    def self.show_error(e)


Lets just go ahead and change this to def self.unwrap_error(e) then we can get rid of the comment below

I moved the ugly bits into StandardErrorResollver. It's still ugly, but at least it's contained to one place more-or-less...

tyler-ball · 2018-04-30T20:42:26Z

components/chef-workstation/lib/chef-workstation/ui/error_printer.rb

+        e.jobs.each do |j|
+          # ErrorPrinter only instantiates with a WrappedError:
+          wrapped = ChefWorkstation::WrappedError.new(j.exception, j.target_host)
+          # This is silly, but necessary 'til we clean up (or do away with)


https://chefio.atlassian.net/browse/SHACK-160

tyler-ball · 2018-04-30T20:44:53Z

components/chef-workstation/lib/chef-workstation/ui/terminal.rb

            end
          end
          multispinner.auto_spin
        end

-        def render_action(msg, prefix: "", &block)
-          klass = Object.const_get("ChefWorkstation::UI::#{ChefWorkstation::Config.dev.spinner}")
+        # TODO this should also accept a job.


Would this TODO mean accepting a job object instead of the &block? I would like to see this TODO have more details, or we could just address it as part of this PR

Yeah, it's to replace the block so we can use a consistent interface. I'd been on the fence about doing it in this PR; but it is actually pretty small and it does go together with these changes.

cheeseplus

This makes sense and I really like the idea of adding a chef show errors, thinking similar to journalctl style quick access to failures

tyler-ball

This wraps the actions for a multi-target command (converge in this case) in a Terminal::Job instance, which tracks its failure state. After all jobs are complete, this collects those that failed and sends them over to ErrorPrinter for formatting and capture to file, while a top level MULTI001 (basically: one or more jobs failed) error is shown to the user with the location of the errors file. Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>

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

marcparadise requested a review from a team April 30, 2018 19:52

marcparadise force-pushed the SHACK-145/multitarget/error-handling branch 2 times, most recently from 9cafe8f to 0d8a568 Compare April 30, 2018 20:05

marcparadise commented Apr 30, 2018

View reviewed changes

jonsmorrow reviewed Apr 30, 2018

View reviewed changes

tyler-ball suggested changes Apr 30, 2018

View reviewed changes

marcparadise force-pushed the SHACK-145/multitarget/error-handling branch from d9a822c to c0e3000 Compare May 2, 2018 17:59

marcparadise changed the title ~~Add handling for multi-target errors~~ [SHACK-145] Add handling for multi-target errors May 2, 2018

marcparadise force-pushed the SHACK-145/multitarget/error-handling branch from c0e3000 to 96e9225 Compare May 2, 2018 18:01

cheeseplus previously approved these changes May 4, 2018

View reviewed changes

tyler-ball previously approved these changes May 4, 2018

View reviewed changes

jonsmorrow previously approved these changes May 4, 2018

View reviewed changes

marcparadise dismissed stale reviews from jonsmorrow, tyler-ball, and cheeseplus via aad6479 May 4, 2018 15:50

marcparadise force-pushed the SHACK-145/multitarget/error-handling branch from 3a8f5e1 to aad6479 Compare May 4, 2018 15:50

marcparadise added 2 commits May 4, 2018 11:53

Code review comments, additional testing

99404b7

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

marcparadise force-pushed the SHACK-145/multitarget/error-handling branch from aad6479 to 99404b7 Compare May 4, 2018 15:53

marcparadise merged commit 7741db6 into master May 4, 2018

chef-ci deleted the SHACK-145/multitarget/error-handling branch May 4, 2018 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SHACK-145] Add handling for multi-target errors #98

[SHACK-145] Add handling for multi-target errors #98

marcparadise commented Apr 30, 2018

marcparadise Apr 30, 2018

tyler-ball Apr 30, 2018

jonsmorrow left a comment

tyler-ball left a comment

tyler-ball Apr 30, 2018

tyler-ball Apr 30, 2018

marcparadise May 1, 2018

tyler-ball May 4, 2018

tyler-ball Apr 30, 2018

tyler-ball Apr 30, 2018

marcparadise May 2, 2018

tyler-ball Apr 30, 2018

marcparadise May 1, 2018

tyler-ball Apr 30, 2018

marcparadise May 1, 2018

cheeseplus left a comment

tyler-ball left a comment

		@@ -33,13 +33,43 @@ def t
		DEFAULT_ERROR_NO = "CHEFINT001"

		def self.show_error(e)

[SHACK-145] Add handling for multi-target errors #98

[SHACK-145] Add handling for multi-target errors #98

Conversation

marcparadise commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonsmorrow left a comment

Choose a reason for hiding this comment

tyler-ball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheeseplus left a comment

Choose a reason for hiding this comment

tyler-ball left a comment

Choose a reason for hiding this comment