Require steps to be explicitly marked as skippable for whitelisting

definitions/checks/foreman/check_tuning_requirements.rb

@@ -8,7 +8,6 @@ class TuningRequirements < ForemanMaintain::Check
         confine do
           feature(:katello)
         end
-        do_not_whitelist
       end
 
       def run

definitions/checks/repositories/check_upstream_repository.rb

@@ -5,6 +5,7 @@ class Checks::CheckUpstreamRepository < ForemanMaintain::Check
     label :check_upstream_repository
     description 'Check if any upstream repositories are enabled on system'
     tags :pre_upgrade
+    skippable
     preparation_steps do
       [Checks::Repositories::CheckNonRhRepository.new,
        Procedures::Packages::Install.new(:packages => %w[yum-utils])]

definitions/checks/repositories/validate.rb

@@ -2,6 +2,8 @@ module Checks::Repositories
   class Validate < ForemanMaintain::Check
     metadata do
       description 'Validate availability of repositories'
+      skippable
+
       preparation_steps do
         Checks::Repositories::CheckNonRhRepository.new
       end

definitions/procedures/content/prepare.rb

@@ -4,7 +4,6 @@ class Prepare < ForemanMaintain::Procedure
       description 'Prepare content for Pulp 3'
       for_feature :pulpcore
       param :quiet, 'Keep the output on a single line', :flag => true, :default => false
-      do_not_whitelist
     end
 
     def run

definitions/procedures/content/switchover.rb

@@ -9,7 +9,6 @@ class Switchover < ForemanMaintain::Procedure
       end
 
       param :skip_deb, 'Do not run debian options in installer.'
-      do_not_whitelist
     end
 
     def run

definitions/procedures/repositories/setup.rb

@@ -2,6 +2,7 @@ module Procedures::Repositories
   class Setup < ForemanMaintain::Procedure
     metadata do
       description 'Setup repositories'
+      skippable
       confine do
         feature(:instance).downstream || feature(:upstream)
       end

lib/foreman_maintain/concerns/metadata.rb

@@ -100,8 +100,8 @@ def advanced_run(advanced_run)
           @data[:advanced_run] = advanced_run
         end
 
-        def do_not_whitelist
-          @data[:do_not_whitelist] = true
+        def skippable
+          @data[:skippable] = true
         end
 
         def self.eval_dsl(metadata, &block)

lib/foreman_maintain/reporter/cli_reporter.rb

@@ -315,19 +315,17 @@ def scenario_failure_message(scenario)
         end
 
         steps_with_error = scenario.steps_with_error(:whitelisted => false)
-        steps_with_skipped = scenario.steps_with_skipped(:whitelisted => true)
-        not_skippable_steps = scenario.steps_with_error.select do |step|
-          step.metadata[:do_not_whitelist] == true
+        skippable_steps = scenario.steps_with_error.select do |step|
+          step.metadata[:skippable] == true
         end
 
-        steps_to_whitelist = steps_with_error + steps_with_skipped - not_skippable_steps
         unless steps_with_error.empty?
           message << format(<<-MESSAGE.strip_heredoc, format_steps(steps_with_error, "\n", 2))
           The following steps ended up in failing state:
 
           %s
           MESSAGE
-          whitelist_labels = steps_to_whitelist.map(&:label_dashed).join(',')
+          whitelist_labels = skippable_steps.map(&:label_dashed).join(',')
           unless whitelist_labels.empty?
             recommend << if scenario.detector.feature(:instance).downstream
                            format(<<-MESSAGE.strip_heredoc, whitelist_labels)

lib/foreman_maintain/runner.rb

@@ -52,6 +52,8 @@ def run_scenario(scenario)
       return if scenario.steps.empty?
       raise 'The runner is already in quit state' if quit? && !rescue?
 
+      validate_steps(scenario.steps)
+
       confirm_scenario(scenario)
       return if quit? && !rescue?
 
@@ -177,5 +179,13 @@ def ask_about_offered_steps(step, scenario)
     def rerun_check?(step)
       @last_decision_step == step
     end
+
+    def validate_steps(steps)
+      steps.each do |step|
+        if whitelisted_step?(step) && !step.metadata[:skippable]
+          raise "#{step} is not skippable. Please remove from whitelist."
+        end
+      end
+    end
   end
 end

test/definitions/checks/foreman/check_tuning_profile_test.rb

@@ -42,8 +42,4 @@
     assert_includes result.output, 'The number of CPU cores for the system is 6 but the currently configured tuning profile requires 8.' # rubocop:disable Metrics/LineLength
     assert result.fail?
   end
-
-  it 'does not allow being whitelisted' do
-    assert subject.metadata[:do_not_whitelist]
-  end
 end

test/lib/cli/upgrade_command_test.rb

