Give better message to the use for missing value at path (#390)

pkg/controller/workload/conditions.go

@@ -16,8 +16,10 @@ package workload
 
 import (
 	"fmt"
+	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	"github.com/vmware-tanzu/cartographer/pkg/apis/v1alpha1"
 )
@@ -87,12 +89,25 @@ func TemplateObjectRetrievalFailureCondition(err error) metav1.Condition {
 	}
 }
 
-func MissingValueAtPathCondition(resourceName, expression string) metav1.Condition {
+func MissingValueAtPathCondition(obj *unstructured.Unstructured, expression string) metav1.Condition {
+	var fullyQualifiedType string
+	if obj.GetObjectKind().GroupVersionKind().Group == "" {
+		fullyQualifiedType = strings.ToLower(obj.GetKind())
+	} else {
+		fullyQualifiedType = fmt.Sprintf("%s.%s", strings.ToLower(obj.GetKind()),
+			obj.GetObjectKind().GroupVersionKind().Group)
+	}
+
+	var namespaceMsg string
+	if obj.GetNamespace() != "" {
+		namespaceMsg = fmt.Sprintf(" in namespace [%s]", obj.GetNamespace())
+	}
 	return metav1.Condition{
-		Type:    v1alpha1.WorkloadResourceSubmitted,
-		Status:  metav1.ConditionUnknown,
-		Reason:  v1alpha1.MissingValueAtPathResourcesSubmittedReason,
-		Message: fmt.Sprintf("Resource '%s' is waiting to read value '%s'", resourceName, expression),
+		Type:   v1alpha1.WorkloadResourceSubmitted,
+		Status: metav1.ConditionUnknown,
+		Reason: v1alpha1.MissingValueAtPathResourcesSubmittedReason,
+		Message: fmt.Sprintf("Waiting to read value [%s] from resource [%s/%s]%s",
+			expression, fullyQualifiedType, obj.GetName(), namespaceMsg),
 	}
 }
 

pkg/controller/workload/conditions_test.go

@@ -0,0 +1,56 @@
+// Copyright 2021 VMware
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package workload_test
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	"github.com/vmware-tanzu/cartographer/pkg/controller/workload"
+)
+
+var _ = Describe("Conditions", func() {
+
+	Describe("MissingValueAtPath", func() {
+		var obj *unstructured.Unstructured
+		BeforeEach(func() {
+			obj = &unstructured.Unstructured{}
+			obj.SetGroupVersionKind(schema.GroupVersionKind{
+				Group:   "thing.io",
+				Version: "alphabeta1",
+				Kind:    "Widget",
+			})
+			obj.SetName("my-widget")
+		})
+
+		Context("stamped object has a namespace", func() {
+			It("has the correct message", func() {
+				obj.SetNamespace("my-ns")
+
+				condition := workload.MissingValueAtPathCondition(obj, "spec.foo")
+				Expect(condition.Message).To(Equal("Waiting to read value [spec.foo] from resource [widget.thing.io/my-widget] in namespace [my-ns]"))
+			})
+		})
+
+		Context("stamped object does not have a namespace", func() {
+			It("has the correct message", func() {
+				condition := workload.MissingValueAtPathCondition(obj, "spec.foo")
+				Expect(condition.Message).To(Equal("Waiting to read value [spec.foo] from resource [widget.thing.io/my-widget]"))
+			})
+		})
+	})
+})

pkg/controller/workload/reconciler.go

@@ -91,7 +91,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 			r.conditionManager.AddPositive(TemplateRejectedByAPIServerCondition(typedErr))
 			err = controller.NewUnhandledError(err)
 		case realizer.RetrieveOutputError:
-			r.conditionManager.AddPositive(MissingValueAtPathCondition(typedErr.Resource.Name, typedErr.JsonPathExpression()))
+			r.conditionManager.AddPositive(MissingValueAtPathCondition(typedErr.StampedObject, typedErr.JsonPathExpression()))
 		default:
 			r.conditionManager.AddPositive(UnknownResourceErrorCondition(typedErr))
 			err = controller.NewUnhandledError(err)

pkg/controller/workload/reconciler_test.go

