From 1887a6518e02c0f9ef83250f4717b01e82d37536 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 22 Mar 2023 09:45:50 -0700 Subject: [PATCH] Commander needn't accumulate configuration Commander had version/destination solely to incrementally accumulate CLI options. Simpler to configure in one shot. Clarifies responsibility and lets us introduce things like `abbreviated_version` in one spot - Configuration. --- lib/mrsk/cli/app.rb | 2 +- lib/mrsk/cli/base.rb | 17 +++++++------- lib/mrsk/cli/build.rb | 2 +- lib/mrsk/cli/main.rb | 2 +- lib/mrsk/commander.rb | 36 +++++++++++++---------------- lib/mrsk/configuration.rb | 26 +++++++++++++++++---- test/cli/main_test.rb | 32 ++++++++++++++++++++++--- test/commander_test.rb | 14 +++-------- test/configuration_test.rb | 28 ++++++++++++++++------ test/fixtures/deploy_with_roles.yml | 5 ++-- 10 files changed, 105 insertions(+), 59 deletions(-) diff --git a/lib/mrsk/cli/app.rb b/lib/mrsk/cli/app.rb index 796cd367..0898a509 100644 --- a/lib/mrsk/cli/app.rb +++ b/lib/mrsk/cli/app.rb @@ -37,7 +37,7 @@ class Mrsk::Cli::App < Mrsk::Cli::Base desc "start", "Start existing app container on servers" def start on(MRSK.hosts) do - execute *MRSK.auditor.record("Started app version #{MRSK.version}"), verbosity: :debug + execute *MRSK.auditor.record("Started app version #{MRSK.config.version}"), verbosity: :debug execute *MRSK.app.start, raise_on_non_zero_exit: false end end diff --git a/lib/mrsk/cli/base.rb b/lib/mrsk/cli/base.rb index c7a87964..4e0f2104 100644 --- a/lib/mrsk/cli/base.rb +++ b/lib/mrsk/cli/base.rb @@ -39,14 +39,6 @@ module Mrsk::Cli def initialize_commander(options) MRSK.tap do |commander| - commander.config_file = Pathname.new(File.expand_path(options[:config_file])) - commander.destination = options[:destination] - commander.version = options[:version] - - commander.specific_hosts = options[:hosts]&.split(",") - commander.specific_roles = options[:roles]&.split(",") - commander.specific_primary! if options[:primary] - if options[:verbose] ENV["VERBOSE"] = "1" # For backtraces via cli/start commander.verbosity = :debug @@ -55,6 +47,15 @@ module Mrsk::Cli if options[:quiet] commander.verbosity = :error end + + commander.configure \ + config_file: Pathname.new(File.expand_path(options[:config_file])), + destination: options[:destination], + version: options[:version] + + commander.specific_hosts = options[:hosts]&.split(",") + commander.specific_roles = options[:roles]&.split(",") + commander.specific_primary! if options[:primary] end end diff --git a/lib/mrsk/cli/build.rb b/lib/mrsk/cli/build.rb index 3e8912d7..c9845e4f 100644 --- a/lib/mrsk/cli/build.rb +++ b/lib/mrsk/cli/build.rb @@ -29,7 +29,7 @@ class Mrsk::Cli::Build < Mrsk::Cli::Base desc "pull", "Pull app image from registry onto servers" def pull on(MRSK.hosts) do - execute *MRSK.auditor.record("Pulled image with version #{MRSK.version}"), verbosity: :debug + execute *MRSK.auditor.record("Pulled image with version #{MRSK.config.version}"), verbosity: :debug execute *MRSK.builder.clean, raise_on_non_zero_exit: false execute *MRSK.builder.pull end diff --git a/lib/mrsk/cli/main.rb b/lib/mrsk/cli/main.rb index 1524b49e..eb441d30 100644 --- a/lib/mrsk/cli/main.rb +++ b/lib/mrsk/cli/main.rb @@ -68,7 +68,7 @@ class Mrsk::Cli::Main < Mrsk::Cli::Base desc "rollback [VERSION]", "Rollback app to VERSION" def rollback(version) - MRSK.version = version + MRSK.config.version = version if container_name_available?(MRSK.config.service_with_version) say "Start version #{version}, then wait #{MRSK.config.readiness_delay}s for app to boot before stopping the old version...", :magenta diff --git a/lib/mrsk/commander.rb b/lib/mrsk/commander.rb index be2fd2e3..5aa24fc7 100644 --- a/lib/mrsk/commander.rb +++ b/lib/mrsk/commander.rb @@ -1,19 +1,26 @@ require "active_support/core_ext/enumerable" +require "active_support/core_ext/module/delegation" class Mrsk::Commander - attr_accessor :config_file, :destination, :verbosity, :version + attr_accessor :verbosity - def initialize(config_file: nil, destination: nil, verbosity: :info) - @config_file, @destination, @verbosity = config_file, destination, verbosity + def initialize + self.verbosity = :info end + def config - @config ||= \ - Mrsk::Configuration - .create_from(config_file, destination: destination, version: cascading_version) - .tap { |config| configure_sshkit_with(config) } + @config ||= Mrsk::Configuration.create_from(**@config_kwargs).tap do |config| + @config_kwargs = nil + configure_sshkit_with(config) + end end + def configure(**kwargs) + @config, @config_kwargs = nil, kwargs + end + + attr_accessor :specific_hosts def specific_primary! @@ -90,26 +97,15 @@ class Mrsk::Commander SSHKit.config.output_verbosity = old_level end + # Test-induced damage! def reset - @config = @config_file = @destination = @version = nil + @config = nil @app = @builder = @traefik = @registry = @prune = @auditor = nil @verbosity = :info end private - def cascading_version - version.presence || ENV["VERSION"] || current_commit_hash - end - - def current_commit_hash - if system("git rev-parse") - `git rev-parse HEAD`.strip - else - raise "Can't use commit hash as version, no git repository found in #{Dir.pwd}" - end - end - # Lazy setup of SSHKit def configure_sshkit_with(config) SSHKit::Backend::Netssh.configure { |ssh| ssh.ssh_options = config.ssh_options } diff --git a/lib/mrsk/configuration.rb b/lib/mrsk/configuration.rb index 1babea37..f95de700 100644 --- a/lib/mrsk/configuration.rb +++ b/lib/mrsk/configuration.rb @@ -9,13 +9,12 @@ class Mrsk::Configuration delegate :service, :image, :servers, :env, :labels, :registry, :builder, to: :raw_config, allow_nil: true delegate :argumentize, :argumentize_env_with_secrets, to: Mrsk::Utils - attr_accessor :version attr_accessor :destination attr_accessor :raw_config class << self - def create_from(base_config_file, destination: nil, version: "missing") - raw_config = load_config_files(base_config_file, *destination_config_file(base_config_file, destination)) + def create_from(config_file:, destination: nil, version: nil) + raw_config = load_config_files(config_file, *destination_config_file(config_file, destination)) new raw_config, destination: destination, version: version end @@ -38,14 +37,22 @@ class Mrsk::Configuration end end - def initialize(raw_config, destination: nil, version: "missing", validate: true) + def initialize(raw_config, destination: nil, version: nil, validate: true) @raw_config = ActiveSupport::InheritableOptions.new(raw_config) @destination = destination - @version = version + @declared_version = version valid? if validate end + def version=(version) + @declared_version = version + end + + def version + @declared_version.presence || ENV["VERSION"] || current_commit_hash + end + def abbreviated_version Mrsk::Utils.abbreviate_version(version) end @@ -203,4 +210,13 @@ class Mrsk::Configuration def role_names raw_config.servers.is_a?(Array) ? [ "web" ] : raw_config.servers.keys.sort end + + def current_commit_hash + @current_commit_hash ||= + if system("git rev-parse") + `git rev-parse HEAD`.strip + else + raise "Can't use commit hash as version, no git repository found in #{Dir.pwd}" + end + end end diff --git a/test/cli/main_test.rb b/test/cli/main_test.rb index 086067ff..151fa7b8 100644 --- a/test/cli/main_test.rb +++ b/test/cli/main_test.rb @@ -126,7 +126,7 @@ class CliMainTest < CliTestCase end test "config" do - run_command("config").tap do |output| + run_command("config", config_file: "deploy_with_accessories").tap do |output| config = YAML.load(output) assert_equal ["web"], config[:roles] @@ -138,6 +138,32 @@ class CliMainTest < CliTestCase end end + test "config with roles" do + run_command("config", config_file: "deploy_with_roles").tap do |output| + config = YAML.load(output) + + assert_equal ["web", "workers"], config[:roles] + assert_equal ["1.1.1.1", "1.1.1.2", "1.1.1.3", "1.1.1.4"], config[:hosts] + assert_equal "999", config[:version] + assert_equal "registry.digitalocean.com/dhh/app", config[:repository] + assert_equal "registry.digitalocean.com/dhh/app:999", config[:absolute_image] + assert_equal "app-999", config[:service_with_version] + end + end + + test "config with destination" do + run_command("config", "-d", "world", config_file: "deploy_for_dest").tap do |output| + config = YAML.load(output) + + assert_equal ["web"], config[:roles] + assert_equal ["1.1.1.1", "1.1.1.2"], config[:hosts] + assert_equal "999", config[:version] + assert_equal "registry.digitalocean.com/dhh/app", config[:repository] + assert_equal "registry.digitalocean.com/dhh/app:999", config[:absolute_image] + assert_equal "app-999", config[:service_with_version] + end + end + test "init" do Pathname.any_instance.expects(:exist?).returns(false).twice FileUtils.stubs(:mkdir_p) @@ -227,7 +253,7 @@ class CliMainTest < CliTestCase end private - def run_command(*command) - stdouted { Mrsk::Cli::Main.start([*command, "-c", "test/fixtures/deploy_with_accessories.yml"]) } + def run_command(*command, config_file: "deploy_with_accessories") + stdouted { Mrsk::Cli::Main.start([*command, "-c", "test/fixtures/#{config_file}.yml"]) } end end diff --git a/test/commander_test.rb b/test/commander_test.rb index b4ce24dc..278d9896 100644 --- a/test/commander_test.rb +++ b/test/commander_test.rb @@ -2,23 +2,15 @@ require "test_helper" class CommanderTest < ActiveSupport::TestCase setup do - @mrsk = Mrsk::Commander.new config_file: Pathname.new(File.expand_path("fixtures/deploy_with_roles.yml", __dir__)) + @mrsk = Mrsk::Commander.new.tap do |mrsk| + mrsk.configure config_file: Pathname.new(File.expand_path("fixtures/deploy_with_roles.yml", __dir__)) + end end test "lazy configuration" do assert_equal Mrsk::Configuration, @mrsk.config.class end - test "commit hash as version" do - assert_equal `git rev-parse HEAD`.strip, @mrsk.config.version - end - - test "commit hash as version but not in git" do - @mrsk.expects(:system).with("git rev-parse").returns(nil) - error = assert_raises(RuntimeError) { @mrsk.config } - assert_match /no git repository found/, error.message - end - test "overwriting hosts" do assert_equal [ "1.1.1.1", "1.1.1.2", "1.1.1.3", "1.1.1.4" ], @mrsk.hosts diff --git a/test/configuration_test.rb b/test/configuration_test.rb index 5cd7856b..30c4b049 100644 --- a/test/configuration_test.rb +++ b/test/configuration_test.rb @@ -3,6 +3,7 @@ require "test_helper" class ConfigurationTest < ActiveSupport::TestCase setup do ENV["RAILS_MASTER_KEY"] = "456" + ENV["VERSION"] = "missing" @deploy = { service: "app", image: "dhh/app", @@ -21,7 +22,8 @@ class ConfigurationTest < ActiveSupport::TestCase end teardown do - ENV["RAILS_MASTER_KEY"] = nil + ENV.delete("RAILS_MASTER_KEY") + ENV.delete("VERSION") end test "ensure valid keys" do @@ -66,8 +68,20 @@ class ConfigurationTest < ActiveSupport::TestCase end test "version" do - assert_equal "missing", @config.version - assert_equal "123", Mrsk::Configuration.new(@deploy, version: "123").version + ENV.delete("VERSION") + + @config.expects(:system).with("git rev-parse").returns(nil) + error = assert_raises(RuntimeError) { @config.version} + assert_match /no git repository found/, error.message + + @config.expects(:current_commit_hash).returns("git-version") + assert_equal "git-version", @config.version + + ENV["VERSION"] = "env-version" + assert_equal "env-version", @config.version + + @config.version = "arg-version" + assert_equal "arg-version", @config.version end test "repository" do @@ -158,17 +172,17 @@ class ConfigurationTest < ActiveSupport::TestCase end test "erb evaluation of yml config" do - config = Mrsk::Configuration.create_from Pathname.new(File.expand_path("fixtures/deploy.erb.yml", __dir__)) + config = Mrsk::Configuration.create_from config_file: Pathname.new(File.expand_path("fixtures/deploy.erb.yml", __dir__)) assert_equal "my-user", config.registry["username"] end test "destination yml config merge" do dest_config_file = Pathname.new(File.expand_path("fixtures/deploy_for_dest.yml", __dir__)) - config = Mrsk::Configuration.create_from dest_config_file, destination: "world" + config = Mrsk::Configuration.create_from config_file: dest_config_file, destination: "world" assert_equal "1.1.1.1", config.all_hosts.first - config = Mrsk::Configuration.create_from dest_config_file, destination: "mars" + config = Mrsk::Configuration.create_from config_file: dest_config_file, destination: "mars" assert_equal "1.1.1.3", config.all_hosts.first end @@ -176,7 +190,7 @@ class ConfigurationTest < ActiveSupport::TestCase dest_config_file = Pathname.new(File.expand_path("fixtures/deploy_for_dest.yml", __dir__)) assert_raises(RuntimeError) do - config = Mrsk::Configuration.create_from dest_config_file, destination: "missing" + config = Mrsk::Configuration.create_from config_file: dest_config_file, destination: "missing" end end diff --git a/test/fixtures/deploy_with_roles.yml b/test/fixtures/deploy_with_roles.yml index 345c6301..0e405241 100644 --- a/test/fixtures/deploy_with_roles.yml +++ b/test/fixtures/deploy_with_roles.yml @@ -5,8 +5,9 @@ servers: - 1.1.1.1 - 1.1.1.2 workers: - - 1.1.1.3 - - 1.1.1.4 + hosts: + - 1.1.1.3 + - 1.1.1.4 env: REDIS_URL: redis://x/y registry: