-
-
Notifications
You must be signed in to change notification settings - Fork 1.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 our terms of service and privacy 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?
Conversation
Signed-off-by: Joseph Riddle <[email protected]>
Signed-off-by: Joseph Riddle <[email protected]>
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 the password: $TOKEN syntax that's supported normally but not for embedding. And then both password: ${TOKEN} should work and the embedded case of nats://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 the TOKEN here. After removing the authorization block such that it can only fail on the embedded syntax, the test doesn't fail anymore and contains an unresolved nats://user:[email protected]: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 where password: is used with the other syntax. Currently if you try $$TOKEN here it fails.
Signed-off-by: Joseph Riddle <[email protected]>
|
@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 |
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 Riddle [email protected]