Skip to content
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

Merged

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented Jul 5, 2021

Fixes: #6128

Description
Introduce new default function, modelled on sprig's default function:

 {{default "value" .MISSING}}

results in "value" if .MISSING is nil or a zero value.

@briandealwis briandealwis requested a review from a team as a code owner July 5, 2021 21:56
@briandealwis briandealwis requested a review from nkubala July 5, 2021 21:56
@google-cla google-cla bot added the cla: yes label Jul 5, 2021
@codecov
Copy link

codecov bot commented Jul 5, 2021

Codecov Report

Merging #6136 (a3146db) into master (823896b) will increase coverage by 1.15%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
pkg/skaffold/util/env_template.go 94.36% <87.50%> (-1.09%) ⬇️
pkg/skaffold/schema/latest/v2/config.go 5.40% <0.00%> (-94.60%) ⬇️
pkg/skaffold/schema/v3alpha1/config.go 2.70% <0.00%> (-47.30%) ⬇️
pkg/skaffold/render/generate/generate.go 52.38% <0.00%> (-24.98%) ⬇️
pkg/skaffold/initializer/build/builders.go 60.00% <0.00%> (-10.00%) ⬇️
pkg/skaffold/build/gcb/types.go 44.44% <0.00%> (-9.41%) ⬇️
cmd/skaffold/app/cmd/version.go 77.77% <0.00%> (-7.94%) ⬇️
pkg/skaffold/status/provider.go 75.00% <0.00%> (-6.82%) ⬇️
pkg/skaffold/build/cluster/types.go 93.33% <0.00%> (-6.67%) ⬇️
pkg/skaffold/sync/provider.go 65.00% <0.00%> (-6.43%) ⬇️
... and 471 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 823896b...a3146db. Read the comment docs.

gsquared94
gsquared94 previously approved these changes Jul 6, 2021
@briandealwis
Copy link
Member Author

Holding off for input from @nkubala @viglesiasce

nkubala
nkubala previously approved these changes Jul 8, 2021
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6128 (comment)

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:

  1. that's probably a very fringe corner case, and
  2. this is probably the way we should have been doing this to begin with.

@tejal29
Copy link
Member

tejal29 commented Jul 8, 2021

Why not template.New("envTemplate").Option("missingkey=error").Parse(t) and error out when evaluating the template?

@briandealwis
Copy link
Member Author

Why not template.New("envTemplate").Option("missingkey=error").Parse(t) and error out when evaluating the template?

I think the general expectation is that we should handle environment variables like the shell, which replaces non-existent variables with "".

$ echo hi${THIS_DOES_NOT_EXIST}there
hithere

@briandealwis
Copy link
Member Author

That said, @tejal29's suggestion has some appeal. We could provide something like Helm's default function. That comes from the sprig template function library that Helm incorporates; that library adds more than a hundred functions.

@briandealwis briandealwis dismissed stale reviews from nkubala and gsquared94 July 12, 2021 21:20

Reworking as discussed in triage

@briandealwis briandealwis marked this pull request as draft July 12, 2021 21:20
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 12, 2021
@briandealwis
Copy link
Member Author

I've reverted our template behaviour back to the original behaviour and introduced a new default function modelled on the sprig project's default.

@briandealwis briandealwis marked this pull request as ready for review July 12, 2021 22:32
Copy link
Contributor

@nkubala nkubala left a 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:
Copy link
Contributor

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?

Copy link
Member Author

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.

@nkubala nkubala merged commit a0b7a80 into GoogleContainerTools:master Jul 13, 2021
@briandealwis briandealwis changed the title Replace missing template values with empty string Improve handling of missing template values Jul 14, 2021
aaron-prindle pushed a commit that referenced this pull request Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pseudo-empty string "<no value>" returned by setValueTemplate when undefined environment variable
4 participants