https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #2 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 102521 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102521&action=edit How update in the GUI looks when a die() is given -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Stop updatedatabase.pl when |Gracefully stop |a database change cannot be |updatedatabase.pl when a |applied |database change cannot be | |applied -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Here are some occurrences where we drop a message and continue the update process. 7285 print "Upgrade to $DBversion done (Bug 10636 - patronimage should have borrowernumber as PK, not cardnumber) failed! Transaction aborted because $@\n"; 7286 eval { $dbh->rollback }; 8057 print "Upgrade to $DBversion failed (Bug 7372: Move road types from the roadtype table to the ROADTYPE authorised values.\nTransaction aborted because $@\n)"; 8058 $dbh->rollback; 17027 warn "Upgrade to $DBversion failed: $@\n"; 11100 print "Upgrade to $DBversion done (Bug 10020: This database contains data related to 'ethnicity'. No change will be done on the DB structure but note that the Koha codebase does not use it)\n"; 12596 print "WARNING: You have corrupted data in your items table!! The table contains $cnt references to biblio records that do not exist.\nPlease correct your data IMMEDIATELY after this upgrade and ma nually add the foreign key constraint for biblionumber in the items table.\n"; -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

On the other hand I can imagine that one could want to get all the errors at once, and deal with them in one go. That's the behavior what I would like to get if I had to deal with upgrade process. In that case I imagine a switch would be needed. Do you mean executing all the db updates and gather errors from all of them? If an error occurs we should stop immediately in order to avoid the next steps

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #6 from Lari Taskula <lari.taskula@hypernova.fi> --- (In reply to Jonathan Druart from comment #3) potentially causing damage to the database. I like the idea of exception handling for db updates, but this seems like a big change to the installer considering it already sort of works the way it is as shown in the screenshot. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Gracefully stop |Gracefully stop |updatedatabase.pl when a |updatedatabase/installer |database change cannot be |when a database change |applied |cannot be applied -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=24131 --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- How it works so far: from the IU, the updatedatabase.pl is called, the outputs redirected to tmp files, reopened and displayed. That's ugly and does not let us much flexibility. I would like to rewrite it and have 2 scripts, the command line script, then the UI script. Both would call a method for each update step. So we could retrieve the outputs, exceptions or whatever we want easily. As I said I would prefer to have the whole thing ran in one go. And you don't:) That only means we need a switch that would be easy to implement if things are correctly written. I am happy to collect any ideas you have to improve the update process. I will try and work on that during the next dev cycle. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #9 from Tomás Cohen Arazi <tomascohen@gmail.com> --- It should definitely stop when an error is found. Each DBRev should be run inside a transaction as well. And it should report meaningful things :-D -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #11 from Agustín Moyano <agustinmoyano@theke.io> --- A question, can we have upgrade tests?.. I mean, a way to set some tables in a certain condition, execute the upgrade process and check the results all within a single transaction to rollback? Is it possible? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 116033 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116033&action=edit Bug 25078: [POC] Put the db entries in a structure to handle them better If we have have a hashref (ideally in a module!) we could have do something more powerful and deal correctly with them. More work is needed: - The "export" of the $db_entries hashref from updatedatabase.pl is really dirty, we don't want to do that - We should not need to eval { require $updatedatabase_path } from .pm, but retrieve it from another pm The main problems with this approach is: 1. Someone will have to move all(?) the entries into the new structure/file 2. Habit will change, updatedatabase.pl will become a 10 lines script that will loop over the db entries and display the output In my opinion it's what we want. Don't be afraid by the diff size, most of the changes is a move of the subroutine from updatedatabase.pl to C4/Installer.pm -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |25026 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25026 [Bug 25026] RaiseError must be set for the UI -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116033|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 116034 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116034&action=edit Bug 25078: [POC] Put the db entries in a structure to handle them better If we have have a hashref (ideally in a module!) we could have do something more powerful and deal correctly with them. More work is needed: - The "export" of the $db_entries hashref from updatedatabase.pl is really dirty, we don't want to do that - We should not need to eval { require $updatedatabase_path } from .pm, but retrieve it from another pm The main problems with this approach is: 1. Someone will have to move all(?) the entries into the new structure/file 2. Habit will change, updatedatabase.pl will become a 10 lines script that will loop over the db entries and display the output In my opinion it's what we want. Don't be afraid by the diff size, most of the changes is a move of the subroutine from updatedatabase.pl to C4/Installer.pm -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116032|0 |1 is obsolete| | --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 116208 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116208&action=edit Bug 25078: [POC] Lazy version This is the lazy, quick and dirty, version. We use a try catch block and execute the db entry in a transaction. When I error is found, we stop. The main problem is that we cannot do anything with the raw output in the template. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- URL| |https://gitlab.com/joubu/Ko | |ha/-/tree/bug_25078 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to a remove branch, I won't be able to maintain those patches up-to-date because of the changes to updatedatabase.pl and Koha.pm https://gitlab.com/joubu/Koha/-/tree/bug_25078 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 117084 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=117084&action=edit Bug 25078: [POC] Move each DBrev to a file -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #24 from Julian Maurice <julian.maurice@biblibre.com> --- I suggest to go even further by removing the tie between Koha version and database updates. It is not needed that each database update increments the Koha version (and it gives RM/RMaints additional work that could be avoided). I wrote something with that idea in mind: https://gitlab.com/jajm/Koha/-/commits/migrations - https://gitlab.com/jajm/Koha/-/commit/58bb8addbd4bb8deedd4643b8c465d8ac1857d... Basically it's like atomicupdate, but the filename is prefixed by a timestamp to keep the updates ordered and the filename is saved in a table once successfully executed (so that updates are not executed twice). No need to replace XXX by the correct version, and it should be backport-friendly (but only once this get pushed into stable releases... can't win everywhere :/) This makes the 'Version' system preference useless, so a lot of the patch is for removing usage of it. I realize that it's out of the scope of the current bug, but since other patches tend to go that way (splitting DB updates), I think that having another alternative is good for discussion. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Migrations is a really interesting idea.. I'm not sure why I'd not thought of it.. certainly worth considering. -- You are receiving this mail because: You are watching all bug changes.

