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

Support aliases in environment variables and YAML configuration #825

Merged
merged 7 commits into from
Jun 29, 2023

Conversation

siladu
Copy link
Contributor

@siladu siladu commented Jun 27, 2023

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

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Testing

  • I thought about testing these changes in a realistic/non-local environment.

Copy link
Contributor

@gfukushima gfukushima left a 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'
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@usmansaleem usmansaleem left a comment

Choose a reason for hiding this comment

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

LGTM

@siladu siladu merged commit d43c899 into Consensys:master Jun 29, 2023
2 checks passed
@siladu siladu deleted the support-aliases branch June 29, 2023 01:00
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.

3 participants