Repository: knative/pkg
Pull request #1359 (2020-05-26 20:41:56)

Fix beta duck conversion & omit conversion if nil
Some tests were failing for `nil != map{}` check (which is better off with `cmpopts.EquateEmpty` anyway, especially when considering wire roundtrip). 
But regardless, no need to create a map if source is nil.

Also apprarently beta duck is a thing.

/assign @dprotaso @vaikas 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1359#" title="Author self-approved">vagababov</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **vaikas**
You can assign the PR to them by writing `/assign @vaikas` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[apis/duck/OWNERS](https://github.com/knative/pkg/blob/master/apis/duck/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["vaikas"]} -->
vaikas: /lgtm
/approve
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1359#" title="Author self-approved">vagababov</a>*, *<a href="https://github.com/knative/pkg/pull/1359#issuecomment-634268446" title="Approved">vaikas</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[apis/duck/OWNERS](https://github.com/knative/pkg/blob/master/apis/duck/OWNERS)~~ [vaikas]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/apis/duck/v1/status_types.go b/apis/duck/v1/status_types.go
index ed2279273..53006708f 100644
--- a/apis/duck/v1/status_types.go
+++ b/apis/duck/v1/status_types.go
@@ -94,8 +94,10 @@ func (s *Status) GetCondition(t apis.ConditionType) *apis.Condition {
 // return true the condition type will be copied to the sink
 func (source *Status) ConvertTo(ctx context.Context, sink *Status, predicates ...func(apis.ConditionType) bool) {
 	sink.ObservedGeneration = source.ObservedGeneration
-	// This will deep copy the map.
-	sink.Annotations = kmeta.UnionMaps(source.Annotations, nil)
+	if source.Annotations != nil {
+		// This will deep copy the map.
+		sink.Annotations = kmeta.UnionMaps(source.Annotations, nil)
+	}
 
 	conditions := make(apis.Conditions, 0, len(source.Conditions))
 	for _, c := range source.Conditions {
diff --git a/apis/duck/v1beta1/status_types.go b/apis/duck/v1beta1/status_types.go
index a046dbedf..d90e7dc69 100644
--- a/apis/duck/v1beta1/status_types.go
+++ b/apis/duck/v1beta1/status_types.go
@@ -26,6 +26,7 @@ import (
 
 	"knative.dev/pkg/apis"
 	"knative.dev/pkg/apis/duck"
+	"knative.dev/pkg/kmeta"
 )
 
 // +genduck
@@ -107,6 +108,9 @@ func (s *Status) GetCondition(t apis.ConditionType) *apis.Condition {
 // ConvertTo helps implement apis.Convertible for types embedding this Status.
 func (source *Status) ConvertTo(ctx context.Context, sink *Status) {
 	sink.ObservedGeneration = source.ObservedGeneration
+	if source.Annotations != nil {
+		sink.Annotations = kmeta.UnionMaps(source.Annotations, nil)
+	}
 	for _, c := range source.Conditions {
 		switch c.Type {
 		// Copy over the "happy" condition, which is the only condition that


Test patch:
diff --git a/apis/duck/v1/status_types_test.go b/apis/duck/v1/status_types_test.go
index fd8842a4e..a53410b57 100644
--- a/apis/duck/v1/status_types_test.go
+++ b/apis/duck/v1/status_types_test.go
@@ -184,4 +184,11 @@ func TestConditionSet(t *testing.T) {
 	if got, want := s2.Annotations, s.Annotations; !cmp.Equal(got, want) {
 		t.Errorf("Annotations mismatch: diff(-want,+got):\n%s", cmp.Diff(want, got))
 	}
+
+	s.Annotations = nil
+	s2 = &Status{}
+	s.ConvertTo(context.Background(), s2)
+	if s2.Annotations != nil {
+		t.Error("Annotations were not nil:", s2.Annotations)
+	}
 }
diff --git a/apis/duck/v1beta1/status_types_test.go b/apis/duck/v1beta1/status_types_test.go
index 1d41afaa9..5e8612018 100644
--- a/apis/duck/v1beta1/status_types_test.go
+++ b/apis/duck/v1beta1/status_types_test.go
@@ -60,7 +60,13 @@ func TestConditionSet(t *testing.T) {
 
 	const wantGeneration = 42
 
-	s := &Status{ObservedGeneration: wantGeneration}
+	s := &Status{
+		ObservedGeneration: wantGeneration,
+		Annotations: map[string]string{
+			"burning": "the",
+			"bridges": "down",
+		},
+	}
 	mgr := condSet.Manage(s)
 
 	mgr.InitializeConditions()
@@ -82,9 +88,12 @@ func TestConditionSet(t *testing.T) {
 		t.Errorf("len(s2.Conditions) = %d, wanted %d", got, want)
 	}
 	if gotGeneration := s2.ObservedGeneration; wantGeneration != gotGeneration {
-		t.Errorf("len(s2.ObservedGeneration) = %d, wanted %d",
+		t.Errorf("s2.ObservedGeneration = %d, wanted %d",
 			gotGeneration, wantGeneration)
 	}
+	if got, want := s2.Annotations, s.Annotations; !cmp.Equal(got, want) {
+		t.Errorf("Annotations mismatch: diff(-want,+got):\n%s", cmp.Diff(want, got))
+	}
 
 	for _, c := range []apis.ConditionType{"Foo"} {
 		mgr.MarkFalse(c, "bad", "for business")
@@ -135,4 +144,10 @@ func TestConditionSet(t *testing.T) {
 		t.Errorf("len(s2.ObservedGeneration) = %d, wanted %d",
 			gotGeneration, wantGeneration)
 	}
+	s.Annotations = nil
+	s2 = &Status{}
+	s.ConvertTo(context.Background(), s2)
+	if s2.Annotations != nil {
+		t.Error("Annotations were not nil:", s2.Annotations)
+	}
 }