Bug 24153 - Add a confirm flag to the cleanup_database.pl cronjob
Summary: Add a confirm flag to the cleanup_database.pl cronjob
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 24114 24152
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-03 15:49 UTC by Jonathan Druart
Modified: 2020-11-16 14:34 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Text to go in the release notes:
This enhancement adds a --confirm flag to the cleanup_database.pl cronjob, that allows a dry-run mode with a verbose output.
Version(s) released in:
20.11.00


Attachments
Bug 24153: Add a confirm flag to cleanup_database (17.40 KB, patch)
2019-12-03 18:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Add a confirm flag to cleanup_database (16.91 KB, patch)
2020-01-11 18:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Add a confirm flag to cleanup_database (16.98 KB, patch)
2020-03-20 16:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Remove warning for message queue (1.25 KB, patch)
2020-06-23 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Improve verbose messages in dry-run mode (8.31 KB, patch)
2020-06-24 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Make sure $count will not be reused for another purpose (3.11 KB, patch)
2020-06-24 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Add the confirm flag to the crontab calls (2.54 KB, patch)
2020-06-24 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Add a confirm flag to cleanup_database (16.98 KB, patch)
2020-06-26 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Remove warning for message queue (1.25 KB, patch)
2020-06-26 11:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Improve verbose messages in dry-run mode (8.31 KB, patch)
2020-06-26 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Make sure $count will not be reused for another purpose (3.11 KB, patch)
2020-06-26 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Add the confirm flag to the crontab calls (2.54 KB, patch)
2020-06-26 11:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Add a confirm flag to cleanup_database (17.01 KB, patch)
2020-06-26 11:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Remove warning for message queue (1.25 KB, patch)
2020-06-26 11:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Improve verbose messages in dry-run mode (8.41 KB, patch)
2020-06-26 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Make sure $count will not be reused for another purpose (3.11 KB, patch)
2020-06-26 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Add the confirm flag to the crontab calls (2.54 KB, patch)
2020-06-26 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24153: Add a confirm flag to cleanup_database (17.12 KB, patch)
2020-07-02 08:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24153: Remove warning for message queue (1.35 KB, patch)
2020-07-02 08:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24153: Improve verbose messages in dry-run mode (8.51 KB, patch)
2020-07-02 08:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24153: Make sure $count will not be reused for another purpose (3.20 KB, patch)
2020-07-02 08:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24153: Add the confirm flag to the crontab calls (2.64 KB, patch)
2020-07-02 08:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24153: (QA follow-up) Removing one redundant initialization line (871 bytes, patch)
2020-07-02 08:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-12-03 15:49:26 UTC
That will allow to make it run in dry-run mode with a useful verbose output
Comment 1 Jonathan Druart 2019-12-03 18:13:20 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2020-01-11 18:15:05 UTC
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/
Comment 3 Koha Team University Lyon 3 2020-03-20 09:04:08 UTC
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
Comment 4 Jonathan Druart 2020-03-20 16:03:49 UTC
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>
Comment 5 Julian Maurice 2020-06-12 11:04:28 UTC
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 ?
Comment 6 Jonathan Druart 2020-06-12 11:21:27 UTC
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.
Comment 7 Julian Maurice 2020-06-12 12:19:38 UTC
(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)
Comment 8 Julian Maurice 2020-06-12 13:45:54 UTC
Changing status to BLOCKED because of dependency on bug 24152, which is BLOCKED
Comment 9 Jonathan Druart 2020-06-12 14:54:32 UTC
(In reply to Julian Maurice from comment #8)
> Changing status to BLOCKED because of dependency on bug 24152, which is
> BLOCKED

Rebased
Comment 10 Marcel de Rooy 2020-06-19 08:40:58 UTC
+# 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.
Comment 11 Jonathan Druart 2020-06-23 10:18:22 UTC
(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.
Comment 12 Jonathan Druart 2020-06-23 10:19:26 UTC
Created attachment 106197 [details] [review]
Bug 24153: Remove warning for message queue
Comment 13 Marcel de Rooy 2020-06-24 12:38:31 UTC
(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.
Comment 14 Marcel de Rooy 2020-06-24 12:39:06 UTC
Another observation: We need to adjust the cron tab files and examples to add this flag.
Comment 15 Jonathan Druart 2020-06-24 14:17:44 UTC
Created attachment 106252 [details] [review]
Bug 24153: Improve verbose messages in dry-run mode
Comment 16 Jonathan Druart 2020-06-24 14:17:47 UTC
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.
Comment 17 Jonathan Druart 2020-06-24 14:17:51 UTC
Created attachment 106254 [details] [review]
Bug 24153: Add the confirm flag to the crontab calls
Comment 18 Jonathan Druart 2020-06-24 14:18:28 UTC
Marcel, I think the last patches answer your different concerns.
Comment 19 Marcel de Rooy 2020-06-26 07:29:53 UTC
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
Comment 20 Jonathan Druart 2020-06-26 11:00:55 UTC
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>
Comment 21 Jonathan Druart 2020-06-26 11:00:59 UTC
Created attachment 106328 [details] [review]
Bug 24153: Remove warning for message queue
Comment 22 Jonathan Druart 2020-06-26 11:01:03 UTC
Created attachment 106329 [details] [review]
Bug 24153: Improve verbose messages in dry-run mode
Comment 23 Jonathan Druart 2020-06-26 11:01:06 UTC
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.
Comment 24 Jonathan Druart 2020-06-26 11:01:10 UTC
Created attachment 106331 [details] [review]
Bug 24153: Add the confirm flag to the crontab calls
Comment 25 Jonathan Druart 2020-06-26 11:04:55 UTC
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>
Comment 26 Jonathan Druart 2020-06-26 11:04:58 UTC
Created attachment 106333 [details] [review]
Bug 24153: Remove warning for message queue
Comment 27 Jonathan Druart 2020-06-26 11:05:02 UTC
Created attachment 106334 [details] [review]
Bug 24153: Improve verbose messages in dry-run mode
Comment 28 Jonathan Druart 2020-06-26 11:05:06 UTC
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.
Comment 29 Jonathan Druart 2020-06-26 11:05:10 UTC
Created attachment 106336 [details] [review]
Bug 24153: Add the confirm flag to the crontab calls
Comment 30 Jonathan Druart 2020-06-26 11:06:46 UTC
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.
Comment 31 Marcel de Rooy 2020-07-02 07:17:27 UTC
(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..
Comment 32 Marcel de Rooy 2020-07-02 08:37:06 UTC
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>
Comment 33 Marcel de Rooy 2020-07-02 08:37:10 UTC
Created attachment 106466 [details] [review]
Bug 24153: Remove warning for message queue

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2020-07-02 08:37:15 UTC
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>
Comment 35 Marcel de Rooy 2020-07-02 08:37:19 UTC
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>
Comment 36 Marcel de Rooy 2020-07-02 08:37:23 UTC
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>
Comment 37 Marcel de Rooy 2020-07-02 08:37:28 UTC
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>
Comment 38 Marcel de Rooy 2020-07-02 08:54:44 UTC
Minor observation, no blocker:
RemoveOldSessions (when you run with sessdays)
Not executed in dry mode.
So no numbers.
Comment 39 Jonathan Druart 2020-07-20 15:48:24 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 40 Lucas Gass 2020-07-25 18:12:00 UTC
Not backporting enhancement to 20.05.x series.