I suggested that approach ~8 years ago, and it got abandoned (bug 7167). I am not against the idea but it's weird we finally get back to it... Maybe you were a misunderstood avant-gardiste :-) On a more serious note, I really liked the idea of bug 7167 at the time, but I

I personally don't think we should get rid of the "version" syspref. Mainly because of this: if ( Koha::Migrations->pending_migrations > 0 ) {

We are going to execute it for every hit (at the OPAC at least): 1 SQL query + 1 list of the directory. There are probably ways of making it less painful for performance, like caching

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #28 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #27) think it tried to do too much at once (with a UI, and ability to re-execute failed DB updates from the UI, ...), which made it complicated to test the directory listing in memcached. The Version syspref is in cache too, so it should not change anything from a "behaviour" point of view. Another way could be to move the check to the start of the application (in the .psgi), so that it's checked only once (if we update the sources, we have to restart starman anyway) This implementation is a proof of concept. I'm sure it can be improved. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #30 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Julian Maurice from comment #29)

About the performance of directory listing, I ran a small test:

% mktemp -d /tmp/tmp.t2Pdul9xNn

% grep 'DBversion =' installer/data/mysql/updatedatabase.pl | wc -l 1240 # That is the number of DB updates since 3.0

% for i in $(seq 1 1240); do touch /tmp/tmp.t2Pdul9xNn/$(openssl rand -hex 32); done

% perl -MTime::HiRes=tv_interval,gettimeofday -E 'my $t0 = [gettimeofday]; opendir(my $dh, $ARVG[0]); my @names; foreach my $fname (sort readdir($dh)) { push @names, $fname; } closedir($dh); my $elapsed = tv_interval($t0); say 1000*1000*$elapsed . "µs";' /tmp/tmp.t2Pdul9xNn/ 25µs

I ran it several times and the result is always between 10µs and 50µs, so... I think there are better things to worry about.

Oops. My bad... perl did not warn about the typo (ARVG vs ARGV). I should learn to use -w more So the results are more between 1ms and 4ms :) Maybe some caching would be useful after all -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #32 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- IMO we are going to far and the 2 approaches are not in opposition. We could go further on a separate bug report if we really want to. Here we want to improve the way the DBrevs are executed from 2 different scripts/methods (CLI and UI). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #34 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to David Cook from comment #33)

I don't even know what's going on anymore. What's the current proposal?

Hum? There are 3 POC patches attached to this bug report, see their messages. Then Julian's patches is a separated patch. I am suggesting now to: - move forward with my patches with Julian's idea in mind (adjustments are needed) - move Julian's patch to a separate but report (adjustments will be needed as well) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #36 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Julian Maurice from comment #35)

