Bug 13068

Summary: New feature for DB update and sandbox
Product: Koha Reporter: Paul Poulain <paul.poulain>
Component: Installation and upgrade (web-based installer)Assignee: Paul Poulain <paul.poulain>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: brendan, dcook, frederic, gmcharlt, jonathan.druart, katrin.fischer, koha, kyle, m.de.rooy, mtompset, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11390
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13883
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 13893    
Attachments: Bug 13068, updatedatabase improvement for developer process
Bug 13068 sample
Bug 13068 follow-up
Bug 13068, updatedatabase improvement for developer process
Bug 13068, updatedatabase improvement for developer process
Bug 13068 sample
Bug 13068 follow-up
Bug 13068 QA follow-up
Bug 13068: Follow-up for atomicupdate
[COUNTERPATCH] Bug 13068: Counterpatch using atomicupdate directory
[COUNTERPATCH] Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Follow-up for cleaning up atomicupdate folder
Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Db rev for new syspref DevUpdateControl
Bug 13068: Db rev for new syspref DevUpdateControl
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Follow-up for cleaning up atomicupdate folder
Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Follow-up for cleaning up atomicupdate folder
Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Raise an error if a pl script has a sql error
Bug 13068: Raise an error if a pl script has a sql error
Bug 13068: Follow-up for cleaning up atomicupdate folder
Bug 13068: Counterpatch using atomicupdate directory
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Raise an error if a pl script has a sql error
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Raise an error if a pl script has a sql error
Bug 13068: Counterpatch follow-up for controlling workflow
Bug 13068: Raise an error if a pl script has a sql error
updatedatabase improvement
bz 13068 atomicupdate directory cleaning
bz 13068 follow-up : using slurp & removing oai-sets.sql
SAMPLE sql file, not for pushing
[SIGNED OFF] Bug 13068: updatedatabase improvement for developer process
[SIGNED OFF] Bug 13068: Follow-up for cleaning up atomicupdate folder
[SIGNED OFF] Bug 13068: (QA followup) Remove old files, use C4::Installer
Bug 13068: (follow-up) Remove old files, use C4::Installer
[PASSED QA] Bug 13068: updatedatabase improvement for developer process
[PASSED QA] Bug 13068: Follow-up for cleaning up atomicupdate folder
[PASSED QA] Bug 13068: (QA followup) Remove old files, use C4::Installer
[PASSED QA] Bug 13068: (follow-up) Remove old files, use C4::Installer
Bug 13068 [QA Followup] - Fix reading of sql files list
Bug 13068 [QA Followup] - Remove use File::Slurp
Bug 13068: [QA Follow-up] Remove the Talking-Tech-itiva file from atomicupdate
Bug 13068: [QA Follow-up] Remove the Talking-Tech-itiva file from atomicupdate
Bug 8753 - Add forgot password link to OPAC

Description Paul Poulain 2014-10-10 14:37:07 UTC
Most patches with a DB update cannot be applied automatically on sandboxes, because they have a conflict in updatedatabase.pl

I propose to have a new directory (say dev_update), that will be used during the development process.
 * when you submit a patch with a db update, you developer must put the DB update part into the dev_update/ directory, NOT in updatedatabase.pl
 * the updatedatabase.pl, at the end of the update process, checks if there is something in the dev_update. If there is, what's here is applied.
 * when the patch is pushed by the Release Manager, the DB update is moved out from the dev_update directory, and included into updatedatabase.pl
