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

config options split into two files #1024

Merged
merged 11 commits into from Aug 17, 2018

Conversation

Projects
5 participants
@ihla

ihla commented Jun 5, 2018

this PR addresses the option 3 from suggested solutions of #149

@abitmore

This comment has been minimized.

Member

abitmore commented Jun 5, 2018

Thanks. I think this is not actually the option 3, but more like option 4. Actually we can see it as option 5.

@ryanRfox ryanRfox requested review from oxarbitrage, pmconrad and abitmore Jun 6, 2018

@abitmore abitmore requested review from jmjatlanta and removed request for abitmore Jun 6, 2018

@jmjatlanta

This is a quick and easy fix for the config / logging config issue.

static void load_logging_config_file
(
const fc::path& config_ini_path
)

This comment has been minimized.

@jmjatlanta

jmjatlanta Jun 7, 2018

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

This comment has been minimized.

@ihla
(
const fc::path& config_ini_path,
const fc::path& data_dir
)

This comment has been minimized.

@jmjatlanta

jmjatlanta Jun 7, 2018

style - same as above

This comment has been minimized.

@ihla
const auto logging_ini_path = data_dir / "logging.ini";
if (!exists(logging_ini_path))
{
create_logging_config_file (logging_ini_path, data_dir);

This comment has been minimized.

@jmjatlanta

jmjatlanta Jun 7, 2018

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

This comment has been minimized.

@ihla

ihla Jun 8, 2018

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

This comment has been minimized.

@jmjatlanta

jmjatlanta Jun 8, 2018

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.

This comment has been minimized.

@ryanRfox

ryanRfox Jun 8, 2018

Member

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.

This comment has been minimized.

@ihla

ihla Jul 30, 2018

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

lubos.ilcik

@ryanRfox ryanRfox added this to To Do in Feature Release (201808) via automation Jun 13, 2018

@abitmore abitmore moved this from To Do to In Development in Feature Release (201808) Jun 25, 2018

@ihla

This comment has been minimized.

ihla commented Jul 16, 2018

I am free now and could work on the requested changes for this PR as per @jmjatlanta pseudo code - pls give me GO to be sure I am not interfering with anybody plans.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Jul 16, 2018

I believe this is a go. Thanks Lubos!

@abitmore abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Jul 21, 2018

@abitmore abitmore removed this from In Development in Feature Release (201808) Jul 21, 2018

@abitmore abitmore removed this from New -Awaiting Core Team Evaluation in Project Backlog Jul 29, 2018

@abitmore abitmore added this to In progress in Feature release (201810) via automation Jul 29, 2018

@pmconrad

Nice job, thanks!
FTR, lines 46-288 were mostly copied from main.cpp. Functional changes are contained in create_logging_config_file and load_configuration_options.

};
// logging config is too complicated to be parsed by boost::program_options,
// so we do it by hand

This comment has been minimized.

@pmconrad

pmconrad Jul 31, 2018

Please move these two lines to the load_logging_config_from_ini_file function below.

if(!load_logging_config_file(config_ini_path))
{
// config_ini_path doesn't contain valid logging options - fall back to the defaults
create_logging_config_file(logging_ini_path, data_dir);

This comment has been minimized.

@pmconrad

pmconrad Jul 31, 2018

Please don't create a logging config file in this case.

Reason: if there is a config.ini but no logging.ini the config.ini was likely created by an old witness_node and modified by the user. The user has probably either commented out the logging-related stuff, or added other options so that the old logging parser fails.
In both cases adding a logging.ini will lead to confusion because the logging-related stuff is still present in config.ini but will be ignored.

fc::create_directories(data_dir);
auto modify_option_defaults = [](const boost::shared_ptr<bpo::option_description>& o) -> const boost::shared_ptr<bpo::option_description> {
const std::basic_string<char, std::char_traits<char>, std::allocator<char>> & name = o->long_name();

This comment has been minimized.

@pmconrad

pmconrad Jul 31, 2018

Why not simply use std::string like the original code?

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 1, 2018

Please fix conflicts.

By the way need to make sure the code moving didn't change any logic (I haven't carefully compared them yet)

@pmconrad

This comment has been minimized.

pmconrad commented Aug 1, 2018

I verified the code move.

@ihla

This comment has been minimized.

ihla commented Aug 2, 2018

I have found one difference in create_new_config_file(() - line 256

if( name == "max-ops-per-account" )
          return new_option_description(name, bpo::value<int>()->default_value(1000), o->description() );

The default_value is set to 100 in develop.

@pmconrad

Thanks!

@ihla

This comment has been minimized.

ihla commented Aug 15, 2018

is there anything expected from me to complete this PR? i've lost track..

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 15, 2018

I'll leave it for @jmjatlanta to decide.

Feature release (201810) automation moved this from In progress to In Testing Aug 17, 2018

@jmjatlanta

Compiled with Ubuntu 18.04 / Boost 1.67 / OpenSSL 1.1. Ran app_test without errors. Code well written IMO. Thank you @ihla

@abitmore abitmore merged commit 65db5c8 into bitshares:develop Aug 17, 2018

1 of 2 checks passed

ci/dockercloud Your tests have been canceled in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Aug 17, 2018

@abitmore abitmore referenced this pull request Aug 17, 2018

Closed

witness_node uses two incompatible parsers for config.ini #149

3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment