Merge pull request #562 from karlkfi/karl-filter-errors

feat: expose skip reason as error

Author: k8s-ci-robot

examples/alphaTestExamples/pruneAndDelete.md

@@ -133,6 +133,9 @@ Apply the three resources to the cluster.
 <!-- @runApply @testE2EAgainstLatestRelease -->
 ```
 kapply apply $BASE --reconcile-timeout=1m | tee $OUTPUT/status
+
+expectedOutputLine "3 resource(s) applied. 3 created, 0 unchanged, 0 configured, 0 failed"
+expectedOutputLine "3 resource(s) reconciled, 0 skipped, 0 failed to reconcile, 0 timed out"
 ```
 
 Use the preview command to show what will happen if we run destroy. This should
@@ -143,10 +146,9 @@ command.
 kapply preview --destroy $BASE | tee $OUTPUT/status
 
 expectedOutputLine "configmap/firstmap deleted"
-
-expectedOutputLine "configmap/secondmap delete skipped"
-
-expectedOutputLine "configmap/thirdmap delete skipped"
+expectedOutputLine 'configmap/secondmap delete skipped: annotation prevents deletion ("cli-utils.sigs.k8s.io/on-remove": "keep")'
+expectedOutputLine 'configmap/thirdmap delete skipped: annotation prevents deletion ("client.lifecycle.config.k8s.io/deletion": "detach")'
+expectedOutputLine "1 resource(s) deleted, 2 skipped"
 ```
 
 We run the destroy command and see that the resource without the annotations (firstmap)
@@ -157,12 +159,15 @@ cluster.
 kapply destroy $BASE | tee $OUTPUT/status
 
 expectedOutputLine "configmap/firstmap deleted"
+expectedOutputLine 'configmap/secondmap delete skipped: annotation prevents deletion ("cli-utils.sigs.k8s.io/on-remove": "keep")'
+expectedOutputLine 'configmap/thirdmap delete skipped: annotation prevents deletion ("client.lifecycle.config.k8s.io/deletion": "detach")'
+expectedOutputLine "1 resource(s) deleted, 2 skipped"
 
-expectedOutputLine "configmap/secondmap delete skipped"
-
-expectedOutputLine "configmap/thirdmap delete skipped"
+expectedOutputLine "configmap/firstmap reconciled"
+expectedOutputLine "configmap/secondmap reconcile skipped"
+expectedOutputLine "configmap/thirdmap reconcile skipped"
+expectedOutputLine "1 resource(s) reconciled, 2 skipped, 0 failed to reconcile, 0 timed out"
 
-expectedOutputLine "1 resource(s) deleted, 2 skipped"
 expectedNotFound "resource(s) pruned"
 
 kubectl get cm --no-headers | awk '{print $1}' | tee $OUTPUT/status
@@ -194,7 +199,6 @@ will instead be skipped due to the lifecycle directive.
 kapply preview $BASE | tee $OUTPUT/status
 
 expectedOutputLine "configmap/secondmap prune skipped"
-
 expectedOutputLine "configmap/thirdmap prune skipped"
 ```
 
@@ -204,7 +208,6 @@ Run apply and verify that secondmap and thirdmap are still in the cluster.
 kapply apply $BASE | tee $OUTPUT/status
 
 expectedOutputLine "configmap/secondmap prune skipped"
-
 expectedOutputLine "configmap/thirdmap prune skipped"
 
 kubectl get cm --no-headers | awk '{print $1}' | tee $OUTPUT/status

examples/alphaTestExamples/pruneNamespace.md

