Hi Related to #33645 (and probably #25634), I notice that koha-foreach always returns 0 (success) even if the commands it tried to run failed. I think the correct behavior for this script should be that koha-foreach will return success if all the commands it tried to run succeeded, but failure if any of the commands failed. Test plan: 0) `koha-foreach /bin/true; echo $?` 1) Note that the exit status is 0 2) `koha-foreach /bin/false; echo $?` 3) Note that the exit status is 0 (incorrect) 4) Apply patch 5) `koha-foreach /bin/true; echo $?` 6) Note that the exit status is 0 7) `koha-foreach /bin/false; echo $?` 8) Note that the exit status is 1 (correct)
Created attachment 155160 [details] [review] Fix te exit code for koha-foreach
It would be good to test this with a command that works for the first instance but not the next instance. If I call correctly "set -e" should cause the whole koha-foreach to fail at that point so in theory this patch should work...
The test for this one is a bit too simplistic, and it misses some steps. For instance, after "Apply patch", you need to either copy debian/scripts/koha-foreach to /usr/sbin/koha-foreach, or run the command from "debian/scripts/koha-foreach". As for the simplicity, I've created an alternative test: 1. koha-create --create-db test 2. vi test.sh #!/bin/sh if [ $USER = 'kohadev-koha' ]; then echo "FAILED"; exit 1; else echo "SUCCESS"; exit 0; fi root@kohadevbox:koha(bug_34653)$ debian/scripts/koha-foreach sh test.sh FAILED kohadev: 1 status returned by "sh test.sh" SUCCESS root@kohadevbox:koha(bug_34653)$ echo $? 1 -- With this patch, the exit code will be whatever the last non-zero exit code was in the koha-foreach loop. I'm not sure that's really what we want. I think it might be better if we exit with "1" if any command has a non-zero exit code. To do that, we could keep the current patch for the most part, or we could increment an error counter, or something else. Because we're running in a loop and apparently koha-foreach doesn't ditch out on non-zero exit code, I don't think we should just return the last $rv encountered.
(Little reminder to change the Assignee to yourself after posting a patch as well.)
Thanks David That's all very helpful feedback. I think understand everything you're saying, so I'll update the patch to include all that. We'll find out if I truly understand soon :) Thanks again
Created attachment 155248 [details] [review] Fix to exit code for koha-foreach
Oh, I realise i need to update the status now... Please let me know if there is anything else required. Thanks
Hi Evan. This seems to work - using the test plan David Cook provided in comment #3. I haven't signed this off, as the patch needs to be formatted as per the Commit messages guideline - see https://wiki.koha-community.org/wiki/Commit_messages David (Nind)
Created attachment 155770 [details] [review] Bug 34653: Make koha-foreach return the correct status code I think the correct behavior for this script should be that koha-foreach will return 0 (success) if all the commands it tried to run succeeded, but 1 (failure) if any of the commands failed. To test: 1. $ koha-create --create-db test 2. $ vi test.sh if [ $USER = 'kohadev-koha' ]; then echo "FAILED"; exit 1; else echo "SUCCESS"; exit 0; fi 3. $ debian/scripts/koha-foreach sh test.sh FAILED kohadev: 1 status returned by "sh test.sh" SUCCESS 4. $ echo $? 5. Note that the exit status is 0 (success) 6. Apply patch 7. $ debian/scripts/koha-foreach sh test.sh FAILED kohadev: 1 status returned by "sh test.sh" SUCCESS 8. $ echo $? 9. Note that the exit status is 1 (failure)
Hi David I did see the "Commit Messages" wiki page, and tried to follow the guidelines there. Apologies for being difficult, but could you please let me know which conventions I missed in the commit message? It's not obvious to me as a beginner.. I don't know if it helps, but I have just installed & configured "git bz" to re-upload the patch. I see that it's done some things differently (maybe better?) Any advice welcome! Thanks
Created attachment 155777 [details] [review] Bug 34653: Make koha-foreach return the correct status code I think the correct behavior for this script should be that koha-foreach will return 0 (success) if all the commands it tried to run succeeded, but 1 (failure) if any of the commands failed. To test: 1. $ koha-create --create-db test 2. $ vi test.sh if [ $USER = 'kohadev-koha' ]; then echo "FAILED"; exit 1; else echo "SUCCESS"; exit 0; fi 3. $ debian/scripts/koha-foreach sh test.sh FAILED kohadev: 1 status returned by "sh test.sh" SUCCESS 4. $ echo $? 5. Note that the exit status is 0 (success) 6. Apply patch 7. $ debian/scripts/koha-foreach sh test.sh FAILED kohadev: 1 status returned by "sh test.sh" SUCCESS 8. $ echo $? 9. Note that the exit status is 1 (failure) Signed-off-by: David Nind <david@davidnind.com>
(In reply to Evan Giles from comment #10) > Hi David > > I did see the "Commit Messages" wiki page, and tried to follow the > guidelines there. Apologies for being difficult, but could you please let > me know which conventions I missed in the commit message? It's not obvious > to me as a beginner.. > > I don't know if it helps, but I have just installed & configured "git bz" to > re-upload the patch. I see that it's done some things differently (maybe > better?) > > Any advice welcome! > Thanks Hi Evan. It looks like it was my mistake - I was expecting to see the bug title, description and test plan as part of comment #6. Looking at the details for the obsoleted patches, it looks like it was there all along! I'm not sure, but I guess it must depend on how the patch is attached to the bug. I normally (using koha-testing-docker) I go: - git so X - git bz attach -e bugnumber HEAD~X.. (where X = the number of patches) It all looks good now, and I've signed off. Thanks! David
Created attachment 156042 [details] [review] Bug 34653: Make koha-foreach return the correct status code I think the correct behavior for this script should be that koha-foreach will return 0 (success) if all the commands it tried to run succeeded, but 1 (failure) if any of the commands failed. To test: 1. $ koha-create --create-db test 2. $ vi test.sh if [ $USER = 'kohadev-koha' ]; then echo "FAILED"; exit 1; else echo "SUCCESS"; exit 0; fi 3. $ debian/scripts/koha-foreach sh test.sh FAILED kohadev: 1 status returned by "sh test.sh" SUCCESS 4. $ echo $? 5. Note that the exit status is 0 (success) 6. Apply patch 7. $ debian/scripts/koha-foreach sh test.sh FAILED kohadev: 1 status returned by "sh test.sh" SUCCESS 8. $ echo $? 9. Note that the exit status is 1 (failure) Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.04
Nice work everyone! Pushed to oldstable for 22.11.x