From 676b8e57f639a654699c4a2b34b92a7a339a6266 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Thu, 11 Apr 2024 15:10:11 -0400 Subject: [PATCH 01/16] Refactor Config module to use environment variables; remove references to CONFIG_YML_PATH; remove config.example.yml --- .dockerignore | 1 + .gitignore | 1 + Rakefile | 4 +- config/config.example.yml | 62 ---------- config/example.env | 57 +++++++++ docker-compose.yml | 2 +- lib/config.rb | 241 +++++++++++++++++++++++++------------- run_dark_blue.rb | 4 +- test/setup_db.rb | 5 +- verify_aptrust.rb | 4 +- 10 files changed, 222 insertions(+), 159 deletions(-) delete mode 100644 config/config.example.yml create mode 100644 config/example.env diff --git a/.dockerignore b/.dockerignore index caacdae..bb28664 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,4 +1,5 @@ **/config.yml +**/.env* .ssh/* export/* prep/* diff --git a/.gitignore b/.gitignore index 101a88e..445a220 100644 --- a/.gitignore +++ b/.gitignore @@ -7,6 +7,7 @@ source .ssh config/config.yml +**/.env* test/bag_test_dir test/remote_test diff --git a/Rakefile b/Rakefile index c2b29d5..966da14 100644 --- a/Rakefile +++ b/Rakefile @@ -13,9 +13,7 @@ namespace :db do Sequel.extension :migration - db_config = Config::ConfigService.database_config_from_file( - ENV.fetch("CONFIG_YML_PATH", File.join(".", "config", "config.yml")) - ) + db_config = Config::ConfigService.database_config_from_env if !db_config raise DatabaseError, "Migration failed. A database connection is not configured." end diff --git a/config/config.example.yml b/config/config.example.yml deleted file mode 100644 index 78d5a71..0000000 --- a/config/config.example.yml +++ /dev/null @@ -1,62 +0,0 @@ -# Bag Courier Config Example/Template - -# Settings -# One of info, debug, error, warn, trace, fatal -LogLevel: "info" -WorkingDir: "./prep" -ExportDir: "./export" -# Determines whether the process skips sending bag(s) -DryRun: false -# Limit for the size of objects to be processed (optional) -# This is useful for development or in environments where larger files cannot be processed. -ObjectSizeLimit: 2000000000 - -# Database (optional) -# Values provided should work for local development with Docker -# While technically optional, it is required when running all tests. -Database: - Host: database - Database: dpact_pres_service - Port: 3306 - User: dpact_user - Password: dpact_pw - -DarkBlue: - ArchivematicaInstances: - # Name is used for the context in the bag identifier - - Name: - # RepositoryName is used to create a Repository database record - RepositoryName: - API: - Username: - APIKey: - BaseURL: - LocationUUID: - Remote: - Type: file_system - Settings: - FileSystemRemotePath: - # Type: sftp - # Settings: - # User: - # Host: - # KeyPath: - -# Repository -Repository: -RepositoryDescription: - -APTrust: - API: - Username: - APIKey: - BaseURL: - Remote: - Type: aptrust - Settings: - # APTrust AWS remote settings - ReceivingBucket: - RestoreBucket: - BucketRegion: - AwsAccessKeyId: - AwsSecretAccessKey: diff --git a/config/example.env b/config/example.env new file mode 100644 index 0000000..402b8ee --- /dev/null +++ b/config/example.env @@ -0,0 +1,57 @@ +# Bag Courier Environment Variables Example/Template + +# Settings +# One of info, debug, error, warn, trace, fatal +SETTINGS_LOG_LEVEL=debug +SETTINGS_WORKING_DIR=./prep +SETTINGS_EXPORT_DIR=./export +# Determines whether the process skips sending bag(s) +SETTINGS_DRY_RUN=false +# Limit for the size (in bytes) of objects to be processed (optional) +# This is useful for development or in environments where larger files cannot be processed. +SETTINGS_OBJECT_SIZE_LIMIT= + +# Repository +# Name that will be used for the repository at the beginning of bag identifiers +REPOSITORY_NAME= +# Generic description that will be shared among all items in a repository +REPOSITORY_DESCRIPTION= + +# Database (optional) +# Values provided should work for local development with Docker +# While technically optional, it is required when running all tests. +DATABASE_HOST=database +DATABASE_DATABASE=dpact_pres_service +DATABASE_PORT=3306 +DATABASE_USER=dpact_user +DATABASE_PASSWORD=dpact_pw + +# Archivematica instance(s) +# Settings for each Archivematica instance should have the same stem, +# one of ARCHIVEMATICA_DEV, ARCHIVEMATICA_AMI, ARCHIVEMATICA_LAB, or ARCHIVEMATICA_VGA +ARCHIVEMATICA_DEV_NAME= +ARCHIVEMATICA_DEV_REPOSITORY_NAME= +# REST API +ARCHIVEMATICA_DEV_API_USERNAME= +ARCHIVEMATICA_DEV_API_API_KEY= +ARCHIVEMATICA_DEV_BASE_URL= +# Remote +ARCHIVEMATICA_DEV_REMOTE_TYPE=file_system +ARCHIVEMATICA_DEV_REMOTE_SETTINGS_FILE_SYSTEM_REMOTE_PATH= +# ARCHIVEMATICA_DEV_REMOTE_TYPE=sftp +# ARCHIVEMATICA_DEV_REMOTE_SETTINGS_USER= +# ARCHIVEMATICA_DEV_REMOTE_SETTINGS_HOST= +# ARCHIVEMATICA_DEV_REMOTE_SETTINGS_KEY_PATH= + +# APTrust +# REST API settings +APTRUST_API_USERNAME= +APTRUST_API_API_KEY= +APTRUST_API_BASE_URL= +# Remote settings +APTRUST_REMOTE_TYPE=aptrust +APTRUST_REMOTE_SETTINGS_RECEIVING_BUCKET= +APTRUST_REMOTE_SETTINGS_RESTORE_BUCKET= +APTRUST_REMOTE_SETTINGS_BUCKET_REGION= +APTRUST_REMOTE_SETTINGS_AWS_ACCESS_KEY_ID= +APTRUST_REMOTE_SETTINGS_AWS_SECRET_ACCESS_KEY= diff --git a/docker-compose.yml b/docker-compose.yml index 7e6df84..173ad49 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -12,7 +12,7 @@ services: target: /run/host-services/ssh-auth.sock environment: - SSH_AUTH_SOCK=/run/host-services/ssh-auth.sock - - CONFIG_YML_PATH=./config/config.tests.yml + env_file: config/.env depends_on: database: condition: "service_healthy" diff --git a/lib/config.rb b/lib/config.rb index e3fd8c1..672d25a 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -1,5 +1,3 @@ -require "yaml" - require "semantic_logger" module Config @@ -113,139 +111,216 @@ module Config class ConfigError < StandardError end - class ConfigService - include SemanticLogger::Loggable - - def self.raise_error(key, value) + module ErrorRaising + def raise_error(key:, value:) raise ConfigError, "Value for \"#{key}\" is not valid: #{value}" end + end - def self.verify_string(key, value) - if !value.is_a?(String) - raise_error(key, value) - end + class CheckBase + def verify(key:, value:) + raise NotImplementedError + end + end + + class NotNilCheck < CheckBase + include ErrorRaising + def verify(key:, value:) + raise_error(key: key, value: value) if value.nil? value end + end - def self.verify_int(key, value) - if !value.is_a?(Integer) - raise_error(key, value) - end + # class StringCheck < CheckBase + # include ErrorRaising + # def verify(key, value) + # raise_error(key: key, value: value) if !value.is_a?(String) + # value + # end + # end + + class IntegerCheck < CheckBase + include ErrorRaising + def verify(key:, value:) + raise_error(key: key, value: value) if !Integer(value, exception: false).is_a?(Integer) value end + end - def self.verify_boolean(key, value) - if ![true, false].include?(value) - raise_error(key, value) - end + class BooleanCheck < CheckBase + include ErrorRaising + def verify(key:, value:) + raise_error(key: key, value: value) if !["true", "false"].include?(value) value end + end + + class LogLevelCheck < CheckBase + include ErrorRaising + LOG_LEVELS = ["info", "debug", "trace", "warn", "error", "fatal"] - def self.read_data_from_file(yaml_path) - logger.debug("yaml_path=#{yaml_path}") - YAML.safe_load_file(yaml_path) + def verify(key:, value:) + raise_error(key: key, value: value) if !LOG_LEVELS.include?(value) + value end + end - # TO DO - # Support config from environment? + class RemoteTypeCheck < CheckBase + include ErrorRaising + LOG_LEVELS = ["file_system", "aptrust", "sftp"] + + def verify(key:, value:) + raise_error(key: key, value: value) if !LOG_LEVELS.include?(value) + value + end + end + + class CheckableData + attr_reader :data + + def initialize(data) + @data = data + end + + def inspect + @data.to_s + end + + def to_s + @data.to_s + end + + def get_value( + key:, + checks: [], + optional: false + ) + value = data.fetch(key) + NotNilCheck.new.verify(key: key, value: value) if !optional + checks.each { |check| check.new.verify(key: key, value: value) } + value + end + + def keys + @data.keys + end + + def get_subset_by_key_stem(stem) + matching_keys = data.keys.filter { |k| k.start_with?(stem) } + filtered_data = data.slice(*matching_keys) + new_data = {} + filtered_data.each_pair do |key, value| + new_key = key.sub(stem, "") + new_data[new_key] = value + end + CheckableData.new(new_data) + end + end + + class ConfigService + include SemanticLogger::Loggable + attr_reader :data + + ARCHIVEMATICA_INSTANCES = ["ARCHIVEMATICA_DEV", "ARCHIVEMATICA_LAB", "ARCHIVEMATICA_AMI", "ARCHIVEMATICA_VGA"] + + def self.create_database_config(data) + DatabaseConfig.new( + host: data.get_value(key: "HOST"), + database: data.get_value(key: "DATABASE"), + port: data.get_value(key: "PORT", checks=[IntegerCheck]).to_i, + user: data.get_value(key: "USER"), + password: data.get_value(key: "PASSWORD") + ) + end def self.create_remote_config(data) - type = verify_string("Type", data["Type"]).to_sym - settings = data["Settings"] + type = data.get_value(key: "TYPE", checks: [RemoteTypeCheck]).to_sym + settings = data.get_subset_by_key_stem("SETTINGS_") RemoteConfig.new( type: type, settings: ( case type when :aptrust AptrustAwsRemoteConfig.new( - region: verify_string("BucketRegion", settings["BucketRegion"]), - receiving_bucket: verify_string("ReceivingBucket", settings["ReceivingBucket"]), - restore_bucket: verify_string("RestoreBucket", settings["RestoreBucket"]), - access_key_id: verify_string("AwsAccessKeyId", settings["AwsAccessKeyId"]), - secret_access_key: verify_string("AwsSecretAccessKey", settings["AwsSecretAccessKey"]) + region: settings.get_value(key: "BUCKET_REGION"), + receiving_bucket: settings.get_value(key: "RECEIVING_BUCKET"), + restore_bucket: settings.get_value(key: "RESTORE_BUCKET"), + access_key_id: settings.get_value(key: "AWS_ACCESS_KEY_ID"), + secret_access_key: settings.get_value(key: "AWS_SECRET_ACCESS_KEY") ) when :file_system FileSystemRemoteConfig.new( - remote_path: verify_string("FileSystemRemotePath", settings["FileSystemRemotePath"]) + remote_path: settings.get_value("FILE_SYSTEM_REMOTE_PATH") ) when :sftp SftpRemoteConfig.new( - user: verify_string("User", settings["User"]), - host: verify_string("Host", settings["Host"]), - key_path: verify_string("KeyPath", settings["KeyPath"]) + user: settings.get_value(key: "USER"), + host: settings.get_value(key: "HOST"), + key_path: settings.get_value(key: "KEY_PATH") ) else - raise ConfigError, "Remote type #{type} is not supported" + raise ConfigError, "Remote type #{type} is not supported." end ) ) end - def self.create_database_config(data) - DatabaseConfig.new( - host: verify_string("Host", data["Host"]), - database: verify_string("Database", data["Database"]), - port: verify_int("Port", data["Port"]), - user: verify_string("User", data["User"]), - password: verify_string("Password", data["Password"]) + def self.create_archivematica_config(data) + ArchivematicaConfig.new( + name: data.get_value(key: "NAME"), + repository_name: data.get_value(key: "REPOSITORY_NAME"), + api: ArchivematicaAPIConfig.new( + base_url: data.get_value(key: "API_BASE_URL"), + username: data.get_value(key: "API_USERNAME"), + api_key: data.get_value(key: "API_API_KEY"), + location_uuid: data.get_value(key: "API_LOCATION_UUID") + ), + remote: create_remote_config(data.get_subset_by_key_stem("REMOTE_")) ) end def self.create_config(data) - logger.debug(data) - db_data = data.fetch("Database", nil) - aptrust_data = data["APTrust"] - Config.new( + data = CheckableData.new(data) + db_data = data.get_subset_by_key_stem("DATABASE_") + + arch_configs = [] + ARCHIVEMATICA_INSTANCES.each do |instance_name| + instance_data = data.get_subset_by_key_stem(instance_name + "_") + arch_configs << create_archivematica_config(instance_data) if instance_data.keys.length > 0 + end + + config = Config.new( settings: SettingsConfig.new( - log_level: verify_string("LogLevel", data["LogLevel"]).to_sym, - working_dir: verify_string("WorkingDir", data["WorkingDir"]), - export_dir: verify_string("ExportDir", data["ExportDir"]), - dry_run: verify_boolean("DryRun", data["DryRun"]), - object_size_limit: data["ObjectSizeLimit"] && verify_int("ObjectSizeLimit", data["ObjectSizeLimit"]) + log_level: data.get_value(key: "SETTINGS_LOG_LEVEL", checks: [LogLevelCheck]).to_sym, + working_dir: data.get_value(key: "SETTINGS_WORKING_DIR"), + export_dir: data.get_value(key: "SETTINGS_EXPORT_DIR"), + dry_run: data.get_value(key: "SETTINGS_DRY_RUN", checks: [BooleanCheck]) == "true", + object_size_limit: data.get_value(key: "SETTINGS_OBJECT_SIZE_LIMIT", checks: [IntegerCheck]).to_i ), repository: RepositoryConfig.new( - name: verify_string("Repository", data["Repository"]), - description: verify_string("RepositoryDescription", data["RepositoryDescription"]) - ), - database: db_data && create_database_config(db_data), - dark_blue: DarkBlueConfig.new( - archivematicas: ( - data["DarkBlue"]["ArchivematicaInstances"].map do |arch_data| - api_data = arch_data["API"] - remote_data = arch_data["Remote"] - ArchivematicaConfig.new( - name: verify_string("Name", arch_data["Name"]), - repository_name: verify_string("RepositoryName", arch_data["RepositoryName"]), - api: ArchivematicaAPIConfig.new( - base_url: verify_string("BaseURL", api_data["BaseURL"]), - username: verify_string("Username", api_data["Username"]), - api_key: verify_string("APIKey", api_data["APIKey"]), - location_uuid: verify_string("LocationUUID", api_data["LocationUUID"]) - ), - remote: create_remote_config(remote_data) - ) - end - ) + name: data.get_value(key: "REPOSITORY_NAME"), + description: data.get_value(key: "REPOSITORY_DESCRIPTION") ), + database: db_data.keys.length > 0 ? create_database_config(db_data) : nil, + dark_blue: DarkBlueConfig.new(archivematicas: arch_configs), aptrust: APTrustConfig.new( api: APTrustAPIConfig.new( - username: verify_string("Username", aptrust_data["API"]["Username"]), - api_key: verify_string("APIKey", aptrust_data["API"]["APIKey"]), - base_url: verify_string("BaseURL", aptrust_data["API"]["BaseURL"]) + username: data.get_value(key: "APTRUST_API_USERNAME"), + api_key: data.get_value(key: "APTRUST_API_API_KEY"), + base_url: data.get_value(key: "APTRUST_API_BASE_URL") ), - remote: create_remote_config(aptrust_data["Remote"]) + remote: create_remote_config(data.get_subset_by_key_stem("APTRUST_REMOTE_")) ) ) end - def self.database_config_from_file(yaml_path) - data = read_data_from_file(yaml_path) - db_data = data.fetch("Database", nil) - db_data && create_database_config(db_data) + def self.database_config_from_env + db_data = CheckableData.new(ENV.to_hash).get_subset_by_key_stem("DATABASE_") + db_data.keys.length > 0 ? create_database_config(db_data) : nil end - def self.from_file(yaml_path) - create_config(read_data_from_file(yaml_path)) + def self.from_env + create_config(ENV.to_hash) end end end diff --git a/run_dark_blue.rb b/run_dark_blue.rb index 6de84c2..f54fe24 100644 --- a/run_dark_blue.rb +++ b/run_dark_blue.rb @@ -6,9 +6,7 @@ require_relative "lib/config" SemanticLogger.add_appender(io: $stderr, formatter: :color) -config = Config::ConfigService.from_file( - ENV.fetch("CONFIG_YML_PATH", File.join(".", "config", "config.yml")) -) +config = Config::ConfigService.from_env SemanticLogger.default_level = config.settings.log_level DB = config.database && Sequel.connect( diff --git a/test/setup_db.rb b/test/setup_db.rb index d23b60c..c568419 100644 --- a/test/setup_db.rb +++ b/test/setup_db.rb @@ -6,10 +6,7 @@ Sequel.extension :migration -config_yml_path = ENV.fetch("CONFIG_YML_PATH", File.join(".", "config", "config.yml")) -db_config = Config::ConfigService.database_config_from_file( - File.join(__dir__, "..", config_yml_path) -) +db_config = Config::ConfigService.database_config_from_env if !db_config message = "A database connection is not configured. This is required for some tests." diff --git a/verify_aptrust.rb b/verify_aptrust.rb index 5fd8a37..eb92072 100644 --- a/verify_aptrust.rb +++ b/verify_aptrust.rb @@ -4,9 +4,7 @@ require_relative "lib/config" SemanticLogger.add_appender(io: $stderr, formatter: :color) -config = Config::ConfigService.from_file( - ENV.fetch("CONFIG_YML_PATH", File.join(".", "config", "config.yml")) -) +config = Config::ConfigService.from_env SemanticLogger.default_level = config.settings.log_level DB = config.database && Sequel.connect( From e7546b9963f6b5c2607cb0993d4a935f4ea0eb96 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Thu, 11 Apr 2024 15:12:24 -0400 Subject: [PATCH 02/16] Change test GitHub Action to use environment variables --- .github/workflows/tests.yml | 6 +++++- config/config.tests.yml | 8 -------- 2 files changed, 5 insertions(+), 9 deletions(-) delete mode 100644 config/config.tests.yml diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 1eef084..34586c7 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -37,4 +37,8 @@ jobs: - name: Run tests run: bundle exec rake test env: - CONFIG_YML_PATH: "./config/config.tests.yml" + DATABASE_HOST: 127.0.0.1 + DATABASE_DATABASE: test_database + DATABASE_PORT: 3306 + DATABASE_USER: user + DATABASE_PASSWORD: password diff --git a/config/config.tests.yml b/config/config.tests.yml deleted file mode 100644 index c2dd488..0000000 --- a/config/config.tests.yml +++ /dev/null @@ -1,8 +0,0 @@ -# Configuration for GitHub Action defined in .github/workflows/tests.yaml - -Database: - Host: 127.0.0.1 - Database: test_database - Port: 3306 - User: user - Password: password From 9f45920b2749460254108d73261704f55c36e348 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Thu, 11 Apr 2024 15:21:06 -0400 Subject: [PATCH 03/16] Fix syntax error; fix optionality; make object size limit optional --- lib/config.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/config.rb b/lib/config.rb index 672d25a..4d64ac8 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -190,13 +190,11 @@ def to_s @data.to_s end - def get_value( - key:, - checks: [], - optional: false - ) - value = data.fetch(key) - NotNilCheck.new.verify(key: key, value: value) if !optional + def get_value(key:, checks: [], optional: false) + value = data.fetch(key, nil) + return value if optional && value.nil? + + NotNilCheck.new.verify(key: key, value: value) checks.each { |check| check.new.verify(key: key, value: value) } value end @@ -227,7 +225,7 @@ def self.create_database_config(data) DatabaseConfig.new( host: data.get_value(key: "HOST"), database: data.get_value(key: "DATABASE"), - port: data.get_value(key: "PORT", checks=[IntegerCheck]).to_i, + port: data.get_value(key: "PORT", checks: [IntegerCheck]).to_i, user: data.get_value(key: "USER"), password: data.get_value(key: "PASSWORD") ) @@ -295,7 +293,9 @@ def self.create_config(data) working_dir: data.get_value(key: "SETTINGS_WORKING_DIR"), export_dir: data.get_value(key: "SETTINGS_EXPORT_DIR"), dry_run: data.get_value(key: "SETTINGS_DRY_RUN", checks: [BooleanCheck]) == "true", - object_size_limit: data.get_value(key: "SETTINGS_OBJECT_SIZE_LIMIT", checks: [IntegerCheck]).to_i + object_size_limit: data.get_value( + key: "SETTINGS_OBJECT_SIZE_LIMIT", checks: [IntegerCheck], optional: true + ).to_i ), repository: RepositoryConfig.new( name: data.get_value(key: "REPOSITORY_NAME"), From 40399b0eda7f299f6c67c215fb6882936ce99359 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Thu, 11 Apr 2024 15:39:26 -0400 Subject: [PATCH 04/16] Fix remote types constant name --- lib/config.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/config.rb b/lib/config.rb index 4d64ac8..3a3bf13 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -167,10 +167,10 @@ def verify(key:, value:) class RemoteTypeCheck < CheckBase include ErrorRaising - LOG_LEVELS = ["file_system", "aptrust", "sftp"] + REMOTE_TYPES = ["file_system", "aptrust", "sftp"] def verify(key:, value:) - raise_error(key: key, value: value) if !LOG_LEVELS.include?(value) + raise_error(key: key, value: value) if !REMOTE_TYPES.include?(value) value end end From d1af57bbfcbac4665751ed8d738a28ae87c7abaf Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Fri, 12 Apr 2024 11:10:14 -0400 Subject: [PATCH 05/16] Make checks return booleans; move error raising to CheckableData; make config responsible for newing checks --- lib/config.rb | 66 ++++++++++++++++++++++----------------------------- 1 file changed, 28 insertions(+), 38 deletions(-) diff --git a/lib/config.rb b/lib/config.rb index 3a3bf13..5cb6d24 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -111,67 +111,49 @@ module Config class ConfigError < StandardError end - module ErrorRaising - def raise_error(key:, value:) - raise ConfigError, "Value for \"#{key}\" is not valid: #{value}" - end - end - class CheckBase - def verify(key:, value:) + def check?(value) raise NotImplementedError end end class NotNilCheck < CheckBase - include ErrorRaising - def verify(key:, value:) - raise_error(key: key, value: value) if value.nil? - value + def check?(value) + value.nil? end end # class StringCheck < CheckBase - # include ErrorRaising - # def verify(key, value) - # raise_error(key: key, value: value) if !value.is_a?(String) - # value + # def check?(value) + # value.is_a?(String) # end # end class IntegerCheck < CheckBase - include ErrorRaising - def verify(key:, value:) - raise_error(key: key, value: value) if !Integer(value, exception: false).is_a?(Integer) - value + def check?(value) + Integer(value, exception: false).is_a?(Integer) end end class BooleanCheck < CheckBase - include ErrorRaising - def verify(key:, value:) - raise_error(key: key, value: value) if !["true", "false"].include?(value) - value + def check?(value) + ["true", "false"].include?(value) end end class LogLevelCheck < CheckBase - include ErrorRaising LOG_LEVELS = ["info", "debug", "trace", "warn", "error", "fatal"] - def verify(key:, value:) - raise_error(key: key, value: value) if !LOG_LEVELS.include?(value) - value + def check?(value) + LOG_LEVELS.include?(value) end end class RemoteTypeCheck < CheckBase - include ErrorRaising REMOTE_TYPES = ["file_system", "aptrust", "sftp"] - def verify(key:, value:) - raise_error(key: key, value: value) if !REMOTE_TYPES.include?(value) - value + def check?(value) + REMOTE_TYPES.include?(value) end end @@ -190,12 +172,20 @@ def to_s @data.to_s end + def raise_error(key:, value:, reason: nil) + raise ConfigError, "Value \"#{value}\" for key \"#{key}\" is not valid. #{reason}" + end + private :raise_error + def get_value(key:, checks: [], optional: false) value = data.fetch(key, nil) return value if optional && value.nil? - NotNilCheck.new.verify(key: key, value: value) - checks.each { |check| check.new.verify(key: key, value: value) } + NotNilCheck.new.check?(value) + checks.each do |check| + result = check.check?(value) + raise_error(key: key, value: value, reason: "#{check.class} failed.") if !result + end value end @@ -225,14 +215,14 @@ def self.create_database_config(data) DatabaseConfig.new( host: data.get_value(key: "HOST"), database: data.get_value(key: "DATABASE"), - port: data.get_value(key: "PORT", checks: [IntegerCheck]).to_i, + port: data.get_value(key: "PORT", checks: [IntegerCheck.new]).to_i, user: data.get_value(key: "USER"), password: data.get_value(key: "PASSWORD") ) end def self.create_remote_config(data) - type = data.get_value(key: "TYPE", checks: [RemoteTypeCheck]).to_sym + type = data.get_value(key: "TYPE", checks: [RemoteTypeCheck.new]).to_sym settings = data.get_subset_by_key_stem("SETTINGS_") RemoteConfig.new( type: type, @@ -289,12 +279,12 @@ def self.create_config(data) config = Config.new( settings: SettingsConfig.new( - log_level: data.get_value(key: "SETTINGS_LOG_LEVEL", checks: [LogLevelCheck]).to_sym, + log_level: data.get_value(key: "SETTINGS_LOG_LEVEL", checks: [LogLevelCheck.new]).to_sym, working_dir: data.get_value(key: "SETTINGS_WORKING_DIR"), export_dir: data.get_value(key: "SETTINGS_EXPORT_DIR"), - dry_run: data.get_value(key: "SETTINGS_DRY_RUN", checks: [BooleanCheck]) == "true", + dry_run: data.get_value(key: "SETTINGS_DRY_RUN", checks: [BooleanCheck.new]) == "true", object_size_limit: data.get_value( - key: "SETTINGS_OBJECT_SIZE_LIMIT", checks: [IntegerCheck], optional: true + key: "SETTINGS_OBJECT_SIZE_LIMIT", checks: [IntegerCheck.new], optional: true ).to_i ), repository: RepositoryConfig.new( From 86e5a0fd4164a55e175453d39ea96fbb42542fa6 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Fri, 12 Apr 2024 14:31:50 -0400 Subject: [PATCH 06/16] Move Check-related code up; fix NotNilCheck and usage; add a few tests --- lib/config.rb | 195 ++++++++++++++++++++++---------------------- test/test_config.rb | 32 ++++++++ 2 files changed, 130 insertions(+), 97 deletions(-) create mode 100644 test/test_config.rb diff --git a/lib/config.rb b/lib/config.rb index 5cb6d24..cb07660 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -1,6 +1,104 @@ require "semantic_logger" module Config + class ConfigError < StandardError + end + + class CheckBase + def check?(value) + raise NotImplementedError + end + end + + class NotNilCheck < CheckBase + def check?(value) + !value.nil? + end + end + + # class StringCheck < CheckBase + # def check?(value) + # value.is_a?(String) + # end + # end + + class IntegerCheck < CheckBase + def check?(value) + Integer(value, exception: false).is_a?(Integer) + end + end + + class BooleanCheck < CheckBase + def check?(value) + ["true", "false"].include?(value) + end + end + + class LogLevelCheck < CheckBase + LOG_LEVELS = ["info", "debug", "trace", "warn", "error", "fatal"] + + def check?(value) + LOG_LEVELS.include?(value) + end + end + + class RemoteTypeCheck < CheckBase + REMOTE_TYPES = ["file_system", "aptrust", "sftp"] + + def check?(value) + REMOTE_TYPES.include?(value) + end + end + + class CheckableData + attr_reader :data + + def initialize(data) + @data = data + end + + def inspect + @data.to_s + end + + def to_s + @data.to_s + end + + def keys + @data.keys + end + + def raise_error(key:, value:, reason: nil) + raise ConfigError, "Value \"#{value}\" for key \"#{key}\" is not valid. #{reason}" + end + private :raise_error + + def get_value(key:, checks: [], optional: false) + value = data.fetch(key, nil) + return value if optional && value.nil? + + [NotNilCheck.new, *checks].each do |check| + if !check.check?(value) + reason = "#{check.class.name.split("::").last} failed." + raise_error(key: key, value: value, reason: reason) + end + end + value + end + + def get_subset_by_key_stem(stem) + matching_keys = data.keys.filter { |k| k.start_with?(stem) } + filtered_data = data.slice(*matching_keys) + new_data = {} + filtered_data.each_pair do |key, value| + new_key = key.sub(stem, "") + new_data[new_key] = value + end + CheckableData.new(new_data) + end + end + SettingsConfig = Struct.new( "SettingsConfig", :log_level, @@ -108,103 +206,6 @@ module Config keyword_init: true ) - class ConfigError < StandardError - end - - class CheckBase - def check?(value) - raise NotImplementedError - end - end - - class NotNilCheck < CheckBase - def check?(value) - value.nil? - end - end - - # class StringCheck < CheckBase - # def check?(value) - # value.is_a?(String) - # end - # end - - class IntegerCheck < CheckBase - def check?(value) - Integer(value, exception: false).is_a?(Integer) - end - end - - class BooleanCheck < CheckBase - def check?(value) - ["true", "false"].include?(value) - end - end - - class LogLevelCheck < CheckBase - LOG_LEVELS = ["info", "debug", "trace", "warn", "error", "fatal"] - - def check?(value) - LOG_LEVELS.include?(value) - end - end - - class RemoteTypeCheck < CheckBase - REMOTE_TYPES = ["file_system", "aptrust", "sftp"] - - def check?(value) - REMOTE_TYPES.include?(value) - end - end - - class CheckableData - attr_reader :data - - def initialize(data) - @data = data - end - - def inspect - @data.to_s - end - - def to_s - @data.to_s - end - - def raise_error(key:, value:, reason: nil) - raise ConfigError, "Value \"#{value}\" for key \"#{key}\" is not valid. #{reason}" - end - private :raise_error - - def get_value(key:, checks: [], optional: false) - value = data.fetch(key, nil) - return value if optional && value.nil? - - NotNilCheck.new.check?(value) - checks.each do |check| - result = check.check?(value) - raise_error(key: key, value: value, reason: "#{check.class} failed.") if !result - end - value - end - - def keys - @data.keys - end - - def get_subset_by_key_stem(stem) - matching_keys = data.keys.filter { |k| k.start_with?(stem) } - filtered_data = data.slice(*matching_keys) - new_data = {} - filtered_data.each_pair do |key, value| - new_key = key.sub(stem, "") - new_data[new_key] = value - end - CheckableData.new(new_data) - end - end - class ConfigService include SemanticLogger::Loggable attr_reader :data diff --git a/test/test_config.rb b/test/test_config.rb new file mode 100644 index 0000000..eb06bfb --- /dev/null +++ b/test/test_config.rb @@ -0,0 +1,32 @@ +require "minitest/autorun" +require "minitest/pride" + +require_relative "../lib/config" + +class CheckableDataTest < Minitest::Test + include Config + + def test_get_value_when_it_exists + data = CheckableData.new({"some_key" => "some_value"}) + assert_equal "some_value", data.get_value(key: "some_key") + end + + def test_get_value_when_key_does_not_exist + data = CheckableData.new({}) + expected_message = "Value \"\" for key \"some_key\" is not valid. NotNilCheck failed." + error = assert_raises(ConfigError) { data.get_value(key: "some_key") } + assert_equal expected_message, error.message + end + + def test_get_value_with_integer_string + data = CheckableData.new({"some_key" => "111"}) + assert_equal "111", data.get_value(key: "some_key", checks: [IntegerCheck.new]) + end + + def test_get_value_with_invalid_integer_string + data = CheckableData.new({"some_key" => "11a1"}) + expected_message = "Value \"11a1\" for key \"some_key\" is not valid. IntegerCheck failed." + error = assert_raises(ConfigError) { data.get_value(key: "some_key", checks: [IntegerCheck.new]) } + assert_equal expected_message, error.message + end +end From c4e4dd7fadc7d8c8df72a8704f84f5b0606078cf Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Fri, 12 Apr 2024 15:28:53 -0400 Subject: [PATCH 07/16] Add a few more tests --- test/test_config.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/test_config.rb b/test/test_config.rb index eb06bfb..b596d79 100644 --- a/test/test_config.rb +++ b/test/test_config.rb @@ -18,6 +18,12 @@ def test_get_value_when_key_does_not_exist assert_equal expected_message, error.message end + def test_get_value_when_key_does_not_exist_but_optional + data = CheckableData.new({}) + error = assert_raises(ConfigError) { data.get_value(key: "some_key") } + assert_nil data.get_value(key: "some_key", optional: true) + end + def test_get_value_with_integer_string data = CheckableData.new({"some_key" => "111"}) assert_equal "111", data.get_value(key: "some_key", checks: [IntegerCheck.new]) @@ -29,4 +35,16 @@ def test_get_value_with_invalid_integer_string error = assert_raises(ConfigError) { data.get_value(key: "some_key", checks: [IntegerCheck.new]) } assert_equal expected_message, error.message end + + def test_get_value_with_valid_boolean_string + data = CheckableData.new({"some_key" => "true"}) + assert_equal "true", data.get_value(key: "some_key", checks: [BooleanCheck.new]) + end + + def test_get_value_with_invalid_boolean_string + data = CheckableData.new({"some_key" => "YES!!"}) + expected_message = "Value \"YES!!\" for key \"some_key\" is not valid. BooleanCheck failed." + error = assert_raises(ConfigError) { data.get_value(key: "some_key", checks: [BooleanCheck.new]) } + assert_equal expected_message, error.message + end end From b21a966abb3b45a1ae0f4f23bb8133695ade1910 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Fri, 12 Apr 2024 15:40:32 -0400 Subject: [PATCH 08/16] Add tests for get_checks_by_key_stem --- test/test_config.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/test_config.rb b/test/test_config.rb index b596d79..71c6bba 100644 --- a/test/test_config.rb +++ b/test/test_config.rb @@ -47,4 +47,20 @@ def test_get_value_with_invalid_boolean_string error = assert_raises(ConfigError) { data.get_value(key: "some_key", checks: [BooleanCheck.new]) } assert_equal expected_message, error.message end + + def test_get_subset_by_key_stem + input = {"namespace_a" => "1", "namespace_b" => "2", "namespace_c" => "3"} + data = CheckableData.new(input) + data_subset = data.get_subset_by_key_stem("namespace_") + assert data_subset.is_a?(CheckableData) + expected = {"a" => "1", "b" => "2", "c" => "3"} + assert_equal expected, data_subset.data + end + + def test_get_subset_by_key_stem_with_no_matches + input = {"A_blah" => 1, "B_blah" => 2} + data = CheckableData.new(input) + data_subset = data.get_subset_by_key_stem("C_") + assert_equal Hash.new, data_subset.data + end end From 7eaaf8911bd906dfba3dcd002053cacab8bb53ba Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Fri, 12 Apr 2024 17:01:21 -0400 Subject: [PATCH 09/16] Adding dotenv in development group; tweak Dockerfile to leave dotenv out; re-organize *ignore files; move expected location of .env file, move example.env to root --- .dockerignore | 3 +++ .gitignore | 11 ++++++++--- Dockerfile | 6 ++---- Gemfile | 4 ++++ Gemfile.lock | 2 ++ docker-compose.yml | 2 +- config/example.env => example.env | 0 7 files changed, 20 insertions(+), 8 deletions(-) rename config/example.env => example.env (100%) diff --git a/.dockerignore b/.dockerignore index bb28664..40c6fe6 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,5 +1,8 @@ +# Configuration **/config.yml **/.env* .ssh/* + +# Data directories export/* prep/* diff --git a/.gitignore b/.gitignore index 445a220..a2dc1a0 100644 --- a/.gitignore +++ b/.gitignore @@ -1,15 +1,20 @@ +# Data directories export prep restore source -.DS_Store +# Configuration .ssh - -config/config.yml **/.env* +# Former configuration file format; leaving here to prevent accidental inclusion +**/config.yml +# Testing directories test/bag_test_dir test/remote_test test/bag_courier_test test/test_bag_val_dir + +# Misc. +.DS_Store diff --git a/Dockerfile b/Dockerfile index 77fe54e..420b3d9 100644 --- a/Dockerfile +++ b/Dockerfile @@ -22,15 +22,13 @@ COPY . /app FROM base AS development -RUN bundle install +RUN bundle config set --local without development && bundle install CMD ["tail", "-f", "/dev/null"] FROM base AS production -RUN bundle config set --local without test - -RUN bundle install +RUN bundle config set --local without development test && bundle install RUN groupadd -g ${GID} -o ${UNAME} RUN useradd -m -d /app -u ${UID} -g ${GID} -o -s /bin/bash ${UNAME} diff --git a/Gemfile b/Gemfile index eaa8fb1..eafadfb 100644 --- a/Gemfile +++ b/Gemfile @@ -14,6 +14,10 @@ gem "sftp", git: "https://github.com/mlibrary/sftp", tag: "sftp/v0.4.2" +group :development do + gem "dotenv", "~> 3.1.0" +end + group :test do gem "minitest", "~> 5.20" gem "rake", "~> 13.1" diff --git a/Gemfile.lock b/Gemfile.lock index 7dbac71..c2c8338 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -32,6 +32,7 @@ GEM bigdecimal (3.1.6) concurrent-ruby (1.2.3) docopt (0.5.0) + dotenv (3.1.0) faraday (2.9.0) faraday-net_http (>= 2.0, < 3.2) faraday-net_http (3.1.0) @@ -61,6 +62,7 @@ PLATFORMS DEPENDENCIES aws-sdk-s3 (~> 1.136) bagit (~> 0.4.6) + dotenv (~> 3.1.0) faraday (~> 2.9) faraday-retry (~> 2.2) minitar (~> 0.9) diff --git a/docker-compose.yml b/docker-compose.yml index 173ad49..73d0fb3 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -12,7 +12,7 @@ services: target: /run/host-services/ssh-auth.sock environment: - SSH_AUTH_SOCK=/run/host-services/ssh-auth.sock - env_file: config/.env + env_file: .env depends_on: database: condition: "service_healthy" diff --git a/config/example.env b/example.env similarity index 100% rename from config/example.env rename to example.env From c237008c14ca93c87bedfe345801790cc7c7ba92 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Fri, 12 Apr 2024 17:23:09 -0400 Subject: [PATCH 10/16] Update README.md to describe changes; remove verify_aptrust.rb from the Ruby section --- README.md | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 20f0fca..3d7e734 100644 --- a/README.md +++ b/README.md @@ -24,11 +24,11 @@ may be IP address restricted, so ensure you have access to the proper network or In all cases, you will need to set up a configuration file, which you can do by following these steps: -1. Create a configuration YAML file based on the provided template. +1. Create a `.env` file based on the provided template, [`example.env`](/example.env). ```sh - cp config/config.example.yml config/config.yml + cp example.env .env ``` -2. Open `config/config.yml` using your preferred text editor, and add values, +2. Open `.env` using your preferred text editor, and add values, using the comments to guide you. For local development, you may need to set up an SSH key and add it to your `ssh-agent` @@ -46,7 +46,7 @@ The recommended approach is to use the `database` service (defined in [`docker-compose.yml`](/docker-compose.yml) with the Docker approach described below. However, it is possible -- though not explicitly supported -- to use a MariaDB or MySQL database from a server running on your local machine or elsewhere. -The following sections will assume you are not using a database with Ruby alone. +The following sections will assume you are not using a database with Ruby outside a container. ### Ruby @@ -59,14 +59,15 @@ bundle install #### Usage -The primary job or process is `run_dark_blue.rb`. -```sh -ruby run_dark_blue.rb -``` +*Note*: The application expects to find necessary configuration in environment variables. +To assist with local development, it includes the [`dotenv`](https://github.com/bkeepers/dotenv) gem, +which will populate the Ruby global `ENV` with key-value pairs from the prepared `.env` file. +When using only Ruby to execute a file, if configuration is needed, +precede your `ruby script.rb` with `dotenv`, as shown below. -The latest deposits to APTrust can be verified using `verify_aptrust.rb`. +The primary job or process is `run_dark_blue.rb`. ```sh -ruby verify_aptrust.rb +dotenv ruby run_dark_blue.rb ``` If you're working on a new job or just want to try out the classes, @@ -74,13 +75,16 @@ you can copy `run_example.rb` and modify it as necessary. ```sh cp run_example.rb run_test.rb # Tweak as necessary -ruby run_test.rb +dotenv ruby run_test.rb ``` ### Docker #### Installation +*Note*: The provided `docker-compose.yml` file will detect your `.env` and create the appropriate +environment variables. + Build the image for the `dark-blue` service. ```sh docker compose build dark-blue @@ -103,7 +107,8 @@ Run the `dark-blue` service to start `run_dark_blue.rb`. docker compose up dark-blue ``` -To use `verify_aptrust.rb`, override the entry command for the `dark-blue` service with `run`. +The latest deposits to APTrust can be verified using `verify_aptrust.rb`. +Override the entry command for the `dark-blue` service with `run`. ```sh docker compose run dark-blue ruby verify_aptrust.rb ``` From d383a6ac185204f1a7a2ce2f87ac4296548e39b5 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Fri, 12 Apr 2024 17:42:37 -0400 Subject: [PATCH 11/16] Change mentions of dpact to darkblue-aptrust --- docker-compose.yml | 6 +++--- example.env | 6 +++--- mariadb/init.sql | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 73d0fb3..f09b330 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -21,9 +21,9 @@ services: image: mariadb:11 environment: - MARIADB_ROOT_PASSWORD=root_pw - - MARIADB_DATABASE=dpact_pres_service - - MARIADB_USER=dpact_user - - MARIADB_PASSWORD=dpact_pw + - MARIADB_DATABASE=darkblue_aptrust + - MARIADB_USER=darkblue_aptrust_user + - MARIADB_PASSWORD=darkblue_aptrust_pw command: [ '--character-set-server=utf8mb4', '--collation-server=utf8mb4_unicode_ci' diff --git a/example.env b/example.env index 402b8ee..96c436b 100644 --- a/example.env +++ b/example.env @@ -21,10 +21,10 @@ REPOSITORY_DESCRIPTION= # Values provided should work for local development with Docker # While technically optional, it is required when running all tests. DATABASE_HOST=database -DATABASE_DATABASE=dpact_pres_service +DATABASE_DATABASE=darkblue_aptrust DATABASE_PORT=3306 -DATABASE_USER=dpact_user -DATABASE_PASSWORD=dpact_pw +DATABASE_USER=darkblue_aptrust_user +DATABASE_PASSWORD=darkblue_aptrust_pw # Archivematica instance(s) # Settings for each Archivematica instance should have the same stem, diff --git a/mariadb/init.sql b/mariadb/init.sql index 4f98d65..788f29b 100644 --- a/mariadb/init.sql +++ b/mariadb/init.sql @@ -1,2 +1,2 @@ -GRANT ALL PRIVILEGES ON *.* TO dpact_user; +GRANT ALL PRIVILEGES ON *.* TO darkblue_aptrust_user; CREATE DATABASE IF NOT EXISTS test_database; From 01b1a6d61b544e77222b0307500e9ac18dcb68e8 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Fri, 12 Apr 2024 17:51:02 -0400 Subject: [PATCH 12/16] Handle size limit result correctly --- lib/config.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/config.rb b/lib/config.rb index cb07660..ed682e3 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -286,7 +286,7 @@ def self.create_config(data) dry_run: data.get_value(key: "SETTINGS_DRY_RUN", checks: [BooleanCheck.new]) == "true", object_size_limit: data.get_value( key: "SETTINGS_OBJECT_SIZE_LIMIT", checks: [IntegerCheck.new], optional: true - ).to_i + ).tap { |v| !v.nil? && v.to_i } ), repository: RepositoryConfig.new( name: data.get_value(key: "REPOSITORY_NAME"), From 908d0515ea8321cdc713db45101c57b73427841e Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Mon, 15 Apr 2024 10:26:16 -0400 Subject: [PATCH 13/16] Create ControlledVocabularyCheck; add reason method to checks; fix size limit again; tidy up and add comment --- lib/config.rb | 58 ++++++++++++++++++++++++--------------------- test/test_config.rb | 11 +++++---- 2 files changed, 37 insertions(+), 32 deletions(-) diff --git a/lib/config.rb b/lib/config.rb index ed682e3..df8dec0 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -4,52 +4,59 @@ module Config class ConfigError < StandardError end + # Check subclasses will validate the values of environment variables, + # which will either be nil or string. Conversion to other data types, + # if necessary, must be done subsequently. class CheckBase def check?(value) raise NotImplementedError end + + def reason + raise NotImplementedError + end end class NotNilCheck < CheckBase def check?(value) !value.nil? end - end - # class StringCheck < CheckBase - # def check?(value) - # value.is_a?(String) - # end - # end + def reason + "Value cannot be nil." + end + end class IntegerCheck < CheckBase def check?(value) Integer(value, exception: false).is_a?(Integer) end - end - class BooleanCheck < CheckBase - def check?(value) - ["true", "false"].include?(value) + def reason + "Value is not an integer." end end - class LogLevelCheck < CheckBase - LOG_LEVELS = ["info", "debug", "trace", "warn", "error", "fatal"] + class ControlledVocabularyCheck < CheckBase + def initialize(members) + @members = members + end def check?(value) - LOG_LEVELS.include?(value) + @members.include?(value) end - end - - class RemoteTypeCheck < CheckBase - REMOTE_TYPES = ["file_system", "aptrust", "sftp"] - def check?(value) - REMOTE_TYPES.include?(value) + def reason + "Value is not in the list #{@members}." end end + BOOLEAN_CHECK = ControlledVocabularyCheck.new(["true", "false"]) + LOG_LEVEL_CHECK = ControlledVocabularyCheck.new( + ["info", "debug", "trace", "warn", "error", "fatal"] + ) + REMOTE_TYPE_CHECK = ControlledVocabularyCheck.new(["file_system", "aptrust", "sftp"]) + class CheckableData attr_reader :data @@ -79,10 +86,7 @@ def get_value(key:, checks: [], optional: false) return value if optional && value.nil? [NotNilCheck.new, *checks].each do |check| - if !check.check?(value) - reason = "#{check.class.name.split("::").last} failed." - raise_error(key: key, value: value, reason: reason) - end + raise_error(key: key, value: value, reason: check.reason) if !check.check?(value) end value end @@ -223,7 +227,7 @@ def self.create_database_config(data) end def self.create_remote_config(data) - type = data.get_value(key: "TYPE", checks: [RemoteTypeCheck.new]).to_sym + type = data.get_value(key: "TYPE", checks: [REMOTE_TYPE_CHECK]).to_sym settings = data.get_subset_by_key_stem("SETTINGS_") RemoteConfig.new( type: type, @@ -280,13 +284,13 @@ def self.create_config(data) config = Config.new( settings: SettingsConfig.new( - log_level: data.get_value(key: "SETTINGS_LOG_LEVEL", checks: [LogLevelCheck.new]).to_sym, + log_level: data.get_value(key: "SETTINGS_LOG_LEVEL", checks: [LOG_LEVEL_CHECK]).to_sym, working_dir: data.get_value(key: "SETTINGS_WORKING_DIR"), export_dir: data.get_value(key: "SETTINGS_EXPORT_DIR"), - dry_run: data.get_value(key: "SETTINGS_DRY_RUN", checks: [BooleanCheck.new]) == "true", + dry_run: data.get_value(key: "SETTINGS_DRY_RUN", checks: [BOOLEAN_CHECK]) == "true", object_size_limit: data.get_value( key: "SETTINGS_OBJECT_SIZE_LIMIT", checks: [IntegerCheck.new], optional: true - ).tap { |v| !v.nil? && v.to_i } + )&.to_i ), repository: RepositoryConfig.new( name: data.get_value(key: "REPOSITORY_NAME"), diff --git a/test/test_config.rb b/test/test_config.rb index 71c6bba..3996cec 100644 --- a/test/test_config.rb +++ b/test/test_config.rb @@ -13,7 +13,7 @@ def test_get_value_when_it_exists def test_get_value_when_key_does_not_exist data = CheckableData.new({}) - expected_message = "Value \"\" for key \"some_key\" is not valid. NotNilCheck failed." + expected_message = "Value \"\" for key \"some_key\" is not valid. Value cannot be nil." error = assert_raises(ConfigError) { data.get_value(key: "some_key") } assert_equal expected_message, error.message end @@ -31,20 +31,21 @@ def test_get_value_with_integer_string def test_get_value_with_invalid_integer_string data = CheckableData.new({"some_key" => "11a1"}) - expected_message = "Value \"11a1\" for key \"some_key\" is not valid. IntegerCheck failed." + expected_message = "Value \"11a1\" for key \"some_key\" is not valid. Value is not an integer." error = assert_raises(ConfigError) { data.get_value(key: "some_key", checks: [IntegerCheck.new]) } assert_equal expected_message, error.message end def test_get_value_with_valid_boolean_string data = CheckableData.new({"some_key" => "true"}) - assert_equal "true", data.get_value(key: "some_key", checks: [BooleanCheck.new]) + assert_equal "true", data.get_value(key: "some_key", checks: [BOOLEAN_CHECK]) end def test_get_value_with_invalid_boolean_string data = CheckableData.new({"some_key" => "YES!!"}) - expected_message = "Value \"YES!!\" for key \"some_key\" is not valid. BooleanCheck failed." - error = assert_raises(ConfigError) { data.get_value(key: "some_key", checks: [BooleanCheck.new]) } + expected_message = "Value \"YES!!\" for key \"some_key\" is not valid. " \ + "Value is not in the list [\"true\", \"false\"]." + error = assert_raises(ConfigError) { data.get_value(key: "some_key", checks: [BOOLEAN_CHECK]) } assert_equal expected_message, error.message end From 720c091a5a86e5b817926371b0f5d912fc36d1c0 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Mon, 15 Apr 2024 10:39:48 -0400 Subject: [PATCH 14/16] Remove semantic_logger; remove unused data attr_reader --- lib/config.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/lib/config.rb b/lib/config.rb index df8dec0..e7fa1fa 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -1,5 +1,3 @@ -require "semantic_logger" - module Config class ConfigError < StandardError end @@ -211,9 +209,6 @@ def get_subset_by_key_stem(stem) ) class ConfigService - include SemanticLogger::Loggable - attr_reader :data - ARCHIVEMATICA_INSTANCES = ["ARCHIVEMATICA_DEV", "ARCHIVEMATICA_LAB", "ARCHIVEMATICA_AMI", "ARCHIVEMATICA_VGA"] def self.create_database_config(data) From af2f5e13095c6ef2da325cb040147293aabf4dad Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Mon, 15 Apr 2024 11:07:58 -0400 Subject: [PATCH 15/16] Fix linting errors; fix bad test --- lib/config.rb | 10 +++++----- test/test_config.rb | 3 +-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/config.rb b/lib/config.rb index e7fa1fa..2f97f61 100644 --- a/lib/config.rb +++ b/lib/config.rb @@ -17,7 +17,7 @@ def reason class NotNilCheck < CheckBase def check?(value) - !value.nil? + !value.nil? end def reason @@ -209,7 +209,7 @@ def get_subset_by_key_stem(stem) ) class ConfigService - ARCHIVEMATICA_INSTANCES = ["ARCHIVEMATICA_DEV", "ARCHIVEMATICA_LAB", "ARCHIVEMATICA_AMI", "ARCHIVEMATICA_VGA"] + ARCHIVEMATICA_INSTANCES = ["ARCHIVEMATICA_DEV", "ARCHIVEMATICA_LAB", "ARCHIVEMATICA_AMI", "ARCHIVEMATICA_VGA"] def self.create_database_config(data) DatabaseConfig.new( @@ -277,7 +277,7 @@ def self.create_config(data) arch_configs << create_archivematica_config(instance_data) if instance_data.keys.length > 0 end - config = Config.new( + Config.new( settings: SettingsConfig.new( log_level: data.get_value(key: "SETTINGS_LOG_LEVEL", checks: [LOG_LEVEL_CHECK]).to_sym, working_dir: data.get_value(key: "SETTINGS_WORKING_DIR"), @@ -291,7 +291,7 @@ def self.create_config(data) name: data.get_value(key: "REPOSITORY_NAME"), description: data.get_value(key: "REPOSITORY_DESCRIPTION") ), - database: db_data.keys.length > 0 ? create_database_config(db_data) : nil, + database: (db_data.keys.length > 0) ? create_database_config(db_data) : nil, dark_blue: DarkBlueConfig.new(archivematicas: arch_configs), aptrust: APTrustConfig.new( api: APTrustAPIConfig.new( @@ -306,7 +306,7 @@ def self.create_config(data) def self.database_config_from_env db_data = CheckableData.new(ENV.to_hash).get_subset_by_key_stem("DATABASE_") - db_data.keys.length > 0 ? create_database_config(db_data) : nil + (db_data.keys.length > 0) ? create_database_config(db_data) : nil end def self.from_env diff --git a/test/test_config.rb b/test/test_config.rb index 3996cec..61b3241 100644 --- a/test/test_config.rb +++ b/test/test_config.rb @@ -20,7 +20,6 @@ def test_get_value_when_key_does_not_exist def test_get_value_when_key_does_not_exist_but_optional data = CheckableData.new({}) - error = assert_raises(ConfigError) { data.get_value(key: "some_key") } assert_nil data.get_value(key: "some_key", optional: true) end @@ -62,6 +61,6 @@ def test_get_subset_by_key_stem_with_no_matches input = {"A_blah" => 1, "B_blah" => 2} data = CheckableData.new(input) data_subset = data.get_subset_by_key_stem("C_") - assert_equal Hash.new, data_subset.data + assert_equal ({}), data_subset.data end end From 6b9d35e487f72757802d3f1361b73dcef1dc68d5 Mon Sep 17 00:00:00 2001 From: Samuel Sciolla Date: Mon, 15 Apr 2024 11:35:34 -0400 Subject: [PATCH 16/16] Improve test name --- test/test_config.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_config.rb b/test/test_config.rb index 61b3241..9b4041f 100644 --- a/test/test_config.rb +++ b/test/test_config.rb @@ -6,7 +6,7 @@ class CheckableDataTest < Minitest::Test include Config - def test_get_value_when_it_exists + def test_get_value_when_key_exists data = CheckableData.new({"some_key" => "some_value"}) assert_equal "some_value", data.get_value(key: "some_key") end