@@ -170,10 +170,21 @@ test-namespace should **not** be pruned.
 <!-- @applySecondTime @testE2EAgainstLatestRelease -->
 ```
 kapply apply $BASE --reconcile-timeout=1m | tee $OUTPUT/status
+
+expectedOutputLine "configmap/cm-c apply skipped: dependency scheduled for delete: _test-namespace__Namespace"
+expectedOutputLine "1 resource(s) applied. 0 created, 1 unchanged, 0 configured, 0 failed"
+expectedOutputLine "configmap/cm-c reconcile skipped"
+
 expectedOutputLine "configmap/cm-a pruned"
 expectedOutputLine "configmap/cm-b pruned"
-expectedOutputLine "configmap/cm-c unchanged"
-expectedOutputLine "2 resource(s) pruned, 1 skipped"
+expectedOutputLine "namespace/test-namespace prune skipped: namespace still in use: test-namespace"
+expectedOutputLine "2 resource(s) pruned, 1 skipped, 0 failed to prune"
+
+expectedOutputLine "namespace/test-namespace reconcile skipped"
+expectedOutputLine "configmap/cm-a reconciled"
+expectedOutputLine "configmap/cm-b reconciled"
+expectedOutputLine "configmap/cm-c reconcile skipped"
+expectedOutputLine "2 resource(s) reconciled, 2 skipped, 0 failed to reconcile, 0 timed out"
 
 # The test-namespace should not be pruned.
 kubectl get ns test-namespace --no-headers | wc -l | tee $OUTPUT/status

pkg/apply/applier.go

@@ -149,7 +149,7 @@ func (a *Applier) Run(ctx context.Context, invInfo inventory.Info, objects objec
 		// Build list of prune validation filters.
 		pruneFilters := []filter.ValidationFilter{
 			filter.PreventRemoveFilter{},
-			filter.InventoryPolicyFilter{
+			filter.InventoryPolicyPruneFilter{
 				Inv:       invInfo,
 				InvPolicy: options.InventoryPolicy,
 			},

pkg/apply/applier_test.go

@@ -5,7 +5,6 @@ package apply
 
 import (
 	"context"
-	"fmt"
 	"sort"
 	"sync"
 	"testing"
@@ -15,6 +14,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubectl/pkg/scheme"
+	"sigs.k8s.io/cli-utils/pkg/apis/actuation"
 	"sigs.k8s.io/cli-utils/pkg/apply/event"
 	"sigs.k8s.io/cli-utils/pkg/inventory"
 	pollevent "sigs.k8s.io/cli-utils/pkg/kstatus/polling/event"
@@ -875,7 +875,12 @@ func TestApplier(t *testing.T) {
 					ApplyEvent: &testutil.ExpApplyEvent{
 						GroupName:  "apply-0",
 						Identifier: testutil.ToIdentifier(t, resources["deployment"]),
-						Error:      testutil.EqualErrorType(inventory.NewInventoryOverlapError(fmt.Errorf(""))),
+						Operation:  event.Unchanged,
+						Error: &inventory.PolicyPreventedActuationError{
+							Strategy: actuation.ActuationStrategyApply,
+							Policy:   inventory.PolicyMustMatch,
+							Status:   inventory.NoMatch,
+						},
 					},
 				},
 				{
@@ -983,6 +988,11 @@ func TestApplier(t *testing.T) {
 						GroupName:  "prune-0",
 						Operation:  event.PruneSkipped,
 						Identifier: testutil.ToIdentifier(t, resources["deployment"]),
+						Error: &inventory.PolicyPreventedActuationError{
+							Strategy: actuation.ActuationStrategyDelete,
+							Policy:   inventory.PolicyMustMatch,
+							Status:   inventory.NoMatch,
+						},
 					},
 				},
 				{

pkg/apply/destroyer.go

@@ -142,7 +142,7 @@ func (d *Destroyer) Run(ctx context.Context, invInfo inventory.Info, options Des
 		}
 		deleteFilters := []filter.ValidationFilter{
 			filter.PreventRemoveFilter{},
-			filter.InventoryPolicyFilter{
+			filter.InventoryPolicyPruneFilter{
 				Inv:       invInfo,
 				InvPolicy: options.InventoryPolicy,
 			},

pkg/apply/event/event.go

@@ -261,15 +261,13 @@ type PruneEvent struct {
 	Identifier object.ObjMetadata
 	Operation  PruneEventOperation
 	Object     *unstructured.Unstructured
-	// If prune is skipped, this reason string explains why
-	Reason string
-	Error  error
+	Error      error
 }
 
 // String returns a string suitable for logging
 func (pe PruneEvent) String() string {
-	return fmt.Sprintf("PruneEvent{ GroupName: %q, Operation: %q, Identifier: %q, Reason: %q, Error: %q }",
-		pe.GroupName, pe.Operation, pe.Identifier, pe.Reason, pe.Error)
+	return fmt.Sprintf("PruneEvent{ GroupName: %q, Operation: %q, Identifier: %q, Error: %q }",
+		pe.GroupName, pe.Operation, pe.Identifier, pe.Error)
 }
 
 //go:generate stringer -type=DeleteEventOperation
@@ -286,15 +284,13 @@ type DeleteEvent struct {
 	Identifier object.ObjMetadata
 	Operation  DeleteEventOperation
 	Object     *unstructured.Unstructured
-	// If delete is skipped, this reason string explains why
-	Reason string
-	Error  error
+	Error      error
 }
 
 // String returns a string suitable for logging
 func (de DeleteEvent) String() string {
-	return fmt.Sprintf("DeleteEvent{ GroupName: %q, Operation: %q, Identifier: %q, Reason: %q, Error: %q }",
-		de.GroupName, de.Operation, de.Identifier, de.Reason, de.Error)
+	return fmt.Sprintf("DeleteEvent{ GroupName: %q, Operation: %q, Identifier: %q, Error: %q }",
+		de.GroupName, de.Operation, de.Identifier, de.Error)
 }
 
 type ValidationEvent struct {

pkg/apply/filter/current-uids-filter.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
@@ -22,13 +23,31 @@ func (cuf CurrentUIDFilter) Name() string {
 	return "CurrentUIDFilter"
 }
 
-// Filter returns true if the passed object should NOT be pruned (deleted)
-// because it has recently been applied.
-func (cuf CurrentUIDFilter) Filter(obj *unstructured.Unstructured) (bool, string, error) {
-	uid := string(obj.GetUID())
-	if cuf.CurrentUIDs.Has(uid) {
-		reason := fmt.Sprintf("resource just applied (UID: %q)", uid)
-		return true, reason, nil
+// Filter returns a ApplyPreventedDeletionError if the object prune/delete
+// should be skipped.
+func (cuf CurrentUIDFilter) Filter(obj *unstructured.Unstructured) error {
+	uid := obj.GetUID()
+	if cuf.CurrentUIDs.Has(string(uid)) {
+		return &ApplyPreventedDeletionError{UID: uid}
 	}
-	return false, "", nil
+	return nil
+}
+
+type ApplyPreventedDeletionError struct {
+	UID types.UID
+}
+
+func (e *ApplyPreventedDeletionError) Error() string {
+	return fmt.Sprintf("object just applied (UID: %q)", e.UID)
+}
+
+func (e *ApplyPreventedDeletionError) Is(err error) bool {
+	if err == nil {
+		return false
+	}
+	tErr, ok := err.(*ApplyPreventedDeletionError)
+	if !ok {
+		return false
+	}
+	return e.UID == tErr.UID
 }

pkg/apply/filter/current-uids-filter_test.go

@@ -8,38 +8,36 @@ import (
 
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"sigs.k8s.io/cli-utils/pkg/testutil"
 )
 
 func TestCurrentUIDFilter(t *testing.T) {
 	tests := map[string]struct {
-		filterUIDs sets.String
-		objUID     string
-		filtered   bool
+		filterUIDs    sets.String
+		objUID        string
+		expectedError error
 	}{
 		"Empty filter UIDs, object is not filtered": {
 			filterUIDs: sets.NewString(),
 			objUID:     "bar",
-			filtered:   false,
 		},
 		"Empty object UID, object is not filtered": {
 			filterUIDs: sets.NewString("foo"),
 			objUID:     "",
-			filtered:   false,
 		},
 		"Object UID not in filter UID set, object is not filtered": {
 			filterUIDs: sets.NewString("foo", "baz"),
 			objUID:     "bar",
-			filtered:   false,
 		},
 		"Object UID is in filter UID set, object is filtered": {
-			filterUIDs: sets.NewString("foo"),
-			objUID:     "foo",
-			filtered:   true,
+			filterUIDs:    sets.NewString("foo"),
+			objUID:        "foo",
+			expectedError: &ApplyPreventedDeletionError{UID: "foo"},
 		},
 		"Object UID is among several filter UIDs, object is filtered": {
-			filterUIDs: sets.NewString("foo", "bar", "baz"),
-			objUID:     "foo",
-			filtered:   true,
+			filterUIDs:    sets.NewString("foo", "bar", "baz"),
+			objUID:        "foo",
+			expectedError: &ApplyPreventedDeletionError{UID: "foo"},
 		},
 	}
 
@@ -50,19 +48,8 @@ func TestCurrentUIDFilter(t *testing.T) {
 			}
 			obj := defaultObj.DeepCopy()
 			obj.SetUID(types.UID(tc.objUID))
-			actual, reason, err := filter.Filter(obj)
-			if err != nil {
-				t.Fatalf("CurrentUIDFilter unexpected error (%s)", err)
-			}
-			if tc.filtered != actual {
-				t.Errorf("CurrentUIDFilter expected filter (%t), got (%t)", tc.filtered, actual)
-			}
-			if tc.filtered && len(reason) == 0 {
-				t.Errorf("CurrentUIDFilter filtered; expected but missing Reason")
-			}
-			if !tc.filtered && len(reason) > 0 {
-				t.Errorf("CurrentUIDFilter not filtered; received unexpected Reason: %s", reason)
-			}
+			err := filter.Filter(obj)
+			testutil.AssertEqual(t, tc.expectedError, err)
 		})
 	}
 }