@ironcladlou There are no sig labels on this issue. Please add a sig label by:
(1) mentioning a sig: @kubernetes/sig-<team-name>-misc
e.g., @kubernetes/sig-api-machinery-* for API Machinery
(2) specifying the label manually: /sig <label>
e.g., /sig scalability for sig/scalability

Note: method (1) will trigger a notification to the team. You can find the team list here and label list here

it should omit for nils that are omitEmpty, right?

@ash2k I couldn't assign you and @ironcladlou, you're assigned in spirit, so comment away :)

it should omit for nils that are omitEmpty, right?

@deads2k I'm not sure what omitEmpty you are talking about? Unstructured does not have any annotations/knowledge about contents of Object field so it just serializes it into JSON as is.

func TestNil(t *testing.T) {
	u := Unstructured{
		Object: map[string]interface{}{
			"kind":       "Pod",
			"apiVersion": "v1",
			"metadata": map[string]interface{}{
				"name":              "test",
				//"deletionTimestamp": nil,
				"ownerReferences": []map[string]interface{}{
					{
						"controller": nil,
					},
				},
			},
		},
	}
	u.SetDeletionTimestamp(nil)
	data, _ := u.MarshalJSON()
	t.Logf("%s", data)
}

Outputs:

unstructured_test.go:58: {"apiVersion":"v1","kind":"Pod","metadata":{"deletionTimestamp":null,"name":"test","ownerReferences":[{"controller":null}]}}

I think its good to be consistent with how JSON is unmarshalled. Then code that retrieves data from the map does not need to handle nils. At least not for primitive values like int64. Hm, what if actual JSON contains null? I guess the code still needs to handle nil (but not non-nil pointers!). Oh, well.
Anyway, I think its a good idea to not introduce new nil values into the map.

@ash2k whether or not it is present is based on the omitEmpty tag here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L184. If it isn't omit empty, you should write out an explicit nil.

For fields we know about like type and objectMeta, it makes sense to remove. I'm less certain of fields we don't have knowledge of which may or may not be omitEmpty. Leaving it up to the caller seems to make sense.

I think code dealing with maps like this needs to handle both possibilities. That's just defensive coding :/

Functions can of course be written to make the error handling less verbose.

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

/remove-lifecycle stale

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

/remove-lifecycle stale

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

/remove-lifecycle stale