Address more PR feedback

exe/kubernetes-restart

@@ -25,7 +25,7 @@ context = ARGV[1]
 restart = KubernetesDeploy::RestartTask.new(namespace: namespace, context: context,
    global_timeout: max_watch_seconds)
 begin
-  restart.run!(raw_deployments, selector: selector)
+  restart.run!(deployments: raw_deployments, selector: selector)
 rescue KubernetesDeploy::DeploymentTimeoutError
   exit(70)
 rescue KubernetesDeploy::FatalDeploymentError

exe/kubernetes-run

@@ -34,7 +34,7 @@ runner = KubernetesDeploy::RunnerTask.new(
 
 success = runner.run(
   verify_result: verify_result,
-  template: template,
+  filename: template,
   command: entrypoint,
   arguments: ARGV[2..-1],
   env_vars: env_vars

lib/krane/cli/deploy_command.rb

@@ -55,12 +55,12 @@ def self.from_options(namespace, context, options)
         end
 
         ::Krane::OptionsHelper.with_processed_template_paths(options[:filenames],
-          require_explicit_path: true) do |paths|
+          require_explicit_path: true) do |filenames|
           deploy = ::Krane::DeployTask.new(
             namespace: namespace,
             context: context,
             current_sha: options['current-sha'],
-            filenames: paths,
+            filenames: filenames,
             bindings: bindings_parser.parse,
             logger: logger,
             global_timeout: ::Krane::DurationParser.new(options["global-timeout"]).parse!.to_i,

lib/krane/cli/restart_command.rb

@@ -24,7 +24,7 @@ def self.from_options(namespace, context, options)
           global_timeout: ::Krane::DurationParser.new(options["global-timeout"]).parse!.to_i,
         )
         restart.run!(
-          options[:deployments],
+          deployments: options[:deployments],
           selector: selector,
           verify_result: options["verify-result"]
         )

lib/krane/cli/run_command.rb

@@ -19,10 +19,11 @@ class RunCommand
         },
         "verify-result" => { type: :boolean, desc: "Wait for completion and verify pod success", default: true },
         "command" => { type: :array, desc: "Override the default command in the container image" },
