code review

porch/controllers/config/crd/bases/config.porch.kpt.dev_packagevariantsets.yaml

@@ -245,10 +245,75 @@ spec:
           status:
             description: PackageVariantSetStatus defines the observed state of PackageVariantSet
             properties:
-              validationErrors:
-                description: 'TODO: Move this to conditions.'
+              conditions:
+                description: Conditions describes the reconciliation state of the
+                  object.
                 items:
-                  type: string
+                  description: "Condition contains details for one aspect of the current
+                    state of this API Resource. --- This struct is intended for direct
+                    use as an array at the field path .status.conditions.  For example,
+                    \n type FooStatus struct{ // Represents the observations of a
+                    foo's current state. // Known .status.conditions.type are: \"Available\",
+                    \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge
+                    // +listType=map // +listMapKey=type Conditions []metav1.Condition
+                    `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"
+                    protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }"
+                  properties:
+                    lastTransitionTime:
+                      description: lastTransitionTime is the last time the condition
+                        transitioned from one status to another. This should be when
+                        the underlying condition changed.  If that is not known, then
+                        using the time when the API field changed is acceptable.
+                      format: date-time
+                      type: string
+                    message:
+                      description: message is a human readable message indicating
+                        details about the transition. This may be an empty string.
+                      maxLength: 32768
+                      type: string
+                    observedGeneration:
+                      description: observedGeneration represents the .metadata.generation
+                        that the condition was set based upon. For instance, if .metadata.generation
+                        is currently 12, but the .status.conditions[x].observedGeneration
+                        is 9, the condition is out of date with respect to the current
+                        state of the instance.
+                      format: int64
+                      minimum: 0
+                      type: integer
+                    reason:
+                      description: reason contains a programmatic identifier indicating
+                        the reason for the condition's last transition. Producers
+                        of specific condition types may define expected values and
+                        meanings for this field, and whether the values are considered
+                        a guaranteed API. The value should be a CamelCase string.
+                        This field may not be empty.
+                      maxLength: 1024
+                      minLength: 1
+                      pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$
+                      type: string
+                    status:
+                      description: status of the condition, one of True, False, Unknown.
+                      enum:
+                      - "True"
+                      - "False"
+                      - Unknown
+                      type: string
+                    type:
+                      description: type of condition in CamelCase or in foo.example.com/CamelCase.
+                        --- Many .condition.type values are consistent across resources
+                        like Available, but because arbitrary conditions can be useful
+                        (see .node.status.conditions), the ability to deconflict is
+                        important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
+                      maxLength: 316
+                      pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$
+                      type: string
+                  required:
+                  - lastTransitionTime
+                  - message
+                  - reason
+                  - status
+                  - type
+                  type: object
                 type: array
             type: object
         type: object

porch/controllers/packagevariantsets/api/v1alpha1/packagevariantset_types.go

@@ -132,8 +132,8 @@ type ValueOrFromField struct {
 
 // PackageVariantSetStatus defines the observed state of PackageVariantSet
 type PackageVariantSetStatus struct {
-	// TODO: Move this to conditions.
-	ValidationErrors []string `json:"validationErrors,omitempty"`
+	// Conditions describes the reconciliation state of the object.
+	Conditions []metav1.Condition `json:"conditions,omitempty"`
 }
 
 //+kubebuilder:object:root=true

porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go

@@ -25,24 +25,29 @@ import (
 )
 
 type fakeClient struct {
-	output []string
+	objects []client.Object
 	client.Client
 }
 
 var _ client.Client = &fakeClient{}
 
 func (f *fakeClient) Create(_ context.Context, obj client.Object, _ ...client.CreateOption) error {
-	f.output = append(f.output, fmt.Sprintf("creating object: %s", obj.GetName()))
+	f.objects = append(f.objects, obj)
 	return nil
 }
 
 func (f *fakeClient) Delete(_ context.Context, obj client.Object, _ ...client.DeleteOption) error {
-	f.output = append(f.output, fmt.Sprintf("deleting object: %s", obj.GetName()))
+	var newObjects []client.Object
+	for _, old := range f.objects {
+		if obj.GetName() != old.GetName() {
+			newObjects = append(newObjects, old)
+		}
+	}
+	f.objects = newObjects
 	return nil
 }
 
 func (f *fakeClient) List(_ context.Context, obj client.ObjectList, _ ...client.ListOption) error {
-	f.output = append(f.output, fmt.Sprintf("listing objects"))
 	podList := `apiVersion: v1
 kind: PodList
 metadata:
@@ -93,12 +98,20 @@ items:
       repo: dn-2
       package: dn-2`
 
+	var err error
 	switch v := obj.(type) {
 	case *unstructured.UnstructuredList:
-		return yaml.Unmarshal([]byte(podList), v)
+		err = yaml.Unmarshal([]byte(podList), v)
+		for _, o := range v.Items {
+			f.objects = append(f.objects, o.DeepCopy())
+		}
 	case *pkgvarapi.PackageVariantList:
-		return yaml.Unmarshal([]byte(pvList), v)
+		err = yaml.Unmarshal([]byte(pvList), v)
+		for _, o := range v.Items {
+			f.objects = append(f.objects, o.DeepCopy())
+		}
 	default:
-		return nil
+		return fmt.Errorf("unsupported type")
 	}
+	return err
 }

porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go

@@ -38,6 +38,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/client-go/kubernetes/scheme"
+	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
@@ -80,13 +81,21 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re
 		return ctrl.Result{}, nil
 	}
 
+	var conditions []metav1.Condition
+
 	if errs := validatePackageVariantSet(pvs); len(errs) > 0 {
-		pvs.Status.ValidationErrors = nil
 		for _, validationErr := range errs {
 			if validationErr.Error() != "" {
-				pvs.Status.ValidationErrors = append(pvs.Status.ValidationErrors, validationErr.Error())
+				conditions = append(conditions, metav1.Condition{
+					Type:               "Valid",
+					Status:             "False",
+					Reason:             "ValidationFailed",
+					Message:            validationErr.Error(),
+					LastTransitionTime: metav1.Now(),
+				})
 			}
 		}
+		pvs.Status.Conditions = conditions
 		statusUpdateErr := r.Client.Status().Update(ctx, pvs)
 		return ctrl.Result{}, statusUpdateErr
 	}
@@ -102,7 +111,20 @@ func (r *PackageVariantSetReconciler) Reconcile(ctx context.Context, req ctrl.Re
 	downstreams, err := r.unrollDownstreamTargets(ctx, pvs,
 		upstream.Package)
 	if err != nil {
-		return ctrl.Result{}, err
+		if strings.Contains(err.Error(), "unable to list specified objects in cluster") {
+			conditions = append(conditions, metav1.Condition{
+				Type:               "ObjectsListed",
+				Status:             "False",
+				Reason:             "Error",
+				Message:            err.Error(),
+				LastTransitionTime: metav1.Now(),
+			})
+			pvs.Status.Conditions = conditions
+			statusUpdateErr := r.Client.Status().Update(ctx, pvs)
+			return ctrl.Result{}, statusUpdateErr
+		} else {
+			return ctrl.Result{}, err
+		}
 	}
 
 	if err := r.ensurePackageVariants(ctx, upstream, downstreams, pvs); err != nil {
@@ -231,7 +253,7 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex
 	pvs *api.PackageVariantSet,
 	upstreamPackageName string) ([]*pkgvarapi.Downstream, error) {
 	var result []*pkgvarapi.Downstream
-	for _, target := range pvs.Spec.Targets {
+	for i, target := range pvs.Spec.Targets {
 		switch {
 		case target.Package != nil:
 			// an explicit repo/package name pair
@@ -249,6 +271,9 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex
 				client.MatchingLabelsSelector{Selector: selector}); err != nil {
 				return nil, err
 			}
+			if len(repoList.Items) == 0 {
+				klog.Warningf("no repositories selected by spec.targets[%d]", i)
+			}
 			pkgs, err := r.repositorySet(&target, upstreamPackageName, &repoList)
 			if err != nil {
 				return nil, fmt.Errorf("error when selecting repository set: %v", err)
@@ -261,6 +286,9 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex
 			if err != nil {
 				return nil, err
 			}
+			if len(selectedObjects) == 0 {
+				klog.Warningf("no objects selected by spec.targets[%d]", i)
+			}
 			pkgs, err := r.objectSet(&target, upstreamPackageName, selectedObjects)
 			if err != nil {
 				return nil, fmt.Errorf("error when selecting object set: %v", err)
@@ -343,15 +371,16 @@ func (r *PackageVariantSetReconciler) getSelectedObjects(ctx context.Context, se
 			Kind:    selector.Kind,
 		})
 
-		labelSelector, err := metav1.LabelSelectorAsSelector(selector.Labels)
-		if err != nil {
-			return nil, err
+		opts := []client.ListOption{client.InNamespace(selector.Namespace)}
+		if selector.Labels != nil {
+			labelSelector, err := metav1.LabelSelectorAsSelector(selector.Labels)
+			if err != nil {
+				return nil, err
+			}
+			opts = append(opts, client.MatchingLabelsSelector{Selector: labelSelector})
 		}
-
-		if err := r.Client.List(ctx, uList,
-			client.InNamespace(selector.Namespace),
-			client.MatchingLabelsSelector{Selector: labelSelector}); err != nil {
-			return nil, fmt.Errorf("unable to list objects in cluster: %v", err)
+		if err := r.Client.List(ctx, uList, opts...); err != nil {
+			return nil, fmt.Errorf("unable to list specified objects in cluster: %v", err)
 		}
 
 		for _, u := range uList.Items {
@@ -446,7 +475,6 @@ func (r *PackageVariantSetReconciler) fetchValue(value *api.ValueOrFromField,
 func (r *PackageVariantSetReconciler) ensurePackageVariants(ctx context.Context,
 	upstream *pkgvarapi.Upstream, downstreams []*pkgvarapi.Downstream,
 	pvs *api.PackageVariantSet) error {
-
 	var pvList pkgvarapi.PackageVariantList
 	if err := r.Client.List(ctx, &pvList,
 		client.InNamespace(pvs.Namespace),

porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go

@@ -353,13 +353,11 @@ func TestEnsurePackageVariants(t *testing.T) {
 		{Repo: "dn-1", Package: "dn-1"},
 		{Repo: "dn-3", Package: "dn-3"},
 	}
-	pvs := &api.PackageVariantSet{ObjectMeta: metav1.ObjectMeta{Name: "my-pvs"}}
-
 	fc := &fakeClient{}
 	reconciler := &PackageVariantSetReconciler{Client: fc}
-	require.NoError(t, reconciler.ensurePackageVariants(context.Background(), upstream, downstreams, pvs))
-	require.Equal(t, []string{"listing objects",
-		"deleting object: my-pv-2",
-		"creating object: my-pvs-59bfcf77a6b032a656d78b14e2d92fa8c1e978a3",
-	}, fc.output)
+	require.NoError(t, reconciler.ensurePackageVariants(context.Background(), upstream, downstreams,
+		&api.PackageVariantSet{ObjectMeta: metav1.ObjectMeta{Name: "my-pvs"}}))
+	require.Equal(t, 2, len(fc.objects))
+	require.Equal(t, "my-pv-1", fc.objects[0].GetName())
+	require.Equal(t, "my-pvs-28ace69e71f644931cd8cc1e8e9388f4de486901", fc.objects[1].GetName())
 }