We introduced color into our db update output, but we never implemented it in the upstream code that catches database level failures.
Created attachment 174250 [details] [review] Bug 38299: [DO NOT PUSH] Test dbrev
Created attachment 174251 [details] [review] Bug 38299: Make filehandler optional for colored outputs Allow say_success, say_failure, etc, to omit the filehandler parameter so that they can be used without explicitly opening a filehandler if you are just printing to STDOUT and don't already have one. If a filehandler is passed, the existing behavior is unchanged.
Created attachment 174252 [details] [review] Bug 38299: Use say_failure when printing update errors to the console Bug 35681 added subroutines to print colored output from database updates. Incorporate the subroutine say_failure into the error handling in the upstream code, so that errors caught upstream can take advantage of this new subroutine and display in red. To test: 1. Apply patch for test dbrev 2. From the kshell, run updatedatabase --> The messages for success, info, warning, and failure that were explicitly printed in the dbrev are colored, but the database error is not colored. 3. Apply other patches 4. Run updatedatabase again --> All messages including the database error are now colored
Created attachment 174254 [details] [review] Bug 38299: [DO NOT PUSH] Test dbrev
Created attachment 174255 [details] [review] Bug 38299: Make filehandler optional for colored outputs Allow say_success, say_failure, etc, to omit the filehandler parameter so that they can be used without explicitly opening a filehandler if you are just printing to STDOUT and don't already have one. If a filehandler is passed, the existing behavior is unchanged.
Created attachment 174256 [details] [review] Bug 38299: Use say_failure when printing update errors to the console Bug 35681 added subroutines to print colored output from database updates. Incorporate the subroutine say_failure into the error handling in the upstream code, so that errors caught upstream can take advantage of this new subroutine and display in red. To test: 1. Apply patch for test dbrev 2. From the kshell, run updatedatabase --> The messages for success, info, warning, and failure that were explicitly printed in the dbrev are colored, but the database error is not colored. 3. Apply other patches 4. Run updatedatabase again --> All messages including the database error are now colored
Created attachment 174259 [details] [review] Bug 38299: Update tests To test: prove -v t/Koha/Installer/Output.t
Created attachment 174290 [details] [review] Bug 38299: [DO NOT PUSH] Test dbrev Signed-off-by: David Nind <david@davidnind.com>
Created attachment 174291 [details] [review] Bug 38299: Make filehandler optional for colored outputs Allow say_success, say_failure, etc, to omit the filehandler parameter so that they can be used without explicitly opening a filehandler if you are just printing to STDOUT and don't already have one. If a filehandler is passed, the existing behavior is unchanged. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 174292 [details] [review] Bug 38299: Use say_failure when printing update errors to the console Bug 35681 added subroutines to print colored output from database updates. Incorporate the subroutine say_failure into the error handling in the upstream code, so that errors caught upstream can take advantage of this new subroutine and display in red. To test: 1. Apply patch for test dbrev 2. From the kshell, run updatedatabase --> The messages for success, info, warning, and failure that were explicitly printed in the dbrev are colored, but the database error is not colored. 3. Apply other patches 4. Run updatedatabase again --> All messages including the database error are now colored Signed-off-by: David Nind <david@davidnind.com>
Created attachment 174293 [details] [review] Bug 38299: Update tests To test: prove -v t/Koha/Installer/Output.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 174421 [details] [review] Bug 38299: (discussion) Move failure into same level This patch adds the failures in at the same level as the rest of the revision output (i.e. indented under the specific database revision) and uses the standard failure coloring instead of the local update_error class. This is a proposal and may need more work to clarify and perhaps remove the duplicated/collected errors at the bottom of the update. Now that we hard stop if an update fails, I'm not sure collecting errors at the bottom makes as much sense as it once did?
Created attachment 174423 [details] [review] Bug 38299: (discussion) Move failure into same level This patch adds the failures in at the same level as the rest of the revision output (i.e. indented under the specific database revision) and uses the standard failure coloring instead of the local update_error class. This is a proposal and may need more work to clarify and perhaps remove the duplicated/collected errors at the bottom of the update. Now that we hard stop if an update fails, I'm not sure collecting errors at the bottom makes as much sense as it once did?
Created attachment 174424 [details] With proper indentation and class
It feels somewhat odd looking to me that we collect some errors separately at the end.. I've added a patch that moves the output up a level into the output block for database revision being run.. I've left the output to stderr in place for now so we're getting both the collected set and the inlined error in the screenshot. Wondering what others think here.. now that we die on error I'm thinking it no longer makes sense to collect such errors separately at the bottom and we could just remove that bit of handling.
Created attachment 174455 [details] [review] Bug 38299: (follow-up) (discussion) Move failure into same level
(In reply to Martin Renvoize (ashimema) from comment #13) > Created attachment 174423 [details] [review] [review] > Bug 38299: (discussion) Move failure into same level > > This patch adds the failures in at the same level as the rest of the > revision output (i.e. indented under the specific database revision) and > uses the standard failure coloring instead of the local update_error > class. > > This is a proposal and may need more work to clarify and perhaps remove > the duplicated/collected errors at the bottom of the update. Now that we > hard stop if an update fails, I'm not sure collecting errors at the > bottom makes as much sense as it once did? Looks ok to me, Emily can you confirm you agree with this change?
I think I might have preferred to have the fh as an 'out' option, like we do for 'up', for not blocker.
I think I might have preferred to have the fh as an 'out' option, like we do for 'up', but not blocker.
Looks good to me! Thanks for the feedback on this!
Created attachment 174673 [details] [review] Bug 38299: Make filehandler optional for colored outputs Allow say_success, say_failure, etc, to omit the filehandler parameter so that they can be used without explicitly opening a filehandler if you are just printing to STDOUT and don't already have one. If a filehandler is passed, the existing behavior is unchanged. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 174674 [details] [review] Bug 38299: Use say_failure when printing update errors to the console Bug 35681 added subroutines to print colored output from database updates. Incorporate the subroutine say_failure into the error handling in the upstream code, so that errors caught upstream can take advantage of this new subroutine and display in red. To test: 1. Apply patch for test dbrev 2. From the kshell, run updatedatabase --> The messages for success, info, warning, and failure that were explicitly printed in the dbrev are colored, but the database error is not colored. 3. Apply other patches 4. Run updatedatabase again --> All messages including the database error are now colored Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 174675 [details] [review] Bug 38299: Update tests To test: prove -v t/Koha/Installer/Output.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 174676 [details] [review] Bug 38299: (discussion) Move failure into same level This patch adds the failures in at the same level as the rest of the revision output (i.e. indented under the specific database revision) and uses the standard failure coloring instead of the local update_error class. This is a proposal and may need more work to clarify and perhaps remove the duplicated/collected errors at the bottom of the update. Now that we hard stop if an update fails, I'm not sure collecting errors at the bottom makes as much sense as it once did? Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 174677 [details] [review] Bug 38299: (follow-up) (discussion) Move failure into same level Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed for 24.11! Well done everyone, thank you!
It looks like we caused some bad breakage on Jenkins with this one, because I pushed the "DO NOT PUSH". Will revert.