-
Notifications
You must be signed in to change notification settings - Fork 153
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
Fix ContextualNetAddress parsing #433
Fix ContextualNetAddress parsing #433
Conversation
This fixes Overall, we need to ensure that all the parameters can be passed in somehow, so it'd be great to just cover every param and try setting them via config and cli to see that they all still work. |
for some reason it doesn't like the line appdir="K:\Testnet11\Data" testnet=true .\kaspad.exe --configfile "K:\Testnet11\Binaries\configfile.toml" seems like a double backslash does work as the backslash may be an escape character this works
|
All the below will run, except add-peers="192.168.3.170:16311"...it throws this error
...however, not seeing the perf metrics displayed, so looks like it's accepting the config file, but not applying and showing metrics. here is the equivalent CLI. Note that in order to show
|
add-peers expects an array so it would be
or single value Maybe ini format is closer to what you're expecting, I'll check the library aspect mentioned |
the TODO
|
tested the following and they're accepted in the config file and seem to be working. Perf metrics are showing and "add-peers" is accepting single and multiple with port number and connecting. Need to run some additional tests over time to verify that the "ram-scale" is applying correctly and limiting RAM to the value provided.
|
9242160
to
c976d4d
Compare
Perhaps serde rename should be applied to:
and perhaps other fields I'm missing. Ideally we find a way to avoid maintaining these multiple renames (both on args definition and serde attributes), but that can be a future fix |
This works.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
README.md
Outdated
@@ -259,7 +259,7 @@ cargo run --release --bin kaspad -- --configfile /path/to/configfile.toml | |||
# or | |||
cargo run --release --bin kaspad -- -C /path/to/configfile.toml | |||
``` | |||
The config file should be a list of \<CLI argument\> = \<value\> separated by newlines. | |||
The config file should be a list of \<CLI argument\> = \<value\> separated by newlines, values with special characters like '.' or '=' will require quoting the value i.e \<CLI argument\> = "\<value\>". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The readme should reflect the latest changes. Should add a list example + fix the arg names (like testnet-suffix -> netsuffix). Title of this section should be "Using a configuration file"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remove the sentence "and some use kebab-case", no?
49ae850
to
478c420
Compare
* Fix ContextualNetAddress parsing * fix kaspanet#434 * Fix handling for all addresses + arrays * Fix log-level taking default instead of config file * Align Args deserialize names to CLI flag names * More renames to align deserialize struct like cli * update README
* Fix ContextualNetAddress parsing * fix kaspanet#434 * Fix handling for all addresses + arrays * Fix log-level taking default instead of config file * Align Args deserialize names to CLI flag names * More renames to align deserialize struct like cli * update README
* Fix ContextualNetAddress parsing * fix kaspanet#434 * Fix handling for all addresses + arrays * Fix log-level taking default instead of config file * Align Args deserialize names to CLI flag names * More renames to align deserialize struct like cli * update README
No description provided.