(In reply to Jonathan Druart from comment #34)

...

(In reply to David Cook from comment #33)

...

I don't even know what's going on anymore. What's the current proposal?

Hum? There are 3 POC patches attached to this bug report, see their messages.

Then Julian's patches is a separated patch.

I am suggesting now to: - move forward with my patches with Julian's idea in mind (adjustments are needed) - move Julian's patch to a separate but report (adjustments will be needed as well)

I will attach my patch to a separate bug report. Let's focus on stopping the updatedatabase process.

To be fair I should add that stopping the upgrade process when a SQL error occurs is already the current behaviour (since bug 25026). With these patches we will have each DBrev executed in a transaction and better error handling. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #38 from Julian Maurice <julian.maurice@biblibre.com> ---

I will attach my patch to a separate bug report. Let's focus on stopping the updatedatabase process.

For those interested, it is here: Bug 25078 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #40 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Didn't we decide it should depend on this one? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #42 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Julian Maurice from comment #41)

(In reply to Jonathan Druart from comment #40)

...

Didn't we decide it should depend on this one?

I don't remember this decision...

That's what I understood from our previous comments:

...

I am suggesting now to: - move forward with my patches with Julian's idea in mind (adjustments are needed) - move Julian's patch to a separate but report (adjustments will be needed as well)

I will attach my patch to a separate bug report. Let's focus on stopping the updatedatabase process.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #44 from Julian Maurice <julian.maurice@biblibre.com> --- So, the goal of this bug report is to go from attachment 102521 to https://snipboard.io/lpETMg.jpg ? I suggest to change the bug title then, because it was not clear to me. From my point of view the updatedatabase is already stopped gracefully (with an ugly error message, yes, but the error is handled and shown to the user). I agree that bug 27880 is out of scope here, but it can answer problem number 2 quite easily. Maybe we can find an intermediate step before going to bug 27880, but without having to rewrite everything twice. For instance, keep the file naming conventions of 27880, but with the version added. Something like: 20210305153137-201200013-create-table-foo.pl (I prefer .pl over .pm because there is no need to duplicate the name inside the file) The database version can still be extracted from the file name, like in your 3rd patch. And the upgrade path to 27880 should be straightforward Is that what you had in mind when you said "- move forward with my patches with Julian's idea in mind (adjustments are needed)" ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_21_05_candidate |rel_21_11_candidate -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #46 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- This is ready for testing again. I'd like it to be pushed at the beginning of this release cycle, to avoid unnecessary work with updatedatabase.pl Here we are going to: - Move each DBrev into a separate .pl file - Make the display/output nice for CLI and UI - Execute each DBrev in a transaction Improvement to this (on a separate bug report, see bug 27880) could be: - Use the database to store the dbrevs that have been executed already - Remove versioning and use YYYY-MM-DD-bug-42424.pl instead of the version => Easier for backporting => No longer need to be idempotent => Track of the execution (error/success) in the DB -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #48 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 121910 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121910&action=edit Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #116209|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=27880 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #52 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Jonathan Druart from comment #51)

Are you suggesting you should change this behaviour?

s/you should/we should :D -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #54 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #51)

It's not what is done (and has never been done). We display display the output when the DBrev has been executed. You're right, the output has always been displayed after the execution. What I'm saying is that it would be nice to be able to display a description of the updates before their execution. I know it's not the goal of this bug, but since it changes the structure of database updates, it's a good time to think about future improvements, to avoid changing the structure again later. I'll submit a patch later to show what I have in mind (maybe on a remote git repository to not pollute this bug)

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #56 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Julian Maurice from comment #54)

I'll submit a patch later to show what I have in mind (maybe on a remote git repository to not pollute this bug) https://gitlab.com/jajm/Koha/-/commit/864372e553d312d1a533a75f389d9a7aa143c6...

(In reply to Jonathan Druart from comment #55)

Then we would need 2 kinds of "description", as we want to keep the ability to output something dependent on what is in DB (like raise a warning if we find data inconsistency). I agree (but I think that data inconsistency, or any other anomaly detected should stop the update until it's fixed)

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #58 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Note to myself, the first patch should: - my $report = update( $files, { force => 1 } ); + my $report = update( $files, { force => $force } ); -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121909|0 |1 is obsolete| | --- Comment #60 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 121982 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121982&action=edit Bug 25078: Put db revs into different files to handle them better This patch suggests to stop using updatedatabase.pl to add new DB revs. Each DB rev will be in a separate pl files (installer/data/mysql/db_revs). The switch should ideally be done from 21.06.00.000. Each DBrev is executed in a try block and a transaction. If something went wrong, the whole DB rev is rolled back. Why do /var/log/koha/kohadev/updatedatabase_*.log (not -error) contain Status: 500 Content-type: text/html <h1>Software error:</h1> etc. Test plan: - git checkout 5f9333ffda0 (master on 2021-06-14) - Set the version syspref to 21.0500000: > update systempreferences set value="21.0500000" where variable="version"; - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) - Read the different DBrevs created as examples - Make sure the different use cases are covered - execute the updatedatabase script (CLI) - Set the version syspref to 21.0500000 - Update the DB from the UI - Set the version syspref to 21.0500000 - execute the updatedatabase script with the --force parameter (for testing purpose) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #62 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 121984 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121984&action=edit Bug 25078: Re-introduce NewVersion This is ugly, we re-add the code we removed in the previous patch. We need to continue supporting "old" versions. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #64 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Martin Renvoize from comment #63)

This has moved a bit.. looking good.

I wonder about the --force option we've introduced a little.. might be good to have a 'force_once' or something so allow the user to pick to ignore the first error but have subsequent errors still catch.

I added the --force for testing purpose, we can remove it.

Generally, I think I'm happy here.. I do think Julians $report way of doing multi-line description blocks is cleaner looking somehow.. and I can see his point about passing dbh rather than suggesting we rely on C4::Context.. it is somewhat future-proofing (and it's a familiar concept to me to pass dbh around this way as it's how DBIx::Class::DeploymentHandler does it (though in that case.. they often also pass a dbic schema for the before and after upgrade step so you can use dbic during upgrades.. takes some getting used to, but is pretty cool)

Julian, could you attach a patch here? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121983|0 |1 is obsolete| | --- Comment #68 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 122323 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122323&action=edit Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122208|0 |1 is obsolete| | --- Comment #70 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 122325 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122325&action=edit Bug 25078: Separate update "report" from its description Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #72 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Looking good to me, signing off. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #74 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Just added a follow-up atomicupdate (do not push it) to show how I tested that backwards compatibility stuff. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #76 from Victor Grousset/tuxayo <victor@tuxayo.net> --- I get the following conflict when applying the rest of patches after «- Make sure the different use cases are covered» <<<<<<< HEAD ---- this is the code after applying «DB revs for testing» $VERSION = "21.06.00.100"; ||||||| constructed merge base $VERSION = "21.06.00.004"; --- code in master before the patch that I'm applying ======= $VERSION = "21.06.00.005"; --- code from the patch that I'm applying

...

...

...

...

...

> Bug 25078: Put db revs into different files to handle them better

I should keep 21.06.00.100 right? But I'm confused that «Put db revs into different [...]» changes the version. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122323|0 |1 is obsolete| | --- Comment #78 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122666 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122666&action=edit Bug 25078: [DO NOT PUSH] DB revs for testing Use with the --force flag: % updatedatabase --force Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122325|0 |1 is obsolete| | --- Comment #80 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122668 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122668&action=edit Bug 25078: Separate update "report" from its description Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #82 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122670 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122670&action=edit Bug 25078: [DO NOT PUSH] Another Test for the backward shim -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #84 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Victor Grousset/tuxayo from comment #75)

description => [qw(Multi lines description)],

It's multiline and it doesn't matter that we actually use it, just having this syntax work is enough right?

description => ["Testing", "multi lines", " failure"], up => sub { my $dbh = C4::Context->dbh; $dbh->do(q{ALTER TABLE Foo}); },

Is that useful have another test for this multi line case but successful?

Should we have a failing test for this syntax? qw(Multi lines description)

...

- Make sure the different use cases are covered

Seems good but quite over my head.

I don't understand what you mean exactly. The description can return a scalar (one line description) or an arrayref (multi lines description). [qw(Multi lines description)] is similar to ["Multi", "lines", " description"], if it it what disturb you. (In reply to Victor Grousset/tuxayo from comment #76)

I get the following conflict when applying the rest of patches after «- Make sure the different use cases are covered»

<<<<<<< HEAD ---- this is the code after applying «DB revs for testing» $VERSION = "21.06.00.100"; ||||||| constructed merge base $VERSION = "21.06.00.004"; --- code in master before the patch that I'm applying ======= $VERSION = "21.06.00.005"; --- code from the patch that I'm applying

...

...

...

...

...

>> Bug 25078: Put db revs into different files to handle them better

I should keep 21.06.00.100 right? But I'm confused that «Put db revs into different [...]» changes the version.

I rebased the patch, adjusted the commit messages and added a new patch to move the 5 last db revs. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #86 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 122679 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122679&action=edit Bug 25078: Close open filehandle -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #88 from Julian Maurice <julian.maurice@biblibre.com> --- I tested this patch with the CLI and the Web installer. Both display the Unicode characters correctly. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #90 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Testing note: always restart services after changing the version syspref. ==== A bit confused about the test plan:

- Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) - Read the different DBrevs created as examples - Make sure the different use cases are covered - execute the updatedatabase script (CLI)

Actually all patches should be applied from the start, right? Moving on with this assumption. ======

- execute the updatedatabase script (CLI)

output:

Wide character in say at /kohadevbox/koha/installer/data/mysql/db_revs/210600000.pl line 11. Upgrade to 21.06.00.000 done [01:12:42]: Increase DBRev for 21.06 - 🎵 Run, rabbit run. 🎶 - Dig that hole, forget the sun, - And when at last the work is done - Don't sit down it's time to dig another one. Upgrade to 21.06.00.001 done [01:12:42]: Bug 28489 - Modify sessions.a_session from longtext to longblob Upgrade to 21.06.00.002 done [01:12:42]: Bug 28490 - Bring back accidentally deleted relationship columns Upgrade to 21.06.00.003 done [01:12:43]: Bug 24434 - Add 'WrongTransfer' to branchtransfers.cancellation_reason enum Upgrade to 21.06.00.004 done [01:12:43]: Bug 15788 - Split edit_borrowers permission Upgrade to 21.06.00.005 done [01:12:43]: Bug 26205 - Add new system preference NewsLog to log news changes Upgrade to 21.06.00.081 done [01:12:43]: Bug 42322 - Single line description, no report Upgrade to 21.06.00.082 done [01:12:43]: Bug 42422 - Testing number of patrons - Number of patrons: 53 Upgrade to 21.06.00.083 done [01:12:43]: Bug 42323 - Multi lines report - Multi - lines - report Upgrade to 21.06.00.090 failed [01:12:43]: Bug 42420 - Testing failure ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Table 'koha_kohadev.Foo' doesn't exist [for Statement "ALTER TABLE Foo"] at /kohadevbox/koha/C4/Installer.pm line 734

DEV atomic update: bug_25078.perl Upgrade to XXX [01:12:43]: Bug 25078 - Description - Boom, boom - Chick-a-boom

The Chick-a-boom atomic update is executed after the failure of the previous update. Is that an issue? «Wide character in say» Shouldn't it be fixed by «Fix encoding issue with db_revs output» =======

- Update the DB from the UI ↓↓↓ User koha_kohadev doesn't have enough privilege on database koha_kohadev.

What?! This is obviously unrelated but worth mentioning in case others using koha-testing-docker or not also have that. Anyway, moving on. Ok lol, we can actually move on. ¯\_ (ツ)_/¯ =====

DEV atomic update: bug_25078.perl Upgrade to XXX [23:11:53]: Bug 25078 - Description              - Boom, boom              - Chick-a-boom

Upgrade to 21.06.00.000 done [23:11:53]: Increase DBRev for 21.06 - 🎵 Run, rabbit run. 🎶 - Dig that hole, forget the sun, - And when at last the work is done - Don't sit down it's time to dig another one. Upgrade to 21.06.00.001 done [23:11:53]: Bug 28489 - Modify sessions.a_session from longtext to longblob Upgrade to 21.06.00.002 done [23:11:53]: Bug 28490 - Bring back accidentally deleted relationship columns Upgrade to 21.06.00.003 done [23:11:53]: Bug 24434 - Add 'WrongTransfer' to branchtransfers.cancellation_reason enum Upgrade to 21.06.00.004 done [23:11:53]: Bug 15788 - Split edit_borrowers permission Upgrade to 21.06.00.005 done [23:11:53]: Bug 26205 - Add new system preference NewsLog to log news changes Upgrade to 21.06.00.081 done [23:11:53]: Bug 42322 - Single line description, no report Upgrade to 21.06.00.082 done [23:11:53]: Bug 42422 - Testing number of patrons - Number of patrons: 53 Upgrade to 21.06.00.083 done [23:11:53]: Bug 42323 - Multi lines report - Multi - lines - report

Update error :

Upgrade to 21.06.00.090 failed [23:11:53]: Bug 42420 - Testing failure ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Table 'koha_kohadev.Foo' doesn't exist [for Statement "ALTER TABLE Foo"] at /kohadevbox/koha/C4/Installer.pm line 734

This time the Chick-a-boom update is first! Does it matter? And no «Wide character in say» warning. ====== Oh a "try again" button, let's press it. Same error, as expected. The Chick-a-boom atomic update is again ran. But we don't care, right? ====== --force test looks good :D Additional test: commenting the failing part of the transaction test caused the cities table to have a new line. As expected! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #92 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122875 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122875&action=edit Bug 25078: Fix 'Wide character' from CLI -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #93 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- As it, we are moving the atomic updates to the db_rev directory. Is it the behaviour we want? Should we 1. support both syntaxes (.perl and .pl) in installer/data/mysql/atomicupdate or 2. keep this directory for the old syntax and new atomic updates will be expected in installer/data/mysql/db_revs? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #123356|0 |1 is obsolete| | --- Comment #95 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123357 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123357&action=edit Bug 25078: Keep atomic updates in "atomicupdate" dir The atomicupdate directory will contain all the atomic update files (old and new version). That will ease job for the RM and RMaints, no file (except skeletons) must be in this directory before push. This patch also fixes an inconsistency we had: the atomic update was run before the other db revs on the UI but after when the CLI script was used. Now we make sure that the CLI does not deal with the atomic update files when called from the installer (UI) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122665|0 |1 is obsolete| | --- Comment #97 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123358 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123358&action=edit Bug 25078: Put db revs into different files to handle them better This patch suggests to stop using updatedatabase.pl to add new DB revs. Each DB rev will be in a separate pl files (installer/data/mysql/db_revs). The switch should ideally be done from 21.06.00.000. Each DBrev is executed in a try block and a transaction. If something went wrong, the whole DB rev is rolled back. Why do /var/log/koha/kohadev/updatedatabase_*.log (not -error) contain Status: 500 Content-type: text/html <h1>Software error:</h1> etc. Test plan: - git checkout c4b4db21d25 (master on 2021-07-08) - Set the version syspref to 21.0500000: > update systempreferences set value="21.0500000" where variable="version"; - Apply "Bug 25078: [DO NOT PUSH] DB revs for testing" (restart_all) - Read the different DBrevs created as examples - Make sure the different use cases are covered - execute the updatedatabase script (CLI) - Set the version syspref to 21.0500000 - Update the DB from the UI - Set the version syspref to 21.0500000 - execute the updatedatabase script with the --force parameter (for testing purpose) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122667|0 |1 is obsolete| | --- Comment #99 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123360 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123360&action=edit Bug 25078: Re-introduce NewVersion This is ugly, we re-add the code we removed in the previous patch. We need to continue supporting "old" versions. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122669|0 |1 is obsolete| | --- Comment #101 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123362 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123362&action=edit Bug 25078: (follow-up) Update 'NewVersion' for output_version changes This patch updates the NewVersion compatability method to add arrayref description handling to split it into description + report. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122679|0 |1 is obsolete| | --- Comment #103 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123364 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123364&action=edit Bug 25078: Close open filehandle -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122875|0 |1 is obsolete| | --- Comment #105 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123366 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123366&action=edit Bug 25078: Fix 'Wide character' from CLI -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #107 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Patches (and remote branch) rebased on top of master. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #109 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Awesome! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #110 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123850 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123850&action=edit Bug 25078: Move skeleton file -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #113 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123853 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123853&action=edit Bug 25078: Add exec flags -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #115 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 123937 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=123937&action=edit Bug 25078: Explicitely return from DB revs to avoid warnings % perl -wc installer/data/mysql/atomicupdate/skeleton.pl Useless use of anonymous hash ({}) in void context at installer/data/mysql/atomicupdate/skeleton.pl -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_21_11_candidate, | |RM_priority | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 --- Comment #118 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Jonathan Druart from comment #117)

Created attachment 124269 [details] [review] Bug 25078: Fix indentation and style for additional descriptions

Pushed to master. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27880 See Also|https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=27880 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27880 [Bug 27880] Store each database migrations state in database -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |38299 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38299 [Bug 38299] Errors with updates caught in C4::Installer should be colored/highlighted -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|36644 |36640 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36640 [Bug 36640] Upgrade DataTables from 1.13.6 to 2.x https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36644 [Bug 36644] Add option for 'Holds per item group' to circulation rules -- You are receiving this mail because: You are watching all bug changes.