Merge pull request #1432 from Shopify/ts/support-explicit-rollback-variables

Allow rollbacks to specify their own variables

Author: timothysmith0609

CHANGELOG.md

@@ -1,5 +1,7 @@
 # Unreleased
 
+* Allow explicit configuration of rollback variables (#1432)
+
 # 0.41.1
 
 * Fix commit statuses (#1424)

README.md

@@ -352,6 +352,38 @@ rollback:
 ```
 <br>
 
+You can also accept custom environment variables defined by the user that triggers the rollback:
+
+**<code>rollback.variables</code>** contains an array of variable definitions.
+
+For example:
+
+```yaml
+rollback:
+  variables:
+    -
+      name: RUN_MIGRATIONS
+      title: Run database migrations on rollback
+      default: 1
+```
+<br>
+
+**<code>rollback.variables.select</code>** will turn the input into a `<select>` of values.
+
+For example:
+
+```yaml
+rollback:
+  variables:
+    -
+      name: REGION
+      title: Run a rollback in a given region
+      select:
+        - east
+        - west
+        - north
+```
+<br>
 
 **<code>fetch</code>** contains an array of the shell commands that Shipit executes to check the revision of the currently-deployed version. This key defaults to `disabled`.
 
@@ -362,7 +394,7 @@ fetch:
 ```
 
 **Note:** Currently, deployments in emergency mode are configured to occur concurrently via [the `build_deploy` method](https://github.com/Shopify/shipit-engine/blob/main/app/models/shipit/stack.rb),
-whose `allow_concurrency` keyword argument defaults to `force`, where `force` is true when emergency mode is enabled. 
+whose `allow_concurrency` keyword argument defaults to `force`, where `force` is true when emergency mode is enabled.
 If you'd like to separate these two from one another, override this method as desired in your service.
 
 <h3 id="kubernetes">Kubernetes</h3>

app/controllers/shipit/api/rollbacks_controller.rb

@@ -15,7 +15,7 @@ def create
         commit = stack.commits.by_sha(params.sha) || param_error!(:sha, 'Unknown revision')
         param_error!(:force, "Can't rollback a locked stack") if !params.force && stack.locked?
         deploy = stack.deploys.find_by(until_commit: commit) || param_error!(:sha, 'Cant find associated deploy')
-        deploy_env = stack.filter_deploy_envs(params.env)
+        rollback_env = stack.filter_rollback_envs(params.env)
 
         response = nil
         if !params.force && stack.active_task?
@@ -25,7 +25,7 @@ def create
           active_task.abort!(aborted_by: current_user, rollback_once_aborted_to: deploy, rollback_once_aborted: true)
           response = active_task
         else
-          response = deploy.trigger_rollback(current_user, env: deploy_env, force: params.force, lock: params.lock)
+          response = deploy.trigger_rollback(current_user, env: rollback_env, force: params.force, lock: params.lock)
         end
 
         render_resource(response, status: :accepted)

app/controllers/shipit/rollbacks_controller.rb

@@ -27,7 +27,7 @@ def load_deploy
     end
 
     def rollback_params
-      params.require(:rollback).permit(:parent_id, env: @stack.deploy_variables.map(&:name))
+      params.require(:rollback).permit(:parent_id, env: @stack.rollback_variables.map(&:name))
     end
   end
 end

app/models/shipit/deploy_spec.rb

@@ -139,6 +139,15 @@ def rollback_steps!
       rollback_steps || cant_detect!(:rollback)
     end
 
+    def rollback_variables
+      if config('rollback', 'variables').nil?
+        # For backwards compatibility, fallback to using deploy_variables if no explicit rollback variables are set
+        deploy_variables
+      else
+        Array.wrap(config('rollback', 'variables')).map(&VariableDefinition.method(:new))
+      end
+    end
+
     def retries_on_rollback
       config('rollback', 'retries') { nil }
     end
@@ -166,6 +175,10 @@ def filter_deploy_envs(env)
       EnvironmentVariables.with(env).permit(deploy_variables)
     end
 
+    def filter_rollback_envs(env)
+      EnvironmentVariables.with(env).permit(rollback_variables)
+    end
+
     def review_checklist
       (config('review', 'checklist') || discover_review_checklist || []).map(&:strip).select(&:present?)
     end

app/models/shipit/deploy_spec/file_system.rb

@@ -78,7 +78,8 @@ def cacheable_config
           },
           'rollback' => {
             'override' => rollback_steps,
-            'retries' => retries_on_rollback
+            'retries' => retries_on_rollback,
+            'variables' => rollback_variables.map(&:to_h)
           },
           'fetch' => fetch_deployed_revision_steps,
           'tasks' => cacheable_tasks

app/models/shipit/rollback.rb

@@ -38,6 +38,10 @@ def report_complete!
       complete!
     end
 
+    def variables
+      stack.rollback_variables
+    end
+
     private
 
     def update_release_status

app/models/shipit/stack.rb

@@ -519,8 +519,9 @@ def self.from_param!(param)
     end
 
     delegate :plugins, :task_definitions, :hidden_statuses, :required_statuses, :soft_failing_statuses,
-             :blocking_statuses, :deploy_variables, :filter_task_envs, :filter_deploy_envs,
-             :maximum_commits_per_deploy, :pause_between_deploys, :retries_on_deploy, :retries_on_rollback,
+             :blocking_statuses, :deploy_variables, :rollback_variables, :filter_task_envs, :filter_deploy_envs,
+             :filter_rollback_envs, :maximum_commits_per_deploy, :pause_between_deploys, :retries_on_deploy,
+             :retries_on_rollback,
              to: :cached_deploy_spec
 
     def monitoring?

test/models/deploy_spec_test.rb

@@ -243,6 +243,31 @@ class DeploySpecTest < ActiveSupport::TestCase
       assert_equal ["kubernetes-deploy --max-watch-seconds 900 foo bar"], @spec.rollback_steps
     end
 
+    test "#rollback_variables returns an empty array by default" do
+      assert_equal [], @spec.rollback_variables
+    end
+
+    test "#rollback_variables returns an array of VariableDefinition instances" do
+      @spec.stubs(:load_config).returns('rollback' => { 'variables' => [{
+                                          'name' => 'SAFETY_DISABLED',
+                                          'title' => 'Set to 1 to do dangerous things',
+                                          'default' => 0
+                                        }] })
+
+      assert_equal 1, @spec.rollback_variables.size
+      variable_definition = @spec.rollback_variables.first
+      assert_equal 'SAFETY_DISABLED', variable_definition.name
+    end
+
+    test "#rollback_variables falls back to deploy_variables if rollback_variables itself is empty" do
+      @spec.stubs(:load_config).returns('deploy' => { 'variables' => [{
+                                          'name' => 'SAFETY_DISABLED',
+                                          'title' => 'Set to 1 to do dangerous things',
+                                          'default' => 0
+                                        }] })
+      assert_equal @spec.rollback_variables.map(&:to_h), @spec.deploy_variables.map(&:to_h)
+    end
+
     test "#discover_task_definitions include a kubernetes restart command if `kubernetes` is present" do
       @spec.stubs(:load_config).returns(
         'kubernetes' => {
@@ -381,7 +406,8 @@ class DeploySpecTest < ActiveSupport::TestCase
         },
         'rollback' => {
           'override' => nil,
-          'retries' => nil
+          'retries' => nil,
+          'variables' => []
         },
         'fetch' => nil,
         'tasks' => {}

test/models/shipit/deploy_spec/file_system_test.rb

@@ -93,6 +93,34 @@ class FileSystemTest < ActiveSupport::TestCase
         assert_not loaded_config.include?(Shipit::DeploySpec::FileSystem::SHIPIT_CONFIG_INHERIT_FROM_KEY)
       end
 
+      test '#load_config accurately deserializes deploy variables' do
+        Shipit.expects(:respect_bare_shipit_file?).returns(true).at_least_once
+        stack = shipit_stacks(:shipit)
+        deploy_spec = Shipit::DeploySpec::FileSystem.new(Dir.tmpdir, stack)
+        deploy_spec.expects(:config_file_path).returns("#{Pathname.new(Dir.tmpdir)}/shipit.yml").at_least_once
+        deploy_spec.expects(:read_config).returns(SafeYAML.load(deploy_spec_with_variables_yaml))
+
+        deploy_vars = deploy_spec.deploy_variables
+        assert_equal 1, deploy_vars.length
+
+        assert_equal 'TEST_VAR', deploy_vars[0].name
+        assert_equal 'deploy_default', deploy_vars[0].default
+      end
+
+      test '#load_config accurately deserializes rollback variables' do
+        Shipit.expects(:respect_bare_shipit_file?).returns(true).at_least_once
+        stack = shipit_stacks(:shipit)
+        deploy_spec = Shipit::DeploySpec::FileSystem.new(Dir.tmpdir, stack)
+        deploy_spec.expects(:config_file_path).returns("#{Pathname.new(Dir.tmpdir)}/shipit.yml").at_least_once
+        deploy_spec.expects(:read_config).returns(SafeYAML.load(deploy_spec_with_variables_yaml))
+
+        rollback_vars = deploy_spec.rollback_variables
+        assert_equal 1, rollback_vars.length
+
+        assert_equal 'TEST_VAR', rollback_vars[0].name
+        assert_equal 'rollback_default', rollback_vars[0].default
+      end
+
       def deploy_spec_yaml
         <<~EOYAML
           deploy:
@@ -120,6 +148,19 @@ def deploy_spec_missing_deploy_yaml
               - production-unrestricted-1234
         EOYAML
       end
+
+      def deploy_spec_with_variables_yaml
+        <<~EOYAML
+          deploy:
+            variables:
+              - name: TEST_VAR
+                default: "deploy_default"
+          rollback:
+            variables:
+              - name: TEST_VAR
+                default: "rollback_default"
+        EOYAML
+      end
     end
   end
 end