migrate more backup action tests, remove obsolete test code

pkg/backup/backup_new_test.go

@@ -34,6 +34,7 @@ import (
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -42,13 +43,15 @@ import (
 	dynamicfake "k8s.io/client-go/dynamic/fake"
 	kubefake "k8s.io/client-go/kubernetes/fake"
 
+	v1 "github.com/heptio/velero/pkg/apis/velero/v1"
 	velerov1 "github.com/heptio/velero/pkg/apis/velero/v1"
 	"github.com/heptio/velero/pkg/client"
 	"github.com/heptio/velero/pkg/discovery"
 	"github.com/heptio/velero/pkg/generated/clientset/versioned/fake"
 	"github.com/heptio/velero/pkg/kuberesource"
 	"github.com/heptio/velero/pkg/plugin/velero"
 	"github.com/heptio/velero/pkg/test"
+	kubeutil "github.com/heptio/velero/pkg/util/kube"
 )
 
 // TestBackupResourceFiltering runs backups with different combinations
@@ -618,6 +621,51 @@ func TestBackupResourceOrdering(t *testing.T) {
 	}
 }
 
+// recordResourcesAction is a backup item action that can be configured
+// to run for specific resources/namespaces and simply records the items
+// that it is executed for.
+type recordResourcesAction struct {
+	selector        velero.ResourceSelector
+	ids             []string
+	backups         []v1.Backup
+	additionalItems []velero.ResourceIdentifier
+}
+
+func (a *recordResourcesAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
+	metadata, err := meta.Accessor(item)
+	if err != nil {
+		return item, a.additionalItems, err
+	}
+	a.ids = append(a.ids, kubeutil.NamespaceAndName(metadata))
+	a.backups = append(a.backups, *backup)
+
+	return item, a.additionalItems, nil
+}
+
+func (a *recordResourcesAction) AppliesTo() (velero.ResourceSelector, error) {
+	return a.selector, nil
+}
+
+func (a *recordResourcesAction) ForResource(resource string) *recordResourcesAction {
+	a.selector.IncludedResources = append(a.selector.IncludedResources, resource)
+	return a
+}
+
+func (a *recordResourcesAction) ForNamespace(namespace string) *recordResourcesAction {
+	a.selector.IncludedNamespaces = append(a.selector.IncludedNamespaces, namespace)
+	return a
+}
+
+func (a *recordResourcesAction) ForLabelSelector(selector string) *recordResourcesAction {
+	a.selector.LabelSelector = selector
+	return a
+}
+
+func (a *recordResourcesAction) WithAdditionalItems(items []velero.ResourceIdentifier) *recordResourcesAction {
+	a.additionalItems = items
+	return a
+}
+
 // TestBackupActionsRunsForCorrectItems runs backups with backup item actions, and
 // verifies that each backup item action is run for the correct set of resources based on its
 // AppliesTo() resource selector. Verification is done by using the recordResourcesAction struct,
@@ -1156,6 +1204,34 @@ func TestBackupActionAdditionalItems(t *testing.T) {
 				"resources/pods/namespaces/ns-2/pod-2.json",
 			},
 		},