Comment 1 Paul Poulain 2014-10-10 15:31:41 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2014-10-10 15:31:45 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2014-10-10 15:37:25 UTC Comment hidden (obsolete)
Comment 4 Brendan Gallagher 2014-10-10 15:47:40 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2014-10-10 15:48:05 UTC
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.
Comment 6 Brendan Gallagher 2014-10-10 15:49:15 UTC Comment hidden (obsolete)
Comment 7 Brendan Gallagher 2014-10-10 15:53:03 UTC Comment hidden (obsolete)
Comment 8 Brendan Gallagher 2014-10-10 15:54:45 UTC Comment hidden (obsolete)
Comment 9 Paul Poulain 2014-10-10 18:34:48 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2014-10-14 08:50:24 UTC
Interesting! Working on this one now.. This is the moment to cleanup the atomicupdate folder.
Comment 11 Marcel de Rooy 2014-10-14 09:34:48 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2014-10-14 13:14:38 UTC
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.
Comment 13 Marcel de Rooy 2014-10-14 13:15:39 UTC Comment hidden (obsolete)
Comment 14 Paul Poulain 2014-10-14 15:11:49 UTC
(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
Comment 15 Paul Poulain 2014-10-14 16:28:31 UTC
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...
Comment 16 Marcel de Rooy 2014-10-15 08:24:22 UTC
(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?
Comment 17 Marcel de Rooy 2014-10-15 10:23:35 UTC
(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?
Comment 18 Marcel de Rooy 2014-10-16 07:46:00 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2014-10-16 08:22:16 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2014-10-16 08:26:02 UTC
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.
Comment 21 Marcel de Rooy 2014-10-20 07:42:26 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2014-10-20 07:42:30 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2014-10-24 08:01:42 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2014-10-24 08:01:47 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2014-10-27 07:59:25 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2014-10-27 07:59:30 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2014-10-28 14:48:55 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2014-10-28 14:49:01 UTC Comment hidden (obsolete)
Comment 29 Marcel de Rooy 2014-10-28 14:52:29 UTC
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?
Comment 30 Marcel de Rooy 2014-10-28 14:56:08 UTC Comment hidden (obsolete)
Comment 31 Marcel de Rooy 2014-10-28 14:56:12 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2014-10-28 14:56:16 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2014-10-29 14:41:42 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2014-10-29 14:48:38 UTC
Taking the liberty now to change status and obsolete some old patches. Paul plans to add a new signoff one of these days..
Comment 35 Marcel de Rooy 2014-10-29 14:51:58 UTC Comment hidden (obsolete)
Comment 36 Marcel de Rooy 2014-10-29 15:03:29 UTC
(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?
Comment 37 David Cook 2014-11-21 04:20:24 UTC
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...
Comment 38 Marcel de Rooy 2014-11-23 12:36:24 UTC
(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)..
Comment 39 David Cook 2014-11-23 22:57:19 UTC
(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.
Comment 40 Marc Véron 2014-11-24 07:41:58 UTC
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
Comment 41 Marcel de Rooy 2014-11-24 08:49:39 UTC
(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..
Comment 42 Katrin Fischer 2014-11-24 08:53:11 UTC
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?
Comment 43 Marcel de Rooy 2014-11-24 09:14:21 UTC
(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 :)
Comment 44 Katrin Fischer 2014-11-24 09:16:41 UTC
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?
Comment 45 Marcel de Rooy 2014-11-24 09:25:26 UTC
(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.
Comment 46 Paul Poulain 2014-11-24 10:36:28 UTC
(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...
)
Comment 47 Marcel de Rooy 2014-11-24 14:23:07 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2014-11-24 14:24:15 UTC
OK Two against one: I moved it to koha-conf.xml
Please test and sign-off..
Comment 49 David Cook 2014-11-24 22:55:21 UTC
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 :).
Comment 50 Paul Poulain 2014-12-02 17:40:30 UTC Comment hidden (obsolete)
Comment 51 Paul Poulain 2014-12-02 17:40:39 UTC Comment hidden (obsolete)
Comment 52 Paul Poulain 2014-12-02 17:40:48 UTC Comment hidden (obsolete)
Comment 53 Paul Poulain 2014-12-02 17:41:30 UTC
made many tests using TRACK mode, as well as PROD mode, and could not find any problem.

signed-off
Comment 54 Marcel de Rooy 2014-12-03 12:10:13 UTC
(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.
Comment 55 Jonathan Druart 2014-12-26 14:34:01 UTC
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?
Comment 56 Marcel de Rooy 2015-01-05 11:36:18 UTC
(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.
Comment 57 Marcel de Rooy 2015-01-05 11:36:43 UTC
I will the -> I will add the
Comment 58 Marcel de Rooy 2015-01-05 11:43:16 UTC
> 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.
Comment 59 Marcel de Rooy 2015-01-05 11:46:28 UTC Comment hidden (obsolete)
Comment 60 Marcel de Rooy 2015-01-05 11:46:35 UTC Comment hidden (obsolete)
Comment 61 Marcel de Rooy 2015-01-05 11:46:39 UTC Comment hidden (obsolete)
Comment 62 Marcel de Rooy 2015-01-05 11:47:59 UTC
> 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');
Comment 63 Marcel de Rooy 2015-01-05 11:58:36 UTC
> 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.
Comment 64 Marcel de Rooy 2015-01-05 12:04:37 UTC
> 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.
Comment 65 Marcel de Rooy 2015-01-05 12:19:14 UTC
> 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?
Comment 66 Jonathan Druart 2015-01-05 16:04:29 UTC Comment hidden (obsolete)
Comment 67 Jonathan Druart 2015-01-05 16:05:09 UTC
(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?
Comment 68 Marcel de Rooy 2015-01-07 12:43:54 UTC Comment hidden (obsolete)
Comment 69 Marcel de Rooy 2015-01-07 12:44:50 UTC
(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.
Comment 70 Jonathan Druart 2015-01-07 13:20:23 UTC Comment hidden (obsolete)
Comment 71 Jonathan Druart 2015-01-07 13:20:30 UTC Comment hidden (obsolete)
Comment 72 Jonathan Druart 2015-01-07 13:20:35 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2015-01-07 13:20:40 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2015-01-07 13:25:29 UTC
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.
Comment 75 Marcel de Rooy 2015-01-19 10:30:05 UTC Comment hidden (obsolete)
Comment 76 Marcel de Rooy 2015-01-19 10:30:11 UTC Comment hidden (obsolete)
Comment 77 Marcel de Rooy 2015-01-19 10:30:54 UTC
Trivial rebase on debian/templates/koha-conf-site.xml.in
Comment 78 Marcel de Rooy 2015-01-19 10:34:11 UTC Comment hidden (obsolete)
Comment 79 Marcel de Rooy 2015-01-19 10:35:21 UTC Comment hidden (obsolete)
Comment 80 Paul Poulain 2015-03-06 13:56:38 UTC
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)
Comment 81 Paul Poulain 2015-03-06 16:26:58 UTC Comment hidden (obsolete)
Comment 82 Paul Poulain 2015-03-06 16:27:29 UTC Comment hidden (obsolete)
Comment 83 Paul Poulain 2015-03-06 16:28:08 UTC Comment hidden (obsolete)
Comment 84 Paul Poulain 2015-03-06 16:28:29 UTC
Created attachment 36719 [details] [review]
SAMPLE sql file, not for pushing
Comment 85 Tomás Cohen Arazi 2015-03-10 19:49:02 UTC Comment hidden (obsolete)
Comment 86 Tomás Cohen Arazi 2015-03-10 19:49:20 UTC Comment hidden (obsolete)
Comment 87 Tomás Cohen Arazi 2015-03-10 19:49:33 UTC Comment hidden (obsolete)
Comment 88 Tomás Cohen Arazi 2015-03-10 19:50:47 UTC
I'm pretty confident, just asking for another QA just in case.
Comment 89 Jonathan Druart 2015-03-11 09:52:57 UTC
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.
Comment 90 Jonathan Druart 2015-03-11 09:53:43 UTC
(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.
Comment 91 Paul Poulain 2015-03-11 10:05:06 UTC
(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...
Comment 92 Jonathan Druart 2015-03-11 10:23:21 UTC
(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.[...]"
Comment 93 Jonathan Druart 2015-03-11 13:28:16 UTC Comment hidden (obsolete)
Comment 94 Kyle M Hall 2015-03-20 17:25:35 UTC
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>
Comment 95 Kyle M Hall 2015-03-20 17:25:43 UTC
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>
Comment 96 Kyle M Hall 2015-03-20 17:25:46 UTC
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>
Comment 97 Kyle M Hall 2015-03-20 17:25:49 UTC
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>
Comment 98 Kyle M Hall 2015-03-20 17:25:53 UTC
Created attachment 37065 [details] [review]
Bug 13068 [QA Followup] - Fix reading of sql files list
Comment 99 Kyle M Hall 2015-03-20 17:32:20 UTC
Created attachment 37066 [details] [review]
Bug 13068 [QA Followup] - Remove use File::Slurp
Comment 100 Kyle M Hall 2015-03-20 17:40:09 UTC
I wasn't able to get this feature to work without my QA followup.
Comment 101 Marcel de Rooy 2015-03-21 10:33:14 UTC Comment hidden (obsolete)
Comment 102 Marcel de Rooy 2015-03-21 10:57:54 UTC
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.
Comment 103 Marcel de Rooy 2015-03-21 11:08:00 UTC
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).
Comment 104 Kyle M Hall 2015-03-23 12:24:53 UTC
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).
Comment 105 Mark Tompsett 2015-03-30 22:32:56 UTC
Please rebase.
Comment 106 Tomás Cohen Arazi 2015-03-30 23:14:50 UTC
(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).
Comment 107 Tomás Cohen Arazi 2015-03-31 14:37:22 UTC
Improvement pushed to master.

Thanks Paul and Marcel.
Please help me update the wiki to reflect the new workflow.
Comment 108 Blou 2015-04-03 19:14:28 UTC Comment hidden (obsolete)
Comment 109 Blou 2015-04-03 19:17:06 UTC
Sorry about that, wrong bug.  I wish I could just completely remove my previous comment.
Comment 110 Jonathan Druart 2015-04-09 10:12:16 UTC
(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
Comment 111 Jonathan Druart 2015-04-09 10:16:45 UTC
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?