-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve handling of missing template values #6136
Improve handling of missing template values #6136
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6136 +/- ##
==========================================
+ Coverage 69.94% 71.10% +1.15%
==========================================
Files 478 484 +6
Lines 18286 21617 +3331
==========================================
+ Hits 12791 15370 +2579
- Misses 4548 5264 +716
- Partials 947 983 +36
Continue to review full report at Codecov.
|
Holding off for input from @nkubala @viglesiasce |
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'm fine with this, but ParseEnvTemplate()
is also used when computing tags in the EnvTagger
. I suppose this could change some behavior that users may have been relying on when their referenced values aren't set....but:
- that's probably a very fringe corner case, and
- this is probably the way we should have been doing this to begin with.
Why not |
I think the general expectation is that we should handle environment variables like the shell, which replaces non-existent variables with "".
|
That said, @tejal29's suggestion has some appeal. We could provide something like Helm's |
Reworking as discussed in triage
I've reverted our template behaviour back to the original behaviour and introduced a new |
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.
nice. I wonder how many other functions from https://masterminds.github.io/sprig/ would be useful to support here... 🤔
} | ||
v := reflect.ValueOf(value) | ||
switch v.Kind() { | ||
case reflect.Array, reflect.Slice: |
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.
are there other types we want to consider here? or another way of posing this question: how much of https://github.com/Masterminds/sprig/blob/master/defaults.go#L35-L60 should we reuse here?
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.
Given that most of our uses will be environment strings, I don't expect any.
Fixes: #6128
Description
Introduce new
default
function, modelled on sprig'sdefault
function:results in
"value"
if.MISSING
is nil or a zero value.