+		{
+			name:   "if there's an error backing up additional items, the item the action was run for isn't backed up",
+			backup: defaultBackup().Backup(),
+			apiResources: []*apiResource{
+				pods(
+					newPod("ns-1", "pod-1"),
+					newPod("ns-2", "pod-2"),
+					newPod("ns-3", "pod-3"),
+				),
+			},
+			actions: []velero.BackupItemAction{
+				&pluggableAction{
+					selector: velero.ResourceSelector{IncludedNamespaces: []string{"ns-1"}},
+					executeFunc: func(item runtime.Unstructured, backup *velerov1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
+						additionalItems := []velero.ResourceIdentifier{
+							{GroupResource: kuberesource.Pods, Namespace: "ns-4", Name: "pod-4"},
+							{GroupResource: kuberesource.Pods, Namespace: "ns-5", Name: "pod-5"},
+						}
+
+						return item, additionalItems, nil
+					},
+				},
+			},
+			want: []string{
+				"resources/pods/namespaces/ns-2/pod-2.json",
+				"resources/pods/namespaces/ns-3/pod-3.json",
+			},
+		},
 	}
 
 	for _, tc := range tests {

pkg/backup/backup_test.go

@@ -25,7 +25,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
-	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -39,63 +38,10 @@ import (
 	"github.com/heptio/velero/pkg/podexec"
 	"github.com/heptio/velero/pkg/restic"
 	"github.com/heptio/velero/pkg/util/collections"
-	kubeutil "github.com/heptio/velero/pkg/util/kube"
 	"github.com/heptio/velero/pkg/util/logging"
 	velerotest "github.com/heptio/velero/pkg/util/test"
 )
 
-var (
-	trueVal      = true
-	falseVal     = false
-	truePointer  = &trueVal
-	falsePointer = &falseVal
-)
-
-// recordResourcesAction is a backup item action that can be configured
-// to run for specific resources/namespaces and simply records the items
-// that it is executed for.
-type recordResourcesAction struct {
-	selector        velero.ResourceSelector
-	ids             []string
-	backups         []v1.Backup
-	additionalItems []velero.ResourceIdentifier
-}
-
-func (a *recordResourcesAction) Execute(item runtime.Unstructured, backup *v1.Backup) (runtime.Unstructured, []velero.ResourceIdentifier, error) {
-	metadata, err := meta.Accessor(item)
-	if err != nil {
-		return item, a.additionalItems, err
-	}
-	a.ids = append(a.ids, kubeutil.NamespaceAndName(metadata))
-	a.backups = append(a.backups, *backup)
-
-	return item, a.additionalItems, nil
-}
-
-func (a *recordResourcesAction) AppliesTo() (velero.ResourceSelector, error) {
-	return a.selector, nil
-}
-
-func (a *recordResourcesAction) ForResource(resource string) *recordResourcesAction {
-	a.selector.IncludedResources = append(a.selector.IncludedResources, resource)
-	return a
-}
-
-func (a *recordResourcesAction) ForNamespace(namespace string) *recordResourcesAction {
-	a.selector.IncludedNamespaces = append(a.selector.IncludedNamespaces, namespace)
-	return a
-}
-
-func (a *recordResourcesAction) ForLabelSelector(selector string) *recordResourcesAction {
-	a.selector.LabelSelector = selector
-	return a
-}
-
-func (a *recordResourcesAction) WithAdditionalItems(items []velero.ResourceIdentifier) *recordResourcesAction {
-	a.additionalItems = items
-	return a
-}
-
 var (
 	v1Group = &metav1.APIResourceList{
 		GroupVersion: "v1",

pkg/backup/item_backupper_test.go

@@ -46,24 +46,19 @@ import (
 
 func TestBackupItemNoSkips(t *testing.T) {
 	tests := []struct {
-		name                                  string
-		item                                  string
-		namespaceIncludesExcludes             *collections.IncludesExcludes
-		expectError                           bool
-		expectExcluded                        bool
-		expectedTarHeaderName                 string
-		tarWriteError                         bool
-		tarHeaderWriteError                   bool
-		customAction                          bool
-		expectedActionID                      string
-		customActionAdditionalItemIdentifiers []velero.ResourceIdentifier
-		customActionAdditionalItems           []runtime.Unstructured
-		groupResource                         string
-		snapshottableVolumes                  map[string]velerotest.VolumeBackupInfo
-		snapshotError                         error
-		additionalItemError                   error
-		trackedPVCs                           sets.String
-		expectedTrackedPVCs                   sets.String
+		name                      string
+		item                      string
+		namespaceIncludesExcludes *collections.IncludesExcludes
+		expectError               bool
+		expectExcluded            bool
+		expectedTarHeaderName     string
+		tarWriteError             bool
+		tarHeaderWriteError       bool
+		groupResource             string
+		snapshottableVolumes      map[string]velerotest.VolumeBackupInfo
+		snapshotError             error
+		trackedPVCs               sets.String
+		expectedTrackedPVCs       sets.String
 	}{
 		{
 			name:                "tar header write error",
@@ -77,33 +72,6 @@ func TestBackupItemNoSkips(t *testing.T) {
 			expectError:   true,
 			tarWriteError: true,
 		},
-		{
-			name:                      "action invoked - additional items - error",
-			namespaceIncludesExcludes: collections.NewIncludesExcludes().Includes("*"),
-			item:                      `{"metadata":{"namespace": "myns", "name":"bar"}}`,
-			expectError:               true,
-			expectExcluded:            false,
-			expectedTarHeaderName:     "resources/resource.group/namespaces/myns/bar.json",
-			customAction:              true,
-			expectedActionID:          "myns/bar",
-			customActionAdditionalItemIdentifiers: []velero.ResourceIdentifier{
-				{
-					GroupResource: schema.GroupResource{Group: "g1", Resource: "r1"},
-					Namespace:     "ns1",
-					Name:          "n1",
-				},
-				{
-					GroupResource: schema.GroupResource{Group: "g2", Resource: "r2"},
-					Namespace:     "ns2",
-					Name:          "n2",
-				},
-			},
-			customActionAdditionalItems: []runtime.Unstructured{
-				velerotest.UnstructuredOrDie(`{"apiVersion":"g1/v1","kind":"r1","metadata":{"namespace":"ns1","name":"n1"}}`),
-				velerotest.UnstructuredOrDie(`{"apiVersion":"g2/v1","kind":"r1","metadata":{"namespace":"ns2","name":"n2"}}`),
-			},
-			additionalItemError: errors.New("foo"),
-		},
 		{
 			name:                      "takePVSnapshot is not invoked for PVs when volumeSnapshotter == nil",
 			namespaceIncludesExcludes: collections.NewIncludesExcludes().Includes("*"),
@@ -177,7 +145,6 @@ func TestBackupItemNoSkips(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			var (
-				action        *recordResourcesAction
 				backup        = new(Request)
 				groupResource = schema.ParseGroupResource("resource.group")
 				backedUpItems = make(map[itemKey]struct{})
@@ -212,18 +179,6 @@ func TestBackupItemNoSkips(t *testing.T) {
 				w.writeError = errors.New("error")
 			}
 
-			if test.customAction {
-				action = new(recordResourcesAction).WithAdditionalItems(test.customActionAdditionalItemIdentifiers)
-				backup.ResolvedActions = []resolvedAction{
-					{
-						BackupItemAction:          action,
-						namespaceIncludesExcludes: collections.NewIncludesExcludes(),
-						resourceIncludesExcludes:  collections.NewIncludesExcludes().Includes(groupResource.String()),
-						selector:                  labels.Everything(),
-					},
-				}
-			}
-
 			podCommandExecutor := &velerotest.MockPodCommandExecutor{}
 			defer podCommandExecutor.AssertExpectations(t)
 
@@ -276,20 +231,6 @@ func TestBackupItemNoSkips(t *testing.T) {
 			itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, backup.ResourceHooks, hookPhasePre).Return(nil)
 			itemHookHandler.On("handleHooks", mock.Anything, groupResource, obj, backup.ResourceHooks, hookPhasePost).Return(nil)
 
-			for i, item := range test.customActionAdditionalItemIdentifiers {
-				if test.additionalItemError != nil && i > 0 {
-					break
-				}
-				itemClient := &velerotest.FakeDynamicClient{}
-				defer itemClient.AssertExpectations(t)
-
-				dynamicFactory.On("ClientForGroupVersionResource", item.GroupResource.WithVersion("").GroupVersion(), metav1.APIResource{Name: item.Resource}, item.Namespace).Return(itemClient, nil)
-
-				itemClient.On("Get", item.Name, metav1.GetOptions{}).Return(test.customActionAdditionalItems[i], nil)
-
-				additionalItemBackupper.On("backupItem", mock.AnythingOfType("*logrus.Entry"), test.customActionAdditionalItems[i], item.GroupResource).Return(test.additionalItemError)
-			}
-
 			err = b.backupItem(velerotest.NewLogger(), obj, groupResource)
 			gotError := err != nil
 			if e, a := test.expectError, gotError; e != a {
@@ -330,17 +271,6 @@ func TestBackupItemNoSkips(t *testing.T) {
 				t.Errorf("data: expected %s, got %s", e, a)
 			}
 
-			if test.customAction {
-				if len(action.ids) != 1 {
-					t.Errorf("unexpected custom action ids: %v", action.ids)
-				} else if e, a := test.expectedActionID, action.ids[0]; e != a {
-					t.Errorf("action.ids[0]: expected %s, got %s", e, a)
-				}
-
-				require.Equal(t, 1, len(action.backups), "unexpected custom action backups: %#v", action.backups)
-				assert.Equal(t, backup.Backup, &(action.backups[0]), "backup")
-			}
-
 			if test.snapshottableVolumes != nil {
 				require.Equal(t, len(test.snapshottableVolumes), len(volumeSnapshotter.SnapshotsTaken))
 			}