Remove batched dry-run validation code (Shopify#946)

powerhome · Jan 30, 2024 · ed0b0f4 · ed0b0f4
1 parent 63799d2
commit ed0b0f4
Show file tree

Hide file tree

Showing 10 changed files with 32 additions and 68 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -6,7 +6,7 @@ jobs:
   ruby-tests:
     runs-on: ubuntu-latest
 
-    name: "Tests - Ruby ${{ matrix.ruby }} with Kubernetes ${{ matrix.kubernetes_version }}"
+    name: "Tests (${{matrix.test_suite}}) - Ruby ${{ matrix.ruby }} with Kubernetes ${{ matrix.kubernetes_version }}"
     strategy:
       fail-fast: false
       matrix:
@@ -20,6 +20,11 @@ jobs:
           - "1.26.4"
           - "1.24.13"
           - "1.23.17"
+        test_suite:
+          - "unit_test"
+          - "cli_test"
+          - "serial_integration_test"
+          - "integration_test"
         include:
           - kubernetes_version: "1.27.3"
             kind_image: "kindest/node:v1.27.3@sha256:9dd3392d79af1b084671b05bcf65b21de476256ad1dcc853d9f3b10b4ac52dde"
@@ -52,4 +57,4 @@ jobs:
 
       - name: Run tests
         run: |
-          bin/test
+          bin/test ${{matrix.test_suite}}
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,8 +1,13 @@
 ## next
 
+# 3.4.2
+
+- Remove flag `--skip-dry-run` (see [#946](https://github.com/Shopify/krane/pull/946))
+- Remove support for batched server-side dry-run ([#946](https://github.com/Shopify/krane/pull/946))
+
 # 3.4.1
 
-- Added flag `--skip-dry-run` to completely opt out of dry run validation. 
+- Added flag `--skip-dry-run` to completely opt out of dry run validation.
 
 # 3.4.0
 

diff --git a/bin/test b/bin/test
@@ -33,16 +33,23 @@ if [[ "${CI:-0}" != "0" ]]; then
   PARALLELISM=2
 fi
 
-print_header "Run CLI Tests"
-bundle exec rake cli_test
+test_type=$1
 
-print_header "Run Unit Tests"
-bundle exec rake unit_test
 
-print_header "Run Non-Parallel Integration Tests"
-bundle exec rake serial_integration_test
+case $test_type in
+  cli_test | unit_test | serial_integration_test)
+    print_header $test_type
+    bundle exec rake $test_type
+    ;;
 
-print_header "Run Parallel Integration Tests (MT_CPU=$PARALLELISM)"
-PARALLELIZE_ME=1 MT_CPU=$PARALLELISM bundle exec rake integration_test
+  integration_test)
+    print_header "Run Parallel Integration Tests (MT_CPU=$PARALLELISM)"
+    PARALLELIZE_ME=1 MT_CPU=$PARALLELISM bundle exec rake integration_test
+    ;;
+
+  *)
+    echo "Argument must be one of: unit_test, cli_test, serial_integration_test, integration_test"
+    ;;
+esac
 
 test $err -eq 0
diff --git a/lib/krane/cli/deploy_command.rb b/lib/krane/cli/deploy_command.rb
@@ -33,7 +33,6 @@ class DeployCommand
                                   default: false },
         "verify-result" => { type: :boolean, default: true,
                              desc: "Verify workloads correctly deployed" },
-        "skip-dry-run" => { type: :boolean, desc: "Enable skipping dry run", default: false},
       }
 
       def self.from_options(namespace, context, options)
@@ -72,7 +71,6 @@ def self.from_options(namespace, context, options)
             selector: selector,
             selector_as_filter: selector_as_filter,
             protected_namespaces: protected_namespaces,
-            skip_dry_run: options["skip-dry-run"]
           )
 
           deploy.run!(

diff --git a/lib/krane/deploy_task.rb b/lib/krane/deploy_task.rb
@@ -106,7 +106,7 @@ def server_version
     # @param render_erb [Boolean] Enable ERB rendering
     def initialize(namespace:, context:, current_sha: nil, logger: nil, kubectl_instance: nil, bindings: {},
       global_timeout: nil, selector: nil, selector_as_filter: false, filenames: [], protected_namespaces: nil,
-      render_erb: false, kubeconfig: nil, skip_dry_run: false)
+      render_erb: false, kubeconfig: nil)
       @logger = logger || Krane::FormattedLogger.build(namespace, context)
       @template_sets = TemplateSets.from_dirs_and_files(paths: filenames, logger: @logger, render_erb: render_erb)
       @task_config = Krane::TaskConfig.new(context, namespace, @logger, kubeconfig)
