|
Description
Paul Poulain
2014-10-10 14:37:07 UTC
Created attachment 32173 [details] [review] Bug 13068, updatedatabase improvement for developer process with this patch, the updatedatabase now checks info the db_update directory. If there is something here, it's executed. Note that there is no version update, so if you run the updatedatabase script twice, you'll get an sql error probably. This patch will be very useful for sandbox testing process. When the RM pushes a patch with something in db_update, he will have to copy it into updatedatabase.pl, give it a number, and remove the file in db_update Test plan: * apply the patch * create a files with a .sql extension in the db_update directory * run updatedatabase.pl * look at your database, what was in the .sql file must have been run Created attachment 32174 [details] [review] Bug 13068 sample This file is an example to test the patch. it must NOT BE PUSHED ! Created attachment 32175 [details] [review] Bug 13068 follow-up Removing 2 lines of dead code Created attachment 32177 [details] [review] Bug 13068, updatedatabase improvement for developer process with this patch, the updatedatabase now checks info the db_update directory. If there is something here, it's executed. Note that there is no version update, so if you run the updatedatabase script twice, you'll get an sql error probably. This patch will be very useful for sandbox testing process. When the RM pushes a patch with something in db_update, he will have to copy it into updatedatabase.pl, give it a number, and remove the file in db_update Test plan: * apply the patch * create a files with a .sql extension in the db_update directory * run updatedatabase.pl * look at your database, what was in the .sql file must have been run Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Side comment : the atomicupdate directory already exist, and could have been used. However, there is some code, that seems dead, but I don't want to introduce any regression. cleaning atomicupdate should be made in another patch if it's relevant. 2nd side comment : this patch does not address the case where the update must be made in a .pl file, because it's too complex. However, this case is very very rare, so this patch will fix 90% of the cases. The 10% remaining will not apply on sandboxes, but that's already a big improvement. Created attachment 32178 [details] [review] Bug 13068, updatedatabase improvement for developer process with this patch, the updatedatabase now checks info the db_update directory. If there is something here, it's executed. Note that there is no version update, so if you run the updatedatabase script twice, you'll get an sql error probably. This patch will be very useful for sandbox testing process. When the RM pushes a patch with something in db_update, he will have to copy it into updatedatabase.pl, give it a number, and remove the file in db_update Test plan: * apply the patch * create a files with a .sql extension in the db_update directory * run updatedatabase.pl * look at your database, what was in the .sql file must have been run Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Created attachment 32179 [details] [review] Bug 13068 sample This file is an example to test the patch. it must NOT BE PUSHED ! Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Created attachment 32180 [details] [review] Bug 13068 follow-up Removing 2 lines of dead code Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> Created attachment 32185 [details] [review] Bug 13068 QA follow-up using the proper file handle to pass koha-qa.pl ! Interesting! Working on this one now.. This is the moment to cleanup the atomicupdate folder. Created attachment 32294 [details] [review] Bug 13068: Follow-up for atomicupdate The folder atomicupdate can be cleaned up. It mainly contains old files that are not used any more. I found two files that are still referenced: [1] Bug-4246-Talking-Tech-itiva-phone-notifications.pl A readme file refers to it. The code in this file slightly differs from the TalkingTech db revision. It can probably be deleted too, but I will first check. [2] oai_sets.sql This file should remain. It is used in dbrev 3.07.00.029. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Paul, I favor your approach to this problem. But I see some problems: [1] You read sql files line by line. I would use the installer. [2] I would like to run perl files too. (Or print from a text file.) [3] Running it several times may/will cause problems or at least generate lots of warnings. [4] You use a new folder; but we already have atomicupdates. See my cleanup patch. [5] There is slightly more hand work for the RM that pushes a db rev. With these points in mind, I am attaching a counter patch based on your idea. Appreciate any feedback. Created attachment 32300 [details] [review] [COUNTERPATCH] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); I left two debuggings prints, helpful while testing this. When we are ready, they can be removed. (In reply to M. de Rooy from comment #12) > Paul, > I favor your approach to this problem. But I see some problems: > [1] You read sql files line by line. I would use the installer. +1000 ! I was uncomfortable with the line by line reading, using the installer is much better ! > [2] I would like to run perl files too. (Or print from a text file.) there are only a few updates that requires perl, so I thought it could be a later enhancement. But i've nothing against an immediate one ;-) > [3] Running it several times may/will cause problems or at least generate > lots of warnings. yep, but does not seemed a problem to me as it was for developer only. > [4] You use a new folder; but we already have atomicupdates. See my cleanup > patch. +1 for the cleanup patch. Can you tell me what you made to be sure those files can be removed safely ? I tried, but was not sure of avoiging a side effect. Thus my use of another directory. > [5] There is slightly more hand work for the RM that pushes a db rev. Yep, but so much improvement for sandboxes and daily developer life... (and Tomas told me it was fine) > With these points in mind, I am attaching a counter patch based on your idea. > Appreciate any feedback. My main comment with your patch is the workflow. ON A DEV INSTALL : * dev submit a patch with a db update. On 1st run, the update is applied. On the next ones, it is not, because it's in the syspref * the patch is signed-off / qa-ed, puched (and get a number) * on the next run, the patch will be applied and an error thrown (but not a problem according to me, because it's a dev install) The syspref is cleaned (which is OK. A potential side-effect can happen, in case the developer has applied 2 patches, one that has been pushed, one that hasn't. But that would just throw a sql error, which is fine, because it's a dev setup) ON A PROD SERVER : * sysop update Koha * on 1st run, the update is applied, the version is update, syspref untouched => no change in the behaviour. If I'm right, then I'm fine with this behaviour tried this patch, and it seems to work, but I don't understand what makes the atomicupdate/something.sql not being executed when the file is added with a number: I've added a DBRev for my test.sql, and the updater says " Skipping Bug-4246-Talking-Tech-itiva-phone-notifications.pl Skipping oai_sets.sql Skipping test.sql " which is exactly what I was expecting, but I don't understand what makes updatedatabse know that test.sql has been added "officially". If you can explain... (In reply to Paul Poulain from comment #15) > tried this patch, and it seems to work, but I don't understand what makes > the atomicupdate/something.sql not being executed when the file is added > with a number: > > I've added a DBRev for my test.sql, and the updater says " > Skipping Bug-4246-Talking-Tech-itiva-phone-notifications.pl > Skipping oai_sets.sql > Skipping test.sql > " > which is exactly what I was expecting, but I don't understand what makes > updatedatabse know that test.sql has been added "officially". If you can > explain... Will still respond to your comment before this one. There are two points here: [1] ExecDBRev checks the version; if CheckVersion is false, it returns just as we always did. But it ALWAYS adds the file to the atomic hash. So it knows later (in RunRemainingAtomicUpdates) that this file belongs to an official db rev. [2] If you add a dbrev, you also need to upgrade the version number in kohaversion.pl. CheckVersion checks the codebase version via the final_linear_version function. (Adding a dbrev without upgrading kohaversion does not work. This is existing functionality.) So, if you add the dbrev and upgrade kohaversion, updatedatabase will run your file the first time as an official db rev. It will skip the file in the RunRemaining sub. When running it a second time, the version check will skip the dbrev but RunRemaining still knows that it should also skip the file. Hope I was clear enough? (In reply to Paul Poulain from comment #14) > there are only a few updates that requires perl, so I thought it could be a > later enhancement. But i've nothing against an immediate one ;-) If we start doing more with DBIC, we should probably stop with db revisions in sql format, but use the perl format. > +1 for the cleanup patch. Can you tell me what you made to be sure those > files can be removed safely ? I tried, but was not sure of avoiging a side > effect. Thus my use of another directory. I have git-grepped atomicupdate first. This brought up the two exceptions immediately. I have git-grepped the 15 removed filenames: no results. I have glanced thru these files: Most of them have been converted to db revisions in updatedatabase (no exhaustive research however). Enough evidence to remove them. > My main comment with your patch is the workflow. > ON A DEV INSTALL : > * dev submit a patch with a db update. On 1st run, the update is applied. > On the next ones, it is not, because it's in the syspref > * the patch is signed-off / qa-ed, puched (and get a number) > * on the next run, the patch will be applied and an error thrown (but not a > problem according to me, because it's a dev install) The syspref is cleaned > (which is OK. A potential side-effect can happen, in case the developer has > applied 2 patches, one that has been pushed, one that hasn't. But that would > just throw a sql error, which is fine, because it's a dev setup) > > ON A PROD SERVER : > * sysop update Koha > * on 1st run, the update is applied, the version is update, syspref > untouched > => no change in the behaviour. > > If I'm right, then I'm fine with this behaviour That describes the workflow correctly imo. Note that if you are really cautious, you should not update the codebase and run a official db revision after having changed the db structure with a dev update. Strictly speaking, you should first revert the dev update somehow or restore your database. In practice, you (and I) will not always do that. But at that point we should know what we are doing! Thinking about that, we could make a small addition to the counterpatch and a real syspref that controls dev updates with four modes: [1] DevUpdateControl= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. (Default behavior?) [2] DevUpdateControl= RESET: Reinstall dev updates after each official db rev. This is what my counterpatch now does. [3] DevUpdateControl= ALWAYS: Run the dev updates at each upgrade (as your patch does). This actually ignores the local pref listing the applied ones. [4] DevUpdateControl=TRACK: Only install new dev updates. So do not reset the local pref. This should not be recommended imo; you should know what you are doing.. Note that changing or clearing the local pref that lists the applied dev updates, is another 'smart' way to mimic modes 3 and 4 now already. What do you think? Created attachment 32398 [details] [review] [COUNTERPATCH] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); Amended on Oct 16: Promoted the two debugging prints to actual prints. They will be helpful for any developer. Added a sort for safety. Added some POD comments. Created attachment 32399 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with pref DevUpdateControl: [1] DevUpdateControl= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] DevUpdateControl= RESET: Reinstall dev updates after each official db rev. [3] DevUpdateControl= ALWAYS: Run the dev updates at each upgrade. [4] DevUpdateControl=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Added the last patch to illustrate my comment17. If we want to continue on this road, I will add a real db rev to insert this pref and add it to admin.pref. And write some unit tests. First I will wait for your comments and testing. So to test the various devupdatecontrol modes, add it at the Local Use tab and toggle between its different modes. Created attachment 32525 [details] [review] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); Amended on Oct 16: Promoted the two debugging prints to actual prints. They will be helpful for any developer. Added a sort for safety. Added some POD comments. Created attachment 32526 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with pref DevUpdateControl: [1] DevUpdateControl= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] DevUpdateControl= RESET: Reinstall dev updates after each official db rev. [3] DevUpdateControl= ALWAYS: Run the dev updates at each upgrade. [4] DevUpdateControl=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Created attachment 32658 [details] [review] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); Amended on Oct 16: Promoted the two debugging prints to actual prints. They will be helpful for any developer. Added a sort for safety. Added some POD comments. Created attachment 32659 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with pref DevUpdateControl: [1] DevUpdateControl= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] DevUpdateControl= RESET: Reinstall dev updates after each official db rev. [3] DevUpdateControl= ALWAYS: Run the dev updates at each upgrade. [4] DevUpdateControl=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Created attachment 32784 [details] [review] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); Amended on Oct 16: Promoted the two debugging prints to actual prints. They will be helpful for any developer. Added a sort for safety. Added some POD comments. Created attachment 32785 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with pref DevUpdateControl: [1] DevUpdateControl= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] DevUpdateControl= RESET: Reinstall dev updates after each official db rev. [3] DevUpdateControl= ALWAYS: Run the dev updates at each upgrade. [4] DevUpdateControl=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Created attachment 32839 [details] [review] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); This line should follow the last dbrev and preceed the call to RunRemainingAtomicUpdates. Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Run updatestructure and check if all db revs and dev updates are run. Run updatestructure again. Nothing should be run now. Clear/delete the _LocalAtomicUpdates pref. Run updatestructure again. Only dev updates should be run. Reset the database version. You could edit Version in local preferences. Run updatestructure and check if all db revs and dev updates are run. Edit _LocalAtomicUpdates and remove one filename. Run updatestructure again. Only one dev update should be run. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 32840 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with pref DevUpdateControl: [1] DevUpdateControl= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] DevUpdateControl= RESET: Reinstall dev updates after an official db rev. [3] DevUpdateControl= ALWAYS: Run the dev updates at each upgrade. [4] DevUpdateControl=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Note: If the pref does not exist or is empty, we fall back to PROD (safest). If the pref differs from PROD, ALWAYS or TRACK, it falls back to RESET. Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Repeat for the various modes of DevUpdateControl (see above): Reset the database version. You could edit Version in local preferences. Optionally clear/delete the _LocalAtomicUpdates pref. Run updatestructure and check which db revs and dev updates are run or skipped. Run it again and check which dev updates are run or skipped. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased again. Made trivial change to make it less 'rebase prone'. Moved some comments for the RM from updatedatabase to the commit message. Removed one print message. Added test plans to the last two counterpatches. Paul: Would you agree to reset the status of this report and obsolete some patches? Created attachment 32841 [details] [review] Bug 13068: Follow-up for cleaning up atomicupdate folder The folder atomicupdate can be cleaned up. It mainly contains old files that are not used any more. I found two files that are still referenced: [1] Bug-4246-Talking-Tech-itiva-phone-notifications.pl A readme file refers to it. The code in this file slightly differs from the TalkingTech db revision. It can probably be deleted too, but I will first check. [2] oai_sets.sql This file should remain. It is used in dbrev 3.07.00.029. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 32842 [details] [review] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); This line should follow the last dbrev and preceed the call to RunRemainingAtomicUpdates. Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Run updatestructure and check if all db revs and dev updates are run. Run updatestructure again. Nothing should be run now. Clear/delete the _LocalAtomicUpdates pref. Run updatestructure again. Only dev updates should be run. Reset the database version. You could edit Version in local preferences. Run updatestructure and check if all db revs and dev updates are run. Edit _LocalAtomicUpdates and remove one filename. Run updatestructure again. Only one dev update should be run. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 32843 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with pref DevUpdateControl: [1] DevUpdateControl= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] DevUpdateControl= RESET: Reinstall dev updates after an official db rev. [3] DevUpdateControl= ALWAYS: Run the dev updates at each upgrade. [4] DevUpdateControl=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Note: If the pref does not exist or is empty, we fall back to PROD (safest). If the pref differs from PROD, ALWAYS or TRACK, it falls back to RESET. Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Repeat for the various modes of DevUpdateControl (see above): Reset the database version. You could edit Version in local preferences. Optionally clear/delete the _LocalAtomicUpdates pref. Run updatestructure and check which db revs and dev updates are run or skipped. Run it again and check which dev updates are run or skipped. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 32919 [details] [review] Bug 13068: Db rev for new syspref DevUpdateControl This patch updates sysprefs.sql and admin.pref for the new syspref DevUpdateControl. Also it adds a atomic update file to insert the pref. NOTE FOR SIGNOFF: If you did not yet create this pref manually when testing this feature, updatedatabase will not yet pickup this db rev (since it is in PROD mode by default). See test plan. NOTE TO RM: Please add the atomic update 13068 to updatedatabase.pl by pasting the next line and correcting the version(!): ExecDBRev( $aupd, '13068', 'New syspref DevUpdateControl', '13068_DevUpdateControl.sql', '3.17.00.???'); And indeed, update kohaversion.pl. Test plan: [1] Run sysprefs.sql (perhaps partially?). Delete the pref again. [2] Add the above line to updatedatabase and correct the version. Also update kohaversion.pl. Run updatestructure. Did you see the DevUpdateControl dbrev? Remove the line again from updatedatabase. Reset kohaversion.pl. [3] Run updatestructure again. Nothing to be done (PROD mode). [4] Change to ALWAYS mode. Warning for double insert is expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Taking the liberty now to change status and obsolete some old patches. Paul plans to add a new signoff one of these days.. Created attachment 32921 [details] [review] Bug 13068: Db rev for new syspref DevUpdateControl This patch updates sysprefs.sql and admin.pref for the new syspref DevUpdateControl. Also it adds a atomic update file to insert the pref. NOTE FOR SIGNOFF: If you did not yet create this pref manually when testing this feature, updatedatabase will not yet pickup this db rev (since it is in PROD mode by default). See test plan. NOTE TO RM: Please add the atomic update 13068 to updatedatabase.pl by pasting the next line and correcting the version(!): ExecDBRev( $aupd, '13068', 'New syspref DevUpdateControl', '13068_DevUpdateControl.sql', '3.17.00.???'); And indeed, update kohaversion.pl. Test plan: [1] Run sysprefs.sql (perhaps partially?). Delete the pref again. [2] Add the above line to updatedatabase and correct the version. Also update kohaversion.pl. Run updatestructure. Did you see the DevUpdateControl dbrev? Remove the line again from updatedatabase. Reset kohaversion.pl. [3] Run updatestructure again. Nothing to be done (PROD mode). [4] Change to ALWAYS mode. Update again. Double insert warn expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to M. de Rooy from comment #20) > If we want to continue on this road, I will add a real db rev to insert this > pref and add it to admin.pref. And write some unit tests. Well, I did insert the actual db rev. I would suggest to add some unit tests on another report (at that point in time we should check all subroutines in updatedatabase and move at least some of them to a module). Would you agree with that approach, Tomas? I'm hoping to do more work on http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11390, so that might be relevant to this as well. I'm juggling a few different projects at the moment, but I'd love to see us using deploy() and upgrade(). -- Side note: I'm not sure what I think about the syspref "DevUpdateControl". It doesn't really seem like a "system preference", so much as a configuration item... especially for developers. Why not put this in koha-conf.xml and access it via C4::Context? Much less chance of something bad happening if an overzealous superlibrarian changes it, I imagine... (In reply to David Cook from comment #37) > I'm juggling a few different projects at the moment, but I'd love to see us > using deploy() and upgrade(). Added a see also to 11390. We should indeed start using DBIC here. > Side note: I'm not sure what I think about the syspref "DevUpdateControl". > It doesn't really seem like a "system preference", so much as a > configuration item... especially for developers. Why not put this in > koha-conf.xml and access it via C4::Context? Much less chance of something > bad happening if an overzealous superlibrarian changes it, I imagine... It would of course be easy to move it into the conf.xml. But for me it seems to be just somewhat more a preference than an configuration item. It has a slightly more dynamic than static nature. But yes, this is an arbitrary choice. The overzealous superlibrarian-argument doing something bad, is imo not really relevant. [Side note on the side note: changing marc flavour or various other prefs would be much more harmful.] Changing this pref on a production machine should not make a difference, since there should not be dev updates. If there would be, the superlibrarian should also start install.pl with updatestructure and he should know the koha admin password. If these three conditions hold, we are talking about bad maintenance, security and personnel. Running the dev update(s) at the fourth step would probably not do that much: if they are there, they are likely to have been executed before. So just some warnings (in most cases).. (In reply to M. de Rooy from comment #38) > (In reply to David Cook from comment #37) > > I'm juggling a few different projects at the moment, but I'd love to see us > > using deploy() and upgrade(). > > Added a see also to 11390. We should indeed start using DBIC here. > > > Side note: I'm not sure what I think about the syspref "DevUpdateControl". > > It doesn't really seem like a "system preference", so much as a > > configuration item... especially for developers. Why not put this in > > koha-conf.xml and access it via C4::Context? Much less chance of something > > bad happening if an overzealous superlibrarian changes it, I imagine... > > It would of course be easy to move it into the conf.xml. But for me it seems > to be just somewhat more a preference than an configuration item. It has a > slightly more dynamic than static nature. But yes, this is an arbitrary > choice. > > The overzealous superlibrarian-argument doing something bad, is imo not > really relevant. [Side note on the side note: changing marc flavour or > various other prefs would be much more harmful.] Changing this pref on a > production machine should not make a difference, since there should not be > dev updates. If there would be, the superlibrarian should also start > install.pl with updatestructure and he should know the koha admin password. > If these three conditions hold, we are talking about bad maintenance, > security and personnel. Running the dev update(s) at the fourth step would > probably not do that much: if they are there, they are likely to have been > executed before. So just some warnings (in most cases).. You're right about changing the MARC flavour. That probably shouldn't be a system preference either :p. I don't know. As I look through these patches, I see other system preferences as well. So maybe the overzealous superlibrarian isn't relevant, but are any developers going to know what to do with these system preferences? It seems finicky. Of course, I might just be saying that because I haven't learned enough about it. Maybe it will be easier than the current situation which is also quite finicky. Regarding the System preferences discussed below: Maybe we should have System Preferences that are available for the Database User only. This would prevent overzealous superlibrarians to change things they shouldn't. See Bug 5511 as an other example for such preferences. Marc (In reply to David Cook from comment #39) > It seems finicky. Of course, I might just be saying that > because I haven't learned enough about it. Maybe it will be easier than the > current situation which is also quite finicky. Hmm. I looked up finicky, not really positive :) In the new situation the developer just puts a file in atomicupdate for his db rev instead of editing updatedatabase with version XXX, etc., rebasing it multiple times. So imo the finicky-degree goes down. Please test.. I think it sounds like something we should try and that would not break our existing update system. So the directory for the atomicupdates would be cleaned out as the database update gets moved to updatedatabase? (In reply to Katrin Fischer from comment #42) > I think it sounds like something we should try and that would not break our > existing update system. So the directory for the atomicupdates would be > cleaned out as the database update gets moved to updatedatabase? No, the db rev stays in this directory. The RM just adds one line to updatedatabase to make it an official dbrev. At some point in time we can decide to start a new atomicupdate folder (for e.g. Koha 4 :) Hm. so how does the system figure out which is the new dev one it needs to run? By comparing all entries in updatedatabase to the directory's contents? (In reply to Katrin Fischer from comment #44) > Hm. so how does the system figure out which is the new dev one it needs to > run? By comparing all entries in updatedatabase to the directory's contents? Paul raised the same question earlier. It is answered in comment16. The code builds a hash with all db revs. The time needed for that is limited, since it does not really inspect the file itself. It just sees the reference to the file. (In reply to M. de Rooy from comment #45) > (In reply to Katrin Fischer from comment #44) > > Hm. so how does the system figure out which is the new dev one it needs to > > run? By comparing all entries in updatedatabase to the directory's contents? > > Paul raised the same question earlier. It is answered in comment16. The code > builds a hash with all db revs. The time needed for that is limited, since > it does not really inspect the file itself. It just sees the reference to > the file. And what worried me also was the time dedicated to this test on each page. As the $version system does not change, this test, with a hash is run only when needed, which is A-OK (about the idea to put it in a configuration, I like the idea. About having some "forbidden to change, or with a specific warning syspref", i'm also A-OK, even if: * no-one I know ever made something as silly as changing the MARCflavour avec going live ;-) * the list of such syspref would probably result in forever lasting debates... ) Created attachment 33859 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with devupdatecontrol: [1] devupdatecontrol= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] devupdatecontrol= RESET: Reinstall dev updates after an official db rev. [3] devupdatecontrol= ALWAYS: Run the dev updates at each upgrade. [4] devupdatecontrol=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Note: devupdatecontrol is a configuration variable from koha-conf.xml. If it does not exist or is empty, we fall back to PROD (safest). If it differs from PROD, ALWAYS or TRACK, it falls back to RESET. Note2: Initially, devupdatecontrol was a preference. It has been moved to koha-conf.xml as requested by several developers (Nov 24). Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Repeat for the various modes of devupdatecontrol (see above): Reset the database version. You could edit Version in local preferences. Optionally clear/delete the _LocalAtomicUpdates pref. Run updatestructure and check which db revs and dev updates are run or skipped. Run it again and check which dev updates are run or skipped. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> OK Two against one: I moved it to koha-conf.xml Please test and sign-off.. Thanks for the explanations and further work, Marcel :). My plate is pretty full today, but I'm definitely interested in testing this. Will take a look as soon as I can :). Created attachment 34070 [details] [review] Bug 13068: Follow-up for cleaning up atomicupdate folder The folder atomicupdate can be cleaned up. It mainly contains old files that are not used any more. I found two files that are still referenced: [1] Bug-4246-Talking-Tech-itiva-phone-notifications.pl A readme file refers to it. The code in this file slightly differs from the TalkingTech db revision. It can probably be deleted too, but I will first check. [2] oai_sets.sql This file should remain. It is used in dbrev 3.07.00.029. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 34071 [details] [review] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); This line should follow the last dbrev and preceed the call to RunRemainingAtomicUpdates. Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Run updatestructure and check if all db revs and dev updates are run. Run updatestructure again. Nothing should be run now. Clear/delete the _LocalAtomicUpdates pref. Run updatestructure again. Only dev updates should be run. Reset the database version. You could edit Version in local preferences. Run updatestructure and check if all db revs and dev updates are run. Edit _LocalAtomicUpdates and remove one filename. Run updatestructure again. Only one dev update should be run. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 34072 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with devupdatecontrol: [1] devupdatecontrol= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] devupdatecontrol= RESET: Reinstall dev updates after an official db rev. [3] devupdatecontrol= ALWAYS: Run the dev updates at each upgrade. [4] devupdatecontrol=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Note: devupdatecontrol is a configuration variable from koha-conf.xml. If it does not exist or is empty, we fall back to PROD (safest). If it differs from PROD, ALWAYS or TRACK, it falls back to RESET. Note2: Initially, devupdatecontrol was a preference. It has been moved to koha-conf.xml as requested by several developers (Nov 24). Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Repeat for the various modes of devupdatecontrol (see above): Reset the database version. You could edit Version in local preferences. Optionally clear/delete the _LocalAtomicUpdates pref. Run updatestructure and check which db revs and dev updates are run or skipped. Run it again and check which dev updates are run or skipped. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> made many tests using TRACK mode, as well as PROD mode, and could not find any problem. signed-off (In reply to Paul Poulain from comment #53) > made many tests using TRACK mode, as well as PROD mode, and could not find > any problem. Thanks. Marcel, all looks good to me but I have few questions: 1/ Could you please detail when the RESET mode should be used? 2/ ExecDBRev takes 5 parameters, but you use only 3, the idea is to call this routine for each db rev in updatedatabase.pl? 3/ What means the $rv flag? 4/ Don't you think we should move the 2 files to skip into another directory (and use this one exclusively for this use)? 5/ 9691 $rv = 1 if !$! && !$@; #apparently, the file does not end with 1; I tested with a file without 1; and I don't get any error. 6/ From http://perldoc.perl.org/functions/do.html, it seems that 9690 print "ERROR: $!\n" if $!; should be 9690 print "ERROR: $!\n" if $! and not $@; 7/ In TRACK mode, given the following file: 1 use Modern::Perl; 2 use C4::Context; 3 my $dbh = C4::Context->dbh; 4 $dbh->do("delete from foo"); I executed perl installer/data/mysql/updatedatabase.pl and I got DBD::mysql::db do failed: Table 'koha.foo' doesn't exist at /home/koha/src/installer/data/mysql/atomicupdate/test3.pl line 4. Dev upgrade test3.pl done The filename is appended to _localatomicupdates, it seems it should not, could you confirm? (In reply to Jonathan Druart from comment #55) > Marcel, all looks good to me but I have few questions: Thanks for testing, Jonathan. I hope to address your questions now. > 6/ From http://perldoc.perl.org/functions/do.html, it seems that > print "ERROR: $!\n" if $!; > should be > print "ERROR: $!\n" if $! and not $@; OK. That is fine with me. I first check $@; in the second print I will the condition. I will the -> I will add the > 3/ What means the $rv flag? rv= return value (from do or something similar); I use it later in the routine to check if I should call the atomicmemory routine: _atomic_memory( $atomicupd, $file ) if $rv; > 5/ > $rv = 1 if !$! && !$@; #apparently, the file does not end with 1; > I tested with a file without 1; and I don't get any error. That's just what this line did for you. If your file did not return 1 and I discovered no error, then I set rv to 1 for you. Created attachment 34949 [details] [review] Bug 13068: Follow-up for cleaning up atomicupdate folder The folder atomicupdate can be cleaned up. It mainly contains old files that are not used any more. I found two files that are still referenced: [1] Bug-4246-Talking-Tech-itiva-phone-notifications.pl A readme file refers to it. The code in this file slightly differs from the TalkingTech db revision. It can probably be deleted too, but I will first check. [2] oai_sets.sql This file should remain. It is used in dbrev 3.07.00.029. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 34950 [details] [review] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); This line should follow the last dbrev and preceed the call to RunRemainingAtomicUpdates. Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Run updatestructure and check if all db revs and dev updates are run. Run updatestructure again. Nothing should be run now. Clear/delete the _LocalAtomicUpdates pref. Run updatestructure again. Only dev updates should be run. Reset the database version. You could edit Version in local preferences. Run updatestructure and check if all db revs and dev updates are run. Edit _LocalAtomicUpdates and remove one filename. Run updatestructure again. Only one dev update should be run. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Created attachment 34951 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with devupdatecontrol: [1] devupdatecontrol= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] devupdatecontrol= RESET: Reinstall dev updates after an official db rev. [3] devupdatecontrol= ALWAYS: Run the dev updates at each upgrade. [4] devupdatecontrol=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Note: devupdatecontrol is a configuration variable from koha-conf.xml. If it does not exist or is empty, we fall back to PROD (safest). If it differs from PROD, ALWAYS or TRACK, it falls back to RESET. Note2: Initially, devupdatecontrol was a preference. It has been moved to koha-conf.xml as requested by several developers (Nov 24). Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Repeat for the various modes of devupdatecontrol (see above): Reset the database version. You could edit Version in local preferences. Optionally clear/delete the _LocalAtomicUpdates pref. Run updatestructure and check which db revs and dev updates are run or skipped. Run it again and check which dev updates are run or skipped. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> > 2/ ExecDBRev takes 5 parameters, but you use only 3, the idea is to call this
> routine for each db rev in updatedatabase.pl?
Yes. I gave this example in one of the commit messages:
ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028');
> 1/ Could you please detail when the RESET mode should be used?
The difference between TRACK and RESET concentrates on what you do when you run an official db rev on your dev machine.
If you are very cautious, you got rid of all things that were done in dev updates BEFORE you run a real db rev. In that case RESET is your friend: it will reinstall all dev updates again. It may theoretically be better.
If you know that the dev updates you installed do not interfere with the db rev at hand, TRACK will not try to run them again. More pragmatic.
All depends on how you use them and how you interpret the *risks* on such a dev box. Instead of choosing one road to go, I offer them both.
> 4/ Don't you think we should move the 2 files to skip into another directory > (and use this one exclusively for this use)? I will open up a new report to move or delete them. After that we can clean up the hash variable. (See bug 13514.) This way they will not provide further noise in this report. > 7/ In TRACK mode, given the following file:
> 1 use Modern::Perl;
> 2 use C4::Context;
> 3 my $dbh = C4::Context->dbh;
> 4 $dbh->do("delete from foo");
> I executed perl installer/data/mysql/updatedatabase.pl
> and I got
> DBD::mysql::db do failed: Table 'koha.foo' doesn't exist
> at /home/koha/src/installer/data/mysql/atomicupdate/test3.pl line 4.
> Dev upgrade test3.pl done
> The filename is appended to _localatomicupdates, it seems it should not,
> could you confirm?
Yes, I can reproduce this (does not depend on the mode). Problem here is if the db rev does not care about the errors raised, the calling program (updatedatabase) does not see them any more.
Your last statement $dbh->do returns undef to the caller. But when you are back in updatedatabase, the $! and $@ are not set any more. (Note that if you had a trailing 1; in your file, I even would have received a 1.)
In this case I assume everything is fine and I append your file to the pref. I agree that we ideally should not. But how should we catch this in a simple and elegant way?
Created attachment 34960 [details] [review] Bug 13068: Raise an error if a pl script has a sql error (In reply to M. de Rooy from comment #65) > I agree that we ideally should not. But how should we catch this in a simple > and elegant way? What about the last patch? Created attachment 34998 [details] [review] Bug 13068: Raise an error if a pl script has a sql error Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with some perl and sql dev updates. Checked the list of dev updates for the TRACK mode. Slightly amended the patch for printing the error message. Also the message now may tell you that a dev update failed. (In reply to Jonathan Druart from comment #67) > (In reply to M. de Rooy from comment #65) > > I agree that we ideally should not. But how should we catch this in a simple > > and elegant way? > > What about the last patch? Great! Signed off. Created attachment 35000 [details] [review] Bug 13068: Follow-up for cleaning up atomicupdate folder The folder atomicupdate can be cleaned up. It mainly contains old files that are not used any more. I found two files that are still referenced: [1] Bug-4246-Talking-Tech-itiva-phone-notifications.pl A readme file refers to it. The code in this file slightly differs from the TalkingTech db revision. It can probably be deleted too, but I will first check. [2] oai_sets.sql This file should remain. It is used in dbrev 3.07.00.029. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 35001 [details] [review] Bug 13068: Counterpatch using atomicupdate directory This patch makes a distinction between atomicupdates belonging to an official db revision and development updates. It also keeps track of the dev updates since the last official db rev. (This is done via a local pref; so easily accessible if needed.) The db revs can be txt files, perl scripts or sql files. Sql files are run via the installer. A new db revision is now only a matter of dropping a file in atomicupdates for the developer. The RM only adds one line to updatedatabase like: ExecDBRev( $aupd, '13068', 'Refined dbrev', '13068.pl', '3.17.00.028'); This line should follow the last dbrev and preceed the call to RunRemainingAtomicUpdates. Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Run updatestructure and check if all db revs and dev updates are run. Run updatestructure again. Nothing should be run now. Clear/delete the _LocalAtomicUpdates pref. Run updatestructure again. Only dev updates should be run. Reset the database version. You could edit Version in local preferences. Run updatestructure and check if all db revs and dev updates are run. Edit _LocalAtomicUpdates and remove one filename. Run updatestructure again. Only one dev update should be run. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 35002 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with devupdatecontrol: [1] devupdatecontrol= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] devupdatecontrol= RESET: Reinstall dev updates after an official db rev. [3] devupdatecontrol= ALWAYS: Run the dev updates at each upgrade. [4] devupdatecontrol=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Note: devupdatecontrol is a configuration variable from koha-conf.xml. If it does not exist or is empty, we fall back to PROD (safest). If it differs from PROD, ALWAYS or TRACK, it falls back to RESET. Note2: Initially, devupdatecontrol was a preference. It has been moved to koha-conf.xml as requested by several developers (Nov 24). Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Repeat for the various modes of devupdatecontrol (see above): Reset the database version. You could edit Version in local preferences. Optionally clear/delete the _LocalAtomicUpdates pref. Run updatestructure and check which db revs and dev updates are run or skipped. Run it again and check which dev updates are run or skipped. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 35003 [details] [review] Bug 13068: Raise an error if a pl script has a sql error Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with some perl and sql dev updates. Checked the list of dev updates for the TRACK mode. Slightly amended the patch for printing the error message. Also the message now may tell you that a dev update failed. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> QA comments: All works as expected, we really need this to avoid recurrent conflicts on the updatedatabase.pl file. At some places, the code is not really obvious, but it's a good start. It could be great to have an entry on the wiki (http://wiki.koha-community.org/wiki/Database_updates ?) to list the different available options and explain the new workflow (don't append to updatedatabase.pl but create a new file). Marked as Passed QA. Created attachment 35366 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with devupdatecontrol: [1] devupdatecontrol= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] devupdatecontrol= RESET: Reinstall dev updates after an official db rev. [3] devupdatecontrol= ALWAYS: Run the dev updates at each upgrade. [4] devupdatecontrol=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Note: devupdatecontrol is a configuration variable from koha-conf.xml. If it does not exist or is empty, we fall back to PROD (safest). If it differs from PROD, ALWAYS or TRACK, it falls back to RESET. Note2: Initially, devupdatecontrol was a preference. It has been moved to koha-conf.xml as requested by several developers (Nov 24). Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Repeat for the various modes of devupdatecontrol (see above): Reset the database version. You could edit Version in local preferences. Optionally clear/delete the _LocalAtomicUpdates pref. Run updatestructure and check which db revs and dev updates are run or skipped. Run it again and check which dev updates are run or skipped. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 35367 [details] [review] Bug 13068: Raise an error if a pl script has a sql error Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with some perl and sql dev updates. Checked the list of dev updates for the TRACK mode. Slightly amended the patch for printing the error message. Also the message now may tell you that a dev update failed. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Trivial rebase on debian/templates/koha-conf-site.xml.in Created attachment 35368 [details] [review] Bug 13068: Counterpatch follow-up for controlling workflow The workflow for dev updates can be altered with devupdatecontrol: [1] devupdatecontrol= PROD for production machines: Do not execute RunRemainingDevUpdates, so only official db revs. This is the default. [2] devupdatecontrol= RESET: Reinstall dev updates after an official db rev. [3] devupdatecontrol= ALWAYS: Run the dev updates at each upgrade. [4] devupdatecontrol=TRACK: Only install new dev updates. It does not reset the local pref listing all recently applied dev updates. ONLY use this if you know what you are doing! Note: devupdatecontrol is a configuration variable from koha-conf.xml. If it does not exist or is empty, we fall back to PROD (safest). If it differs from PROD, ALWAYS or TRACK, it falls back to RESET. Note2: Initially, devupdatecontrol was a preference. It has been moved to koha-conf.xml as requested by several developers (Nov 24). Test plan: [1] Add a few lines calling ExecDBRev to updatedatabase with corresponding files in atomicupdate like (insert the correct version!!): ExecDBRev( $aupd, '13068', 'My test', '13068.pl', '3.17.00.028'); Do not forget to update kohaversion.pl accordingly. Add some other files in atomicupdate as dev updates. [2] Repeat for the various modes of devupdatecontrol (see above): Reset the database version. You could edit Version in local preferences. Optionally clear/delete the _LocalAtomicUpdates pref. Run updatestructure and check which db revs and dev updates are run or skipped. Run it again and check which dev updates are run or skipped. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Paul Poulain <paul.poulain@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 35370 [details] [review] Bug 13068: Raise an error if a pl script has a sql error Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested with some perl and sql dev updates. Checked the list of dev updates for the TRACK mode. Slightly amended the patch for printing the error message. Also the message now may tell you that a dev update failed. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Katrin, Tomas, Jonathan, Brendan and I had a discussion about this patch during the 2015 hackfest in Marseille. We think Marcel proposal does too much things, and add some complexity where initial my patch just fixed the main problem : applying patches on sandboxes fails. After a discussion, I'm about to resurrect my initial patches, keep the cleaning atomicupdate one, as they're interesting, and we will go forward. (I'll take care of Marcel comment 12 #1 into accout) Created attachment 36716 [details] [review] updatedatabase improvement Created attachment 36717 [details] [review] bz 13068 atomicupdate directory cleaning Created attachment 36718 [details] [review] bz 13068 follow-up : using slurp & removing oai-sets.sql Created attachment 36719 [details] [review] SAMPLE sql file, not for pushing Created attachment 36779 [details] [review] [SIGNED OFF] Bug 13068: updatedatabase improvement for developer process with this patch, the updatedatabase now checks info the atomicupdate directory. If there is some SQL here, it's executed. Note that there is no version update, so if you run the updatedatabase script twice, you'll get an sql error probably. This patch will be very useful for sandbox testing process. When the RM pushes a patch with something in i'atomicupdate', he will have to copy it into updatedatabase.pl, give it a number, and remove the file in db_update Test plan: * apply the patch * create a files with a .sql extension in the atomicupdate directory * run updatedatabase.pl * look at your database, what was in the .sql file must have been run Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Created attachment 36780 [details] [review] [SIGNED OFF] Bug 13068: Follow-up for cleaning up atomicupdate folder The folder atomicupdate can be cleaned up. It mainly contains old files that are not used any more. I found two files that are still referenced: [1] Bug-4246-Talking-Tech-itiva-phone-notifications.pl A readme file refers to it. The code in this file slightly differs from the TalkingTech db revision. It can probably be deleted too, but I will first check. [2] oai_sets.sql This file should remain. It is used in dbrev 3.07.00.029. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Created attachment 36781 [details] [review] [SIGNED OFF] Bug 13068: (QA followup) Remove old files, use C4::Installer * moving oai-sets.sql from atomicupdate to updatedatabase itself * using Installer file loader to load & execute SQL file Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> I'm pretty confident, just asking for another QA just in case. Comment on attachment 36781 [details] [review] [SIGNED OFF] Bug 13068: (QA followup) Remove old files, use C4::Installer Review of attachment 36781 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/updatedatabase.pl @@ +4945,5 @@ > + $dbh->do(qq{ > +DROP TABLE IF EXISTS `oai_sets_descriptions`; > +DROP TABLE IF EXISTS `oai_sets_mappings`; > +DROP TABLE IF EXISTS `oai_sets_biblios`; > +DROP TABLE IF EXISTS `oai_sets`; This does not work. It's not possible to execute more than 1 query using the do method. (In reply to Jonathan Druart from comment #89) > This does not work. > It's not possible to execute more than 1 query using the do method. I will try to submit a follow-up. (In reply to Jonathan Druart from comment #89) > Comment on attachment 36781 [details] [review] [review] > [SIGNED OFF] Bug 13068: (QA followup) Remove old files, use C4::Installer > > Review of attachment 36781 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: installer/data/mysql/updatedatabase.pl > @@ +4945,5 @@ > > + $dbh->do(qq{ > > +DROP TABLE IF EXISTS `oai_sets_descriptions`; > > +DROP TABLE IF EXISTS `oai_sets_mappings`; > > +DROP TABLE IF EXISTS `oai_sets_biblios`; > > +DROP TABLE IF EXISTS `oai_sets`; > > This does not work. > It's not possible to execute more than 1 query using the do method. Sure ? When we tested this patch, with Tomas, I think we started from a 3.0 DB, and I didn't saw any error. I must be wrong... (In reply to Paul Poulain from comment #91) > Sure ? When we tested this patch, with Tomas, I think we started from a 3.0 > DB, and I didn't saw any error. I must be wrong... Yep, sure: http://search.cpan.org/dist/DBI/DBI.pm#do "Prepare and execute a single statement.[...]" Created attachment 36792 [details] [review] Bug 13068: (follow-up) Remove old files, use C4::Installer The do method does not accept more than 1 sql query. Created attachment 37061 [details] [review] [PASSED QA] Bug 13068: updatedatabase improvement for developer process with this patch, the updatedatabase now checks info the atomicupdate directory. If there is some SQL here, it's executed. Note that there is no version update, so if you run the updatedatabase script twice, you'll get an sql error probably. This patch will be very useful for sandbox testing process. When the RM pushes a patch with something in i'atomicupdate', he will have to copy it into updatedatabase.pl, give it a number, and remove the file in db_update Test plan: * apply the patch * create a files with a .sql extension in the atomicupdate directory * run updatedatabase.pl * look at your database, what was in the .sql file must have been run Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 37062 [details] [review] [PASSED QA] Bug 13068: Follow-up for cleaning up atomicupdate folder The folder atomicupdate can be cleaned up. It mainly contains old files that are not used any more. I found two files that are still referenced: [1] Bug-4246-Talking-Tech-itiva-phone-notifications.pl A readme file refers to it. The code in this file slightly differs from the TalkingTech db revision. It can probably be deleted too, but I will first check. [2] oai_sets.sql This file should remain. It is used in dbrev 3.07.00.029. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 37063 [details] [review] [PASSED QA] Bug 13068: (QA followup) Remove old files, use C4::Installer * moving oai-sets.sql from atomicupdate to updatedatabase itself * using Installer file loader to load & execute SQL file Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 37064 [details] [review] [PASSED QA] Bug 13068: (follow-up) Remove old files, use C4::Installer The do method does not accept more than 1 sql query. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 37065 [details] [review] Bug 13068 [QA Followup] - Fix reading of sql files list Created attachment 37066 [details] [review] Bug 13068 [QA Followup] - Remove use File::Slurp I wasn't able to get this feature to work without my QA followup. Created attachment 37075 [details] [review] Bug 13068: [QA Follow-up] Remove the Talking-Tech-itiva file from atomicupdate The file can be removed. The code is included in the db revisions 3.09.00.014 and 3.15.00.040. Also I adjusted the text of TalkingTech.README and removed the reference to the atomicupdate file. NOTE: I open a new report for adding TalkingTechItivaPhoneNotification to sysprefs.sql. Probably, the text inserted in the db revisions should have been inserted into the sample notices sql text too. These corrections are outside the scope of this report. Created attachment 37076 [details] [review] Bug 13068: [QA Follow-up] Remove the Talking-Tech-itiva file from atomicupdate The file can be removed. The code is included in the db revisions 3.09.00.014 and 3.15.00.040. Also I adjusted the text of TalkingTech.README and removed the reference to the atomicupdate file. NOTE: I open a new report for adding TalkingTechItivaPhoneNotification to sysprefs.sql. Probably, the text inserted in the db revisions should have been inserted into the sample notices sql text too. These corrections are outside the scope of this report. There is one thing that I will still want to mention again, with reference to comments 12 and 17: I would like to run perl files too. If we start doing more with DBIC, we should probably stop with db revisions in sql format, but use the perl format. The current implementation switches back to .sql only. Aside from that, this seems to work fine (with some additional work for the RM). Agreed, we will need the ability run perl files in the same manner, but that can be a further enhancement independent of this. (In reply to M. de Rooy from comment #103) > There is one thing that I will still want to mention again, with reference > to comments 12 and 17: > > I would like to run perl files too. > If we start doing more with DBIC, we should probably stop with db revisions > in sql format, but use the perl format. > > The current implementation switches back to .sql only. > Aside from that, this seems to work fine (with some additional work for the > RM). Please rebase. (In reply to M. Tompsett from comment #105) > Please rebase. No problem, Mark. It is already applied on my working branch. It will be pushed tomorrow morning (argentinian tz). Improvement pushed to master. Thanks Paul and Marcel. Please help me update the wiki to reflect the new workflow. Created attachment 37491 [details] [review] Bug 8753 - Add forgot password link to OPAC This redo includes the following - usage of the template letters for the email instead of a .tt (that feature didn't exist when the functionnality was initially coded for our client) - removal of the prog version - removal of the code that was initially put into Members.pm - Added a letter into updatedatabase.pl and sample_notices.sql - Of course, rebase to latest master. The rest remain unchanged since the previous comments/approvals. As such, what worked before should still work. TEST PLAN: 1) apply the patch 2) go to system preferences OPAC>>Privacy and set 'OpacResetPassword' to ON. That will cause the link 'Forgot yo 2b) make sure that OpacPasswordChange is also ON. 3) refresh front page, click on 'Forgot your password' and enter a VALID address (one that is associated to an en 3b) Also try an INVALID address (valid yet not in your koha db). An error message will show up. 4) An email should be received at that address with a link. 5) Follow the link in the mail to fill the new password. Until a satisfactory new password is entered, the old password is not reset. 6) Go to main page try the new password. Sorry about that, wrong bug. I wish I could just completely remove my previous comment. (In reply to Tomás Cohen Arazi from comment #107) > Improvement pushed to master. > > Thanks Paul and Marcel. > Please help me update the wiki to reflect the new workflow. See http://wiki.koha-community.org/wiki/Database_updates#updatedatabase.pl Tomas, it would be good to have a test somewhere to be sure a release (stable or master) does not have anything in the atomicupdate directory. Do you launch some tests before pushing into master? |