-        "template" => {
+        "filename" => {
           type: :string,
           desc: "The template file you'll be rendering",
           required: true,
+          aliases: :f,
         },
         "env-vars" => {
           type: :string,
@@ -42,7 +43,7 @@ def self.from_options(namespace, context, options)
 
         runner.run!(
           verify_result: options['verify-result'],
-          template: options['template'],
+          filename: options['filename'],
           command: options['command'],
           arguments: options['arguments']&.split(" "),
           env_vars: options['env-vars'].split(','),

lib/krane/restart_task.rb

@@ -54,7 +54,7 @@ def run(*args)
     # @param verify_result [Boolean] Wait for completion and verify success
     #
     # @return [nil]
-    def run!(deployments = nil, selector: nil, verify_result: true)
+    def run!(deployments: nil, selector: nil, verify_result: true)
       start = Time.now.utc
       @logger.reset
 

lib/krane/runner_task.rb

@@ -43,24 +43,24 @@ def run(*args)
 
     # Runs the task, raising exceptions in case of issues
     #
-    # @param template [String] The template file you'll be rendering (*required*)
+    # @param filename [String] The filename of the template you'll be rendering (*required*)
     # @param command [Array<String>] Override the default command in the container image
     # @param arguments [Array<String>] Override the default arguments for the command
     # @param env_vars [Array<String>] List of env vars
     # @param verify_result [Boolean] Wait for completion and verify pod success
     #
     # @return [nil]
-    def run!(template:, command:, arguments:, env_vars: [], verify_result: true)
+    def run!(filename:, command:, arguments:, env_vars: [], verify_result: true)
       start = Time.now.utc
       @logger.reset
 
       @logger.phase_heading("Initializing task")
 
       @logger.info("Validating configuration")
-      verify_config!(template)
+      verify_config!(filename)
       @logger.info("Using namespace '#{@namespace}' in context '#{@context}'")
 
-      pod = build_pod(template, command, arguments, env_vars, verify_result)
+      pod = build_pod(filename, command, arguments, env_vars, verify_result)
       validate_pod(pod)
 
       @logger.phase_heading("Running pod")

test/exe/restart_test.rb

@@ -16,16 +16,16 @@ def test_restart_parses_global_timeout
   end
 
   def test_restart_passes_deployments_transparently
-    set_krane_restart_expectations(deployments: ['web'])
+    set_krane_restart_expectations(run_args: { deployments: ['web'] })
     krane_restart!(flags: '--deployments web')
-    set_krane_restart_expectations(deployments: ['web', 'jobs'])
+    set_krane_restart_expectations(run_args: { deployments: ['web', 'jobs'] })
     krane_restart!(flags: '--deployments web jobs')
   end
 
   def test_restart_parses_selector
     options = default_options
     response = mock('RestartTask')
-    response.expects(:run!).returns(true).with(options[:deployments],
+    response.expects(:run!).returns(true).with(
       has_entries(selector: is_a(Krane::LabelSelector), verify_result: true))
     Krane::RestartTask.expects(:new).with(options[:new_args]).returns(response)
 
@@ -48,10 +48,10 @@ def test_restart_failure_as_black_box
 
   private
 
-  def set_krane_restart_expectations(new_args: {}, deployments: nil, run_args: {})
-    options = default_options(new_args, deployments, run_args)
+  def set_krane_restart_expectations(new_args: {}, run_args: {})
+    options = default_options(new_args, run_args)
     response = mock('RestartTask')
-    response.expects(:run!).with(options[:deployments], options[:run_args]).returns(true)
+    response.expects(:run!).with(options[:run_args]).returns(true)
     Krane::RestartTask.expects(:new).with(options[:new_args]).returns(response)
   end
 
@@ -67,15 +67,15 @@ def restart_task_config
     @restart_config ||= task_config(namespace: 'test-namespace')
   end
 
-  def default_options(new_args = {}, deployments = nil, run_args = {})
+  def default_options(new_args = {}, run_args = {})
     {
       new_args: {
         namespace: restart_task_config.namespace,
         context: restart_task_config.context,
         global_timeout: 300,
       }.merge(new_args),
-      deployments: deployments,
       run_args: {
+        deployments: nil,
         selector: nil,
         verify_result: true,
       }.merge(run_args),

test/exe/run_test.rb

@@ -34,9 +34,9 @@ def test_run_parses_arguments
     krane_run!(flags: '--arguments hello')
   end
 
-  def test_run_parses_template
-    set_krane_run_expectations(run_args: { template: 'some-name' })
-    krane_run!(flags: '--template some-name')
+  def test_run_parses_filename
+    set_krane_run_expectations(run_args: { filename: 'some-name' })
+    krane_run!(flags: '--filename some-name')
   end
 
   def test_run_parses_env_vars
@@ -45,21 +45,21 @@ def test_run_parses_env_vars
   end
 
   def test_run_failure_with_not_enough_arguments_as_black_box
-    out, err, status = krane_black_box('run', '--template some_template not_enough_arguments')
+    out, err, status = krane_black_box('run', '--filename some_template not_enough_arguments')
     assert_equal(1, status.exitstatus)
     assert_empty(out)
     assert_match("ERROR", err)
   end
 
   def test_run_failure_with_too_many_args_as_black_box
-    out, err, status = krane_black_box('run', '--template some_template ns ctx some_extra_arg')
+    out, err, status = krane_black_box('run', '--filename some_template ns ctx some_extra_arg')
     assert_equal(1, status.exitstatus)
     assert_empty(out)
     assert_match("ERROR", err)
   end
 
   def test_run_failure_with_bad_timeout_as_black_box
-    out, err, status = krane_black_box('run', '--template some_template ns ctx --global-timeout=mittens')
+    out, err, status = krane_black_box('run', '--filename some_template ns ctx --global-timeout=mittens')
     assert_equal(1, status.exitstatus)
     assert_empty(out)
     assert_match("Error parsing duration", err)
@@ -75,7 +75,7 @@ def set_krane_run_expectations(new_args: {}, run_args: {})
   end
 
   def krane_run!(flags: '')
-    flags += " --template #{TASK_TEMPLATE}" unless flags.include?('--template')
+    flags += " --filename #{TASK_TEMPLATE}" unless flags.include?('--filename')
     krane = Krane::CLI::Krane.new(
       [run_task_config.namespace, run_task_config.context],
       flags.split
@@ -96,7 +96,7 @@ def default_options(new_args = {}, run_args = {})
       }.merge(new_args),
       run_args: {
         verify_result: true,
-        template: TASK_TEMPLATE,
+        filename: TASK_TEMPLATE,
         command: nil,
         arguments: nil,
         env_vars: [],

test/helpers/task_runner_test_helper.rb

@@ -21,7 +21,7 @@ def build_task_runner(context: KubeclientHelper::TEST_CONTEXT, ns: @namespace, g
 
   def run_params(log_lines: 5, log_interval: 0.1, verify_result: true)
     {
-      template: 'hello-cloud-template-runner',
+      filename: 'hello-cloud-template-runner',
       command: ['/bin/sh', '-c'],
       arguments: [
         "i=1; " \

test/integration/krane_test.rb

@@ -21,7 +21,7 @@ def test_run_success_black_box
     assert_deploy_success(deploy_fixtures("hello-cloud", subset: ["template-runner.yml", "configmap-data.yml"]))
     template = "hello-cloud-template-runner"
     out, err, status = krane_black_box("run",
-      "#{@namespace} #{KubeclientHelper::TEST_CONTEXT} --template #{template} --command ls --arguments '-a /'")
+      "#{@namespace} #{KubeclientHelper::TEST_CONTEXT} --filename #{template} --command ls --arguments '-a /'")
     assert_match("Success", err)
     assert_empty(out)
     assert_predicate(status, :success?)

test/integration/restart_task_test.rb

@@ -78,7 +78,7 @@ def test_restart_named_deployments_twice
     refute(fetch_restarted_at("web"), "no RESTARTED_AT env on fresh deployment")
 
     restart = build_restart_task
-    assert_restart_success(restart.perform(%w(web)))
+    assert_restart_success(restart.perform(deployments: %w(web)))
 
     assert_logs_match_all([
       "Configured to restart deployments by name: web",
@@ -95,7 +95,7 @@ def test_restart_named_deployments_twice
     assert(first_restarted_at, "RESTARTED_AT is present after first restart")
 
     Timecop.freeze(1.second.from_now) do
-      assert_restart_success(restart.perform(%w(web)))
+      assert_restart_success(restart.perform(deployments: %w(web)))
     end
 
     second_restarted_at = fetch_restarted_at("web")
@@ -110,7 +110,7 @@ def test_restart_with_same_resource_twice
     refute(fetch_restarted_at("web"), "no RESTARTED_AT env on fresh deployment")
 
     restart = build_restart_task
-    assert_restart_success(restart.perform(%w(web web)))
+    assert_restart_success(restart.perform(deployments: %w(web web)))
 
     assert_logs_match_all([
       "Configured to restart deployments by name: web",
@@ -126,7 +126,7 @@ def test_restart_with_same_resource_twice
 
   def test_restart_not_existing_deployment
     restart = build_restart_task
-    assert_restart_failure(restart.perform(%w(web)))
+    assert_restart_failure(restart.perform(deployments: %w(web)))
     assert_logs_match_all([
       "Configured to restart deployments by name: web",
       "Result: FAILURE",
@@ -139,7 +139,7 @@ def test_restart_one_not_existing_deployment
     assert(deploy_fixtures("hello-cloud", subset: ["configmap-data.yml", "web.yml.erb"], render_erb: true))
 
     restart = build_restart_task
-    assert_restart_failure(restart.perform(%w(walrus web)))
+    assert_restart_failure(restart.perform(deployments: %w(walrus web)))
 
     refute(fetch_restarted_at("web"), "no RESTARTED_AT env after failed restart task")
     assert_logs_match_all([
@@ -152,7 +152,7 @@ def test_restart_one_not_existing_deployment
 
   def test_restart_none
     restart = build_restart_task
-    assert_restart_failure(restart.perform([]))
+    assert_restart_failure(restart.perform(deployments: []))
     assert_logs_match_all([
       "Result: FAILURE",
       "Configured to restart deployments by name, but list of names was blank",
@@ -162,7 +162,7 @@ def test_restart_none
 
   def test_restart_deployments_and_selector
     restart = build_restart_task
-    assert_restart_failure(restart.perform(%w(web), selector: Krane::LabelSelector.parse("app=web")))
+    assert_restart_failure(restart.perform(deployments: %w(web), selector: Krane::LabelSelector.parse("app=web")))
     assert_logs_match_all([
       "Result: FAILURE",
       "Can't specify deployment names and selector at the same time",
@@ -176,7 +176,7 @@ def test_restart_not_existing_context
       namespace: @namespace,
       logger: logger
     )
-    assert_restart_failure(restart.perform(%w(web)))
+    assert_restart_failure(restart.perform(deployments: %w(web)))
     assert_logs_match_all([
       "Result: FAILURE",
       /- Context walrus missing from your kubeconfig file\(s\)/,
@@ -190,7 +190,7 @@ def test_restart_not_existing_namespace
       namespace: "walrus",
       logger: logger
     )
-    assert_restart_failure(restart.perform(%w(web)))
+    assert_restart_failure(restart.perform(deployments: %w(web)))
     assert_logs_match_all([
       "Result: FAILURE",
       "- Could not find Namespace: walrus in Context: #{KubeclientHelper::TEST_CONTEXT}",
@@ -220,7 +220,7 @@ def test_restart_failure
     assert_deploy_success(success)
 
     restart = build_restart_task
-    assert_raises(Krane::DeploymentTimeoutError) { restart.perform!(%w(web)) }
+    assert_raises(Krane::DeploymentTimeoutError) { restart.perform!(deployments: %w(web)) }
 
     assert_logs_match_all([
       "Triggered `web` restart",
@@ -249,7 +249,7 @@ def test_restart_successful_with_partial_availability
     assert_deploy_success(result)
 
     restart = build_restart_task
-    assert_restart_success(restart.perform(%w(web)))
+    assert_restart_success(restart.perform(deployments: %w(web)))
 
     pods = kubeclient.get_pods(namespace: @namespace, label_selector: 'name=web,app=slow-cloud')
     new_pods = pods.select do |pod|
@@ -321,7 +321,7 @@ def test_verify_result_false_succeeds_quickly_when_verification_would_timeout
     assert_deploy_success(success)
 
     restart = build_restart_task
-    restart.perform!(%w(web), verify_result: false)
+    restart.perform!(deployments: %w(web), verify_result: false)
 
     assert_logs_match_all([
       "Triggered `web` restart",

test/integration/runner_task_test.rb

@@ -206,7 +206,7 @@ def test_run_fails_if_namespace_is_missing
 
   def test_run_fails_if_template_is_blank
     task_runner = build_task_runner
-    result = task_runner.run(template: '',
+    result = task_runner.run(filename: '',
       command: ['/bin/sh', '-c'],
       arguments: nil,
       env_vars: ["MY_CUSTOM_VARIABLE=MITTENS"])
@@ -224,7 +224,7 @@ def test_run_fails_if_template_is_blank
   def test_run_bang_fails_if_template_is_invalid
     task_runner = build_task_runner
     assert_raises(Krane::TaskConfigurationError) do
-      task_runner.run!(template: '',
+      task_runner.run!(filename: '',
         command: ['/bin/sh', '-c'],
         arguments: nil,
         env_vars: ["MY_CUSTOM_VARIABLE=MITTENS"])
@@ -271,7 +271,7 @@ def test_run_adds_env_vars_provided_to_the_task_container
 
     task_runner = build_task_runner
     result = task_runner.run(
-      template: 'hello-cloud-template-runner',
+      filename: 'hello-cloud-template-runner',
       command: ['/bin/sh', '-c'],
       arguments: ['echo "The value is: $MY_CUSTOM_VARIABLE"'],
       env_vars: ["MY_CUSTOM_VARIABLE=MITTENS"]