Description
Marcel de Rooy
2025-04-24 13:32:09 UTC
Still thinking about Joubu's suggestion on 36932:
> 2. split dev_install in the config and have gitified + dev_install
Actually, I now think that I just need some flag to tell me if it is a production or a test system (not KTD). (Not just that is gitified, since I can have a gitified production and a gitified test system.)
Say that I find $KTD_HOME in the env of some system, then I know that it should be a test system.
But note that not all test systems are using KTD (mine..), so [in that case] I need another var to indicate also that it is a test system. What about TEST_SYSTEM :)
The confusion comes from the fact that DEV_INSTALL actually only means that the setup of some folders is different. Misc, not bin. Lib in root. No cgi-bin in path of opac and intranet folders. But (in some cases like mine) it could be a production system!
Adding TEST_SYSTEM or something similar for non-KTD test systems might be the easiest approach to undo the 36932 side-effects.
Any feedback?
Created attachment 184187 [details] [review] Bug 39740: Add is_test_system to koha-functions, incorporate in koha-plack Result of is_test_system will be determined by looking at KTD_HOME and TEST_SYSTEM in the environment. If is_test_system returns true or the flag --development (introduced on bug 36932) is set when calling koha-plack, koha-plack allows dumping a stack trace. Test plan: Copy koha-plack to /usr/sbin, and koha-functions.sh to /usr/share/koha/bin. Source bin/koha-functions.sh ('. bin/koha-functions.sh') Play with values of KTD_HOME and TEST_SYSTEM and look at results of: if is_test_system; then echo 'Test system'; fi Add a die in e.g. opac/opac-main.pl causing a 500. Do you see a stack trace and is that expected? It will be in KTD, otherwise you need to make sure that TEST_SYSTEM=1 is exported in your environment. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Marking as a minor fix of the change in 36932 which was minor too. (In reply to Marcel de Rooy from comment #3) > Marking as a minor fix of the change in 36932 which was minor too. Minor? Devs do not have stacktrace for several months, it's critical :D (In reply to Jonathan Druart from comment #4) > (In reply to Marcel de Rooy from comment #3) > > Marking as a minor fix of the change in 36932 which was minor too. > > Minor? Devs do not have stacktrace for several months, it's critical :D Well, please test asap :) KTD_HOME is not set within the ktd container, this does not work. (In reply to Jonathan Druart from comment #6) > KTD_HOME is not set within the ktd container, this does not work. Please list the exported env vars. Just curious about this statement in the KTD repo btw: Set some mandatory environment variables: echo "export PROJECTS_DIR=$PROJECTS_DIR" >> ~/.bashrc echo 'export SYNC_REPO=$PROJECTS_DIR/koha' >> ~/.bashrc echo 'export KTD_HOME=$PROJECTS_DIR/koha-testing-docker' >> ~/.bashrc echo 'export PATH=$PATH:$KTD_HOME/bin' >> ~/.bashrc echo 'export LOCAL_USER_ID=$(id -u)' >> ~/.bashrc (In reply to Marcel de Rooy from comment #8) > Just curious about this statement in the KTD repo btw: > > Set some mandatory environment variables: > > > echo "export PROJECTS_DIR=$PROJECTS_DIR" >> ~/.bashrc > echo 'export SYNC_REPO=$PROJECTS_DIR/koha' >> ~/.bashrc > echo 'export KTD_HOME=$PROJECTS_DIR/koha-testing-docker' >> ~/.bashrc > echo 'export PATH=$PATH:$KTD_HOME/bin' >> ~/.bashrc > echo 'export LOCAL_USER_ID=$(id -u)' >> ~/.bashrc Yes, on the host. This is to make the `ktd` script working on the host, to startup the container. I wrote that: https://gitlab.com/joubu/koha-misc4dev/-/commit/ddd5f61b0b62c775326ae7b98898aed50a20fe02 But the flag is lost when restart_all is called, because we restart koha-common which does not (and should not) pass -dev. Why not splitting the config finally? we should not consider ktd is the only koha dev env. Checking for a "ktd" env var from koha-plack is too specific. for me the "is dev env setup" config flag is the correct solution (In reply to Jonathan Druart from comment #11) > Why not splitting the config finally? > > we should not consider ktd is the only koha dev env. Checking for a "ktd" > env var from koha-plack is too specific. > > for me the "is dev env setup" config flag is the correct solution Yeah, ok. The confusing thing is the existing DEV_INSTALL and what it actually means in the code. Created attachment 184432 [details] [review] Bug 39740: Add is_test_system to koha-functions, incorporate in koha-plack Result of is_test_system will be determined by looking at TEST_SYSTEM in the environment. If is_test_system returns true or the flag --development (introduced on bug 36932) is set when calling koha-plack, koha-plack allows dumping a stack trace. Test plan: Copy koha-plack to /usr/sbin, and koha-functions.sh to /usr/share/koha/bin. Source bin/koha-functions.sh ('. bin/koha-functions.sh') Play with value of TEST_SYSTEM and look at results of: if is_test_system; then echo 'Test system'; fi Add a die in e.g. opac/opac-main.pl causing a 500. Do you see a stack trace and is that expected (you need to make sure that TEST_SYSTEM=1 is exported in your environment; e.g. add to $KTD_HOME/.env). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Suggestion (same as before): 1. Replace the existing dev_install/DEV_INSTALL (that is dealing with paths adjustement) with "gitified" 2. Add the dev_install that is going to be used for dev purpose (here passing -dev to koha-plack) We will need a trivial change in ktd (add gitified): files/templates/koha-conf-site.xml.in: <dev_install>1</dev_install> (In reply to Jonathan Druart from comment #14) > Suggestion (same as before): > 1. Replace the existing dev_install/DEV_INSTALL (that is dealing with paths > adjustement) with "gitified" > 2. Add the dev_install that is going to be used for dev purpose (here > passing -dev to koha-plack) > > We will need a trivial change in ktd (add gitified): > files/templates/koha-conf-site.xml.in: <dev_install>1</dev_install> See comment1 (In reply to Jonathan Druart from comment #6) > KTD_HOME is not set within the ktd container, this does not work. Removed from the patch. Waiting for a reponse from Tomas I'd say KTD should set an env flag and we should query for it. (In reply to Tomás Cohen Arazi (tcohen) from comment #18) > I'd say KTD should set an env flag and we should query for it. Hi. This patch checks TEST_SYSTEM in the env. Does that correspond with what you meant here? We could rename DEV_INSTALL to GIT_INSTALL btw to perhaps lower confusion? (In reply to Marcel de Rooy from comment #19) > We could rename DEV_INSTALL to GIT_INSTALL btw to perhaps lower confusion? This is almost what I suggested. However I think we need the flag in the koha config as well. (In reply to Tomás Cohen Arazi (tcohen) from comment #18) > I'd say KTD should set an env flag and we should query for it. Let me clarify my comment :-D I think we should have a `koha-conf.xml` entry or section for this things. I would like to be able to enable stack traces in koha-conf.xml instead of detecting how it is being run. We could also override using variables which `run.sh` (the startup script KTD uses) could set. This could be useful for running in jenkins where we might want to disable stacktraces for (say) Cypress tasks so they are similar to production, but enable them by default in KTD. Created attachment 184798 [details] [review] Bug 39740: Add is_git_install and is_test_system Test plan: Copy koha-functions.sh to /usr/share/koha/bin. Source bin/koha-functions.sh ('. bin/koha-functions.sh') Test in bash (Yes expected; adjust instance name): GIT_INSTALL=1; if is_git_install your_instance; then echo Yes; fi Add/set git_install to 1 in koha-conf.xml. Test in bash (Yes expected): unset GIT_INSTALL; if is_git_install your_instance; then echo Yes; fi Set git_install to 0 in koha-conf.xml. Test in bash (No expected): unset GIT_INSTALL; if ! is_git_install your_instance; then echo No; fi Remove git_install and add/set dev_install to 1 in koha-conf.xml. Test in bash (Yes expected): unset GIT_INSTALL; if is_git_install your_instance; then echo Yes; fi A few similar tests with test_system. Add/set test_system to 0 in koha-conf.xml. Test in bash (Yes expected): TEST_SYSTEM=1; if is_test_system your_instance; then echo Yes; fi Test in bash (No expected): unset TEST_SYSTEM; if ! is_test_system your_instance; then echo No; fi Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 184799 [details] [review] Bug 39740: Rename function to adjust_paths_git_install Logical follow-up for moving away from dev_install to git_install. Result of running git grep -l adjust_paths_dev_install | xargs sed -i -e "s/adjust_paths_dev_install/adjust_paths_git_install/g" Test plan: Read the patch. The scripts will be tested later on. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 184800 [details] [review] Bug 39740: Replace DEV_INSTALL by GIT_INSTALL Resulting from the following sed statement: git grep -l DEV_INSTALL | xargs sed -i -e "s/DEV_INSTALL/GIT_INSTALL/g" Test plan: Read the patch. The scripts will be tested later on. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 184801 [details] [review] Bug 39740: Fix a few occurrences of dev_install in code Test plan: Check if git_install is set correctly for your system in koha-conf.xml. Adjust otherwise. cp debian/scripts/koha-functions.sh /usr/share/koha/bin/ cp debian/scripts/koha-* /usr/sbin/ Run koha-shell your_clone. Does that works like expected? Run t/Koha/Config.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 184802 [details] [review] Bug 39740: Replace envvar check by calling is_git_install on instance level Replace the $GIT_INSTALL check by is_git_install. Some scripts did call is_git_install once while potentially looping over multiple instances. Note: koha-reset-passwd only runs on one instance (unlike most). Test plan: Check/adjust git_install and test_system in koha-conf.xml for used instances. cp debian/scripts/koha-functions.sh /usr/share/koha/bin/ cp debian/scripts/koha-* /usr/sbin/ Note: We will still test koha-plack and -z3950-responder later. Test running a few of the other changed koha-* scripts on one or more instances. Verify that things work as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 184803 [details] [review] Bug 39740: Adjust koha-plack and koha-z3950-responder Changes to koha-plack - Check is_test_system and -development for stack trace - Add check on set $environment for debugger - Move export to do_instance, remove three local's NOTE: GIT_INSTALL ie exported for koha-plack ONLY! The debian scripts call is_git_install. Changes to koha-z3950-responder - debugger needs test_system flag - Rearrange exports before calling start_z3950 and friends. Test plan: Check/adjust git_install and test_system in koha-conf.xml for used instances. cp debian/scripts/koha-functions.sh /usr/share/koha/bin/ cp debian/scripts/koha-* /usr/sbin/ cp debian/templates/plack.psgi /etc/koha/plack.psgi Add a die in e.g. opac/opac-main.pl. Set test_system to 1 in koha-conf.xml or export TEST_SYSTEM=1 in your environment. Restart Plack. Hit opac-main. Do you see a stack trace? Export TEST_SYSTEM=0. Restart Plack. Hit opac-main again. You should not see a stack trace. Start koha-z3950-responder with/without debug flag. Works as expected? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 184804 [details] [review] Bug 39740: Adjust koha-conf.xml Test plan: Look at changes in this patch. Note that we already tested these settings with former patches. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Tomás Cohen Arazi (tcohen) from comment #21) > (In reply to Tomás Cohen Arazi (tcohen) from comment #18) > > I'd say KTD should set an env flag and we should query for it. > > Let me clarify my comment :-D Thanks > I think we should have a `koha-conf.xml` entry or section for this things. I > would like to be able to enable stack traces in koha-conf.xml instead of > detecting how it is being run. > > We could also override using variables which `run.sh` (the startup script > KTD uses) could set. This could be useful for running in jenkins where we > might want to disable stacktraces for (say) Cypress tasks so they are > similar to production, but enable them by default in KTD. Two koha-conf entries: git_install and test_system. Two (optional) env vars overriding them: GIT_INSTALL and TEST_SYSTEM. Please have a look. Patch 5 includes a few more changes for consistency between koha scripts. Excellent, thanks for the patches. Just wondering if TEST_SYSTEM shouldn't actually be DEBUG_MODE (?) (In reply to Jonathan Druart from comment #30) > Excellent, thanks for the patches. > > Just wondering if TEST_SYSTEM shouldn't actually be DEBUG_MODE (?) It was already used ;) But they sound very close indeed.. Hmm. Spotting - if [ "$GIT_INSTALL" = "" ]; then + if ! is_git_install $name; then (In reply to Marcel de Rooy from comment #32) > Hmm. Spotting > > - if [ "$GIT_INSTALL" = "" ]; then > + if ! is_git_install $name; then Thats correct. Scratch it. (In reply to Marcel de Rooy from comment #31) > (In reply to Jonathan Druart from comment #30) > > Excellent, thanks for the patches. > > > > Just wondering if TEST_SYSTEM shouldn't actually be DEBUG_MODE (?) > > It was already used ;) But they sound very close indeed.. The env var is not used DEBUG_MODE yet (is it?). We only using one option in koha-plack (and koha-z3950-responder?), and that would make sense to turn debug-mode ON when $DEBUG_MODE is ON. (In reply to Jonathan Druart from comment #30) > Excellent, thanks for the patches. > > Just wondering if TEST_SYSTEM shouldn't actually be DEBUG_MODE (?) I agree we should pick DEBUG_MODE. (In reply to Tomás Cohen Arazi (tcohen) from comment #35) > (In reply to Jonathan Druart from comment #30) > > Excellent, thanks for the patches. > > > > Just wondering if TEST_SYSTEM shouldn't actually be DEBUG_MODE (?) > > I agree we should pick DEBUG_MODE. Thx Will do. 2 - 1 Created attachment 184873 [details] [review] Bug 39740: (QA follow-up) Rename test_system/TEST_SYSTEM See comment30 and comment36. Result of two sed's: git grep -l test_system | xargs sed -i -e "s/test_system/debug_mode/g" git grep -l TEST_SYSTEM | xargs sed -i -e "s/TEST_SYSTEM/DEBUG_MODE/g" The next patch will take care of the debugger options in the plack and z3950 responder scripts. Test plan: Read the patch. (Testing in next patch.) git grep -i test_system Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 184874 [details] [review] Bug 39740: (QA follow-up) Adjust debugger for koha-plack and koha-z3950-responder This patch mainly renames var debug_mode in both scripts when it refers to the debugger instead of the instance's debug_mode. Note: Removing $environment from responder, probably copied. Test plan: cp debian/scripts/koha-functions.sh /usr/share/koha/bin/ cp debian/scripts/koha-* /usr/sbin/ cp debian/templates/plack.psgi /etc/koha/plack.psgi Repeat steps from patch: "Replace envvar check by calling..." Repeat steps from patch: "Adjust koha-plack and koha-z3950-responder" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Open for testing again ! Created attachment 184920 [details] [review] Bug 39740: Fix adjust_paths_git_install in koha-es-indexer 1. We need to adjust koha-conf's koha-testing-docker https://gitlab.com/koha-community/koha-testing-docker/-/blob/main/files/templates/koha-conf-site.xml.in?ref_type=heads replace dev_install with git_install and add debug_mode 2. I am wondering if there is an easy way to keep backward compatibility for dev_install: for instance ktd images for 25.05 won't be able to test "main". And ktd's main won't be able to test older commits. We will always have to reset_all or adjust koha-conf. (In reply to Jonathan Druart from comment #40) > Created attachment 184920 [details] [review] [review] > Bug 39740: Fix adjust_paths_git_install in koha-es-indexer Patch 2 did change it ;) But another patch restored the old name: + adjust_paths_dev_install $name (In reply to Jonathan Druart from comment #41) > 2. I am wondering if there is an easy way to keep backward compatibility for > dev_install: for instance ktd images for 25.05 won't be able to test "main". > And ktd's main won't be able to test older commits. We will always have to > reset_all or adjust koha-conf. I kept a second check for dev_install in the function: # now check koha-conf; looking at dev_install as historical fallback if [ "$instancename" != "" ] && is_instance $instancename; then git_install=$(run_safe_xmlstarlet $instancename git_install) if [ -z "$git_install" ]; then git_install=$(run_safe_xmlstarlet $instancename dev_install); fi (In reply to Marcel de Rooy from comment #43) > (In reply to Jonathan Druart from comment #41) > > 2. I am wondering if there is an easy way to keep backward compatibility for > > dev_install: for instance ktd images for 25.05 won't be able to test "main". > > And ktd's main won't be able to test older commits. We will always have to > > reset_all or adjust koha-conf. > > I kept a second check for dev_install in the function: > # now check koha-conf; looking at dev_install as historical fallback > if [ "$instancename" != "" ] && is_instance $instancename; then > git_install=$(run_safe_xmlstarlet $instancename git_install) > if [ -z "$git_install" ]; then git_install=$(run_safe_xmlstarlet > $instancename dev_install); fi Perfect, I missed that! Created attachment 184989 [details] [review] Bug 39740: Add is_git_install and is_test_system Test plan: Copy koha-functions.sh to /usr/share/koha/bin. Source bin/koha-functions.sh ('. bin/koha-functions.sh') Test in bash (Yes expected; adjust instance name): GIT_INSTALL=1; if is_git_install your_instance; then echo Yes; fi Add/set git_install to 1 in koha-conf.xml. Test in bash (Yes expected): unset GIT_INSTALL; if is_git_install your_instance; then echo Yes; fi Set git_install to 0 in koha-conf.xml. Test in bash (No expected): unset GIT_INSTALL; if ! is_git_install your_instance; then echo No; fi Remove git_install and add/set dev_install to 1 in koha-conf.xml. Test in bash (Yes expected): unset GIT_INSTALL; if is_git_install your_instance; then echo Yes; fi A few similar tests with test_system. Add/set test_system to 0 in koha-conf.xml. Test in bash (Yes expected): TEST_SYSTEM=1; if is_test_system your_instance; then echo Yes; fi Test in bash (No expected): unset TEST_SYSTEM; if ! is_test_system your_instance; then echo No; fi Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184990 [details] [review] Bug 39740: Rename function to adjust_paths_git_install Logical follow-up for moving away from dev_install to git_install. Result of running git grep -l adjust_paths_dev_install | xargs sed -i -e "s/adjust_paths_dev_install/adjust_paths_git_install/g" Test plan: Read the patch. The scripts will be tested later on. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184991 [details] [review] Bug 39740: Replace DEV_INSTALL by GIT_INSTALL Resulting from the following sed statement: git grep -l DEV_INSTALL | xargs sed -i -e "s/DEV_INSTALL/GIT_INSTALL/g" Test plan: Read the patch. The scripts will be tested later on. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184992 [details] [review] Bug 39740: Fix a few occurrences of dev_install in code Test plan: Check if git_install is set correctly for your system in koha-conf.xml. Adjust otherwise. cp debian/scripts/koha-functions.sh /usr/share/koha/bin/ cp debian/scripts/koha-* /usr/sbin/ Run koha-shell your_clone. Does that works like expected? Run t/Koha/Config.t. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184993 [details] [review] Bug 39740: Replace envvar check by calling is_git_install on instance level Replace the $GIT_INSTALL check by is_git_install. Some scripts did call is_git_install once while potentially looping over multiple instances. Note: koha-reset-passwd only runs on one instance (unlike most). Test plan: Check/adjust git_install and test_system in koha-conf.xml for used instances. cp debian/scripts/koha-functions.sh /usr/share/koha/bin/ cp debian/scripts/koha-* /usr/sbin/ Note: We will still test koha-plack and -z3950-responder later. Test running a few of the other changed koha-* scripts on one or more instances. Verify that things work as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184994 [details] [review] Bug 39740: Adjust koha-plack and koha-z3950-responder Changes to koha-plack - Check is_test_system and -development for stack trace - Add check on set $environment for debugger - Move export to do_instance, remove three local's NOTE: GIT_INSTALL ie exported for koha-plack ONLY! The debian scripts call is_git_install. Changes to koha-z3950-responder - debugger needs test_system flag - Rearrange exports before calling start_z3950 and friends. Test plan: Check/adjust git_install and test_system in koha-conf.xml for used instances. cp debian/scripts/koha-functions.sh /usr/share/koha/bin/ cp debian/scripts/koha-* /usr/sbin/ cp debian/templates/plack.psgi /etc/koha/plack.psgi Add a die in e.g. opac/opac-main.pl. Set test_system to 1 in koha-conf.xml or export TEST_SYSTEM=1 in your environment. Restart Plack. Hit opac-main. Do you see a stack trace? Export TEST_SYSTEM=0. Restart Plack. Hit opac-main again. You should not see a stack trace. Start koha-z3950-responder with/without debug flag. Works as expected? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184995 [details] [review] Bug 39740: Adjust koha-conf.xml Test plan: Look at changes in this patch. Note that we already tested these settings with former patches. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184996 [details] [review] Bug 39740: (QA follow-up) Rename test_system/TEST_SYSTEM See comment30 and comment36. Result of two sed's: git grep -l test_system | xargs sed -i -e "s/test_system/debug_mode/g" git grep -l TEST_SYSTEM | xargs sed -i -e "s/TEST_SYSTEM/DEBUG_MODE/g" The next patch will take care of the debugger options in the plack and z3950 responder scripts. Test plan: Read the patch. (Testing in next patch.) git grep -i test_system Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184997 [details] [review] Bug 39740: (QA follow-up) Adjust debugger for koha-plack and koha-z3950-responder This patch mainly renames var debug_mode in both scripts when it refers to the debugger instead of the instance's debug_mode. Note: Removing $environment from responder, probably copied. Test plan: cp debian/scripts/koha-functions.sh /usr/share/koha/bin/ cp debian/scripts/koha-* /usr/sbin/ cp debian/templates/plack.psgi /etc/koha/plack.psgi Repeat steps from patch: "Replace envvar check by calling..." Repeat steps from patch: "Adjust koha-plack and koha-z3950-responder" Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 184998 [details] [review] Bug 39740: Fix adjust_paths_git_install in koha-es-indexer Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi (tcohen) from comment #45) > Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Thx ! |