diff --git a/config.ru b/config.ru index 685295a84..780fcdf54 100644 --- a/config.ru +++ b/config.ru @@ -6,7 +6,6 @@ app = PactBroker::App.new do | config | config.log_stream = :stdout config.base_urls = ["http://localhost:9292"] config.database_url = "sqlite:////tmp/pact_broker_database.sqlite3" - config.log_configuration # do this last so the logger is configured correctly end run app diff --git a/lib/pact_broker/app.rb b/lib/pact_broker/app.rb index a5f4072e8..dcf1698ed 100644 --- a/lib/pact_broker/app.rb +++ b/lib/pact_broker/app.rb @@ -125,8 +125,7 @@ def prepare_database end def load_configuration_from_database - require "pact_broker/config/load" - PactBroker::Config::Load.call(configuration) + configuration.load_from_database! end def configure_database_connection @@ -156,9 +155,8 @@ def seed_example_data logger.info "Seeding example data" configuration.example_data_seeder.call logger.info "Marking seed as done" - configuration.seed_example_data = false - require "pact_broker/config/save" - PactBroker::Config::Save.call(configuration, [:seed_example_data]) + require "pact_broker/config/repository" + PactBroker::Config::Repository.new.create_or_update_setting(:seed_example_data, false) else logger.info "Not seeding example data" end @@ -285,6 +283,7 @@ def running_app end def print_startup_message + configuration.log_configuration if configuration.log_configuration_on_startup unless configuration.hide_pactflow_messages logger.info "\n\n#{'*' * 80}\n\nWant someone to manage your Pact Broker for you? Check out https://pactflow.io/oss for a hardened, fully supported SaaS version of the Pact Broker with an improved UI + more.\n\n#{'*' * 80}\n" end diff --git a/lib/pact_broker/config/load.rb b/lib/pact_broker/config/load.rb index 62066ad0c..332552770 100644 --- a/lib/pact_broker/config/load.rb +++ b/lib/pact_broker/config/load.rb @@ -9,12 +9,12 @@ class Load include PactBroker::Logging - def self.call configuration - new(configuration).call + def self.call runtime_configuration + new(runtime_configuration).call end - def initialize configuration - @configuration = configuration + def initialize runtime_configuration + @runtime_configuration = runtime_configuration end def call @@ -25,18 +25,29 @@ def call private - attr_reader :configuration + attr_reader :runtime_configuration def configuration_attribute_exists? setting - configuration.respond_to?("#{setting.name}=") + runtime_configuration.respond_to?("#{setting.name}=") + end + + def unset_or_value_from_default? setting + setting_source(setting).nil? || setting_source(setting)[:type] == :defaults + end + + def setting_source(setting) + runtime_configuration.to_source_trace.dig(setting.name, :source) end def set_value_on_configuration setting - if configuration_attribute_exists? setting - logger.debug("Loading #{setting.name} configuration from database.") - configuration.send("#{setting.name}=", setting.value_object) + if configuration_attribute_exists?(setting) + if unset_or_value_from_default?(setting) + runtime_configuration.send("#{setting.name}=", setting.value_object) + else + logger.debug("Ignoring #{setting.name} configuration from database, as it has been set by another source #{setting_source(setting)}") + end else - logger.warn("Could not load configuration setting \"#{setting.name}\" as there is no matching attribute on the Configuration class") + logger.warn("Could not load configuration setting \"#{setting.name}\" as there is no matching attribute on the #{runtime_configuration.class} class") end end end diff --git a/lib/pact_broker/config/runtime_configuration.rb b/lib/pact_broker/config/runtime_configuration.rb index e4f202f42..19444b4a6 100644 --- a/lib/pact_broker/config/runtime_configuration.rb +++ b/lib/pact_broker/config/runtime_configuration.rb @@ -24,7 +24,8 @@ class RuntimeConfiguration < Anyway::Config log_level: :info, log_format: nil, warning_error_class_names: ["Sequel::ForeignKeyConstraintViolation"], - hide_pactflow_messages: false + hide_pactflow_messages: false, + log_configuration_on_startup: true ) on_load :validate_logging_attributes! @@ -65,6 +66,7 @@ class RuntimeConfiguration < Anyway::Config auto_detect_main_branch: true, main_branch_candidates: ["develop", "main", "master"], semver_formats: ["%M.%m.%p%s%d", "%M.%m", "%M"], + seed_example_data: true, features: [] ) diff --git a/lib/pact_broker/config/runtime_configuration_logging_methods.rb b/lib/pact_broker/config/runtime_configuration_logging_methods.rb index f84da990f..3e2693629 100644 --- a/lib/pact_broker/config/runtime_configuration_logging_methods.rb +++ b/lib/pact_broker/config/runtime_configuration_logging_methods.rb @@ -20,19 +20,16 @@ def sensitive_value?(value) module InstanceMethods def log_configuration(logger) - logger.info "------------------------------------------------------------------------" - logger.info "PACT BROKER CONFIGURATION:" to_source_trace.sort_by { |key, _| key }.each { |key, value| log_config_inner(key, value, logger) } - logger.info "------------------------------------------------------------------------" end def log_config_inner(key, value, logger) if !value.has_key? :value value.sort_by { |inner_key, _| inner_key }.each { |inner_key, inner_value| log_config_inner("#{key}:#{inner_key}", inner_value) } elsif self.class.sensitive_value?(key) - logger.info "#{key}=#{redact(key, value[:value])} source=[#{value[:source]}]" + logger.info "#{key}=#{redact(key, value[:value])} source=#{value[:source]}" else - logger.info "#{key}=#{value[:value]} source=[#{value[:source]}]" + logger.info "#{key}=#{value[:value]} source=#{value[:source]}" end end private :log_config_inner diff --git a/lib/pact_broker/configuration.rb b/lib/pact_broker/configuration.rb index d0b93b678..ffa1a7050 100644 --- a/lib/pact_broker/configuration.rb +++ b/lib/pact_broker/configuration.rb @@ -19,34 +19,8 @@ class Configuration delegate PactBroker::Config::RuntimeConfiguration.getter_and_setter_method_names => :runtime_configuration - SAVABLE_SETTING_NAMES = [ - :order_versions_by_date, - :use_case_sensitive_resource_names, - :enable_public_badge_access, - :shields_io_base_url, - :check_for_potential_duplicate_pacticipant_names, - :webhook_retry_schedule, - :semver_formats, - :disable_ssl_verification, - :webhook_http_method_whitelist, - :webhook_scheme_whitelist, - :webhook_host_whitelist, - :webhook_http_code_success, - :base_equality_only_on_content_that_affects_verification_results, - :seed_example_data, - :badge_provider_mode, - :warning_error_class_names, - :base_urls, - :log_dir, - :allow_missing_migration_files, - :auto_migrate_db_data, - :use_rack_protection, - :create_deployed_versions_for_tags, - :metrics_sql_statement_timeout - ] - attr_accessor :database_connection - attr_accessor :example_data_seeder, :seed_example_data + attr_accessor :example_data_seeder attr_accessor :html_pact_renderer, :version_parser, :sha_generator attr_accessor :content_security_policy, :hal_browser_content_security_policy_overrides attr_accessor :api_error_reporters @@ -75,7 +49,6 @@ def self.default_configuration config.html_pact_renderer = default_html_pact_render config.version_parser = PactBroker::Versions::ParseSemanticVersion config.sha_generator = PactBroker::Pacts::GenerateSha - config.seed_example_data = true config.example_data_seeder = lambda do require "pact_broker/db/seed_example_data" PactBroker::DB::SeedExampleData.call @@ -133,7 +106,10 @@ def logger= logger end def log_configuration + logger.info "------------------------------------------------------------------------" + logger.info "PACT BROKER CONFIGURATION:" runtime_configuration.log_configuration(logger) + logger.info "------------------------------------------------------------------------" end def self.default_html_pact_render @@ -214,16 +190,10 @@ def enable_badge_resources= enable_badge_resources self.enable_public_badge_access = enable_badge_resources end - def save_to_database - # Can't require a Sequel::Model class before the connection has been set - require "pact_broker/config/save" - PactBroker::Config::Save.call(self, SAVABLE_SETTING_NAMES) - end - def load_from_database! # Can't require a Sequel::Model class before the connection has been set require "pact_broker/config/load" - PactBroker::Config::Load.call(self) + PactBroker::Config::Load.call(runtime_configuration) end end end diff --git a/spec/lib/pact_broker/config/load_spec.rb b/spec/lib/pact_broker/config/load_spec.rb index c9d444a74..b40a1b253 100644 --- a/spec/lib/pact_broker/config/load_spec.rb +++ b/spec/lib/pact_broker/config/load_spec.rb @@ -3,11 +3,23 @@ module PactBroker module Config describe Load do - describe ".call" do - class MockConfig - attr_accessor :foo, :bar, :nana, :meep, :lalala, :meow, :peebo, :whitelist, :blah + class MockConfig < Anyway::Config + attr_config( + foo: "default", + bar: "default", + nana: "default", + meep: "default", + lalala: "default", + meow: "default", + peebo: "default", + whitelist: "default", + blah: "default", + setting_with_override: "default" + ) + attr_config(:setting_with_no_default) + config_name :foo end before do @@ -21,9 +33,14 @@ class MockConfig Setting.create(name: "unknown", type: "string", value: nil) Setting.create(name: "whitelist", type: "space_delimited_string_list", value: "foo bar") Setting.create(name: "blah", type: "symbol", value: "boop") + Setting.create(name: "setting_with_no_default", type: "string", value: "boop") + Setting.create(name: "setting_with_override", type: "string", value: "meep") + + allow(Load.logger).to receive(:warn) + allow(Load.logger).to receive(:debug) end - let(:configuration) { MockConfig.new } + let(:configuration) { MockConfig.new(setting_with_override: "overridden") } subject { Load.call(configuration) } @@ -72,9 +89,19 @@ class MockConfig expect(configuration.whitelist).to eq ["foo", "bar"] end + it "loads settings where there is no default" do + subject + expect(configuration.setting_with_no_default).to eq "boop" + end + + it "does not overwrite settings that did not come from the default class" do + expect(Load.logger).to receive(:debug).with(/Ignoring.*setting_with_override/) + subject + expect(configuration.setting_with_override).to eq "overridden" + end + it "does not load a setting where the Configuration object does not have a matching property" do - allow(Load.logger).to receive(:warn) - expect(Load.logger).to receive(:warn).with("Could not load configuration setting \"unknown\" as there is no matching attribute on the Configuration class") + expect(Load.logger).to receive(:warn).with("Could not load configuration setting \"unknown\" as there is no matching attribute on the PactBroker::Config::MockConfig class") subject end end diff --git a/spec/lib/pact_broker/config/save_and_load_spec.rb b/spec/lib/pact_broker/config/save_and_load_spec.rb deleted file mode 100644 index 47fb9dec9..000000000 --- a/spec/lib/pact_broker/config/save_and_load_spec.rb +++ /dev/null @@ -1,25 +0,0 @@ -require "pact_broker/config/load" -require "pact_broker/config/save" - -module PactBroker - module Config - describe "Save and Load" do - - let(:setting_names) { configuration_to_save.to_h.keys } - let(:configuration_to_save) do - OpenStruct.new(foo: true, bar: false, wiffle: nil, meep: [1, "2"], mop: {a: "b"}, la: 1, lala: 1.2) - end - - let(:loaded_configuration) do - OpenStruct.new(foo: nil, bar: "1", wiffle: [], meep: nil, mop: nil, la: nil, lala: nil) - end - - subject { Save.call(configuration_to_save, setting_names); Load.call(loaded_configuration) } - - it "the loaded configuration is the same as the saved one" do - subject - expect(loaded_configuration).to eq configuration_to_save - end - end - end -end diff --git a/spec/lib/pact_broker/configuration_spec.rb b/spec/lib/pact_broker/configuration_spec.rb index 8b6242d70..2b845d603 100644 --- a/spec/lib/pact_broker/configuration_spec.rb +++ b/spec/lib/pact_broker/configuration_spec.rb @@ -86,26 +86,6 @@ module PactBroker end end - describe "SETTING_NAMES" do - let(:configuration) { PactBroker::Configuration.new} - - Configuration::SAVABLE_SETTING_NAMES.each do | setting_name | - describe setting_name do - it "exists as a method on a PactBroker::Configuration instance" do - expect(configuration).to respond_to(setting_name) - end - end - end - end - - describe "save_to_database" do - let(:configuration) { PactBroker::Configuration.default_configuration } - - it "saves the configuration to the database" do - expect { configuration.save_to_database }.to change { PactBroker::Config::Setting.count }.by(Configuration::SAVABLE_SETTING_NAMES.size) - end - end - describe "load_from_database!" do let(:configuration) { PactBroker::Configuration.new }