-
Notifications
You must be signed in to change notification settings - Fork 5.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
fix: do not interpolate env variables in helm template options when forceString is true #20002
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
7518c8b
to
571a270
Compare
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.
Thanks for the PR and for providing a workaround on the issue!
Can you add separate tests for the new functionality and preserve the original test?
54ff712
to
06e3cf7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20002 +/- ##
=========================================
Coverage ? 55.80%
=========================================
Files ? 320
Lines ? 44429
Branches ? 0
=========================================
Hits ? 24795
Misses ? 17068
Partials ? 2566 ☔ View full report in Codecov by Sentry. |
06e3cf7
to
80a34fd
Compare
Hello, @reggie-k. I add/edit tests for changed code. Thanks! |
51554b3
to
df07e8d
Compare
…orceString is true Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Signed-off-by: daengdaengLee <gunho1020@gmail.com>
Signed-off-by: daengdaengLee <gunho1020@gmail.com>
df07e8d
to
83d5c39
Compare
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 goal of forceString
is not to decide if the variable should be interpolated or not.
The main goal is to decide when boolean value, either the result of an interpolation or not, should be converted to a string.
This is because the quoting will be removed, and it is impossible to specify myparams: "true"
where the value must be a string.
Closing as this is not the desired behavior. The discussion on the correct implementation to fix the issue should happen in #19269 |
related issue : #19269 (1 problem / 2 problems)
forceString
option.forceString
option is set totrue
, the environment variable interpolation is not performed.forceString
option istrue
. (explained in the docs) If this PR is merged, it will resolve part of the issue Commas and dollar signs improperly passed to Helm #19269 (the problem where the$
character is incorrectly encoded even when theforceString
option istrue
).If I've misunderstood something, please let me know in the comments or close the PR. Thank you! :)
Checklist: