Use reflection to overlay profile onto config

[nkubala]

This changes the profile code to overlay the profile onto the original config using reflection, rather than relying on yaml unmarshalling. Two reasons why we should do this:

1. Will allow us to use yaml:omitEmpty tags on the config fields more liberally, which makes printing out the marshalled yaml to the user nicer.
2. Gives us more control over the actual processing of the profile. If there are fields we want to explicitly persist or ignore from the original config, we can add exceptions for those here.

[balopat]

this is interesting - if I understand correctly you are avoiding to have to do a recursion/iteration on the profile structure by manually picking fields, right? The problem with this is that when we change the config structure, we will have to touch this code too because it introduces a dependency on the structure ... if we'd add a new field, e.g. config.Build.Foo, tests wouldn't fail, right, so we have to remember to add it here too?

[nkubala]

The issue here is that most of the values we're overlaying here are themselves structs (not primitive values), so we can't know which ones to treat as "values" and which ones to treat as "holders of values" (and recurse over them). This solution implicitly chooses the first non-nil value from the profile and uses it as the new value in the final config.

if fieldValue != nil && !reflect.ValueOf(fieldValue).IsNil() {
    // use this value and return
}

For example, the BuildConfig struct (top level in the SkaffoldConfig) contains the TagPolicy struct, which itself is composed of several pointers to structs:

type TagPolicy struct {
	GitTagger         *GitTagger         `yaml:"gitCommit"`
	ShaTagger         *ShaTagger         `yaml:"sha256"`
	EnvTemplateTagger *EnvTemplateTagger `yaml:"envTemplate"`
	DateTimeTagger    *DateTimeTagger    `yaml:"dateTime"`
}

GitTagger is technically of type struct, but we treat it as an actual value. Since we say it's invalid to have multiple values set here, this code just picks the first non-nil one and uses it. This was handled by yaml marshalling before because any value that wasn't set would be set to nil by marshalling the yaml, but if we add yaml:omitempty tags to the config this will break.

If we add something to the top level fields in the SkaffoldConfig, then it's true that tests could still pass and we could potentially miss a silent failure. I'm not sure of a good way to address this apart from either 1) reworking the implementation of the config to not use pointers as a way to implement "one-of" values (see #388), or 2) being more rigorous with our testing when we make changes to the config. WDYT?

[balopat]

Thanks for the detailed explanation!

• Can we somehow mark these top level elements as "one-of" and recognize that in the switch v.Kind()? it would be cool :) (Add yamltags #388 does it on the "option" level not on the "struct" level, maybe we could sync on this @dlorenc ? )
• I would split the overlayProfileField to overlayOneOfField and overlaySpliceField methods (and maybe later do an overlayStructField if necessary) because the fact that the struct fields currently in the profile are "one-of" types has nothing to do inherently with the profile, it is confusing.

[dlorenc]

Can we somehow mark these top level elements as "one-of" and recognize that in the switch v.Kind()? it would be cool :) (#388 does it on the "option" level not on the "struct" level, maybe we could sync on this @dlorenc ? )

Marking them is doable with YAML tags, it should be relatively easy to add in here. I'll chat with Nick.

[r2d4]

I'm not sure we actually want 2) as a feature. I think its better left as a task for kustomize or other tools.

[nkubala]

I'm not sure we actually want 2) as a feature. I think its better left as a task for kustomize or other tools.

@r2d4 sure, that's just a side effect of this change. We don't have to change anything here if we don't want to

[codecov-io]

Codecov Report

Merging #872 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master     #872   +/-   ##
=======================================
  Coverage   38.27%   38.27%           
=======================================
  Files          56       56           
  Lines        2576     2576           
=======================================
  Hits          986      986           
  Misses       1476     1476           
  Partials      114      114

Impacted Files	Coverage Δ
pkg/skaffold/watch/changes.go	76% <0%> (ø)	⬆️
pkg/skaffold/build/gcb/cloud_build.go	13.66% <0%> (ø)	⬆️

---

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 3f11067...b0d8bc4. Read the comment docs.

[balopat]

only one nit otherwise lgtm

[balopat]

nit: I would pull this out into the config package to encourage reuse of the "oneOf" logic instead of having it in v1alpha2

[nkubala]

done