-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Respect environment variables in jvm.options #16834
base: main
Are you sure you want to change the base?
Conversation
…bstituted with their corresponding values.
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.
I like that we skip this if we have a commented or empty line. Doing the regex on every "valid" line in config is probably low overhead in practice. Overall I think this will work for the majority of cases but it does have some requirements and edges that may be worth documenting.
- Spell out the highlights from the regexes
\$\{
[A-Z_][A-Z0-9_]*
(mainly that the substitution must be in form${FOO}
not$FOO
, the name must be all caps and not start with an underscore etc${FOO}
not${foo}
and the default syntax). - Nested substitution wont work
${ROOT:${LEAF}}
- Default values cannot have colons or dollar signs (i think this is relevant because we have an example of such in
Line 33 in 01c8e8b
#-Djava.io.tmpdir=$HOME - There is no escape characters (relates to 3)
Quality Gate passedIssues Measures |
💚 Build Succeeded
History
|
@donoghuc thanks for the edge case checking! I overlooked that we support lowercase variable and the $HOME in jvm.options. Logstash has documented about the dollar sign variable. $VAR is not the pattern Logstash support. Honestly the variables in jvm.options were interpreted in bash script which has different syntax to logstash env variable. Given that no reported issue on resolving the env var in jvm.options and it has been failing to resolve since 8.0, in this PR, I make it align to Logstash syntax. I noticed that we have different convention in log4j. I will raise the question in weekly sync. I updated the pattern to align to the current ruby substitution. We do not support nested variable |
Release notes
Add support for
${VAR:default}
syntax in jvm.options fileWhat does this PR do?
JvmOptionsParser adds support for
${VAR:default}
syntax when parsing jvm.optionsWhy is it important/What is the impact to the user?
Checklist
Author's Checklist
How to test this PR locally
Related issues
jvm.options
are not replaced by theJvmOptionsParser
#16444Use cases
Screenshots
Logs