Summary: | koha-plack: Add parameter for specifically enabling Starman development environment | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | Command-line Utilities | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | Pushed to main --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | minor | ||
Priority: | P5 - low | CC: | david, dcook, jonathan.druart, martin.renvoize, robin, tomascohen, victor |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This adds a --development|-dev option to koha-plack. With this new option, an error trace is displayed instead of an "Error 500" page. This makes it easier to identify issues during development.
Note: If the -dev option is not added, restarting koha-plack defaults to using a deployment environment.
|
Version(s) released in: |
25.05.00
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 39740 | ||
Attachments: |
Bug 36932: Add deployment parameter to koha-plack
Bug 36932: Add deployment parameter to koha-plack Bug 36932: Add development parameter to koha-plack Bug 36932: Add development parameter to koha-plack Bug 36932: Add development parameter to koha-plack Bug 36932: Add development parameter to koha-plack |
Description
Marcel de Rooy
2024-05-23 06:49:56 UTC
Created attachment 167069 [details] [review] Bug 36932: Add deployment parameter to koha-plack Test plan (needs dev_install or ktd): Add a die in a script somewhere to simulate a crash. * For example in opac-main.pl: die "Line 43"; Run koha-plack --restart without -dep and hit that page. Dev output? Run koha-plack --restart with -dep and hit page again. No dev output? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Is that to make running KTD in production safer? Or to someone needing to briefly enable debug mode in production? (In reply to Victor Grousset/tuxayo from comment #2) > Is that to make running KTD in production safer? (In reply to Victor Grousset/tuxayo from comment #3) > Or to someone needing to briefly enable debug mode in production? The patch allows you to make a DEV_INSTALL (gitified package install, not only KTD) safer whether you want that in production or test environment. Ok, so like someone deploying koha with branches for flexibility of managing additional patches? Which would still be using the commands like koha-plack. Created attachment 167256 [details] [review] Bug 36932: Add deployment parameter to koha-plack Test plan (needs dev_install or ktd): Add a die in a script somewhere to simulate a crash. * For example in opac-main.pl: die "Line 43"; Run koha-plack --restart without -dep and hit that page. Dev output? Run koha-plack --restart with -dep and hit page again. No dev output? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> It works! :)
---
Clarification:
> like someone deploying koha with branches
* with a git repo to be able to use branches
---
Not sure about the -dep , it sounds like dependency instead of deployment. No big deal if nothing else sounds better.
---
new param is not documented in the --help
---
Since it's a safeguard to make sure people don't expose errors info in production: shouldn't it be the opposite approach? So like a -dev flag that ktd would have to add in it's startup command line call to koha-plack.
Wait, here with the current proposal it's very easy to switch back to an unsafe state: restart plack without putting the flag again.
How do the people in the usecases needing this flag manage the automated startup of koha on machine boot?
Because that place will need also to not have the -dep forgotten.
Do we have any setting that we already have to a value for dev envs and another one for regular prod instances and prod instance using a gittified setup? (IIUC that's the usecase for this flag).
How do we do it? ENV var that KTD overides and everywhere else it's a prod oriented value?
koha-conf.xml attribute that the KTD startup replaces with a dev oriented value so the everywhere else it's a prod oriented value?
(In reply to Victor Grousset/tuxayo from comment #5) > Ok, so like someone deploying koha with branches for flexibility of managing > additional patches? Which would still be using the commands like koha-plack. Yeah possibly. (In reply to Victor Grousset/tuxayo from comment #7) > Not sure about the -dep , it sounds like dependency instead of deployment. > No big deal if nothing else sounds better. > > > --- > > > new param is not documented in the --help > > --- > > > Since it's a safeguard to make sure people don't expose errors info in > production: shouldn't it be the opposite approach? So like a -dev flag that > ktd would have to add in it's startup command line call to koha-plack. > > Wait, here with the current proposal it's very easy to switch back to an > unsafe state: restart plack without putting the flag again. > How do the people in the usecases needing this flag manage the automated > startup of koha on machine boot? > Because that place will need also to not have the -dep forgotten. > > Do we have any setting that we already have to a value for dev envs and > another one for regular prod instances and prod instance using a gittified > setup? (IIUC that's the usecase for this flag). > How do we do it? ENV var that KTD overides and everywhere else it's a prod > oriented value? > koha-conf.xml attribute that the KTD startup replaces with a dev oriented > value so the everywhere else it's a prod oriented value? Coming back a bit later for these things. Since we are in 24.11 now, blocking this one temporarily. Created attachment 172101 [details] [review] Bug 36932: Add development parameter to koha-plack Test plan: NOTE: Needs dev_install or ktd. [1] Add a die in a script somewhere to simulate a crash. For example in opac-main.pl: die "Line 43"; [2] Run koha-plack --restart YOUR_CLONE (without -dev) and hit that page. See no dev output? [3] Run koha-plack --restart -dev YOUR_CLONE and hit page again. See dev output? [4] BONUS: Do a koha-plack --reload YOUR_CLONE (without -dev). Hit page. Verify that environment did not change. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This works, however the help needs to match the actual command (or vice versa): 1. Using -dev works: koha-plack --restart -dev kohadev 2. Using --development doesn't work: koha-plack --restart --development kohadev ==> Error: invalid option switch (--development) 3. Using --development_environment works. However, this is inconsistent with other options - should be a dash instead of an underscore. In addition, this doesn't match the help, which has --development. Testing notes using KTD: 1. Apply the patch. 2. Reset everything: reset_all 3. Check that the script is updated: koha-plack --help ==> You should see the new option listed in the help output (--development|-dev Enable development environment) 4. Run the rest of the test plan. Help output =========== koha-plack --help koha-plack This script lets you manage the plack daemons for your Koha instances. Usage: koha-plack --start|--stop|--restart|--reload|--status [--quiet|-q] instancename1 [instancename2...] koha-plack --enable|--disable instancename1 [instancename2] koha-plack -h|--help --start Start the plack daemon for the specified instances --stop Stop the plack daemon for the specified instances --restart Restart the plack daemon for the specified instances --reload Reload the plack daemon for the specified instances, letting the busy workers finish processing their requests before restarting them --enable Enable plack for the specified instances --disable Disable plack for the specified instances --status Show the status of Plack for the specified instances --debugger Enable running Plack in debug mode --debugger-key Specify the key the IDE is expecting --debugger-location Specify the host:port for your debugger tool (defaults to localhost:9000) --debugger-path Specify the path for the debugger library --development|-dev Enable development environment --quiet|-q Make the script quiet about non existent instance names (useful for calling from another scripts). --help|-h Display this help message Created attachment 176661 [details] [review] Bug 36932: Add development parameter to koha-plack Test plan: NOTE: Needs dev_install or ktd. [1] Add a die in a script somewhere to simulate a crash. For example in opac-main.pl: die "Line 43"; [2] Run koha-plack --restart YOUR_CLONE (without -dev) and hit that page. See no dev output? [3] Run koha-plack --restart -dev YOUR_CLONE and hit page again. See dev output? [4] BONUS: Do a koha-plack --reload YOUR_CLONE (without -dev). Hit page. Verify that environment did not change. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to David Nind from comment #11) > This works, however the help needs to match the actual command (or vice > versa): > > 1. Using -dev works: koha-plack --restart -dev kohadev > > 2. Using --development doesn't work: koha-plack --restart --development > kohadev > ==> Error: invalid option switch (--development) > > 3. Using --development_environment works. However, this is inconsistent with > other options - should be a dash instead of an underscore. In addition, this > doesn't match the help, which has --development. > I now moved to --development or -dev everywhere ;) (In reply to Marcel de Rooy from comment #13) > (In reply to David Nind from comment #11) > > This works, however the help needs to match the actual command (or vice > > versa): > > > > 1. Using -dev works: koha-plack --restart -dev kohadev > > > > 2. Using --development doesn't work: koha-plack --restart --development > > kohadev > > ==> Error: invalid option switch (--development) > > > > 3. Using --development_environment works. However, this is inconsistent with > > other options - should be a dash instead of an underscore. In addition, this > > doesn't match the help, which has --development. > > > > I now moved to --development or -dev everywhere ;) Thanks Marcel! Sign off on the way. David Created attachment 176700 [details] [review] Bug 36932: Add development parameter to koha-plack Test plan: NOTE: Needs dev_install or ktd. [1] Add a die in a script somewhere to simulate a crash. For example in opac-main.pl: die "Line 43"; [2] Run koha-plack --restart YOUR_CLONE (without -dev) and hit that page. See no dev output? [3] Run koha-plack --restart -dev YOUR_CLONE and hit page again. See dev output? [4] BONUS: Do a koha-plack --reload YOUR_CLONE (without -dev). Hit page. Verify that environment did not change. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Created attachment 179072 [details] [review] Bug 36932: Add development parameter to koha-plack Test plan: NOTE: Needs dev_install or ktd. [1] Add a die in a script somewhere to simulate a crash. For example in opac-main.pl: die "Line 43"; [2] Run koha-plack --restart YOUR_CLONE (without -dev) and hit that page. See no dev output? [3] Run koha-plack --restart -dev YOUR_CLONE and hit page again. See dev output? [4] BONUS: Do a koha-plack --reload YOUR_CLONE (without -dev). Hit page. Verify that environment did not change. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Just wondering: if [ "$debug_mode" = "yes" ]; then - environment="development" + # Maybe we should switch off debug_mode if DEV_INSTALL is not set? If I interpret this correctly we do have a slight behavior change in that the "debug_mode" will no longer activate the development environment, but only the command line switch will? Do we set debug_mode in any of our infrastructure or processes? Pushed for 25.05! Well done everyone, thank you! So we no longer have the stacktrace by default in our dev env... This is not enough. Prior to this patch we had the DEV_INSTALL env var that controlled this behaviour. DEV_INSTALL is set from adjust_paths_dev_install if dev_install is set in $KOHA_CONF. I don't really understand why you need this flag, but this is a regression for ktd users. (In reply to Katrin Fischer from comment #17) > Just wondering: > > if [ "$debug_mode" = "yes" ]; then > - environment="development" > + # Maybe we should switch off debug_mode if DEV_INSTALL is not set? > > > If I interpret this correctly we do have a slight behavior change in that > the "debug_mode" will no longer activate the development environment, but > only the command line switch will? > > Do we set debug_mode in any of our infrastructure or processes? This was no added line. But a moved line. (In reply to Jonathan Druart from comment #20) > Prior to this patch we had the DEV_INSTALL env var that controlled this > behaviour. > DEV_INSTALL is set from adjust_paths_dev_install if dev_install is set in > $KOHA_CONF. > > I don't really understand why you need this flag, but this is a regression > for ktd users. When you use a gitified install in production, you can use DEV_INSTALL but you dont want strack traces. Nobody likes regressions obviously. But shouldnt we just add the new -dev flag to ktd when starting koha=plack? A couple of options: 1. replace --dev with --deployment => no behaviour changes for existing installs and no other changes needed 2. split dev_install in the config and have gitified + dev_install I don't think turning on something called "dev install" in production is a good pattern. (In reply to Jonathan Druart from comment #23) > A couple of options: > 1. replace --dev with --deployment => no behaviour changes for existing > installs and no other changes needed > 2. split dev_install in the config and have gitified + dev_install > > I don't think turning on something called "dev install" in production is a > good pattern. We could rename it :) Defaulting to a dev setup isnt a good pattern too btw. (In reply to Marcel de Rooy from comment #25) > Defaulting to a dev setup isnt a good pattern too btw. I don't understand what you mean here. dev_install is set by ktd, which is used for dev purpose only. Without this flag it's considered a production install. (In reply to Jonathan Druart from comment #26) > (In reply to Marcel de Rooy from comment #25) > > Defaulting to a dev setup isnt a good pattern too btw. > > I don't understand what you mean here. > > dev_install is set by ktd, which is used for dev purpose only. > > Without this flag it's considered a production install. That viewpoint is not completely accurate. As I said, you can use a gitified install in production. So maybe we should not call that a dev install but a git install? But it obviously needs no dev setup. So defaulting a git install to dev is not a good pattern. No stacktrace in ktd is not what we want, so we need to move forward here in some way. What about Joubu's suggestions? (In reply to Katrin Fischer from comment #28) > No stacktrace in ktd is not what we want, so we need to move forward here in > some way. What about Joubu's suggestions? Not clear what he suggests. He is mainly complaining. But I suggested to add -dev to ktd and be done with it? I suspect that this will be needed in ktd in one or just a few places. So should not be a big deal. (In reply to Jonathan Druart from comment #23) > A couple of options: > 1. replace --dev with --deployment => no behaviour changes for existing > installs and no other changes needed > 2. split dev_install in the config and have gitified + dev_install Ah sorry I missed this (In reply to Marcel de Rooy from comment #30) > (In reply to Jonathan Druart from comment #23) > > A couple of options: > > 1. replace --dev with --deployment => no behaviour changes for existing > > installs and no other changes needed I think we should be safer and therefore I switched to -dev. Note that the initial patch did -dep but the default should be safe. > > 2. split dev_install in the config and have gitified + dev_install Could be done but just adding -dev to ktd seems less complex. See also comment7 from Victor. I was not complaining but now you have me out of this one. (In reply to Jonathan Druart from comment #33) > I was not complaining but now you have me out of this one. We continued the discussion on mm. Will consider the split option as a follow-up. Coming back soon here. (In reply to Jonathan Druart from comment #23) > A couple of options: > 1. replace --dev with --deployment => no behaviour changes for existing > installs and no other changes needed > 2. split dev_install in the config and have gitified + dev_install +1 (In reply to Marcel de Rooy from comment #30) > (In reply to Jonathan Druart from comment #23) > ... > > 2. split dev_install in the config and have gitified + dev_install > > Ah sorry I missed this Fair. Nice to see once we find issues, people fixes them. Thanks! Removing additional_work_needed. Please look at bug 39740. |