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.

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

Node configuration auto updater #4579

Merged
merged 7 commits into from
Apr 30, 2024

Conversation

RickiNano
Copy link
Contributor

This pull request introduces a new command line feature --update_config , which updates the nodes current configuration by merging custom settings into the latest configuration template. Custom settings will be uncommented in the new configuration, while default values will be commented.
This feature simplifies the process for node operators to maintain an up-to-date configuration without manually merging the current configuration with the new one.

Features:

  • Updates config file with new entries, tables and documentation
  • Removes entries that are no longer referenced in code
  • Preserves custom values from current configuration
  • Formatted output

Limitations:

  • Currently only works with node configuration file but could easily be extended to rpc an log configs

nano/node/cli.cpp Outdated Show resolved Hide resolved
nano/node/cli.cpp Outdated Show resolved Hide resolved
nano/lib/tomlconfig.cpp Outdated Show resolved Hide resolved
std::istringstream stream_current (current_str);
std::string line_defaults, line_current, result;

while (std::getline (stream_defaults, line_defaults) && std::getline (stream_current, line_current))
Copy link
Contributor

@dsiganos dsiganos Apr 24, 2024

Choose a reason for hiding this comment

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

This loop will only work correctly if the defaults and current lists are sorted and have the same number of elements. Is this guranteed to happen? If yes, how do you know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current list and default list will always follow the exact same layout. This is because we use the serialize_toml function to create the configs. That function will create all the tree nodes, config items, and docs based on the input

@dsiganos
Copy link
Contributor

dsiganos commented Apr 24, 2024

The algorithm converts the structured data in plain text and then does the comparison.
That will work if the output string is always printed in a normalised way.
But in general, I would expect the comparison to be done while the data is held in structured format.

@dsiganos
Copy link
Contributor

A test case would be nice.

@RickiNano
Copy link
Contributor Author

The algorithm converts the structured data in plain text and then does the comparison. That will work if the output string is always printed in a normalised way. But in general, I would expect the comparison to be done while the data is held in structured format.

Unfortunately the TOML library will only write all key/value lines as uncommented line entries. So even if we merge the TOML trees we still have to compare the final string output to the default string output to find what lines should be commented (default values)
It's simpler to directly convert both of them to string and compare them line by line, since we know that they always have the same structure

@RickiNano
Copy link
Contributor Author

@dsiganos I have added a unit test

@@ -1038,3 +1038,47 @@ TEST (toml, log_config_no_required)

ASSERT_FALSE (toml.get_error ()) << toml.get_error ().get_message ();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case, a systest would had been a better option.
But you have done it now...

@clemahieu clemahieu merged commit 401d61c into nanocurrency:develop Apr 30, 2024
26 checks passed
@qwahzi qwahzi added this to the V27 milestone May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants