hotfix: return error instead of log at FromMapAndOption (#5381)

* hotfix: return error instead of log at `FromMapAndOption`

* chore: show error message

* hotfix: use correct function

* hotix: use `t.Helper()` and fix `t *testing.T order

* hotfix: wrapt the error of `FromMapAndOption`

* hotfix: meaningful message for an error

* hotfix: summarize in one line

* hotfix: fix the abandoned error and show meaningful message

* hotfix: start with helper function

* Keep TODO comment

Author: chansuke

api/internal/plugins/utils/utils_test.go

@@ -53,7 +53,7 @@ func makeConfigMap(rf *resource.Factory, name, behavior string, hashValue *strin
 	return r
 }
 
-func makeConfigMapOptions(rf *resource.Factory, name, behavior string, disableHash bool) *resource.Resource {
+func makeConfigMapOptions(rf *resource.Factory, name, behavior string, disableHash bool) (*resource.Resource, error) {
 	return rf.FromMapAndOption(map[string]interface{}{
 		"apiVersion": "v1",
 		"kind":       "ConfigMap",
@@ -89,7 +89,11 @@ func TestUpdateResourceOptions(t *testing.T) {
 		name := fmt.Sprintf("test%d", i)
 		err := in.Append(makeConfigMap(rf, name, c.behavior, c.hashValue))
 		require.NoError(t, err)
-		err = expected.Append(makeConfigMapOptions(rf, name, c.behavior, !c.needsHash))
+		config, err := makeConfigMapOptions(rf, name, c.behavior, !c.needsHash)
+		if err != nil {
+			t.Errorf("expected new instance with an options but got error: %v", err)
+		}
+		err = expected.Append(config)
 		require.NoError(t, err)
 	}
 	actual, err := UpdateResourceOptions(in)

api/resmap/reswrangler_test.go

@@ -822,8 +822,9 @@ func TestAppendAll(t *testing.T) {
 	}
 }
 
-func makeMap1() ResMap {
-	return rmF.FromResource(rf.FromMapAndOption(
+func makeMap1(t *testing.T) ResMap {
+	t.Helper()
+	r, err := rf.FromMapAndOption(
 		map[string]interface{}{
 			"apiVersion": "apps/v1",
 			"kind":       "ConfigMap",
@@ -836,11 +837,16 @@ func makeMap1() ResMap {
 			},
 		}, &types.GeneratorArgs{
 			Behavior: "create",
-		}))
+		})
+	if err != nil {
+		t.Fatalf("expected new intance with an options but got error: %v", err)
+	}
+	return rmF.FromResource(r)
 }
 
-func makeMap2(b types.GenerationBehavior) ResMap {
-	return rmF.FromResource(rf.FromMapAndOption(
+func makeMap2(t *testing.T, b types.GenerationBehavior) ResMap {
+	t.Helper()
+	r, err := rf.FromMapAndOption(
 		map[string]interface{}{
 			"apiVersion": "apps/v1",
 			"kind":       "ConfigMap",
@@ -854,14 +860,19 @@ func makeMap2(b types.GenerationBehavior) ResMap {
 			},
 		}, &types.GeneratorArgs{
 			Behavior: b.String(),
-		}))
+		})
+	if err != nil {
+		t.Fatalf("expected new intance with an options but got error: %v", err)
+	}
+	return rmF.FromResource(r)
 }
 
 func TestAbsorbAll(t *testing.T) {
 	metadata := map[string]interface{}{
 		"name": "cmap",
 	}
-	expected := rmF.FromResource(rf.FromMapAndOption(
+
+	r, err := rf.FromMapAndOption(
 		map[string]interface{}{
 			"apiVersion": "apps/v1",
 			"kind":       "ConfigMap",
@@ -874,24 +885,26 @@ func TestAbsorbAll(t *testing.T) {
 		},
 		&types.GeneratorArgs{
 			Behavior: "create",
-		}))
-	w := makeMap1()
-	assert.NoError(t, w.AbsorbAll(makeMap2(types.BehaviorMerge)))
+		})
+	if err != nil {
+		t.Fatalf("expected new intance with an options but got error: %v", err)
+	}
+	expected := rmF.FromResource(r)
+	w := makeMap1(t)
+	assert.NoError(t, w.AbsorbAll(makeMap2(t, types.BehaviorMerge)))
 	expected.RemoveBuildAnnotations()
 	w.RemoveBuildAnnotations()
 	assert.NoError(t, expected.ErrorIfNotEqualLists(w))
-	w = makeMap1()
+	w = makeMap1(t)
 	assert.NoError(t, w.AbsorbAll(nil))
-	assert.NoError(t, w.ErrorIfNotEqualLists(makeMap1()))
+	assert.NoError(t, w.ErrorIfNotEqualLists(makeMap1(t)))
 
-	w = makeMap1()
-	w2 := makeMap2(types.BehaviorReplace)
+	w = makeMap1(t)
+	w2 := makeMap2(t, types.BehaviorReplace)
 	assert.NoError(t, w.AbsorbAll(w2))
 	w2.RemoveBuildAnnotations()
 	assert.NoError(t, w2.ErrorIfNotEqualLists(w))
-	w = makeMap1()
-	w2 = makeMap2(types.BehaviorUnspecified)
-	err := w.AbsorbAll(w2)
+	err = makeMap1(t).AbsorbAll(makeMap2(t, types.BehaviorUnspecified))
 	assert.Error(t, err)
 	assert.True(
 		t, strings.Contains(err.Error(), "behavior must be merge or replace"))

api/resource/factory.go

@@ -42,7 +42,12 @@ func (rf *Factory) Hasher() ifc.KustHasher {
 
 // FromMap returns a new instance of Resource.
 func (rf *Factory) FromMap(m map[string]interface{}) *Resource {
-	return rf.FromMapAndOption(m, nil)
+	res, err := rf.FromMapAndOption(m, nil)
+	if err != nil {
+		// TODO: return err instead of log.
+		log.Fatalf("failed to create resource from map: %v", err)
+	}
+	return res
 }
 
 // FromMapWithName returns a new instance with the given "original" name.
@@ -52,19 +57,22 @@ func (rf *Factory) FromMapWithName(n string, m map[string]interface{}) *Resource
 
 // FromMapWithNamespaceAndName returns a new instance with the given "original" namespace.
 func (rf *Factory) FromMapWithNamespaceAndName(ns string, n string, m map[string]interface{}) *Resource {
-	r := rf.FromMapAndOption(m, nil)
+	r, err := rf.FromMapAndOption(m, nil)
+	if err != nil {
+		// TODO: return err instead of log.
+		log.Fatalf("failed to create resource from map: %v", err)
+	}
 	return r.setPreviousId(ns, n, r.GetKind())
 }
 
 // FromMapAndOption returns a new instance of Resource with given options.
 func (rf *Factory) FromMapAndOption(
-	m map[string]interface{}, args *types.GeneratorArgs) *Resource {
+	m map[string]interface{}, args *types.GeneratorArgs) (*Resource, error) {
 	n, err := yaml.FromMap(m)
 	if err != nil {
-		// TODO: return err instead of log.
-		log.Fatal(err)
+		return nil, fmt.Errorf("failed to convert map to YAML node: %w", err)
 	}
-	return rf.makeOne(n, args)
+	return rf.makeOne(n, args), nil
 }
 
 // makeOne returns a new instance of Resource.