-
Notifications
You must be signed in to change notification settings - Fork 81
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
Support aliases in environment variables and YAML configuration #825
Conversation
Signed-off-by: Simon Dudley <simon.dudley@consensys.net>
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.
lgtm
@@ -119,9 +119,9 @@ dependencyManagement { | |||
entry 'besu' | |||
entry ('core') { | |||
exclude group: 'com.github.jnr', name: 'jnr-unixsocket' | |||
exclude group: 'org.bouncycastle', name: 'bcprov-jdk15on' |
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.
Why is this still needed?
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.
My guess is that since the artefact name itself changed, it's not possible for gradle to automatically resolve to the jdk18 version. We're using a version of web3j that pulls in the old one. I tried upgrading web3j first and it caused other issues so aborted that, but we should revisit.
I believe the utils module is a dependency of core. In an earlier commit I had introduced the utils entry simply to be able to exclude bouncycastle. In this PR I realised I could simplify that by moving the exclude.
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.
LGTM
PR Description
Supporting PR for #826
Before this PR, the only aliases that were supported were the ones with the longest name. This makes sense when your aliases are always short name versions of the options, e.g.
--logging, -l
.It doesn't make sense when you want to use aliases as a way to rename an option as for #586
This PR allows 'short name options' with a single leading dash to be converted into environment variables and YAML config items.
I have opened another issue to discuss whether we should handle these differently to the long name options: #824
Documentation
doc-change-required
label to this PR if updates are required.Changelog
Testing