@@ -378,18 +378,29 @@ var _ = Describe("Reconciler", func() {
 
 			Context("of type RetrieveOutputError", func() {
 				var retrieveError realizer.RetrieveOutputError
+				var stampedObject *unstructured.Unstructured
 				BeforeEach(func() {
+					stampedObject = &unstructured.Unstructured{}
+					stampedObject.SetGroupVersionKind(schema.GroupVersionKind{
+						Group:   "thing.io",
+						Version: "alphabeta1",
+						Kind:    "MyThing",
+					})
+					stampedObject.SetName("my-obj")
+					stampedObject.SetNamespace("my-ns")
 					jsonPathError := templates.NewJsonPathError("this.wont.find.anything", errors.New("some error"))
 					retrieveError = realizer.RetrieveOutputError{
-						Err:      jsonPathError,
-						Resource: &v1alpha1.SupplyChainResource{Name: "some-resource"},
+						Err:           jsonPathError,
+						Resource:      &v1alpha1.SupplyChainResource{Name: "some-resource"},
+						StampedObject: stampedObject,
 					}
 					rlzr.RealizeReturns(nil, retrieveError)
 				})
 
 				It("calls the condition manager to report", func() {
 					_, _ = reconciler.Reconcile(ctx, req)
-					Expect(conditionManager.AddPositiveArgsForCall(1)).To(Equal(workload.MissingValueAtPathCondition("some-resource", "this.wont.find.anything")))
+					Expect(conditionManager.AddPositiveArgsForCall(1)).To(
+						Equal(workload.MissingValueAtPathCondition(stampedObject, "this.wont.find.anything")))
 				})
 
 				It("does not return an error", func() {
@@ -402,7 +413,7 @@ var _ = Describe("Reconciler", func() {
 
 					Expect(out).To(Say(`"level":"info"`))
 					Expect(out).To(Say(`"msg":"handled error"`))
-					Expect(out).To(Say(`"error":"unable to retrieve outputs from stamped object for resource 'some-resource': evaluate json path 'this.wont.find.anything': some error"`))
+					Expect(out).To(Say(`"unable to retrieve outputs \[this.wont.find.anything\] from stamped object \[my-ns/my-obj\] of type \[mything.thing.io\] for resource \[some-resource\]: evaluate json path 'this.wont.find.anything': some error"`))
 				})
 			})
 

pkg/realizer/workload/component.go

@@ -111,8 +111,9 @@ func (r *resourceRealizer) Do(ctx context.Context, resource *v1alpha1.SupplyChai
 	output, err := template.GetOutput()
 	if err != nil {
 		return stampedObject, nil, RetrieveOutputError{
-			Err:      err,
-			Resource: resource,
+			Err:           err,
+			Resource:      resource,
+			StampedObject: stampedObject,
 		}
 	}
 

pkg/realizer/workload/errors.go

@@ -16,6 +16,7 @@ package workload
 
 import (
 	"fmt"
+	"strings"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
@@ -50,12 +51,16 @@ func (e StampError) Error() string {
 }
 
 type RetrieveOutputError struct {
-	Err      error
-	Resource *v1alpha1.SupplyChainResource
+	Err           error
+	Resource      *v1alpha1.SupplyChainResource
+	StampedObject *unstructured.Unstructured
 }
 
 func (e RetrieveOutputError) Error() string {
-	return fmt.Errorf("unable to retrieve outputs from stamped object for resource '%s': %w", e.Resource.Name, e.Err).Error()
+	return fmt.Errorf("unable to retrieve outputs [%s] from stamped object [%s/%s] of type [%s.%s] for resource [%s]: %w",
+		e.JsonPathExpression(), e.StampedObject.GetNamespace(), e.StampedObject.GetName(),
+		strings.ToLower(e.StampedObject.GetKind()), e.StampedObject.GetObjectKind().GroupVersionKind().Group,
+		e.Resource.Name, e.Err).Error()
 }
 
 type JsonPathErrorContext interface {

tests/kuttl/supplychain/unknown/02-assert.yaml

@@ -26,11 +26,9 @@ status:
     - type: ResourcesSubmitted
       status: "Unknown"
       reason: MissingValueAtPath
-      message: "Resource 'image-provider' is waiting to read value '.data.best_school'"
     - type: Ready
       status: "Unknown"
       reason: MissingValueAtPath
-      message: "Resource 'image-provider' is waiting to read value '.data.best_school'"
   supplyChainRef:
     name: responsible-ops-unknown
     kind: ClusterSupplyChain