That will allow to make it run in dry-run mode with a useful verbose output
Created attachment 95980 [details] [review] Bug 24153: Add a confirm flag to cleanup_database How handy this will be, right? This patch will change the behavior of this script as the --confirm flag will now be required for effective changes. Without --confirm and with --verbose you will see what could have done the script in non dry-run mode. Test plan: Use different options of the script without the --confirm flag and have a look at the output. It now tells you what would have been deleted if the flag was passed. Now use the --confirm flag and confirm that the changes are now effective. Sponsored-by: Association KohaLa - https://koha-fr.org/
Created attachment 97256 [details] [review] Bug 24153: Add a confirm flag to cleanup_database How handy this will be, right? This patch will change the behavior of this script as the --confirm flag will now be required for effective changes. Without --confirm and with --verbose you will see what could have done the script in non dry-run mode. Test plan: Use different options of the script without the --confirm flag and have a look at the output. It now tells you what would have been deleted if the flag was passed. Now use the --confirm flag and confirm that the changes are now effective. Sponsored-by: Association KohaLa - https://koha-fr.org/
I've tested for Association KohaLa. If you launch cleanup_database. pl without the --confirm flag, nothing is done, but with the confirm flag, it works. Sonia BOUIS
Created attachment 101300 [details] [review] Bug 24153: Add a confirm flag to cleanup_database How handy this will be, right? This patch will change the behavior of this script as the --confirm flag will now be required for effective changes. Without --confirm and with --verbose you will see what could have done the script in non dry-run mode. Test plan: Use different options of the script without the --confirm flag and have a look at the output. It now tells you what would have been deleted if the flag was passed. Now use the --confirm flag and confirm that the changes are now effective. Sponsored-by: Association KohaLa - https://koha-fr.org/ Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
From what I understand this a breaking change. For instance, for users that run this script in a cronjob, it will suddenly stop working. If the need is to have a dry-run mode, why not add a --dry-run option instead ?
Yes it will need to be advertised correctly in the release note. I think it makes sense to force the --confirm flag to effectively make the changes, it's what we do in most of the other scripts.
(In reply to Jonathan Druart from comment #6) > Yes it will need to be advertised correctly in the release note. Can it be written now in "Text to go in the release notes" ? :-D > I think it makes sense to force the --confirm flag to effectively make the > changes, it's what we do in most of the other scripts. I agree that it makes sense. This script removes data so we should prevent unwanted data loss. I just don't like breaking changes :-) I kind of like the `git clean` approach which requires a flag (-n, -f or -i), which forces users to read the documentation first. Maybe we can have similar behavior for scripts that remove data (just a thought for future scripts, not blocking)
Changing status to BLOCKED because of dependency on bug 24152, which is BLOCKED
(In reply to Julian Maurice from comment #8) > Changing status to BLOCKED because of dependency on bug 24152, which is > BLOCKED Rebased
+# FIXME The output for dry-run mode needs to be improved Yes. Agreed. + $tokens->delete if $confirm; + say sprintf "Removed %s expired OAuth2 tokens", $count if $verbose; This might be confusing. Among other examples. * perl misc/cronjobs/cleanup_database.pl -m 10 -v Mail queue purge triggered for 10 days. Use of uninitialized value $count in concatenation (.) or string at misc/cronjobs/cleanup_database.pl line 293. messages were deleted from the mail queue. So uninitialized warning that we do not want. But this example could serve to illustrate that even the wrong count could have been printed. Look at: if ( $confirm ) { $sth->execute($mail) or die $dbh->errstr; $count = $sth->rows; } print "$count messages were deleted from the mail queue.\nDone with message_queue purge.\n" if $verbose; => Since the count is not set, it could well be the count of a former statement ! I do not think that this is ready for production. Since we do not really support testing for many options, I do not see why this patch is useful.
(In reply to Marcel de Rooy from comment #10) > +# FIXME The output for dry-run mode needs to be improved > Yes. Agreed. The following line is: # But non trivial changes to C4::Members need to be done before. So cannot be done now. > + $tokens->delete if $confirm; > + say sprintf "Removed %s expired OAuth2 tokens", $count if $verbose; > This might be confusing. Among other examples. What do you mean, can you detail? Do you mean the message could be improved (use of conditional) in dry-run mode? > * perl misc/cronjobs/cleanup_database.pl -m 10 -v > Mail queue purge triggered for 10 days. > Use of uninitialized value $count in concatenation (.) or string at > misc/cronjobs/cleanup_database.pl line 293. > messages were deleted from the mail queue. > > So uninitialized warning that we do not want. > But this example could serve to illustrate that even the wrong count could > have been printed. Look at: > if ( $confirm ) { > $sth->execute($mail) or die $dbh->errstr; > $count = $sth->rows; > } > print "$count messages were deleted from the mail queue.\nDone with > message_queue purge.\n" if $verbose; > => Since the count is not set, it could well be the count of a former > statement ! I fixed the warning. > I do not think that this is ready for production. Since we do not really > support testing for many options, I do not see why this patch is useful. I wrote it for the pseudonymized transactions. The idea was to rework a bit the whole script and introduce a common pattern. That's why I implemented filter_by_last_update and use it all over the script. We now have a base and a pattern to follow. It's not useful yet for all the options, but at least it is for few of them.
Created attachment 106197 [details] [review] Bug 24153: Remove warning for message queue
(In reply to Jonathan Druart from comment #11) > (In reply to Marcel de Rooy from comment #10) > > + $tokens->delete if $confirm; > > + say sprintf "Removed %s expired OAuth2 tokens", $count if $verbose; > > This might be confusing. Among other examples. > > What do you mean, can you detail? Do you mean the message could be improved > (use of conditional) in dry-run mode? Yeah, this holds in general. It might be confusing to say deleted when you are in dry run mode. > > > * perl misc/cronjobs/cleanup_database.pl -m 10 -v > > Mail queue purge triggered for 10 days. > > Use of uninitialized value $count in concatenation (.) or string at > > misc/cronjobs/cleanup_database.pl line 293. > > messages were deleted from the mail queue. > > > > So uninitialized warning that we do not want. > > But this example could serve to illustrate that even the wrong count could > > have been printed. Look at: > > if ( $confirm ) { > > $sth->execute($mail) or die $dbh->errstr; > > $count = $sth->rows; > > } > > print "$count messages were deleted from the mail queue.\nDone with > > message_queue purge.\n" if $verbose; > > => Since the count is not set, it could well be the count of a former > > statement ! > > I fixed the warning. Cool. But are you confident that it is the only location where it could happen? Block X filled $count. And block Y presents it as a result while it actually comes from X.
Another observation: We need to adjust the cron tab files and examples to add this flag.
Created attachment 106252 [details] [review] Bug 24153: Improve verbose messages in dry-run mode
Created attachment 106253 [details] [review] Bug 24153: Make sure $count will not be reused for another purpose The $count variable should be defined per block and set to 0 to avoid confusion.
Created attachment 106254 [details] [review] Bug 24153: Add the confirm flag to the crontab calls
Marcel, I think the last patches answer your different concerns.
Applying: Bug 24153: Add a confirm flag to cleanup_database Applying: Bug 24153: Remove warning for message queue Applying: Bug 24153: Improve verbose messages in dry-run mode error: sha1 information is lacking or useless (misc/cronjobs/cleanup_database.pl). error: could not build fake ancestor Patch failed at 0001 Bug 24153: Improve verbose messages in dry-run mode
Created attachment 106327 [details] [review] Bug 24153: Add a confirm flag to cleanup_database How handy this will be, right? This patch will change the behavior of this script as the --confirm flag will now be required for effective changes. Without --confirm and with --verbose you will see what could have done the script in non dry-run mode. Test plan: Use different options of the script without the --confirm flag and have a look at the output. It now tells you what would have been deleted if the flag was passed. Now use the --confirm flag and confirm that the changes are now effective. Sponsored-by: Association KohaLa - https://koha-fr.org/ Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 106328 [details] [review] Bug 24153: Remove warning for message queue
Created attachment 106329 [details] [review] Bug 24153: Improve verbose messages in dry-run mode
Created attachment 106330 [details] [review] Bug 24153: Make sure $count will not be reused for another purpose The $count variable should be defined per block and set to 0 to avoid confusion.
Created attachment 106331 [details] [review] Bug 24153: Add the confirm flag to the crontab calls
Created attachment 106332 [details] [review] Bug 24153: Add a confirm flag to cleanup_database How handy this will be, right? This patch will change the behavior of this script as the --confirm flag will now be required for effective changes. Without --confirm and with --verbose you will see what could have done the script in non dry-run mode. Test plan: Use different options of the script without the --confirm flag and have a look at the output. It now tells you what would have been deleted if the flag was passed. Now use the --confirm flag and confirm that the changes are now effective. Sponsored-by: Association KohaLa - https://koha-fr.org/ Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 106333 [details] [review] Bug 24153: Remove warning for message queue
Created attachment 106334 [details] [review] Bug 24153: Improve verbose messages in dry-run mode
Created attachment 106335 [details] [review] Bug 24153: Make sure $count will not be reused for another purpose The $count variable should be defined per block and set to 0 to avoid confusion.
Created attachment 106336 [details] [review] Bug 24153: Add the confirm flag to the crontab calls
Looks like I followed-up on top of a local branch which did not have the last patches from bug 24152. Sorry about that, the patches apply cleanly now.
(In reply to Jonathan Druart from comment #30) > Looks like I followed-up on top of a local branch which did not have the > last patches from bug 24152. Sorry about that, the patches apply cleanly now. Yes, they apply. Having another look..
Created attachment 106465 [details] [review] Bug 24153: Add a confirm flag to cleanup_database How handy this will be, right? This patch will change the behavior of this script as the --confirm flag will now be required for effective changes. Without --confirm and with --verbose you will see what could have done the script in non dry-run mode. Test plan: Use different options of the script without the --confirm flag and have a look at the output. It now tells you what would have been deleted if the flag was passed. Now use the --confirm flag and confirm that the changes are now effective. Sponsored-by: Association KohaLa - https://koha-fr.org/ Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 106466 [details] [review] Bug 24153: Remove warning for message queue Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 106467 [details] [review] Bug 24153: Improve verbose messages in dry-run mode Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 106468 [details] [review] Bug 24153: Make sure $count will not be reused for another purpose The $count variable should be defined per block and set to 0 to avoid confusion. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 106469 [details] [review] Bug 24153: Add the confirm flag to the crontab calls Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 106470 [details] [review] Bug 24153: (QA follow-up) Removing one redundant initialization line Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Minor observation, no blocker: RemoveOldSessions (when you run with sessdays) Not executed in dry mode. So no numbers.
Pushed to master for 20.11, thanks to everybody involved!
Not backporting enhancement to 20.05.x series.