Merge pull request #1258 from Shopify/evan/obey-bare-shipit-file

Configurable respect bare shipit file behavior

Author: Archetypically

README.md

@@ -134,6 +134,19 @@ Lastly, if you override the `app_name` configuration in your Shipit deployment,
 
 * * *
 
+<h3 id="respecting-bare-files">Respecting bare <code>shipit.yml</code> files</h3>
+
+Shipit will, by default, respect the "bare" <code>shipit.yml</code> file as a fallback option if no more specifically-named file exists (such as <code>shipit.staging.yml</code>).
+
+You can configure this behavior via the attribute <code>Shipit.respect_bare_shipit_file</code>.
+
+- The value <code>false</code> will disable this behavior and instead cause Shipit to emit an error upon deploy if Shipit cannot find a more specifically-named file.
+- Setting this attribute to any other value (**including <code>nil</code>**), or not setting this attribute, will cause Shipit to use the default behavior of respecting bare <code>shipit.yml</code> files.
+
+You can determine if Shipit is configured to respect bare files using <code>Shipit.respect_bare_shipit_file?</code>.
+
+* * *
+
 <h3 id="installing-dependencies">Installing dependencies</h3>
 
 The **<code>dependencies</code>** step allows you to install all the packages your deploy script needs.

app/models/shipit/deploy_spec/file_system.rb

@@ -91,10 +91,31 @@ def config(*)
       end
 
       def load_config
-        read_config(file("#{app_name}.#{@env}.yml", root: true)) ||
-          read_config(file("#{app_name}.yml", root: true)) ||
-          read_config(file("shipit.#{@env}.yml", root: true)) ||
-          read_config(file('shipit.yml', root: true))
+        return if config_file_path.nil?
+        loaded_config = read_config(config_file_path)
+        return if loaded_config&.empty?
+
+        if !Shipit.respect_bare_shipit_file? && config_file_path.to_s.end_with?(*bare_shipit_filenames)
+          loaded_config["deploy"]["pre"] = [shipit_not_obeying_bare_file_echo_command, "exit 1"]
+        end
+        loaded_config
+      end
+
+      def shipit_file_names_in_priority_order
+        ["#{app_name}.#{@env}.yml", "#{app_name}.yml", "shipit.#{@env}.yml", "shipit.yml"].uniq
+      end
+
+      def bare_shipit_filenames
+        ["#{app_name}.yml", "shipit.yml"].uniq
+      end
+
+      def config_file_path
+        shipit_file_names_in_priority_order.each do |filename|
+          path = file(filename, root: true)
+          return path if path.exist?
+        end
+
+        nil
       end
 
       def app_name
@@ -104,6 +125,14 @@ def app_name
       def read_config(path)
         SafeYAML.load(path.read) if path.exist?
       end