@@ -230,6 +230,7 @@ def foreman_maintain_update_unavailable
       end
 
       it 'with --phase it runs only a specific phase of the upgrade' do
+        foreman_maintain_update_unavailable
         UpgradeRunner.any_instance.expects(:run_phase).with(:pre_migrations)
         assert_cmd(<<-OUTPUT.strip_heredoc, ['--phase=pre_migrations', '--target-version=1.15'])
           Checking for new version of #{ForemanMaintain.main_package_name}...

test/lib/reporter_test.rb

@@ -132,7 +132,7 @@ def decision_question(description)
       contact Red Hat Technical Support.
 
       In case the failures are false positives, use
-      --whitelist="dummy-check-fail,dummy-check-fail2"
+      --whitelist="dummy-check-fail"
       MESSAGE
     end
 
@@ -160,7 +160,7 @@ def decision_question(description)
 
       Resolve the failed steps and rerun the command.
       In case the failures are false positives, use
-      --whitelist="dummy-check-fail,dummy-check-fail2"
+      --whitelist="dummy-check-fail"
       MESSAGE
     end
 

test/lib/support/definitions/checks/dummy.rb

@@ -13,6 +13,7 @@ class Fail < ForemanMaintain::Check
     metadata do
       label :dummy_check_fail
       description 'Check that ends up with fail'
+      skippable
     end
 
     def run
@@ -35,7 +36,6 @@ class FailSkipWhitelist < ForemanMaintain::Check
     metadata do
       label :dummy_check_fail_skipwhitelist
       description 'Check that ends up with fail'
-      do_not_whitelist
     end
 
     def run

test/lib/support/definitions/checks/service_is_stopped.rb

@@ -9,7 +9,7 @@ class Checks::ServiceIsStopped < ForemanMaintain::Check
   end
 
   def run
