Created attachment 3632 [details] [review] fix printoverdues.sh bash odities
Created attachment 3810 [details] [review] Modify all variables set
$ misc/cronjobs/printoverdues.sh ~/Downloads/ /usr/lib/pymodules/python2.6/pyPdf/pdf.py:52: DeprecationWarning: the sets module is deprecated from sets import ImmutableSet Converting /home/atz/Downloads//2_factor.html to /home/atz/Downloads/2_factor.pdf... tar: Cowardly refusing to create an empty archive Try `tar --help' or `tar --usage' for more information. Looks like problems with the tar targetting. $directory is never set to anything (should be $1). This script needs a lot more error checking. For example, we shouldn't be tarring or printing ANYTHING if xhtml2pdf has failed or returned error. This is pretty likely to happen since the script requires an entirely new language dependency and a large package in that language. Command line options are also illogically dependent. We shouldn't have to specify a CSS file in order to target print lp3. However, the actual patch is OK, it's just the script as a whole needs work. Let me know if you want me to proceed with broader fixes.
We shouldn't be tarring the whole directory every time, and CERTAINLY not putting the gzipped tar inside the same directory if we are. Otherwise we'd end up with recursive backup sets where every new backup contains all the other backup sets. This is the wrong way to structure a backup because you will get WORSE compression combining already compressed files and because we would be creating a whole lot of duplicate data by always containing yesterday's file (which contains the day before which contains the day before, etc.). Ideally, we'd invoke something like logrotate, but short of that, we should only backup the HTMLs and/or PDFs that we operated on during this execution. If xhtml2pdf wasn't so obtuse, we would be able to pass it multiple source file arguments directly (rather than an uninterpolated pseudo-glob), and therefore also accept them with this script (rather than a dir). So the question is: do we want to tar/gzip the HTML, the PDFs or both? They are ostensibly the same data, so having both is unnecessary, afaict.
Joe, I don't understand everything from your comments - is this a sign-off but still needs improvement or a failed qa?
Created attachment 5269 [details] [review] Bug 6048: printoverdues.sh bash odities Signed-off-by: Robin Sheat <robin@catalyst.net.nz> Note: this script really needs a rewrite, but this patch does fix up the things it's supposed to fix up.
QA comment : * assigning this bug to Jonathan, that wrote the patch * small script, that fixes some bash complaints * isolated script, no consequences on the rest of Koha The script should all be rewritten, but the patch fixes the warnings, so marking passed QA
Pushed, please test
There have been no further reports of problems so I am marking this bug resolved.