Have consistent warning between CLI, ENV and TOML config #3969
Conversation
Signed-off-by: wcgcyx <wcgcyx@gmail.com>
wcgcyx
force-pushed
the
2069-consistent-warning
branch
from
8111e58 to
2b363c3
Compare
| @@ -2073,6 +2110,31 @@ public void rpcHttpOptionsRequiresServiceToBeEnabled() { | |||
| assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); | |||
| } | |||
|
|
|||
| @Test | |||
| public void rpcHttpOptionsRequiresServiceToBeEnabledToml() throws IOException { | |||
There was a problem hiding this comment.
Since we have junit 5 now, creating a parameterized tests for both command line and toml woud be nice and keep this BesuCOmmandTest a little bit smaller
There was a problem hiding this comment.
Could start a BesuCommandTomlTest file perhaps?
| try { | ||
| return !option.stringValues().isEmpty() | ||
| || commandLine.getDefaultValueProvider().defaultValue(option) != null; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Why do we need the try catch here? Does default value fail?
There was a problem hiding this comment.
defaultValue throws Exception when fails to load default value from provider, in this case it would be an IOException as it reads value from config file. It shouldn't throw because the default value has been loaded prior to this, if fails to load config file, besu exits before here.
| option -> { | ||
| try { | ||
| return !option.stringValues().isEmpty() | ||
| || commandLine.getDefaultValueProvider().defaultValue(option) != null; |
There was a problem hiding this comment.
Does this also work for environment variables?
There was a problem hiding this comment.
yep, default value provider in besu checks for environmental variables before config file.
| @@ -2073,6 +2110,31 @@ public void rpcHttpOptionsRequiresServiceToBeEnabled() { | |||
| assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Don't see any tests for environment variables, can test be added for that as well
| createTempFile( | ||
| "toml", | ||
| "miner-coinbase=\"" | ||
| + requestedCoinbase.toString() |
| @@ -99,10 +99,16 @@ public static void checkMultiOptionDependencies( | |||
| private static String getAffectedOptions( | |||
| final CommandLine commandLine, final List<String> dependentOptionsNames) { | |||
| return commandLine.getCommandSpec().options().stream() | |||
| .filter(option -> Arrays.stream(option.names()).anyMatch(dependentOptionsNames::contains)) | |||
There was a problem hiding this comment.
Pulling this into its own filter makes this a lot cleaner 👍
| try { | ||
| return !option.stringValues().isEmpty() | ||
| || commandLine.getDefaultValueProvider().defaultValue(option) != null; | ||
| } catch (Exception e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Could extract the exception throwing part into its own method to hide this complexity from the filter body.
Also, may want to check for empty string as well as null, e.g. !Strings.isNullOrEmpty(...
…#3969) * Update CommandLineUtils.java Signed-off-by: wcgcyx <wcgcyx@gmail.com> Co-authored-by: Sally MacFarlane <sally.macfarlane@consensys.net>
PR description
Add additional check of option in environment variables and config file for detecting ignored dependent config warning.
Fixed Issue(s)
fixes #2069