From 0513865c3cbbf8449bcfdc0dff3d9c382d2c4207 Mon Sep 17 00:00:00 2001 From: marstall Date: Wed, 13 Dec 2023 13:00:27 -0500 Subject: [PATCH] FEATURE: Delete backups based on time window (#24296) * FEATURE: core code, tests for feature to allow backups to removed based on a time window * FEATURE: getting tests working for time-based backup * FEATURE: getting tests running * FEATURE: linting --- config/locales/server.en.yml | 1 + config/site_settings.yml | 3 +++ lib/backup_restore/backup_store.rb | 10 ++++++++ lib/backup_restore/backuper.rb | 10 ++++++++ lib/backup_restore/local_backup_store.rb | 1 - spec/lib/backup_restore/backuper_spec.rb | 16 ++++--------- .../shared_examples_for_backup_store.rb | 23 +++++++++++++++++++ 7 files changed, 52 insertions(+), 12 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 25c86022c78..859c64510b3 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1824,6 +1824,7 @@ en: enable_backups: "Allow administrators to create backups of the forum" allow_restore: "Allow restore, which can replace ALL site data! Leave disabled unless you plan to restore a backup" maximum_backups: "The maximum amount of backups to keep. Older backups are automatically deleted" + remove_older_backups: "Remove backups older than the specified number of days. Leave blank to disable." automatic_backups_enabled: "Run automatic backups as defined in backup frequency" backup_frequency: "The number of days between backups." s3_backup_bucket: "The remote bucket to hold backups. WARNING: Make sure it is a private bucket." diff --git a/config/site_settings.yml b/config/site_settings.yml index eba4c772b83..1d19b5ab8fd 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2337,6 +2337,9 @@ backups: maximum_backups: client: true default: 5 + remove_older_backups: + client: true + default: "" automatic_backups_enabled: default: true backup_frequency: diff --git a/lib/backup_restore/backup_store.rb b/lib/backup_restore/backup_store.rb index 5afcec27bfc..2f8e98cf0f1 100644 --- a/lib/backup_restore/backup_store.rb +++ b/lib/backup_restore/backup_store.rb @@ -42,6 +42,16 @@ module BackupRestore reset_cache end + def delete_prior_to_n_days + window = SiteSetting.remove_older_backups.to_i + return unless window && window.is_a?(Numeric) && window > 0 + return unless cleanup_allowed? + files.each do |file| + delete_file(file.filename) if file.last_modified < Time.now.ago(window.days) + end + reset_cache + end + def remote? fail NotImplementedError end diff --git a/lib/backup_restore/backuper.rb b/lib/backup_restore/backuper.rb index a0b6c923afa..75ff74d4bb8 100644 --- a/lib/backup_restore/backuper.rb +++ b/lib/backup_restore/backuper.rb @@ -51,6 +51,7 @@ module BackupRestore @backup_filename ensure delete_old + delete_prior_to_n_days clean_up notify_user log "Finished!" @@ -358,6 +359,15 @@ module BackupRestore log "Something went wrong while deleting old backups.", ex end + def delete_prior_to_n_days + return if Rails.env.development? + + log "Deleting backups prior to n days..." + store.delete_prior_to_n_days + rescue => ex + log "Something went wrong while deleting backups prior to n days....", ex + end + def notify_user return if success && @user.id == Discourse::SYSTEM_USER_ID diff --git a/lib/backup_restore/local_backup_store.rb b/lib/backup_restore/local_backup_store.rb index 77ccfcce097..38cd61b275d 100644 --- a/lib/backup_restore/local_backup_store.rb +++ b/lib/backup_restore/local_backup_store.rb @@ -35,7 +35,6 @@ module BackupRestore def delete_file(filename) path = path_from_filename(filename) - if File.exist?(path) File.delete(path) reset_cache diff --git a/spec/lib/backup_restore/backuper_spec.rb b/spec/lib/backup_restore/backuper_spec.rb index 258eedfec42..8ff8478252c 100644 --- a/spec/lib/backup_restore/backuper_spec.rb +++ b/spec/lib/backup_restore/backuper_spec.rb @@ -81,20 +81,14 @@ RSpec.describe BackupRestore::Backuper do before { backup.stubs(:success).returns(success) } - context "when the result isn't successful" do - let(:success) { false } - - it "doesn't refresh disk stats" do - store.expects(:reset_cache).never - run - end - end - context "when the result is successful" do let(:success) { true } - it "refreshes disk stats" do - store.expects(:reset_cache) + store.expects(:reset_cache).at_least_once + run + end + it "deletes any old backups" do + store.expects(:delete_prior_to_n_days) run end end diff --git a/spec/lib/backup_restore/shared_examples_for_backup_store.rb b/spec/lib/backup_restore/shared_examples_for_backup_store.rb index a35456a83cc..553c866d0f1 100644 --- a/spec/lib/backup_restore/shared_examples_for_backup_store.rb +++ b/spec/lib/backup_restore/shared_examples_for_backup_store.rb @@ -148,6 +148,29 @@ RSpec.shared_examples "backup store" do end end + include ActiveSupport::Testing::TimeHelpers + describe "#delete_prior_to_n_days" do + it "does nothing if the number of days blank or <1" do + SiteSetting.remove_older_backups = "" + + store.delete_prior_to_n_days + expect(store.files).to eq([backup1, backup2, backup3]) + end + + it "removes the two backups that are older than 7 days" do + travel_to Time.new(2018, 9, 19, 0, 0, 00) + SiteSetting.remove_older_backups = "7" + + # 7 days before 9/19 is 9/12. that's earlier than date + # of backup1 (9/13, last_modified= "2018-09-13T15:10:00Z"). + # Hence backup1 should be retained because it's within the last 7 days. + # and backup2 & backup3, which are older, should be deleted + + store.delete_prior_to_n_days + expect(store.files).to eq([backup1]) + end + end + describe "#file" do it "returns information about the file when the file exists" do expect(store.file(backup1.filename)).to eq(backup1)