+
+      def shipit_not_obeying_bare_file_echo_command
+        <<~EOM
+          echo \"\e[1;31mShipit is configured to ignore the bare '#{app_name}.yml' file.
+          Please rename this file to more specifically include the environment name.
+          Deployments will fail until a valid '#{app_name}.#{@env}.yml' file is found.\e[0m\"
+        EOM
+      end
     end
   end
 end

lib/shipit.rb

@@ -63,7 +63,7 @@ module Shipit
 
   delegate :table_name_prefix, to: :secrets
 
-  attr_accessor :disable_api_authentication, :timeout_exit_codes, :deployment_checks
+  attr_accessor :disable_api_authentication, :timeout_exit_codes, :deployment_checks, :respect_bare_shipit_file
   attr_writer(
     :internal_hook_receivers,
     :preferred_org_emails,
@@ -76,6 +76,9 @@ def task_execution_strategy
   end
 
   self.timeout_exit_codes = [].freeze
+  self.respect_bare_shipit_file = true
+
+  alias_method :respect_bare_shipit_file?, :respect_bare_shipit_file
 
   def authentication_disabled?
     ENV['SHIPIT_DISABLE_AUTH'].present?

test/models/deploy_spec_test.rb

@@ -286,29 +286,6 @@ class DeploySpecTest < ActiveSupport::TestCase
       assert_equal({ 'GLOBAL' => '1' }, @spec.machine_env)
     end
 
-    test '#load_config can grab the env-specific shipit.yml file' do
-      config = {}
-      config.expects(:exist?).returns(true)
-      config.expects(:read).returns({ 'dependencies' => { 'override' => %w(foo bar baz) } }.to_yaml)
-      spec = DeploySpec::FileSystem.new('.', 'staging')
-      spec.expects(:file).with('shipit.staging.yml', root: true).returns(config)
-      assert_equal %w(foo bar baz), spec.dependencies_steps
-    end
-
-    test '#load_config grabs the global shipit.yml file if there is no env-specific file' do
-      not_config = {}
-      not_config.expects(:exist?).returns(false)
-
-      config = {}
-      config.expects(:exist?).returns(true)
-      config.expects(:read).returns({ 'dependencies' => { 'override' => %w(foo bar baz) } }.to_yaml)
-
-      spec = DeploySpec::FileSystem.new('.', 'staging')
-      spec.expects(:file).with('shipit.staging.yml', root: true).returns(not_config)
-      spec.expects(:file).with('shipit.yml', root: true).returns(config)
-      assert_equal %w(foo bar baz), spec.dependencies_steps
-    end
-
     test '#gemspec gives the path of the repo gemspec if present' do
       spec = DeploySpec::FileSystem.new('foobar/', 'production')
       Dir.expects(:[]).with('foobar/*.gemspec').returns(['foobar/foobar.gemspec'])

test/models/shipit/deploy_spec/file_system_test.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+require 'test_helper'
+require 'tmpdir'
+
+module Shipit
+  class DeploySpec
+    class FileSystemTest < ActiveSupport::TestCase
+      test 'deploy.pre calls "exit 1" if there is a bare shipit file and Shipit is configured to ignore' do
+        Shipit.expects(:respect_bare_shipit_file?).returns(false).at_least_once
+        deploy_spec = Shipit::DeploySpec::FileSystem.new(Dir.tmpdir, 'env')
+        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_yaml))
+        pre_commands = deploy_spec.send(:config, 'deploy', 'pre')
+        assert pre_commands.include?('exit 1')
+        assert pre_commands.first.include?('configured to ignore')
+        refute pre_commands.include?('test 2')
+      end
+
+      test 'deploy.pre does not call "exit 1" if Shipit is not configured to do so' do
+        Shipit.expects(:respect_bare_shipit_file?).returns(true).at_least_once
+        deploy_spec = Shipit::DeploySpec::FileSystem.new(Dir.tmpdir, 'env')
+        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_yaml))
+        pre_commands = deploy_spec.send(:config, 'deploy', 'pre')
+        refute pre_commands.include?('exit 1')
+        assert pre_commands.include?('test 2')
+      end
+
+      test 'Shipit.respect_bare_shipit_file? has no effect if the file is not a bare file' do
+        [true, false].each do |obey_val|
+          Shipit.expects(:respect_bare_shipit_file?).returns(obey_val).at_least_once
+          deploy_spec = Shipit::DeploySpec::FileSystem.new(Dir.tmpdir, 'env')
+          deploy_spec.expects(:config_file_path).returns(Pathname.new(Dir.tmpdir) + '/shipit.env.yml').at_least_once
+          deploy_spec.expects(:read_config).returns(SafeYAML.load(deploy_spec_yaml))
+          pre_commands = deploy_spec.send(:config, 'deploy', 'pre')
+          refute pre_commands.include?('exit 1')
+          assert pre_commands.include?('test 2')
+        end
+      end
+
+      def deploy_spec_yaml
+        <<~EOYAML
+          deploy:
+            pre:
+              - test 2
+            override:
+              - test 1
+        EOYAML
+      end
+    end
+  end
+end