From ed0b0f4ecbc9b039ace540b5fb553fca6fccfa4f Mon Sep 17 00:00:00 2001 From: Timothy Smith <31742287+timothysmith0609@users.noreply.github.com> Date: Tue, 30 Jan 2024 15:18:42 -0500 Subject: [PATCH] Remove batched dry-run validation code (#946) --- .github/workflows/ci.yml | 9 ++++-- CHANGELOG.md | 7 ++++- bin/test | 23 +++++++++----- lib/krane/cli/deploy_command.rb | 2 -- lib/krane/deploy_task.rb | 15 ++------- lib/krane/resource_deployer.rb | 7 ----- lib/krane/version.rb | 2 +- test/exe/deploy_test.rb | 1 - test/helpers/fixture_deploy_helper.rb | 3 +- test/integration-serial/serial_deploy_test.rb | 31 ------------------- 10 files changed, 32 insertions(+), 68 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bfeba2a47..0bedde4f0 100644 --- 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 index 0b68969bc..7da0c6008 100644 --- 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 index 281dbb24a..bde5d13ae 100755 --- 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 index 6e4b089a8..361d89789 100644 --- 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 index 1b0b79fed..4973b48ac 100644 --- 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 index fe0cabe45..3c79ea6ba 100644 --- 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 index ff2f24e2b..be14be275 100644 --- 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 index c8e255068..ce52fb4a5 100644 --- 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 index 1063319d4..390b380de 100644 --- 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 index 92f924de6..39a5553b3 100644 --- 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