Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.7k
Allow variables to be used as part of a value in configuration.#7464
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Signed-off-by: Joseph Riddle <joe@synadia.com>
Signed-off-by: Joseph Riddle <joe@synadia.com>
MauriceVanVeen left a comment
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.
Thank you for getting this going 👍
In general I'd want to advocate for having both syntaxes work for the normal case. And instead of$$TOKEN I'd personally prefer the use of${TOKEN} like was suggested here:#5320 (comment).
Then you'd be able to use thepassword: $TOKEN syntax that's supported normally but not for embedding. And then bothpassword: ${TOKEN} should work and the embedded case ofnats://user:${TOKEN}@localhost:6222.
That would allow someone to fully switch to the new syntax of${TOKEN} also for the normal case, to keep the config consistent.
conf/parse_test.go Outdated
| authorization { | ||
| user: user | ||
| # normal variable syntax | ||
| password: $TOKEN |
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.
The test currently fails due to it not recognizing theTOKEN here. After removing theauthorization block such that it can only fail on the embedded syntax, the test doesn't fail anymore and contains an unresolvednats://user:$$TOKEN@server.example.com:6222.
conf/parse_test.go Outdated
| authorization { | ||
| user: user | ||
| # normal variable syntax | ||
| password: $TOKEN |
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.
Should also add a test wherepassword: is used with the other syntax. Currently if you try$$TOKEN here it fails.
Signed-off-by: Joseph Riddle <joe@synadia.com>
joeriddles commentedOct 29, 2025
@MauriceVanVeen I updated to use the brace syntax you and the commenter on the issue suggested. Also updated the tests based on the changes and your feedback. Thanks |
Uh oh!
There was an error while loading.Please reload this page.
Resolves:#5320
Based on#5544
Requires double dollar signs in strings to use variables (similar to using an environment variable in a Makefile). This solves@wallyqs's comment on the original pull request by preventing values like "$SYS" getting misinterpreted as a variable.
Note that using double dollar signs outside of string quotes is invalid. Existing parsing behavior of a single dollar sign outside of quotes remains unaffected.
Signed-off-by: Joseph Riddlejoe@synadia.com