Set Thor min version to 1.0 (#684)

CHANGELOG.md

@@ -6,6 +6,7 @@
 - Help ruby correctly identify kubectl output encoding. [#646](https://github.com/Shopify/krane/pull/646)
 
 *Other*
+- `--stdin` flag is deprecated. To read from STDIN, use `-f -` (can be combined with other files/directories) [#684](https://github.com/Shopify/krane/pull/684).
 - Reduces the number of container logs printed for failures from 250 to 25 to reduce noise. [#676](https://github.com/Shopify/krane/pull/676)
 - Remove hardcoded cloudsql class. [#680](https://github.com/Shopify/krane/pull/680)
 

README.md

@@ -112,8 +112,7 @@ official compatibility chart below.
 Refer to `krane help` for the authoritative set of options.
 
 
-- `--filenames / -f [PATHS]`: Accepts a list of directories and/or filenames to specify the set of directories/files that will be deployed.
-- `--stdin`: Read from STDIN. Can be combined with `-f` Example: `cat templates_from_stdin/*.yml | krane deploy ns ctx -f path/to/dir path/to/file.yml --stdin`
+- `--filenames / -f [PATHS]`: Accepts a list of directories and/or filenames to specify the set of directories/files that will be deployed, use `-` to specify reading from STDIN.
 - `--no-prune`: Skips pruning of resources that are no longer in your Kubernetes template set. Not recommended, as it allows your namespace to accumulate cruft that is not reflected in your deploy directory.
 - `--global-timeout=duration`: Raise a timeout error if it takes longer than _duration_ for any
 resource to deploy.
@@ -138,7 +137,7 @@ If you need to share a namespace with resources which are managed by other tools
 All templates must be YAML formatted.
 We recommended storing each app's templates in a single directory, `{app root}/config/deploy/{env}`. However, you may use multiple directories.
 
-If you want dynamic templates, you may render ERB with `krane render` and then pipe that result to `krane deploy --stdin`. `krane deploy` supports using both `--filenames` and `--stdin` together.
+If you want dynamic templates, you may render ERB with `krane render` and then pipe that result to `krane deploy -f -`.
 
 ### Customizing behaviour with annotations
 - `krane.shopify.io/timeout-override`: Override the tool's hard timeout for one specific resource. Both full ISO8601 durations and the time portion of ISO8601 durations are valid. Value must be between 1 second and 24 hours.
@@ -329,7 +328,7 @@ Let's walk through what happens when you run the `deploy` task with [this direct
 You can test this out for yourself by running the following command:
 
 ```bash
-krane render -f test/fixtures/hello-cloud --current-sha 1 | krane deploy my-namespace my-k8s-cluster --stdin
+krane render -f test/fixtures/hello-cloud --current-sha 1 | krane deploy my-namespace my-k8s-cluster -f -
 ```
 
 As soon as you run this, you'll start seeing some output being streamed to STDERR.
@@ -434,8 +433,7 @@ $ krane global-deploy my-k8s-context -f my-template.yml --selector app=krane
 
 Refer to `krane global-deploy help` for the authoritative set of options.
 
-- `--filenames / -if [PATHS]`: Accepts a list of directories and/or filenames to specify the set of directories/files that will be deployed
-- `--stdin`: Read from STDIN. Can be combined with `-f`
+- `--filenames / -f [PATHS]`: Accepts a list of directories and/or filenames to specify the set of directories/files that will be deployed. Use `-` to specify STDIN.
 - `--no-prune`: Skips pruning of resources that are no longer in your Kubernetes template set. Not recommended, as it allows your namespace to accumulate cruft that is not reflected in your deploy directory.
 - `--selector`: Instructs krane to only prune resources which match the specified label selector, such as `environment=staging`. By using this option, all resource templates must specify matching labels. See [Sharing a namespace](#sharing-a-namespace) below.
 - `--global-timeout=duration`: Raise a timeout error if it takes longer than _duration_ for any
@@ -542,8 +540,7 @@ krane render -f ./path/to/template/dir/template.yaml.erb > template.yaml
 
 *Options:*
 
-- `--filenames / -f [PATHS]`: Accepts a list of directories and/or filenames to specify the set of directories/files that will be deployed.
-- `--stdin`: Read from STDIN. Can be combined with `-f` Example: `cat templates_from_stdin/*.yml | krane render -f path/to/dir path/to/file.yml --stdin`
+- `--filenames / -f [PATHS]`: Accepts a list of directories and/or filenames to specify the set of directories/files that will be deployed. Use `-` to specify STDIN.
 - `--bindings=BINDINGS`: Makes additional variables available to your ERB templates. For example, `krane render --bindings=color=blue size=large -f some-template.yaml.erb` will expose `color` and `size` to `some-template.yaml.erb`.
 - `--current-sha`: Expose SHA `current_sha` in ERB bindings
 

krane.gemspec

@@ -33,7 +33,7 @@ Gem::Specification.new do |spec|
   spec.add_dependency("oj", "~> 3.0")
   spec.add_dependency("concurrent-ruby", "~> 1.1")
   spec.add_dependency("jsonpath", "~> 0.9.6")
-  spec.add_dependency("thor", ">= 0.20.3", "< 2.0")
+  spec.add_dependency("thor", ">= 1.0", "< 2.0")
 
   # Basics
   spec.add_development_dependency("bundler")

lib/krane/cli/deploy_command.rb

@@ -14,7 +14,7 @@ class DeployCommand
                          aliases: :f, required: false, default: [],
                          desc: "Directories and files that contains the configuration to apply" },
         "stdin" => { type: :boolean, default: false,
-                     desc: "Read resources from stdin" },
+                     desc: "[DEPRECATED] Read resources from stdin" },
         "global-timeout" => { type: :string, banner: "duration", default: DEFAULT_DEPLOY_TIMEOUT,
                               desc: "Max duration to monitor workloads correctly deployed" },
         "protected-namespaces" => { type: :array, banner: "namespace1 namespace2 namespaceN",
@@ -46,11 +46,10 @@ def self.from_options(namespace, context, options)
           protected_namespaces = []
         end
 
-        # never mutate options directly
         filenames = options[:filenames].dup
         filenames << "-" if options[:stdin]
         if filenames.empty?
-          raise Thor::RequiredArgumentMissingError, 'At least one of --filenames or --stdin must be set'
+          raise(Thor::RequiredArgumentMissingError, '--filenames must be set and not empty')
         end
 
         ::Krane::OptionsHelper.with_processed_template_paths(filenames) do |paths|

lib/krane/cli/global_deploy_command.rb

@@ -8,7 +8,8 @@ class GlobalDeployCommand
         "filenames" => { type: :array, banner: 'config/deploy/production config/deploy/my-extra-resource.yml',
                          aliases: :f, required: false, default: [],
                          desc: "Directories and files that contains the configuration to apply" },
-        "stdin" => { type: :boolean, default: false, desc: "Read resources from stdin" },
+        "stdin" => { type: :boolean, default: false,
+                     desc: "[DEPRECATED] Read resources from stdin" },
         "global-timeout" => { type: :string, banner: "duration", default: DEFAULT_DEPLOY_TIMEOUT,
                               desc: "Max duration to monitor workloads correctly deployed" },
         "verify-result" => { type: :boolean, default: true,
@@ -28,11 +29,10 @@ def self.from_options(context, options)
 
         selector = ::Krane::LabelSelector.parse(options[:selector])
 
-        # never mutate options directly
         filenames = options[:filenames].dup
         filenames << "-" if options[:stdin]
         if filenames.empty?
-          raise Thor::RequiredArgumentMissingError, 'At least one of --filenames or --stdin must be set'
+          raise(Thor::RequiredArgumentMissingError, '--filenames must be set and not empty')
         end
 
         ::Krane::OptionsHelper.with_processed_template_paths(filenames) do |paths|

lib/krane/cli/render_command.rb

@@ -7,7 +7,8 @@ class RenderCommand
         "bindings" => { type: :array, banner: "foo=bar abc=def", desc: 'Bindings for erb' },
         "filenames" => { type: :array, banner: 'config/deploy/production config/deploy/my-extra-resource.yml',
                          required: false, default: [], aliases: 'f', desc: 'Directories and files to render' },
-        "stdin" => { type: :boolean, desc: "Read resources from stdin", default: false },
+        "stdin" => { type: :boolean, default: false,
+                     desc: "[DEPRECATED] Read resources from stdin" },
         "current-sha" => { type: :string, banner: "SHA", desc: "Expose SHA `current_sha` in ERB bindings",
                            lazy_default: '' },
       }
@@ -20,11 +21,10 @@ def self.from_options(options)
         bindings_parser = ::Krane::BindingsParser.new
         options[:bindings]&.each { |b| bindings_parser.add(b) }
 
-        # never mutate options directly
         filenames = options[:filenames].dup
         filenames << "-" if options[:stdin]
         if filenames.empty?
-          raise Thor::RequiredArgumentMissingError, 'At least one of --filenames or --stdin must be set'
+          raise(Thor::RequiredArgumentMissingError, '--filenames must be set and not empty')
         end
 
         ::Krane::OptionsHelper.with_processed_template_paths(filenames, render_erb: true) do |paths|

lib/krane/template_sets.rb

@@ -150,7 +150,7 @@ def validate
 
       if rendering_erb_disabled? && deploying_with_erb_files?
         errors << "ERB template discovered with rendering disabled. If you were trying to render ERB and " \
-          "deploy the result, try piping the output of `krane render` to `krane-deploy` with the --stdin flag"
+          "deploy the result, try piping the output of `krane render` to `krane-deploy -f -`"
       end
 
       errors

test/exe/deploy_test.rb

@@ -48,41 +48,66 @@ def test_deploy_passes_protected_namespaces
     krane_deploy!(flags: "--protected-namespaces=''")
   end
 
-  def test_deploy_parses_std_in_without_filenames
+  def test_deploy_parses_std_in_alone
     Dir.mktmpdir do |tmp_path|
+      $stdin.expects("read").returns("")
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
+      set_krane_deploy_expectations(new_args: { filenames: [tmp_path] })
+      krane_deploy!(flags: '-f -')
+
+      # with deprecated --stdin flag
       $stdin.expects("read").returns("")
       Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
       set_krane_deploy_expectations(new_args: { filenames: [tmp_path] })
       krane_deploy!(flags: '--stdin')
     end
   end
 
-  def test_deploy_passes_filename
-    set_krane_deploy_expectations(new_args: { filenames: ['/my/file/path'] })
-    krane_deploy!(flags: '-f /my/file/path')
-    set_krane_deploy_expectations(new_args: { filenames: ['/my/other/file/path'] })
-    krane_deploy!(flags: '--filenames /my/other/file/path')
-  end
-
-  def test_deploy_parses_std_in
+  def test_deploy_parses_std_in_with_multiple_files
     Dir.mktmpdir do |tmp_path|
+      $stdin.expects("read").returns("")
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
+      set_krane_deploy_expectations(new_args: { filenames: ['/my/file/path', tmp_path] })
+      krane_deploy!(flags: '-f /my/file/path -')
+
+      # with deprecated --stdin flag
       $stdin.expects("read").returns("")
       Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
       set_krane_deploy_expectations(new_args: { filenames: ['/my/file/path', tmp_path] })
       krane_deploy!(flags: '-f /my/file/path --stdin')
     end
   end
 
-  def test_deploy_fails_without_filename_and_std_in
+  def test_deploy_passes_filename
+    set_krane_deploy_expectations(new_args: { filenames: ['/my/file/path'] })
+    krane_deploy!(flags: '-f /my/file/path')
+    set_krane_deploy_expectations(new_args: { filenames: ['/my/other/file/path'] })
+    krane_deploy!(flags: '--filenames /my/other/file/path')
+  end
+
+  def test_deploy_fails_without_filename
     krane = Krane::CLI::Krane.new(
       [deploy_task_config.namespace, deploy_task_config.context],
       []
     )
-    assert_raises_message(Thor::RequiredArgumentMissingError, "At least one of --filenames or --stdin must be set") do
+    assert_raises_message(Thor::RequiredArgumentMissingError, "--filenames must be set and not empty") do
       krane.invoke("deploy")
     end
   end
 
+  def test_stdin_flag_deduped_if_specified_multiple_times
+    Dir.mktmpdir do |tmp_path|
+      $stdin.expects("read").returns("").times(2)
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path).times(2)
+      set_krane_deploy_expectations(new_args: { filenames: [tmp_path] })
+      krane_deploy!(flags: '-f - -')
+
+      # with deprecated --stdin flag
+      set_krane_deploy_expectations(new_args: { filenames: [tmp_path] })
+      krane_deploy!(flags: '-f - --stdin')
+    end
+  end
+
   private
 
   def set_krane_deploy_expectations(new_args: {}, run_args: {})
@@ -94,7 +119,7 @@ def set_krane_deploy_expectations(new_args: {}, run_args: {})
   end
 
   def krane_deploy!(flags: '')
-    flags += ' -f /tmp' unless flags.include?('-f') || flags.include?("--stdin")
+    flags += ' -f /tmp' unless flags.include?("-f") || flags.include?("--stdin")
     krane = Krane::CLI::Krane.new(
       [deploy_task_config.namespace, deploy_task_config.context],
       flags.split

test/exe/global_deploy_test.rb

@@ -23,8 +23,14 @@ def test_deploy_passes_verify_result
     krane_global_deploy!(flags: '--verify-result false')
   end
 
-  def test_deploy_parses_std_in_without_filenames
+  def test_deploy_parses_std_in_alone
     Dir.mktmpdir do |tmp_path|
+      $stdin.expects("read").returns("")
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
+      set_krane_global_deploy_expectations!(new_args: { filenames: [tmp_path] })
+      krane_global_deploy!(flags: '--filenames -')
+
+      # with deprecated --stdin flag
       $stdin.expects("read").returns("")
       Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
       set_krane_global_deploy_expectations!(new_args: { filenames: [tmp_path] })
@@ -41,16 +47,35 @@ def test_deploy_passes_filename
 
   def test_deploy_parses_std_in
     Dir.mktmpdir do |tmp_path|
+      $stdin.expects("read").returns("")
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
+      set_krane_global_deploy_expectations!(new_args: { filenames: ['/my/file/path', tmp_path] })
+      krane_global_deploy!(flags: '-f /my/file/path -')
+
+      # with deprecated --stdin flag
       $stdin.expects("read").returns("")
       Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
       set_krane_global_deploy_expectations!(new_args: { filenames: ['/my/file/path', tmp_path] })
       krane_global_deploy!(flags: '-f /my/file/path --stdin')
     end
   end
 
-  def test_deploy_fails_without_filename_and_std_in
+  def test_stdin_flag_deduped_if_specified_multiple_times
+    Dir.mktmpdir do |tmp_path|
+      $stdin.expects("read").returns("").times(2)
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path).times(2)
+      set_krane_global_deploy_expectations!(new_args: { filenames: [tmp_path] })
+      krane_global_deploy!(flags: '-f - -')
+
+      # with deprecated --stdin flag
+      set_krane_global_deploy_expectations!(new_args: { filenames: [tmp_path] })
+      krane_global_deploy!(flags: '-f - --stdin')
+    end
+  end
+
+  def test_deploy_fails_without_filename
     krane = Krane::CLI::Krane.new([task_config.context], %w(--selector app=krane))
-    assert_raises_message(Thor::RequiredArgumentMissingError, "At least one of --filenames or --stdin must be set") do
+    assert_raises_message(Thor::RequiredArgumentMissingError, '--filenames must be set and not empty') do
       krane.invoke("global_deploy")
     end
   end
@@ -79,7 +104,7 @@ def set_krane_global_deploy_expectations!(new_args: {}, run_args: {})
   end
 
   def krane_global_deploy!(flags: '')
-    flags += ' -f /tmp' unless flags.include?('-f') || flags.include?('--stdin')
+    flags += ' -f /tmp' unless flags.include?("-f") || flags.include?("--stdin")
     flags += ' --selector name=web' unless flags.include?('--selector')
     krane = Krane::CLI::Krane.new(
       [task_config.context],

test/exe/render_test.rb

@@ -23,23 +23,48 @@ def test_render_parses_std_in
       $stdin.expects("read").returns("")
       Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
       install_krane_render_expectations(filenames: [file_path, tmp_path])
+      krane_render!("--filenames #{file_path} -")
+
+      # with deprecated --stdin flag
+      $stdin.expects("read").returns("")
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path)
+      install_krane_render_expectations(filenames: [file_path, tmp_path])
       krane_render!("--filenames #{file_path} --stdin")
     end
   end
 
   def test_render_parses_std_in_without_filenames
     Dir.mktmpdir do |tmp_path|
+      $stdin.expects("read").returns("")
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path).once
+      install_krane_render_expectations(filenames: [tmp_path])
+      krane_render!("-f -")
+
+      # with deprecated --stdin flag
       $stdin.expects("read").returns("")
       Dir.expects(:mktmpdir).with("krane").yields(tmp_path).once
       install_krane_render_expectations(filenames: [tmp_path])
       krane_render!("--stdin")
     end
   end
 
+  def test_stdin_flag_deduped_if_specified_multiple_times
+    Dir.mktmpdir do |tmp_path|
+      $stdin.expects("read").returns("").times(2)
+      Dir.expects(:mktmpdir).with("krane").yields(tmp_path).times(2)
+      install_krane_render_expectations(filenames: [tmp_path])
+      krane_render!('-f - -')
+
+      # with deprecated --stdin flag
+      install_krane_render_expectations(filenames: [tmp_path])
+      krane_render!('-f - --stdin')
+    end
+  end
+
   def test_render_fails_without_filename_and_std_in
     krane = Krane::CLI::Krane.new([], %w(--current-sha 1))
 
-    assert_raises_message(Thor::RequiredArgumentMissingError, "At least one of --filenames or --stdin must be set") do
+    assert_raises_message(Thor::RequiredArgumentMissingError, "--filenames must be set and not empty") do
       krane.invoke("render")
     end
   end

test/integration/krane_test.rb

@@ -62,7 +62,7 @@ def test_render_black_box_stdin
     test_sha = rand(10_000).to_s
 
     out, err, status = krane_black_box("render",
-      "--stdin --bindings #{bindings} --current-sha #{test_sha}", stdin: template)
+      "--filenames - --bindings #{bindings} --current-sha #{test_sha}", stdin: template)
 
     assert_predicate(status, :success?)
     assert_match("Success", err)
@@ -93,7 +93,7 @@ def test_deploy_black_box_success_stdin
       "-f #{fixture_path('hello-cloud')} --bindings deployment_id=1 current_sha=123")
     assert_predicate(render_status, :success?)
 
-    out, err, status = krane_black_box("deploy", "#{@namespace} #{KubeclientHelper::TEST_CONTEXT} --stdin",
+    out, err, status = krane_black_box("deploy", "#{@namespace} #{KubeclientHelper::TEST_CONTEXT} --filenames -",
       stdin: render_out)
     assert_empty(out)
     assert_match("Success", err)
@@ -103,7 +103,7 @@ def test_deploy_black_box_success_stdin
   def test_deploy_black_box_failure
     out, err, status = krane_black_box("deploy", "#{@namespace} #{KubeclientHelper::TEST_CONTEXT}")
     assert_empty(out)
-    assert_match("At least one of --filenames or --stdin must be set", err)
+    assert_match("--filenames must be set and not empty", err)
     refute_predicate(status, :success?)
     assert_equal(status.exitstatus, 1)
   end

test/unit/krane/template_sets_test.rb

@@ -39,7 +39,7 @@ def test_template_sets_with_erb_files_are_considered_invalid_when_render_erb_is_
     expected = [
       "File #{bad_filepath} does not have valid suffix (supported suffixes: .yml, .yaml, or secrets.ejson)",
       "ERB template discovered with rendering disabled. If you were trying to render ERB and " \
-        "deploy the result, try piping the output of `krane render` to `krane-deploy` with the --stdin flag",
+        "deploy the result, try piping the output of `krane render` to `krane-deploy -f -`",
     ]
     assert_equal(template_sets.validate, expected)
   end