@bgrant0607 do you know if "update only if previous zero/not set" was the intended behavior?

I've been trying to think of a workaround. We could create an empty struct of the object type and Merge source and destination in an order to get an overriding patch result but there's some more nastiness about this. Using mergo for this, we won't ever be able to set a field to a default value of that field's type e.g. we can never set an int field to 0. Best way to do this might be to somehow write the patch directly onto the versioned json of the api object then decode it. That way we don't have to deal with worrying about default values after we've converted patches to structs.

Bug

cc @brendandburns

FYI there was actually one test case doing an update of a non-empty field that was passing. It looks like this is an upstream bug where mergo.Merge() does not follow the "update only if previous zero/not set" semantics with embedded struct fields. I've documented the bug here darccio/mergo#9.

I haven't looked into the full context of where we're using mergo.Merge() yet, but json unmarshalling in Go appears to do exactly what we want. If you unmarshal json into a struct that already has some values filled in, the values will be overwritten where appropriate: http://play.golang.org/p/THl1k4eNrB

Is there anything not handled properly by that example?

@a-robinson I played around with that idea when I submitted this and I got my failing tests to pass here mikedanese@3f44440. I wasn't convinced that this was a final solution because it still does not allow us to update fields to default values e.g. false or 0 or "". This is because the patch went form v1beta* json -> api/types struct -> api/types json and from internal type to json, the omitempty annotation wipes out all fields with default values. There may be a way to use encoding/json and get the results we want on it but i don't think it's a drop in fix for this.

That makes sense. Any technique that passes data through one of the API structs is going to lose information about whether fields are set. Extra bookkeeping will be needed to track that.

In the meantime though, using json unmarshalling rather than mergo.Merge would at least be a simple fix for the case you brought up in the initial issue.

I can submit this as a partial fix if you think it's reasonable. If there is no objection, what I'd like to do is internalize struct merging responsibility to kubernetes, possibly pkg/util or pkg/util/merge and remove the dependency on Mergo since the embedded struct bug is very nasty and encoding/json will have much better test coverage. As far as --patch I think a satisfying final solution would be to never convert the original patch json into an internal type, and apply the patch json onto a v1beta* type. I can prepare a PR for this as well.

Yeah, we really shouldn't be converting to the internal type in the client (#3955).

It could also make sense to make this a feature of the apiserver and support HTTP PATCH Method on resources. These are relevant: RFC6902 JSON Patch and RFC7386 JSON Merge Patch.

PR darccio/mergo#8 accepted. Maybe it will be useful for you.