Address reviewer comments

Signed-off-by: Predrag Knezevic <predrag.knezevic@upbound.io>

apis/common/v1/condition.go

@@ -85,7 +85,7 @@ type Condition struct {
 }
 
 // Equal returns true if the condition is identical to the supplied condition,
-// ignoring the LastTransitionTime.
+// ignoring the LastTransitionTime and ObservedGeneration.
 func (c Condition) Equal(other Condition) bool {
 	return c.Type == other.Type &&
 		c.Status == other.Status &&

pkg/resource/unstructured/claim/claim.go

@@ -223,7 +223,7 @@ func (c *Unstructured) SetPublishConnectionDetailsTo(ref *xpv1.PublishConnection
 
 // GetCondition of this composite resource claim.
 func (c *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
-	conditioned := xpv1.ResourceStatus{}
+	conditioned := xpv1.ConditionedStatus{}
 	// The path is directly `status` because conditions are inline.
 	if err := fieldpath.Pave(c.Object).GetValueInto("status", &conditioned); err != nil {
 		return xpv1.Condition{}
@@ -233,7 +233,7 @@ func (c *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
 
 // SetConditions of this composite resource claim.
 func (c *Unstructured) SetConditions(conditions ...xpv1.Condition) {
-	conditioned := xpv1.ResourceStatus{}
+	conditioned := xpv1.ConditionedStatus{}
 	// The path is directly `status` because conditions are inline.
 	_ = fieldpath.Pave(c.Object).GetValueInto("status", &conditioned)
 	conditioned.SetConditions(conditions...)
@@ -256,15 +256,15 @@ func (c *Unstructured) SetConnectionDetailsLastPublishedTime(t *metav1.Time) {
 
 // SetObservedGeneration of this composite resource claim.
 func (c *Unstructured) SetObservedGeneration(generation int64) {
-	status := &xpv1.ResourceStatus{}
+	status := &xpv1.ObservedStatus{}
 	_ = fieldpath.Pave(c.Object).GetValueInto("status", status)
 	status.SetObservedGeneration(generation)
-	_ = fieldpath.Pave(c.Object).SetValue("status", status)
+	_ = fieldpath.Pave(c.Object).SetValue("status.observedGeneration", status.ObservedGeneration)
 }
 
 // GetObservedGeneration of this composite resource claim.
 func (c *Unstructured) GetObservedGeneration() int64 {
-	status := &xpv1.ResourceStatus{}
+	status := &xpv1.ObservedStatus{}
 	_ = fieldpath.Pave(c.Object).GetValueInto("status", status)
 	return status.GetObservedGeneration()
 }

pkg/resource/unstructured/claim/claim_test.go

@@ -370,22 +370,26 @@ func TestConnectionDetailsLastPublishedTime(t *testing.T) {
 }
 
 func TestObservedGeneration(t *testing.T) {
-	u := &Unstructured{unstructured.Unstructured{Object: map[string]any{}}}
-	want := int64(123)
-	u.SetObservedGeneration(want)
-
-	if got := u.GetObservedGeneration(); got != want {
-		t.Errorf("u.GetObservedGeneration() got: %v, want %v", got, want)
-	}
-	if g := u.GetUnstructured().Object["status"].(map[string]any)["observedGeneration"]; g != want {
-		t.Errorf("Generations do not match! got: %v (%T)", g, g)
+	cases := map[string]struct {
+		u    *Unstructured
+		want int64
+	}{
+		"Set": {
+			u: New(func(u *Unstructured) {
+				u.SetObservedGeneration(123)
+			}),
+			want: 123,
+		},
+		"NotFound": {
+			u: New(),
+		},
 	}
-}
-
-func TestObservedGenerationNotFound(t *testing.T) {
-	u := &Unstructured{unstructured.Unstructured{Object: map[string]any{}}}
-
-	if g := u.GetObservedGeneration(); g != 0 {
-		t.Errorf("u.GetObservedGeneration(): expected nil, but got %v", g)
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got := tc.u.GetObservedGeneration()
+			if diff := cmp.Diff(tc.want, got); diff != "" {
+				t.Errorf("\nu.GetObservedGeneration(): -want, +got:\n%s", diff)
+			}
+		})
 	}
 }

pkg/resource/unstructured/composed/composed.go

@@ -72,7 +72,7 @@ func (cr *Unstructured) GetUnstructured() *unstructured.Unstructured {
 
 // GetCondition of this Composed resource.
 func (cr *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
-	conditioned := xpv1.ResourceStatus{}
+	conditioned := xpv1.ConditionedStatus{}
 	// The path is directly `status` because conditions are inline.
 	if err := fieldpath.Pave(cr.Object).GetValueInto("status", &conditioned); err != nil {
 		return xpv1.Condition{}
@@ -82,7 +82,7 @@ func (cr *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
 
 // SetConditions of this Composed resource.
 func (cr *Unstructured) SetConditions(c ...xpv1.Condition) {
-	conditioned := xpv1.ResourceStatus{}
+	conditioned := xpv1.ConditionedStatus{}
 	// The path is directly `status` because conditions are inline.
 	_ = fieldpath.Pave(cr.Object).GetValueInto("status", &conditioned)
 	conditioned.SetConditions(c...)
@@ -171,15 +171,15 @@ func (cr *UnstructuredList) GetUnstructuredList() *unstructured.UnstructuredList
 
 // SetObservedGeneration of this composite resource claim.
 func (cr *Unstructured) SetObservedGeneration(generation int64) {
-	status := &xpv1.ResourceStatus{}
+	status := &xpv1.ObservedStatus{}
 	_ = fieldpath.Pave(cr.Object).GetValueInto("status", status)
 	status.SetObservedGeneration(generation)
-	_ = fieldpath.Pave(cr.Object).SetValue("status", status)
+	_ = fieldpath.Pave(cr.Object).SetValue("status.observedGeneration", status.ObservedGeneration)
 }
 
 // GetObservedGeneration of this composite resource claim.
 func (cr *Unstructured) GetObservedGeneration() int64 {
-	status := &xpv1.ResourceStatus{}
+	status := &xpv1.ObservedStatus{}
 	_ = fieldpath.Pave(cr.Object).GetValueInto("status", status)
 	return status.GetObservedGeneration()
 }

pkg/resource/unstructured/composed/composed_test.go

@@ -148,22 +148,26 @@ func TestWriteConnectionSecretToReference(t *testing.T) {
 }
 
 func TestObservedGeneration(t *testing.T) {
-	u := &Unstructured{unstructured.Unstructured{Object: map[string]any{}}}
-	want := int64(123)
-	u.SetObservedGeneration(want)
-
-	if got := u.GetObservedGeneration(); got != want {
-		t.Errorf("u.GetObservedGeneration() got: %v, want %v", got, want)
-	}
-	if g := u.GetUnstructured().Object["status"].(map[string]any)["observedGeneration"]; g != want {
-		t.Errorf("Generations do not match! got: %v (%T)", g, g)
+	cases := map[string]struct {
+		u    *Unstructured
+		want int64
+	}{
+		"Set": {
+			u: New(func(u *Unstructured) {
+				u.SetObservedGeneration(123)
+			}),
+			want: 123,
+		},
+		"NotFound": {
+			u: New(),
+		},
 	}
-}
-
-func TestObservedGenerationNotFound(t *testing.T) {
-	u := &Unstructured{unstructured.Unstructured{Object: map[string]any{}}}
-
-	if g := u.GetObservedGeneration(); g != 0 {
-		t.Errorf("u.GetObservedGeneration(): expected nil, but got %v", g)
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got := tc.u.GetObservedGeneration()
+			if diff := cmp.Diff(tc.want, got); diff != "" {
+				t.Errorf("\nu.GetObservedGeneration(): -want, +got:\n%s", diff)
+			}
+		})
 	}
 }

pkg/resource/unstructured/composite/composite.go

@@ -206,7 +206,7 @@ func (c *Unstructured) SetPublishConnectionDetailsTo(ref *xpv1.PublishConnection
 
 // GetCondition of this Composite resource.
 func (c *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
-	conditioned := xpv1.ResourceStatus{}
+	conditioned := xpv1.ConditionedStatus{}
 	// The path is directly `status` because conditions are inline.
 	if err := fieldpath.Pave(c.Object).GetValueInto("status", &conditioned); err != nil {
 		return xpv1.Condition{}
@@ -216,7 +216,7 @@ func (c *Unstructured) GetCondition(ct xpv1.ConditionType) xpv1.Condition {
 
 // SetConditions of this Composite resource.
 func (c *Unstructured) SetConditions(conditions ...xpv1.Condition) {
-	conditioned := xpv1.ResourceStatus{}
+	conditioned := xpv1.ConditionedStatus{}
 	// The path is directly `status` because conditions are inline.
 	_ = fieldpath.Pave(c.Object).GetValueInto("status", &conditioned)
 	conditioned.SetConditions(conditions...)
@@ -261,15 +261,15 @@ func (c *Unstructured) SetEnvironmentConfigReferences(refs []corev1.ObjectRefere
 
 // SetObservedGeneration of this composite resource claim.
 func (c *Unstructured) SetObservedGeneration(generation int64) {
-	status := &xpv1.ResourceStatus{}
+	status := &xpv1.ObservedStatus{}
 	_ = fieldpath.Pave(c.Object).GetValueInto("status", status)
 	status.SetObservedGeneration(generation)
-	_ = fieldpath.Pave(c.Object).SetValue("status", status)
+	_ = fieldpath.Pave(c.Object).SetValue("status.observedGeneration", status.ObservedGeneration)
 }
 
 // GetObservedGeneration of this composite resource claim.
 func (c *Unstructured) GetObservedGeneration() int64 {
-	status := &xpv1.ResourceStatus{}
+	status := &xpv1.ObservedStatus{}
 	_ = fieldpath.Pave(c.Object).GetValueInto("status", status)
 	return status.GetObservedGeneration()
 }

pkg/resource/unstructured/composite/composite_test.go

@@ -358,22 +358,26 @@ func TestConnectionDetailsLastPublishedTime(t *testing.T) {
 }
 
 func TestObservedGeneration(t *testing.T) {
-	u := &Unstructured{unstructured.Unstructured{Object: map[string]any{}}}
-	want := int64(123)
-	u.SetObservedGeneration(want)
-
-	if got := u.GetObservedGeneration(); got != want {
-		t.Errorf("u.GetObservedGeneration() got: %v, want %v", got, want)
-	}
-	if g := u.GetUnstructured().Object["status"].(map[string]any)["observedGeneration"]; g != want {
-		t.Errorf("Generations do not match! got: %v (%T)", g, g)
+	cases := map[string]struct {
+		u    *Unstructured
+		want int64
+	}{
+		"Set": {
+			u: New(func(u *Unstructured) {
+				u.SetObservedGeneration(123)
+			}),
+			want: 123,
+		},
+		"NotFound": {
+			u: New(),
+		},
 	}
-}
-
-func TestObservedGenerationNotFound(t *testing.T) {
-	u := &Unstructured{unstructured.Unstructured{Object: map[string]any{}}}
-
-	if g := u.GetObservedGeneration(); g != 0 {
-		t.Errorf("u.GetObservedGeneration(): expected nil, but got %v", g)
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got := tc.u.GetObservedGeneration()
+			if diff := cmp.Diff(tc.want, got); diff != "" {
+				t.Errorf("\nu.GetObservedGeneration(): -want, +got:\n%s", diff)
+			}
+		})
 	}
 }