fixes after a review

pkg/controller/periodic/periodic_test.go

@@ -1,7 +1,6 @@
 package periodic
 
 import (
-	"context"
 	"encoding/json"
 	"fmt"
 	"testing"
@@ -17,7 +16,7 @@ import (
 
 func Test_Controller_CustomPeriodGatherer(t *testing.T) {
 	c, mockRecorder := getMocksForPeriodicTest([]gatherers.Interface{
-		&gather.MockGatherer{CanFail: true},
+		&gather.MockGatherer{},
 		&gather.MockCustomPeriodGatherer{Period: 999 * time.Hour},
 	}, 1*time.Hour)
 
@@ -34,7 +33,7 @@ func Test_Controller_CustomPeriodGatherer(t *testing.T) {
 
 func Test_Controller_Run(t *testing.T) {
 	c, mockRecorder := getMocksForPeriodicTest([]gatherers.Interface{
-		&gather.MockGatherer{CanFail: true},
+		&gather.MockGatherer{},
 	}, 1*time.Hour)
 
 	// No delay, 5 gatherers + metadata
@@ -67,7 +66,7 @@ func Test_Controller_Run(t *testing.T) {
 
 func Test_Controller_periodicTrigger(t *testing.T) {
 	c, mockRecorder := getMocksForPeriodicTest([]gatherers.Interface{
-		&gather.MockGatherer{CanFail: true},
+		&gather.MockGatherer{},
 	}, 1*time.Hour)
 
 	// 1 sec interval, 5 gatherers + metadata
@@ -94,7 +93,7 @@ func Test_Controller_periodicTrigger(t *testing.T) {
 }
 
 func Test_Controller_Sources(t *testing.T) {
-	mockGatherer := gather.MockGatherer{CanFail: true}
+	mockGatherer := gather.MockGatherer{}
 	mockCustomPeriodGatherer := gather.MockCustomPeriodGathererNoPeriod{ShouldBeProcessed: true}
 	// 1 Gatherer ==> 1 source
 	c, _ := getMocksForPeriodicTest([]gatherers.Interface{
@@ -113,7 +112,7 @@ func Test_Controller_Sources(t *testing.T) {
 func Test_Controller_CustomPeriodGathererNoPeriod(t *testing.T) {
 	mockGatherer := gather.MockCustomPeriodGathererNoPeriod{ShouldBeProcessed: true}
 	c, mockRecorder := getMocksForPeriodicTest([]gatherers.Interface{
-		&gather.MockGatherer{CanFail: true},
+		&gather.MockGatherer{},
 		&mockGatherer,
 	}, 1*time.Hour)
 
@@ -158,12 +157,12 @@ func Test_Controller_FailingGatherer(t *testing.T) {
 			continue
 		}
 		metadataFound = true
-		b, err := mockRecorder.Records[i].Item.Marshal(context.Background())
+		b, err := mockRecorder.Records[i].Item.Marshal()
 		assert.NoError(t, err)
 		metaData := make(map[string]interface{})
 		err = json.Unmarshal(b, &metaData)
 		assert.NoError(t, err)
-		assert.Len(t, metaData["status_reports"].([]interface{}), 1,
+		assert.Len(t, metaData["status_reports"].([]interface{}), 2,
 			fmt.Sprintf("Only one function for %s expected ", c.gatherers[0].GetName()))
 	}
 	assert.Truef(t, metadataFound, fmt.Sprintf("%s not found in records", recorder.MetadataRecordName))

pkg/gather/gather.go

@@ -100,12 +100,11 @@ func CollectAndRecordGatherer(
 		var recordErrs []error
 
 		if result.Panic != nil {
-			err := fmt.Errorf(
+			recordErrs = append(recordErrs, fmt.Errorf("panic: %v", result.Panic))
+			klog.Error(fmt.Errorf(
 				`gatherer "%v" function "%v" panicked with the error: %v`,
 				gathererName, result.FunctionName, result.Panic,
-			)
-			recordErrs = append(recordErrs, err)
-			klog.Error(err)
+			))
 			allErrors = append(allErrors, fmt.Errorf(`function "%v" panicked`, result.FunctionName))
 		}
 

pkg/gather/gather_test.go

@@ -258,9 +258,19 @@ func Test_CollectAndRecordGatherer(t *testing.T) {
 		},
 		{
 			FuncName: "mock_gatherer/panic",
-			Errors:   []string{"test panic"},
+			Errors:   []string{"panic: test panic"},
 			Panic:    "test panic",
 		},
+		{
+			FuncName:     "mock_gatherer",
+			RecordsCount: 5,
+			Errors: []string{
+				`function "errors" failed with an error`,
+				`function "errors" failed with an error`,
+				`function "errors" failed with an error`,
+				`function "panic" panicked`,
+			},
+		},
 	})
 	assertRecordsOneGatherer(t, mockRecorder.Records, []record.Record{
 		{
@@ -298,9 +308,7 @@ func Test_CollectAndRecordGatherer_Error(t *testing.T) {
 	assert.EqualError(
 		t,
 		err,
-		"gatherer mock_gatherer's function errors failed with error: error1, "+
-			"gatherer mock_gatherer's function errors failed with error: error2, "+
-			"gatherer mock_gatherer's function errors failed with error: error3",
+		`function "errors" failed with an error`,
 	)
 
 	err = RecordArchiveMetadata(functionReports, mockRecorder, nil)
@@ -317,6 +325,15 @@ func Test_CollectAndRecordGatherer_Error(t *testing.T) {
 				"error3",
 			},
 		},
+		{
+			FuncName:     "mock_gatherer",
+			RecordsCount: 0,
+			Errors: []string{
+				`function "errors" failed with an error`,
+				`function "errors" failed with an error`,
+				`function "errors" failed with an error`,
+			},
+		},
 	})
 }
 
@@ -329,14 +346,20 @@ func Test_CollectAndRecordGatherer_Panic(t *testing.T) {
 	})
 
 	functionReports, err := CollectAndRecordGatherer(context.Background(), gatherer, mockRecorder, mockConfigurator)
-	assert.EqualError(t, err, "gatherer mock_gatherer's function panic failed with error: test panic")
-	assert.Len(t, functionReports, 1)
+	assert.EqualError(t, err, `function "panic" panicked`)
+	assert.Len(t, functionReports, 2)
 	functionReports[0].Duration = 0
-	assert.ElementsMatch(t, functionReports, []GathererFunctionReport{{
-		FuncName: "mock_gatherer/panic",
-		Errors:   []string{"test panic"},
-		Panic:    "test panic",
-	}})
+	assert.ElementsMatch(t, functionReports, []GathererFunctionReport{
+		{
+			FuncName: "mock_gatherer/panic",
+			Errors:   []string{"panic: test panic"},
+			Panic:    "test panic",
+		},
+		{
+			FuncName: "mock_gatherer",
+			Errors:   []string{`function "panic" panicked`},
+		},
+	})
 	assert.Len(t, mockRecorder.Records, 0)
 }
 
@@ -366,23 +389,18 @@ func Test_CollectAndRecordGatherer_DuplicateRecords(t *testing.T) {
 	mockConfigurator := config.NewMockConfigurator(nil)
 
 	functionReports, err := CollectAndRecordGatherer(context.Background(), gatherer, rec, mockConfigurator)
-	assert.EqualError(
-		t, err,
-		"unable to record gatherer mock_gatherer_with_provided_functions function function_2' result "+
-			"record_1 because of the error: A record with the name record_1.json was already recorded and had "+
-			"fingerprint 4dc9ed5d5654c1c2b6da4629ac8a0b62 , overwriting with data having "+
-			"fingerprint b0560bacc0b4956efd5b527f9a27910e",
-	)
+	assert.Error(t, err)
 	assert.NotEmpty(t, functionReports)
-	assert.Len(t, functionReports, 3)
+	assert.Len(t, functionReports, 4)
 
 	sort.Slice(functionReports, func(i1, i2 int) bool {
 		return functionReports[i1].FuncName < functionReports[i2].FuncName
 	})
 
-	assert.Equal(t, fmt.Sprintf("%v/%v", gatherer.GetName(), "function_1"), functionReports[0].FuncName)
-	assert.Equal(t, fmt.Sprintf("%v/%v", gatherer.GetName(), "function_2"), functionReports[1].FuncName)
-	assert.Equal(t, fmt.Sprintf("%v/%v", gatherer.GetName(), "function_3"), functionReports[2].FuncName)
+	assert.Equal(t, gatherer.GetName(), functionReports[0].FuncName)
+	assert.Equal(t, fmt.Sprintf("%v/%v", gatherer.GetName(), "function_1"), functionReports[1].FuncName)
+	assert.Equal(t, fmt.Sprintf("%v/%v", gatherer.GetName(), "function_2"), functionReports[2].FuncName)
+	assert.Equal(t, fmt.Sprintf("%v/%v", gatherer.GetName(), "function_3"), functionReports[3].FuncName)
 
 	// the execution is parallel so testing gets a little tricky
 	totalRecordsCount := 0
@@ -395,8 +413,8 @@ func Test_CollectAndRecordGatherer_DuplicateRecords(t *testing.T) {
 		assert.Nil(t, report.Panic)
 	}
 
-	assert.Equal(t, 2, totalRecordsCount)
-	assert.Len(t, totalErrs, 1)
+	assert.Equal(t, 4, totalRecordsCount)
+	assert.Len(t, totalErrs, 2)
 	assert.Len(t, totalWarnings, 1)
 	assert.Len(t, totalWarnings, 1)
 
@@ -420,7 +438,7 @@ func Test_CollectAndRecordGatherer_Warning(t *testing.T) {
 
 	functionReports, err := CollectAndRecordGatherer(context.Background(), gatherer, rec, mockConfigurator)
 	assert.NoError(t, err)
-	assert.Len(t, functionReports, 1)
+	assert.Len(t, functionReports, 2)
 	assert.Equal(t, "mock_gatherer_with_provided_functions/function_1", functionReports[0].FuncName)
 	assert.Equal(t, 0, functionReports[0].RecordsCount)
 	assert.Nil(t, functionReports[0].Errors)
@@ -440,7 +458,11 @@ func assertMetadataOneGatherer(
 
 	for _, rec := range records {
 		if strings.HasSuffix(rec.Name, recorder.MetadataRecordName) {
-			bytes, err := rec.Item.Marshal(context.Background())
+			if len(metadataBytes) > 0 {
+				t.Fatalf("found 2 metadata records")
+			}
+
+			bytes, err := rec.Item.Marshal()
 			assert.NoError(t, err)
 
 			metadataBytes = bytes
@@ -453,7 +475,14 @@ func assertMetadataOneGatherer(
 	assert.NoError(t, err)
 
 	for i := range archiveMetadata.StatusReports {
-		archiveMetadata.StatusReports[i].Duration = 0
+		statusReport := &archiveMetadata.StatusReports[i]
+		statusReport.Duration = 0
+		sort.Slice(statusReport.Errors, func(i1, i2 int) bool {
+			return statusReport.Errors[i1] < statusReport.Errors[i2]
+		})
+		sort.Slice(statusReport.Warnings, func(i1, i2 int) bool {
+			return statusReport.Warnings[i1] < statusReport.Warnings[i2]
+		})
 	}
 
 	assert.Equal(t, isGlobalObfuscationEnabled, archiveMetadata.IsGlobalObfuscationEnabled)

pkg/gatherers/clusterconfig/cluster_config_v1_config_map_test.go

@@ -32,7 +32,7 @@ func Test_gatherClusterConfigV1(t *testing.T) {
 	assert.Len(t, records, 1)
 	assert.Equal(t, "config/configmaps/kube-system/cluster-config-v1", records[0].Name)
 
-	data, err := records[0].Item.Marshal(context.Background())
+	data, err := records[0].Item.Marshal()
 	assert.NoError(t, err)
 
 	installConfig := `baseDomain: \"\"\nmetadata:\n  creationTimestamp: null\nplatform: {}\npullSecret: \"\"\n`

pkg/gatherers/clusterconfig/config_maps_test.go

@@ -69,7 +69,7 @@ func Test_ConfigMap_Anonymizer(t *testing.T) {
 			addAnonymized := func(cmdata map[string]string, dn string, encodebase64 bool, d []byte) {
 				m := record.Marshalable(ConfigMapAnonymizer{v: d, encodeBase64: encodebase64})
 
-				res, err = m.Marshal(context.TODO())
+				res, err = m.Marshal()
 				cmdata[dn] = string(res)
 				mustNotFail(t, err, "serialization failed %+v")
 			}

pkg/gatherers/clusterconfig/host_subnets_test.go

@@ -34,7 +34,7 @@ func Test_GatherHostSubnet(t *testing.T) {
 	if len(records) != 1 {
 		t.Fatalf("unexpected number or records %d", len(records))
 	}
-	_, err = records[0].Item.Marshal(context.TODO())
+	_, err = records[0].Item.Marshal()
 	if err != nil {
 		t.Fatalf("failed to marshal object: %v", err)
 	}

pkg/gatherers/clusterconfig/image_pruners_test.go

@@ -73,7 +73,7 @@ func Test_ImagePruner_Gather(t *testing.T) {
 				return
 			}
 			item := records[0].Item
-			_, err := item.Marshal(context.TODO())
+			_, err := item.Marshal()
 			if err != nil {
 				t.Fatalf("unable to marshal config: %v", err)
 			}

pkg/gatherers/clusterconfig/image_registries_test.go

@@ -202,7 +202,7 @@ func Test_ImageRegistry_Gather(t *testing.T) {
 				return
 			}
 			item := records[0].Item
-			_, err := item.Marshal(context.TODO())
+			_, err := item.Marshal()
 			if err != nil {
 				t.Fatalf("unable to marshal config: %v", err)
 			}

pkg/gatherers/clusterconfig/install_plans_test.go

@@ -117,7 +117,7 @@ func Test_InstallPlans_Gather(t *testing.T) {
 				// copy to new anonymizer with limited max
 				m = InstallPlanAnonymizer{limit: 1, total: m.total, v: m.v}
 			}
-			b, _ := m.Marshal(context.Background())
+			b, _ := m.Marshal()
 			sb := string(b)
 			if sb != test.exp {
 				t.Fatalf("unexpected installplan exp: %s got: %s", test.exp, sb)

pkg/gatherers/clusterconfig/netnamespaces_test.go

@@ -38,7 +38,7 @@ func Test_NetNamespaces_Gather(t *testing.T) {
 		t.Fatalf("unexpected number or records %d", len(rec))
 	}
 	it1 := rec[0].Item
-	it1Bytes, err := it1.Marshal(context.TODO())
+	it1Bytes, err := it1.Marshal()
 	if err != nil {
 		t.Fatalf("unable to marshal: %v", err)
 	}

pkg/gatherers/clusterconfig/operators_test.go

@@ -44,7 +44,7 @@ func Test_Operators_GatherClusterOperators(t *testing.T) {
 		return
 	}
 
-	item, _ := records[0].Item.Marshal(context.TODO())
+	item, _ := records[0].Item.Marshal()
 	var gatheredCO configv1.ClusterOperator
 	openshiftCodec := openshiftscheme.Codecs.LegacyCodec(configv1.SchemeGroupVersion)
 	_, _, err = openshiftCodec.Decode(item, nil, &gatheredCO)

pkg/gatherers/clusterconfig/service_accounts_test.go

@@ -70,7 +70,7 @@ func Test_ServiceAccounts_Gather(t *testing.T) {
 				t.Fatalf("unexpected errors: %#v", errs)
 				return
 			}
-			bts, err := sa[0].Item.Marshal(context.Background())
+			bts, err := sa[0].Item.Marshal()
 			if err != nil {
 				t.Fatalf("error marshaling %s", err)
 			}

pkg/gatherers/clusterconfig/webhookconfigurations_test.go

@@ -62,7 +62,7 @@ func assertWebhookConfigurations(t *testing.T, records []record.Record, expected
 	assert.Len(t, records, 1)
 	assert.Equal(t, records[0].Name, expectedName)
 
-	configurationBytes, err := records[0].Item.Marshal(context.TODO())
+	configurationBytes, err := records[0].Item.Marshal()
 	assert.NoError(t, err)
 
 	assert.JSONEq(t, `{

pkg/record/memory.go

@@ -1,6 +1,9 @@
 package record
 
-import "time"
+import (
+	"fmt"
+	"time"
+)
 
 // MemoryRecord Represents records stored in memory
 type MemoryRecord struct {
@@ -10,6 +13,13 @@ type MemoryRecord struct {
 	Fingerprint string
 }
 
+func (r *MemoryRecord) Print() string {
+	return fmt.Sprintf(
+		`MemoryRecord{Name: "%v", At: "%v", len(Data): %v, Fingerprint: "%v"}`,
+		r.Name, r.At, len(r.Data), r.Fingerprint,
+	)
+}
+
 type MemoryRecords []MemoryRecord
 
 func (r MemoryRecords) Less(i, j int) bool { return r[i].At.After(r[j].At) }

pkg/record/record.go

@@ -17,24 +17,20 @@ type Record struct {
 	Name     string
 	Captured time.Time
 	Item     Marshalable
-
-	fingerprint string
 }
 
-// GetFingerprint returns the fingerprint possibly using the cache
-func (r *Record) GetFingerprint() (string, error) {
-	if len(r.fingerprint) == 0 {
-		content, err := r.Item.Marshal()
-		if err != nil {
-			return "", err
-		}
-
-		h := sha256.New()
-		h.Write(content)
-		r.fingerprint = hex.EncodeToString(h.Sum(nil))
+// Marshal marshals the item and returns its fingerprint
+func (r *Record) Marshal() ([]byte, string, error) {
+	content, err := r.Item.Marshal()
+	if err != nil {
+		return content, "", err
 	}
 
-	return r.fingerprint, nil
+	h := sha256.New()
+	h.Write(content)
+	fingerprint := hex.EncodeToString(h.Sum(nil))
+
+	return content, fingerprint, nil
 }
 
 // GetFilename with extension, if present