[Bug 34653] New: Make koha-foreach return the correct status code
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Bug ID: 34653 Summary: Make koha-foreach return the correct status code Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Command-line Utilities Assignee: koha-bugs@lists.koha-community.org Reporter: evan@catalyst.net.nz QA Contact: testopia@bugs.koha-community.org CC: robin@catalyst.net.nz 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) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Evan Giles <evan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |evan@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 --- Comment #1 from Evan Giles <evan@catalyst.net.nz> --- Created attachment 155160 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155160&action=edit Fix te exit code for koha-foreach -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Evan Giles <evan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Evan Giles <evan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #2 from David Cook <dcook@prosentient.com.au> --- 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... -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #3 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- (Little reminder to change the Assignee to yourself after posting a patch as well.) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 --- Comment #5 from Evan Giles <evan@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 wainuiwitikapark@catalyst.net.nz changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Assignee|koha-bugs@lists.koha-commun |evan@catalyst.net.nz |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Evan Giles <evan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155160|0 |1 is obsolete| | --- Comment #6 from Evan Giles <evan@catalyst.net.nz> --- Created attachment 155248 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155248&action=edit Fix to exit code for koha-foreach -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Evan Giles <evan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #7 from Evan Giles <evan@catalyst.net.nz> --- Oh, I realise i need to update the status now... Please let me know if there is anything else required. Thanks -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #8 from David Nind <david@davidnind.com> --- 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 --- Comment #9 from Evan Giles <evan@catalyst.net.nz> --- Created attachment 155770 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155770&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Evan Giles <evan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155248|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 --- Comment #10 from Evan Giles <evan@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155770|0 |1 is obsolete| | --- Comment #11 from David Nind <david@davidnind.com> --- Created attachment 155777 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155777&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 --- Comment #12 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|Trivial patch |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155777|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 156042 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156042&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Version(s)|23.11.00 |23.11.00,23.05.04 released in| | Status|Pushed to master |Pushed to stable --- Comment #15 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 Jacob O'Mara <jacob.omara@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|23.11.00,23.05.04 |23.11.00,23.05.04,22.11.11 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34653 --- Comment #16 from Jacob O'Mara <jacob.omara@ptfs-europe.com> --- Nice work everyone! Pushed to oldstable for 22.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org