From f9b7038c75ec7780ccc74df50e87b28e4646c4c0 Mon Sep 17 00:00:00 2001 From: Josh Raker Date: Thu, 20 Jun 2024 17:01:59 -0400 Subject: [PATCH 1/2] Add backup_retention_policy tests and prompt when reducing backups --- .../subcommands/backup_retention_policy.rb | 47 +++++-- .../backup_retention_policy_spec.rb | 115 ++++++++++++++++++ spec/fabricators/account_fabricator.rb | 1 + .../backup_retention_policy_fabricator.rb | 18 +++ 4 files changed, 174 insertions(+), 7 deletions(-) create mode 100644 spec/aptible/cli/subcommands/backup_retention_policy_spec.rb create mode 100644 spec/fabricators/backup_retention_policy_fabricator.rb diff --git a/lib/aptible/cli/subcommands/backup_retention_policy.rb b/lib/aptible/cli/subcommands/backup_retention_policy.rb index 1e05e51f..02075738 100644 --- a/lib/aptible/cli/subcommands/backup_retention_policy.rb +++ b/lib/aptible/cli/subcommands/backup_retention_policy.rb @@ -33,7 +33,7 @@ def self.included(thor) desc 'backup_retention_policy:set [ENVIRONMENT_HANDLE] ' \ '[--daily DAILY_BACKUPS] [--monthly MONTHLY_BACKUPS] ' \ '[--yearly YEARLY_BACKUPS] [--make-copy|--no-make-copy] ' \ - '[--keep-final|--no-keep-final]', + '[--keep-final|--no-keep-final] [--force]', "Change the environment's backup retention policy" option :daily, type: :numeric, desc: 'Number of daily backups to retain' @@ -43,12 +43,14 @@ def self.included(thor) desc: 'Number of yearly backups to retain' option :make_copy, type: :boolean, desc: 'If backup copies should be created' - option( - :keep_final, - type: :boolean, - desc: 'If final backups should be kept when databases are ' \ - 'deprovisioned' - ) + option :keep_final, + type: :boolean, + desc: 'If final backups should be kept when databases are ' \ + 'deprovisioned' + option :force, + type: :boolean, + desc: 'Do not prompt for confirmation if the new policy ' \ + 'retains fewer backups than the current policy' define_method 'backup_retention_policy:set' do |env| if options.empty? raise Thor::Error, @@ -74,6 +76,37 @@ def self.included(thor) 'specify all attributes to create one.' end + # Check if the number of backups over any period have been reduced + do_confirm = %i(daily monthly yearly).any? do |a| + cur = current_policy.try(a) + next if cur.nil? + cur > attrs[a] + end + + # Check if any of the boolean fields have been disabled + do_confirm ||= %i(make_copy keep_final).any? do |a| + cur = current_policy.try(a) + next if cur.nil? + cur && !attrs[a] + end + + if do_confirm && !options[:force] + m = 'The specified backup retention policy retains fewer ' \ + "backups than the environment's current policy. This may " \ + 'result in the deletion of existing, automated backups ' \ + "if they exceed the new policy's rules and it may " \ + "violate your company's internal compliance controls. " \ + 'For more information, see https://www.aptible.com/docs' \ + '/core-concepts/managed-databases/managing-databases' \ + '/database-backups.' + + m = set_color(m, :yellow) + print_wrapped m + confirmation = yes?('Do you want to proceed [y/N]:') + puts '' + raise Thor::Error, 'Aborting' unless confirmation + end + new_policy = account.create_backup_retention_policy!(**attrs) Formatter.render(Renderer.current) do |root| diff --git a/spec/aptible/cli/subcommands/backup_retention_policy_spec.rb b/spec/aptible/cli/subcommands/backup_retention_policy_spec.rb new file mode 100644 index 00000000..89843114 --- /dev/null +++ b/spec/aptible/cli/subcommands/backup_retention_policy_spec.rb @@ -0,0 +1,115 @@ +require 'spec_helper' + +describe Aptible::CLI::Agent do + let(:token) { 'some-token' } + let(:account) { Fabricate(:account, handle: 'test') } + let(:database) { Fabricate(:database, account: account, handle: 'some-db') } + let!(:policy) do + # created_at: 2016-06-14 13:24:11 +0000 + Fabricate(:backup_retention_policy, account: account) + end + + let(:default_handle) { 'some-db-at-2016-06-14-13-24-11' } + + before do + allow(subject).to receive(:fetch_token).and_return(token) + allow(Aptible::Api::Account).to receive(:all) { [account] } + end + + describe '#backup_retention_policy' do + it 'raises an error if the environment has no policy' do + allow(account).to receive(:backup_retention_policies).and_return([]) + expect { subject.backup_retention_policy('test') } + .to raise_error(/does not have a custom backup retention policy/) + end + + it "prints the enviroment's current policy" do + subject.backup_retention_policy('test') + out = captured_output_text + expect(out).to match(/daily: 30/i) + expect(out).to match(/monthly: 12/i) + expect(out).to match(/yearly: 6/i) + expect(out).to match(/make copy: true/i) + expect(out).to match(/keep final: true/i) + expect(out).to match(/environment: test/i) + end + end + + describe '#backup_retention_policy:set' do + it 'requires all attributes if the environment has no policy' do + allow(account).to receive(:backup_retention_policies).and_return([]) + opts = { + daily: 3, + monthly: 2, + yearly: 1, + make_copy: false, + keep_final: true + } + + opts.each_key do |k| + missing_opts = opts.clone + missing_opts.delete(k) + + subject.options = missing_opts + expect { subject.send('backup_retention_policy:set', 'test') } + .to raise_error(/please specify all attributes/i) + end + + expect(account).to receive(:create_backup_retention_policy!) + .with(**opts).and_return(Fabricate(:backup_retention_policy)) + subject.options = opts + subject.send('backup_retention_policy:set', 'test') + end + + it 'merges provided options with the current policy' do + expected_opts = { + daily: 5, + monthly: policy.monthly, + yearly: policy.yearly, + make_copy: policy.make_copy, + keep_final: false + } + + expect(account).to receive(:create_backup_retention_policy!) + .with(**expected_opts).and_return(Fabricate(:backup_retention_policy)) + subject.options = { daily: 5, keep_final: false, force: true } + subject.send('backup_retention_policy:set', 'test') + end + + it 'prompts the user if the new policy retains fewer backups' do + subject.options = { daily: 0 } + + # Reject Prompt + expect(subject).to receive(:yes?).with(/do you want to proceed/i) + + expect { subject.send('backup_retention_policy:set', 'test') } + .to raise_error(/aborting/i) + + # Accept Prompt + expect(subject).to receive(:yes?).with(/do you want to proceed/i) + .and_return(true) + + expect(account).to receive(:create_backup_retention_policy!) + .and_return(Fabricate(:backup_retention_policy)) + + subject.send('backup_retention_policy:set', 'test') + end + + it '--force skips the confirmation promt' do + subject.options = { make_copy: false } + + # Reject Prompt + expect(subject).to receive(:yes?).with(/do you want to proceed/i) + + expect { subject.send('backup_retention_policy:set', 'test') } + .to raise_error(/aborting/i) + + # --force + subject.options[:force] = true + expect(account).to receive(:create_backup_retention_policy!) + .and_return(Fabricate(:backup_retention_policy)) + + subject.send('backup_retention_policy:set', 'test') + end + end +end diff --git a/spec/fabricators/account_fabricator.rb b/spec/fabricators/account_fabricator.rb index 1e927449..e1f90dcb 100644 --- a/spec/fabricators/account_fabricator.rb +++ b/spec/fabricators/account_fabricator.rb @@ -28,5 +28,6 @@ def each_certificate(&block) certificates { [] } log_drains { [] } metric_drains { [] } + backup_retention_policies { [] } created_at { Time.now } end diff --git a/spec/fabricators/backup_retention_policy_fabricator.rb b/spec/fabricators/backup_retention_policy_fabricator.rb new file mode 100644 index 00000000..66099e56 --- /dev/null +++ b/spec/fabricators/backup_retention_policy_fabricator.rb @@ -0,0 +1,18 @@ +class StubBackupRetentionPolicy < OpenStruct + def reload + self + end +end + +Fabricator(:backup_retention_policy, from: :stub_backup_retention_policy) do + id { sequence(:backup_retention_policy_id) } + created_at { Time.now } + daily { 30 } + monthly { 12 } + yearly { 6 } + make_copy { true } + keep_final { true } + account + + after_create { |policy| policy.account.backup_retention_policies << policy } +end From a1a1eac3518f75efa19b70ceee7128b6c9b5b206 Mon Sep 17 00:00:00 2001 From: Josh Raker Date: Thu, 20 Jun 2024 22:12:22 -0400 Subject: [PATCH 2/2] Sync README usage --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index dc505e45..e89c075f 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ Commands: aptible backup:purge BACKUP_ID # Permanently delete a backup and any copies of it aptible backup:restore BACKUP_ID [--environment ENVIRONMENT_HANDLE] [--handle HANDLE] [--container-size SIZE_MB] [--disk-size SIZE_GB] [--container-profile PROFILE] [--iops IOPS] [--key-arn KEY_ARN] # Restore a backup aptible backup_retention_policy [ENVIRONMENT_HANDLE] # Show the current backup retention policy for the environment - aptible backup_retention_policy:set [ENVIRONMENT_HANDLE] [--daily DAILY_BACKUPS] [--monthly MONTHLY_BACKUPS] [--yearly YEARLY_BACKUPS] [--make-copy|--no-make-copy] [--keep-final|--no-keep-final] # Change the environment's backup retention policy + aptible backup_retention_policy:set [ENVIRONMENT_HANDLE] [--daily DAILY_BACKUPS] [--monthly MONTHLY_BACKUPS] [--yearly YEARLY_BACKUPS] [--make-copy|--no-make-copy] [--keep-final|--no-keep-final] [--force] # Change the environment's backup retention policy aptible config # Print an app's current configuration aptible config:add [VAR1=VAL1] [VAR2=VAL2] [...] # Add an ENV variable to an app aptible config:get [VAR1] # Print a specific key within an app's current configuration