Fix missing option names, make [lokid]:rpc required #2055
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
At some point between 0.9.9 and 0.9.10 we removed the printing of option
names when a value doesn't have a default, but this means the config is
littered with things like:
# This option sets the greater foo value.
with no actual option name printed out when there is no default.
This fixes it by always printing the option name in such a case, just
with an empty value, e.g.:
# This option sets the greater foo value.
#big-foo=
When running as a service node we can't do anything without a lokid rpc URL, and we don't necessarily have a good default for it. This makes it required so that we fail with an appropriate error message (rather than connect timeouts) if it is not specified.
Default & Required makes no sense: if we have a default it makes no sense to make it required. The previous behaviour when this was specified was to force an (uncommented) value in the config with the value, but this was only used in the test suite. Required & Hidden makes no sense either: if it's required to be specified we definitely don't want to hide it from the generated config file. These are now compile-time failures.
majestrate
reviewed
Nov 15, 2022
majestrate
approved these changes
Nov 15, 2022
|
Hi |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Three changes:
At some point between 0.9.9 and 0.9.10 we removed the printing of option names when a value doesn't have a default, but this means the config is littered with things like:
# This option sets the greater foo value.with no actual option name printed out when there is no default.
This fixes it by always printing the option name in such a case, just with an empty value, e.g.:
This makes the
[lokid]:rpcsetting required, because we cannot actually start up in service node mode if it is missing. The debs, which do have a sensible default, replace the rpc setting in the generated config in postinst (and broke because the commented-out field was missing, because of the above).Default and Required don't make sense together, so we now fail to compile if used together. Similarly for Required and Hidden. The test code had a bunch of places that did use them together anyway: the behaviour of Default+Required was to write the uncommented default value into the config file, but we decided a long time ago that empty configs was our default so supporting that doesn't make sense.