Repository navigation
feat(core)!: Replace YAML config with CLI args and env vars for metadata DB (resolves #1146). #1148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
21944b9
bbce2fc
849b6b4
5806785
144f74a
eece9aa
6d534b3
0cd45e3
cb43f62
8ea2f0a
90353c1
d4c17f4
d3d2146
a0687a5
1bc0502
690e035
e3a24b0
de68007
d21506b
1647e0b
04923b8
aedebe1
5dc87d1
968221c
cae4ce5
aedca2b
9c63298
47fb5d2
54e3483
e39e1d7
7b39750
2ba751d
87cc885
1aa27fe
dc0c899
a4fd245
ea9765f
06bc482
eba4952
b06d71a
e647fcf
ba0c2c5
3745ca2
fe3115c
015f583
67f2d49
bec5805
aab68bf
6279484
230629e
dfde4fa
ae6d1e0
667dce6
57cf478
0f6ca85
8df0d2f
f07d036
3e7637c
7f52c1f
8f6aaf9
e154899
85e00ce
4e829ff
8c0ff24
ba9a0ac
91fe771
179df88
d7e6afb
04c30ea
9648484
1b9d318
c9834f3
4971191
edb6810
20c61a6
71ed500
e3eecde
b8cbe0a
7d1a273
baf993f
399e1bd
a686b54
b2ad166
e0a6ecc
ea08810
e14eb39
a03cf80
dbe94d3
9cf11c3
510bf1e
26f9a56
b886c33
2c10dbb
69b99da
9d5c7ab
5156dc9
f2855e4
9600944
00b22b8
b368802
e524ed7
9807fb2
4a2790e
ab36b4d
2cb4751
602436b
e93131b
87c3524
7ccf42e
f605611
32b06fe
e2ca0b8
e7fd723
0ac028e
5267b9f
a54eae2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,110 +1,97 @@ | ||||||
| #include "GlobalMetadataDBConfig.hpp" | ||||||
|
|
||||||
| #include <cstdlib> | ||||||
| #include <stdexcept> | ||||||
|
|
||||||
| #include <fmt/core.h> | ||||||
| #include <yaml-cpp/yaml.h> | ||||||
|
|
||||||
| using std::exception; | ||||||
|
junhaoliao marked this conversation as resolved.
Outdated
|
||||||
| using std::invalid_argument; | ||||||
| using std::string; | ||||||
|
|
||||||
| static exception get_yaml_missing_key_exception(string const& key_name) { | ||||||
| throw invalid_argument(fmt::format("Missing key '{}'", key_name)); | ||||||
| namespace clp { | ||||||
| auto GlobalMetadataDBConfig::add_command_line_options( | ||||||
| boost::program_options::options_description& options_description | ||||||
| ) -> void { | ||||||
| options_description.add_options() | ||||||
|
haiqi96 marked this conversation as resolved.
kirkrodrigues marked this conversation as resolved.
|
||||||
| ("db-type", | ||||||
| boost::program_options::value<string>()->default_value("sqlite"), | ||||||
| "Database type [sqlite | mysql]") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we define That said, I feel hardcoding them isn't the end of the world since they are referring to very specific term and are very unlikely to change. |
||||||
| ("db-host", | ||||||
| boost::program_options::value<string>()->default_value("127.0.0.1"), | ||||||
| "[db-type=mysql] Database host") | ||||||
| ("db-port", | ||||||
| boost::program_options::value<int>()->default_value(cDefaultMetadataDbPort), | ||||||
| "[db-type=mysql] Database port") | ||||||
| ("db-name", | ||||||
| boost::program_options::value<string>()->default_value("clp-db"), | ||||||
| "[db-type=mysql] Database name") | ||||||
| ("db-table-prefix", | ||||||
| boost::program_options::value<string>()->default_value("clp_"), | ||||||
| "[db-type=mysql] Database table prefix"); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it might be a bit cleaner if we make this non-static and bind these directly to member variables e.g. That way we can rely on the default values in the header for the default values in the command line options instead of having to repeat them here + parse the variables by name from the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or rather I guess we could just merge the bit of validation that |
||||||
| } | ||||||
|
|
||||||
| static exception | ||||||
| get_yaml_unconvertable_value_exception(string const& key_name, string const& destination_type) { | ||||||
| throw invalid_argument( | ||||||
| fmt::format("'{}' could not be converted to type '{}'", key_name, destination_type) | ||||||
| ); | ||||||
| } | ||||||
| auto GlobalMetadataDBConfig::init_from_parsed_options( | ||||||
| boost::program_options::variables_map const& parsed_options | ||||||
| ) -> void { | ||||||
| if (auto const db_type_string{parsed_options["db-type"].as<string>()}; | ||||||
| "sqlite" == db_type_string) | ||||||
| { | ||||||
| m_metadata_db_type = MetadataDBType::SQLite; | ||||||
| } else if ("mysql" == db_type_string) { | ||||||
| m_metadata_db_type = MetadataDBType::MySQL; | ||||||
| } else { | ||||||
| throw invalid_argument("Unknown database type: " + db_type_string); | ||||||
| } | ||||||
|
|
||||||
| namespace clp { | ||||||
| void GlobalMetadataDBConfig::parse_config_file(string const& config_file_path) { | ||||||
| YAML::Node config = YAML::LoadFile(config_file_path); | ||||||
| if (m_metadata_db_type == MetadataDBType::MySQL) { | ||||||
| m_metadata_db_host = parsed_options["db-host"].as<string>(); | ||||||
| m_metadata_db_port = parsed_options["db-port"].as<int>(); | ||||||
| m_metadata_db_name = parsed_options["db-name"].as<string>(); | ||||||
| m_metadata_table_prefix = parsed_options["db-table-prefix"].as<string>(); | ||||||
| } | ||||||
|
Comment on lines
+114
to
+118
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Minor wording consistency in comments. Prefer “does not” over “doesn't” for consistency. - // SQLite doesn't require extra parameters.
+ // SQLite does not require extra parameters.(Apply in both locations.) Also applies to: 131-134 🤖 Prompt for AI Agents |
||||||
| } | ||||||
|
|
||||||
|
Comment on lines
+114
to
119
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consistency nit: prefer “does not” over “doesn't” in comments. Keep comment tone consistent throughout the file. - // SQLite doesn't require extra parameters.
+ // SQLite does not require extra parameters.(Apply similarly at Line 134.) Also applies to: 132-147 🤖 Prompt for AI Agents |
||||||
| if (!config["type"]) { | ||||||
| throw get_yaml_missing_key_exception("type"); | ||||||
| auto GlobalMetadataDBConfig::read_credentials_from_env() -> void { | ||||||
| auto const* db_username{std::getenv("CLP_DB_USER")}; | ||||||
| if (nullptr != db_username) { | ||||||
| m_metadata_db_username = db_username; | ||||||
| } | ||||||
|
|
||||||
| auto db_type_string = config["type"].as<string>(); | ||||||
| if ("sqlite" == db_type_string) { | ||||||
| m_metadata_db_type = MetadataDBType::SQLite; | ||||||
| } else if ("mysql" == db_type_string) { | ||||||
| m_metadata_db_type = MetadataDBType::MySQL; | ||||||
| auto const* db_password{std::getenv("CLP_DB_PASS")}; | ||||||
| if (nullptr != db_password) { | ||||||
| m_metadata_db_password = db_password; | ||||||
| } | ||||||
| } | ||||||
|
haiqi96 marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| if (!config["host"]) { | ||||||
| throw get_yaml_missing_key_exception("host"); | ||||||
| } | ||||||
| try { | ||||||
| m_metadata_db_host = config["host"].as<string>(); | ||||||
| } catch (YAML::BadConversion& e) { | ||||||
| throw get_yaml_unconvertable_value_exception("host", "string"); | ||||||
| } | ||||||
| auto GlobalMetadataDBConfig::validate() const -> void { | ||||||
| if (m_metadata_db_type == MetadataDBType::MySQL) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shall we validate that those parameters are not provided when db_type is not mysql? (unfortunately, it seems the only way to check "not provided" is to compare them with the default value.) It can be helpful to give user an error when they specifying parameters that are not going to have effect. When we work with someone from another company, there was a time when someone provided all paramaters of a fature but missing an |
||||||
| if (m_metadata_db_host.empty()) { | ||||||
| throw invalid_argument("Database 'host' not specified or empty."); | ||||||
| throw invalid_argument("Database '--db-host' is empty."); | ||||||
| } | ||||||
|
|
||||||
| if (!config["port"]) { | ||||||
| throw get_yaml_missing_key_exception("port"); | ||||||
| } | ||||||
| try { | ||||||
| m_metadata_db_port = config["port"].as<int>(); | ||||||
| } catch (YAML::BadConversion& e) { | ||||||
| throw get_yaml_unconvertable_value_exception("port", "int"); | ||||||
| } | ||||||
| if (m_metadata_db_port < 0) { | ||||||
| throw invalid_argument("Database 'port' cannot be negative."); | ||||||
| if (0 > m_metadata_db_port || 65'535 < m_metadata_db_port) { | ||||||
| throw invalid_argument( | ||||||
| "Database '--db-port' is out of range [0, 65535]: " | ||||||
| + std::to_string(m_metadata_db_port) | ||||||
| ); | ||||||
| } | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| if (!config["name"]) { | ||||||
| throw get_yaml_missing_key_exception("name"); | ||||||
| } | ||||||
| try { | ||||||
| m_metadata_db_name = config["name"].as<string>(); | ||||||
| } catch (YAML::BadConversion& e) { | ||||||
| throw get_yaml_unconvertable_value_exception("name", "string"); | ||||||
| } | ||||||
| if (m_metadata_db_name.empty()) { | ||||||
| throw invalid_argument("Database 'name' not specified or empty."); | ||||||
| throw invalid_argument("Database '--db-name' is empty."); | ||||||
| } | ||||||
|
|
||||||
| if (!config["username"]) { | ||||||
| throw get_yaml_missing_key_exception("username"); | ||||||
| } | ||||||
| try { | ||||||
| m_metadata_db_username = config["username"].as<string>(); | ||||||
| } catch (YAML::BadConversion& e) { | ||||||
| throw get_yaml_unconvertable_value_exception("username", "string"); | ||||||
| if (m_metadata_table_prefix.empty()) { | ||||||
| throw invalid_argument("Database '--db-table_prefix' is empty."); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix typo in error message parameter name. The error message contains an underscore instead of a hyphen in the parameter name. - throw invalid_argument("Database '--db-table_prefix' is empty.");
+ throw invalid_argument("Database '--db-table-prefix' is empty.");📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| } | ||||||
|
|
||||||
| if (m_metadata_db_username.empty()) { | ||||||
| throw invalid_argument("Database 'username' not specified or empty."); | ||||||
| throw invalid_argument("Database 'CLP_DB_USER' not specified or empty."); | ||||||
| } | ||||||
|
|
||||||
| if (!config["password"]) { | ||||||
| throw get_yaml_missing_key_exception("password"); | ||||||
| } | ||||||
| try { | ||||||
| m_metadata_db_password = config["password"].as<string>(); | ||||||
| } catch (YAML::BadConversion& e) { | ||||||
| throw get_yaml_unconvertable_value_exception("password", "string"); | ||||||
| } | ||||||
| if (m_metadata_db_password.empty()) { | ||||||
| throw invalid_argument("Database 'password' not specified or empty."); | ||||||
| } | ||||||
|
|
||||||
| if (!config["table_prefix"]) { | ||||||
| throw get_yaml_missing_key_exception("table_prefix"); | ||||||
| throw invalid_argument("Database 'CLP_DB_PASS' not specified or empty."); | ||||||
| } | ||||||
| try { | ||||||
| m_metadata_table_prefix = config["table_prefix"].as<string>(); | ||||||
| } catch (YAML::BadConversion& e) { | ||||||
| throw get_yaml_unconvertable_value_exception("table_prefix", "string"); | ||||||
| } | ||||||
| if (m_metadata_table_prefix.empty()) { | ||||||
| throw invalid_argument("Database 'table_prefix' not specified or empty."); | ||||||
| } | ||||||
| } else { | ||||||
| throw invalid_argument("Unknown type"); | ||||||
| } | ||||||
| } | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
| } // namespace clp | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |
| #include <cstdint> | ||
| #include <string> | ||
|
|
||
| #include <boost/program_options.hpp> | ||
|
|
||
| namespace clp { | ||
| /** | ||
| * Class encapsulating the global metadata database's configuration details | ||
|
|
@@ -16,11 +18,37 @@ class GlobalMetadataDBConfig { | |
| MySQL, | ||
| }; | ||
|
|
||
| // Static functions | ||
| /** | ||
| * Adds database-related command-line options to the given options_description. | ||
| * @param options_description | ||
| */ | ||
| static auto add_command_line_options( | ||
| boost::program_options::options_description& options_description | ||
| ) -> void; | ||
|
|
||
| // Constructors | ||
| GlobalMetadataDBConfig() = default; | ||
|
|
||
| // Methods | ||
| void parse_config_file(std::string const& config_file_path); | ||
| /** | ||
| * Initializes the configuration from parsed command-line options. | ||
| * @param parsed_options | ||
| */ | ||
| auto init_from_parsed_options(boost::program_options::variables_map const& parsed_options) | ||
| -> void; | ||
|
|
||
| /** | ||
| * Reads database credentials from environment variables: | ||
| * - CLP_DB_USER for database username | ||
| * - CLP_DB_PASS for database password | ||
| */ | ||
| auto read_credentials_from_env() -> void; | ||
|
|
||
| /** | ||
| * Validates that all required parameters are provided | ||
|
junhaoliao marked this conversation as resolved.
Outdated
|
||
| */ | ||
| auto validate() const -> void; | ||
|
|
||
| [[nodiscard]] auto get_metadata_db_type() const -> MetadataDBType { return m_metadata_db_type; } | ||
|
|
||
|
|
@@ -52,7 +80,7 @@ class GlobalMetadataDBConfig { | |
|
|
||
| MetadataDBType m_metadata_db_type{MetadataDBType::SQLite}; | ||
|
|
||
| std::string m_metadata_db_host{"localhost"}; | ||
| std::string m_metadata_db_host{"127.0.0.1"}; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if we want to change the default as
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i can revert all related references if this change is confusing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
| int m_metadata_db_port{cDefaultMetadataDbPort}; | ||
| std::string m_metadata_db_name; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you take the suggestion from my other comment we could just set all of the default values here using
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I extracted the default values as constants, and in |
||
|
|
||
|
|
||
|
junhaoliao marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,21 +41,14 @@ CommandLineArguments::parse_arguments(int argc, char const* argv[]) { | |
| config_file_path += '/'; | ||
| } | ||
| config_file_path += cDefaultConfigFilename; | ||
| string global_metadata_db_config_file_path; | ||
| options_general.add_options() | ||
| ("help,h", "Print help") | ||
| ("version,V", "Print version") | ||
| ( | ||
| "config-file", | ||
| po::value<string>(&config_file_path)->value_name("FILE") | ||
| ->default_value(config_file_path), | ||
| "Use configuration options from FILE" | ||
| )( | ||
| "db-config-file", | ||
| po::value<string>(&global_metadata_db_config_file_path)->value_name("FILE") | ||
| ->default_value(global_metadata_db_config_file_path), | ||
| "Global metadata DB YAML config" | ||
| ); | ||
| options_general.add_options()("help,h", "Print help")("version,V", "Print version")( | ||
| "config-file", | ||
| po::value<string>(&config_file_path) | ||
| ->value_name("FILE") | ||
| ->default_value(config_file_path), | ||
| "Use configuration options from FILE" | ||
| ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turn off clang-format and reformat as before. |
||
| GlobalMetadataDBConfig::add_command_line_options(options_general); | ||
|
|
||
| // Define input options | ||
| po::options_description options_input("Input Options"); | ||
|
|
@@ -195,14 +188,14 @@ CommandLineArguments::parse_arguments(int argc, char const* argv[]) { | |
| return ParsingResult::InfoCommand; | ||
| } | ||
|
|
||
| // Parse and validate global metadata DB config | ||
| if (false == global_metadata_db_config_file_path.empty()) { | ||
| try { | ||
| m_metadata_db_config.parse_config_file(global_metadata_db_config_file_path); | ||
| } catch (std::exception& e) { | ||
| SPDLOG_ERROR("Failed to validate metadata database config - {}", e.what()); | ||
| return ParsingResult::Failure; | ||
| } | ||
| // Initialize and validate global metadata DB config | ||
| try { | ||
| m_metadata_db_config.init_from_parsed_options(parsed_command_line_options); | ||
| m_metadata_db_config.read_credentials_from_env(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now addressed as responded at #1148 (comment) |
||
| m_metadata_db_config.validate(); | ||
| } catch (std::exception& e) { | ||
| SPDLOG_ERROR("Failed to validate metadata database config - {}", e.what()); | ||
| return ParsingResult::Failure; | ||
| } | ||
|
|
||
| // Validate archive path was specified | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,24 +41,14 @@ CommandLineArguments::parse_arguments(int argc, char const* argv[]) { | |
| config_file_path += '/'; | ||
| } | ||
| config_file_path += cDefaultConfigFilename; | ||
| string global_metadata_db_config_file_path; | ||
| options_general.add_options() | ||
| ("help,h", "Print help") | ||
| ("version,V", "Print version") | ||
| ( | ||
| "config-file", | ||
| po::value<string>(&config_file_path) | ||
| ->value_name("FILE") | ||
| ->default_value(config_file_path), | ||
| "Use configuration options from FILE" | ||
| ) | ||
| ( | ||
| "db-config-file", | ||
| po::value<string>(&global_metadata_db_config_file_path) | ||
| ->value_name("FILE") | ||
| ->default_value(global_metadata_db_config_file_path), | ||
| "Global metadata DB YAML config" | ||
| ); | ||
| options_general.add_options()("help,h", "Print help")("version,V", "Print version")( | ||
| "config-file", | ||
| po::value<string>(&config_file_path) | ||
| ->value_name("FILE") | ||
| ->default_value(config_file_path), | ||
| "Use configuration options from FILE" | ||
| ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Turn off clang-format and reformat as before. |
||
| GlobalMetadataDBConfig::add_command_line_options(options_general); | ||
|
|
||
| // Define functional options | ||
| po::options_description options_functional("Input Options"); | ||
|
|
@@ -121,14 +111,14 @@ CommandLineArguments::parse_arguments(int argc, char const* argv[]) { | |
| return ParsingResult::InfoCommand; | ||
| } | ||
|
|
||
| // Parse and validate global metadata DB config | ||
| if (false == global_metadata_db_config_file_path.empty()) { | ||
| try { | ||
| m_metadata_db_config.parse_config_file(global_metadata_db_config_file_path); | ||
| } catch (std::exception& e) { | ||
| SPDLOG_ERROR("Failed to validate metadata database config - {}", e.what()); | ||
| return ParsingResult::Failure; | ||
| } | ||
| // Initialize and validate global metadata DB config | ||
| try { | ||
| m_metadata_db_config.init_from_parsed_options(parsed_command_line_options); | ||
| m_metadata_db_config.read_credentials_from_env(); | ||
| m_metadata_db_config.validate(); | ||
| } catch (std::exception& e) { | ||
| SPDLOG_ERROR("Failed to validate metadata database config - {}", e.what()); | ||
| return ParsingResult::Failure; | ||
| } | ||
|
|
||
| // Validate command | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.