-    assert(false, 'service is running',
+    assert(TestHelper.service_is_stopped, 'service is running',
       :next_steps => Procedures::StopService.new)
   end
 end

test/lib/support/definitions/features/present_service.rb

@@ -20,6 +20,6 @@ def restart
   end
 
   def running?
-    false
+    TestHelper.present_service_is_running
   end
 end

test/lib/test_helper.rb

@@ -2,11 +2,13 @@
 
 class TestHelper
   class << self
-    attr_accessor :use_present_service_2, :present_service_is_running, :migrations_fail_at
+    attr_accessor :use_present_service_2, :present_service_is_running, :migrations_fail_at,
+                  :service_is_stopped
 
     def reset
       self.use_present_service_2 = false
       self.present_service_is_running = false
+      self.service_is_stopped = false
       self.migrations_fail_at = nil
     end
   end

test/lib/upgrade_runner_test.rb

@@ -15,11 +15,6 @@ module ForemanMaintain
       UpgradeRunner.new('1.15', reporter)
     end
 
-    let(:upgrade_runner_with_whitelist) do
-      UpgradeRunner.new('1.15', reporter,
-        :whitelist => %w[present-service-is-running service-is-stopped])
-    end
-
     it 'lists versions available for upgrading, based on available scenarios' do
       _(UpgradeRunner.available_targets).must_equal ['1.15']
     end
@@ -36,55 +31,43 @@ module ForemanMaintain
     end
 
     it 'asks for confirmation before getting into pre_migrations from pre upgrade checks' do
-      upgrade_runner_with_whitelist.run
+      TestHelper.present_service_is_running = true
+      TestHelper.service_is_stopped = true
+      upgrade_runner.run
+
       _(reporter.log.last).must_equal ['ask', <<-MESSAGE.strip_heredoc.strip]
         The pre-upgrade checks indicate that the system is ready for upgrade.
         It's recommended to perform a backup at this stage.
         Confirm to continue with the modification part of the upgrade, [y(yes), n(no), q(quit)]
       MESSAGE
-      assert_equal(:pre_upgrade_checks, upgrade_runner_with_whitelist.phase,
+      assert_equal(:pre_upgrade_checks, upgrade_runner.phase,
         'The phase should not be switched until confirmed')
     end
 
-    it 'remembers the state of the previous run of the upgrade' do
-      TestHelper.migrations_fail_at = :migrations
-      reporter.input << 'y'
-      upgrade_runner_with_whitelist.storage.data.clear
-      upgrade_runner_with_whitelist.run
-      upgrade_runner_with_whitelist.save
-      original_scenario = upgrade_runner_with_whitelist.scenario(:pre_upgrade_checks)
-
-      ForemanMaintain.detector.refresh
-      new_upgrade_runner = UpgradeRunner.new('1.15', reporter)
-      new_upgrade_runner.load
-      _(new_upgrade_runner.phase).must_equal :migrations
-      restored_scenario = new_upgrade_runner.scenario(:pre_upgrade_checks)
-      _(restored_scenario.steps.map { |s| s.execution.status }).
-        must_equal(original_scenario.steps.map { |step| step.execution.status })
-    end
-
     it 'remembers the current target version' do
       TestHelper.migrations_fail_at = :migrations
+      TestHelper.present_service_is_running = true
+      TestHelper.service_is_stopped = true
       reporter.input << 'y'
-      upgrade_runner_with_whitelist.storage.data.clear
-      upgrade_runner_with_whitelist.run
-      upgrade_runner_with_whitelist.save
+      upgrade_runner.storage.data.clear
+      upgrade_runner.run
+      upgrade_runner.save
       _(UpgradeRunner.current_target_version).must_equal '1.15'
       _(UpgradeRunner.available_targets).must_equal(['1.15'])
     end
 
     it 'does not remember the current target version when failed on pre_upgrade_checks ===' do
       TestHelper.migrations_fail_at = :pre_upgrade_checks
-      upgrade_runner_with_whitelist.run
-      upgrade_runner_with_whitelist.save
+      upgrade_runner.run
+      upgrade_runner.save
       _(UpgradeRunner.current_target_version).must_be_nil
     end
 
     it 'cleans the state when the upgrade finished successfully' do
       reporter.input << 'y'
-      upgrade_runner_with_whitelist.storage.data.clear
-      upgrade_runner_with_whitelist.run
-      upgrade_runner_with_whitelist.save
+      upgrade_runner.storage.data.clear
+      upgrade_runner.run
+      upgrade_runner.save
 
       new_upgrade_runner = UpgradeRunner.new('1.15', reporter)
       new_upgrade_runner.load
@@ -100,48 +83,47 @@ module ForemanMaintain
     end
 
     it 'runs migrations if pre_migrations succeed' do
+      TestHelper.present_service_is_running = true
+      TestHelper.service_is_stopped = true
       reporter.input << 'y'
-      upgrade_runner_with_whitelist.run
+      upgrade_runner.run
       _(reporter.log).must_include ['before_execution_starts', :upgrade_migration]
     end
 
-    it 'runs post_migrations and post_upgrade checks if pre_migrations fail' do
-      reporter.input << 'y'
-      TestHelper.migrations_fail_at = :pre_migrations
-      upgrade_runner_with_whitelist.run
-      _(upgrade_runner_with_whitelist.phase).must_equal :pre_upgrade_checks
-      _(reporter.log).wont_include ['before_execution_starts', :upgrade_migration]
-      _(reporter.log).must_include ['before_execution_starts', :upgrade_post_migration]
-      _(reporter.log).must_include ['before_execution_starts', :upgrade_post_upgrade_check]
-      _(upgrade_runner_with_whitelist.exit_code).must_equal 1
-    end
-
     it 'runs post_migrations if migrations succeed' do
       reporter.input << 'y'
-      upgrade_runner_with_whitelist.run
+      TestHelper.present_service_is_running = true
+      TestHelper.service_is_stopped = true
+      upgrade_runner.run
       _(reporter.log).must_include ['before_execution_starts', :upgrade_post_migration]
     end
 
     it 'fails if migrations fail' do
       reporter.input << 'y'
       TestHelper.migrations_fail_at = :migrations
-      upgrade_runner_with_whitelist.run
-      _(upgrade_runner_with_whitelist.phase).must_equal :migrations
-      _(upgrade_runner_with_whitelist.exit_code).must_equal 1
+      TestHelper.present_service_is_running = true
+      TestHelper.service_is_stopped = true
+      upgrade_runner.run
+      _(upgrade_runner.phase).must_equal :migrations
+      _(upgrade_runner.exit_code).must_equal 1
     end
 
     it 'runs post_upgrade_checks if post_migrations succeed' do
       reporter.input << 'y'
-      upgrade_runner_with_whitelist.run
+      TestHelper.present_service_is_running = true
+      TestHelper.service_is_stopped = true
+      upgrade_runner.run
       _(reporter.log).must_include ['before_execution_starts', :upgrade_post_upgrade_check]
     end
 
     it 'fails if post_migrations fail' do
       reporter.input << 'y'
       TestHelper.migrations_fail_at = :post_migrations
-      upgrade_runner_with_whitelist.run
-      _(upgrade_runner_with_whitelist.phase).must_equal :post_migrations
-      _(upgrade_runner_with_whitelist.exit_code).must_equal 1
+      TestHelper.present_service_is_running = true
+      TestHelper.service_is_stopped = true
+      upgrade_runner.run
+      _(upgrade_runner.phase).must_equal :post_migrations
+      _(upgrade_runner.exit_code).must_equal 1
     end
   end
 end