Skip to content
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.

Sign up for GitHub

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

Jump to bottom

config options split into two files #1024

Merged
merged 11 commits into from Aug 17, 2018
Merged

config options split into two files #1024

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
28a6f3b
config options split into two files
Jun 5, 2018
b8eb753
fixed styling issues
Jun 8, 2018
e6896ef
load config options logic extracted in a single function able to call…
Jul 27, 2018
48c61a7
app configuration logic moved to a new file
Jul 28, 2018
aabde7f
load_configuration_options basic tests
Jul 29, 2018
0d60379
load logging options from legacy config.ini
Jul 30, 2018
0b5f84d
don't create a logging config file if config_ini_path doesn't contain…
Aug 1, 2018
7e01230
comment moved to right place
Aug 1, 2018
a116369
std::basic_string replaced with std::string
Aug 1, 2018
c8281ba
synced with latest develop & resolved merge conflict
Aug 2, 2018
794cb34
max-ops-per-account option default value set to 100
Aug 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 36 additions & 7 deletions programs/witness_node/main.cpp
View file
Open in desktop
Expand Up @@ -106,18 +106,24 @@ static void load_config_file( const fc::path& config_ini_path, const bpo::option

// get the basic options
bpo::store(bpo::parse_config_file<char>(config_ini_path.preferred_string().c_str(),
unique_options, true), options);
unique_options, true), options);
}

static void load_logging_config_file
(
const fc::path& config_ini_path
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style - parameters on same line, if they fit, makes it easier to distinguish this as a function declaration

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

{
// try to get logging options from the config file.
try
{
fc::optional<fc::logging_config> logging_config = load_logging_config_from_ini_file(config_ini_path);
if (logging_config)
fc::configure_logging(*logging_config);
}
catch (const fc::exception&)
catch (const fc::exception& ex)
{
wlog("Error parsing logging config from config file ${config}, using default config", ("config", config_ini_path.preferred_string()));
wlog("Error parsing logging config from logging config file ${config}, using default config", ("config", config_ini_path.preferred_string()));
}
}

Expand Down Expand Up @@ -169,12 +175,25 @@ static void create_new_config_file( const fc::path& config_ini_path, const fc::p
}
out_cfg << "\n";
}

out_cfg.close();
}

static void create_logging_config_file
(
const fc::path& config_ini_path,
const fc::path& data_dir
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style - same as above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

{
ilog("Writing new config file at ${path}", ("path", config_ini_path));
if (!exists(data_dir))
{
create_directories(data_dir);
}

std::ofstream out_cfg(config_ini_path.preferred_string());
write_default_logging_config_to_stream(out_cfg);
out_cfg.close();
// read the default logging config we just wrote out to the file and start using it
fc::optional<fc::logging_config> logging_config = load_logging_config_from_ini_file(config_ini_path);
if (logging_config)
fc::configure_logging(*logging_config);
}

int main(int argc, char** argv) {
Expand Down Expand Up @@ -239,11 +258,21 @@ int main(int argc, char** argv) {
data_dir = fc::current_path() / data_dir;
}

// load witness node initial configuration
fc::path config_ini_path = data_dir / "config.ini";
if( !fc::exists(config_ini_path) )
create_new_config_file( config_ini_path, data_dir, cfg_options );
load_config_file( config_ini_path, cfg_options, options );

// load witness node logging configuration
const auto logging_ini_path = data_dir / "logging.ini";
if (!exists(logging_ini_path))
{
create_logging_config_file (logging_ini_path, data_dir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the logging parameters that already exist in the config.ini?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's right, our change doesn't handle it because we do not need to consider such use case, I can help with implementation if confirmed this is the way to go - I see some discussions still pending in #149

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the consensus of #149 is:

If (config.ini does not exist)
{
    create config.ini;
    create logging.ini;
}

Parse config.ini for application configuration;

If (logging.ini exists)
{
    Parse logging.ini for logging configuration;
} 
else 
{
    Parse config.ini for logging configuration;
}

@pmconrad @abitmore @ryanRfox please verify that consensus has been reached, and my pseudo-code meets the requirements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @jmjatlanta I agree with your pseudo-code above. Build that for #149.

Notes for a future Enhancement Request:

  • Consider versioning the .ini files to match the Release ID
  • When the software starts and encounters a legacy .ini file, it may be beneficial to warn/log/notify the user/system of and their impacts using a legacy .ini file with this Release.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the implementation extended to include the case when logging parameters already exist in the config.ini

}

load_logging_config_file( logging_ini_path );

bpo::notify(options);
node->initialize(data_dir, options);
node->initialize_plugins( options );
Expand Down