@@ -121,7 +121,6 @@ def initialize(namespace:, context:, current_sha: nil, logger: nil, kubectl_inst
       @selector_as_filter = selector_as_filter
       @protected_namespaces = protected_namespaces || PROTECTED_NAMESPACES
       @render_erb = render_erb
-      @skip_dry_run = skip_dry_run
     end
 
     # Runs the task, returning a boolean representing success or failure
@@ -287,15 +286,9 @@ def validate_configuration(prune:)
 
     def validate_resources(resources)
       validate_globals(resources)
-      batch_dry_run_success = @skip_dry_run || validate_dry_run(resources)
       resources.select! { |r| r.selected?(@selector) } if @selector_as_filter
       Krane::Concurrency.split_across_threads(resources) do |r|
-        # No need to pass in kubectl (and do per-resource dry run apply) if batch dry run succeeded
-        if batch_dry_run_success
-          r.validate_definition(kubectl: nil, selector: @selector, dry_run: false)
-        else
-          r.validate_definition(kubectl: kubectl, selector: @selector, dry_run: true)
-        end
+        r.validate_definition(kubectl: kubectl, selector: @selector, dry_run: true)
       end
       failed_resources = resources.select(&:validation_failed?)
       if failed_resources.present?
@@ -321,10 +314,6 @@ def validate_globals(resources)
         "Use GlobalDeployTask instead."
     end
 
-    def validate_dry_run(resources)
-      resource_deployer.dry_run(resources)
-    end
-
     def namespace_definition
       @namespace_definition ||= begin
         definition, _err, st = kubectl.run("get", "namespace", @namespace, use_namespace: false,

diff --git a/lib/krane/resource_deployer.rb b/lib/krane/resource_deployer.rb
@@ -20,13 +20,6 @@ def initialize(task_config:, prune_allowlist:, global_timeout:, current_sha: nil
       @statsd_tags = statsd_tags
     end
 
-    def dry_run(resources)
-      apply_all(resources, true, dry_run: true)
-      true
-    rescue FatalDeploymentError
-      false
-    end
-
     def deploy!(resources, verify_result, prune)
       if verify_result
         deploy_all_resources(resources, prune: prune, verify: true)

diff --git a/lib/krane/version.rb b/lib/krane/version.rb
@@ -1,4 +1,4 @@
 # frozen_string_literal: true
 module Krane
-  VERSION = "3.4.1"
+  VERSION = "3.4.2"
 end
diff --git a/test/exe/deploy_test.rb b/test/exe/deploy_test.rb
@@ -164,7 +164,6 @@ def default_options(new_args = {}, run_args = {})
         selector: nil,
         selector_as_filter: false,
         protected_namespaces: ["default", "kube-system", "kube-public"],
-        skip_dry_run: false,
       }.merge(new_args),
       run_args: {
         verify_result: true,

diff --git a/test/helpers/fixture_deploy_helper.rb b/test/helpers/fixture_deploy_helper.rb
@@ -87,7 +87,7 @@ def deploy_raw_fixtures(set, wait: true, bindings: {}, subset: nil, render_erb:
     success
   end
 
-  def deploy_dirs_without_profiling(dirs, wait: true, prune: true, skip_dry_run: false, bindings: {},
+  def deploy_dirs_without_profiling(dirs, wait: true, prune: true, bindings: {},
     sha: "k#{SecureRandom.hex(6)}", kubectl_instance: nil, global_timeout: nil,
     selector: nil, selector_as_filter: false,
     protected_namespaces: nil, render_erb: false, context: KubeclientHelper::TEST_CONTEXT)
@@ -106,7 +106,6 @@ def deploy_dirs_without_profiling(dirs, wait: true, prune: true, skip_dry_run: f
       selector_as_filter: selector_as_filter,
       protected_namespaces: protected_namespaces,
       render_erb: render_erb,
-      skip_dry_run: skip_dry_run
     )
     deploy.run(
       verify_result: wait,

diff --git a/test/integration-serial/serial_deploy_test.rb b/test/integration-serial/serial_deploy_test.rb
@@ -526,37 +526,6 @@ def test_resource_discovery_stops_deploys_when_fetch_crds_kubectl_errs
     ], in_order: true)
   end
 
-  def test_batch_dry_run_apply_failure_falls_back_to_individual_resource_dry_run_validation
-    Krane::KubernetesResource.any_instance.expects(:validate_definition).with do |kwargs|
-      kwargs[:kubectl].is_a?(Krane::Kubectl) && kwargs[:dry_run]
-    end
-    deploy_fixtures("hello-cloud", subset: %w(secret.yml)) do |fixtures|
-      secret = fixtures["secret.yml"]["Secret"].first
-      secret["bad_field"] = "bad_key"
-    end
-  end
-
-  def test_batch_dry_run_apply_success_precludes_individual_resource_dry_run_validation
-    Krane::KubernetesResource.any_instance.expects(:validate_definition).with { |params| params[:dry_run] == false }
-    result = deploy_fixtures("hello-cloud", subset: %w(secret.yml))
-    assert_deploy_success(result)
-    assert_logs_match_all([
-      "Result: SUCCESS",
-      "Successfully deployed 1 resource",
-    ], in_order: true)
-  end
-
-  def test_skip_dry_run_apply_success
-    Krane::KubernetesResource.any_instance.expects(:validate_definition).with { |params| params[:dry_run] == false }
-    Krane::ResourceDeployer.any_instance.expects(:dry_run).never
-    result = deploy_fixtures("hello-cloud", subset: %w(secret.yml), skip_dry_run: true)
-    assert_deploy_success(result)
-    assert_logs_match_all([
-      "Result: SUCCESS",
-      "Successfully deployed 1 resource",
-    ], in_order: true)
-  end
-
   private
 
   def rollout_conditions_annotation_key