Support nested lists when discovering manifests

[dyson]

Resolves feedback in #122

This commit modifies the generation of kubernetes manifests so that we
can support lists of resources, at arbitrary levels of nesting. As an
example, where before the following jsonnet would fail:

{
  resources: [
    {
      apiVersion: "v1",
      kind: "ConfigMap",
      metadata: {
        name: "config",
      },
    }
  ],
}

We will now return a valid configmap resource.

[sh0rez]

Thanks for picking this up! Did a first pass, commented on everything that caught my eye.

If I were too critical somewhere, just add a comment below and we can discuss 😄

Most of this is related to refactoring mixed in a feature PR, which I am not a big fan of because it makes it harder to review. In general, we always try to touch as few lines as possible when implementing a new feature, without refactoring anything if it works without.

Once merged, we can always come back and make the code look as clean and cool as possible :D

[sh0rez]

Thanks @lawrencejones @dyson @jackatbancast for your efforts! This already looks really good, found some nits (tests, etc). After these are addressed nothing prevents this anymore to be merged :D (finally)

[jackatbancast]

@sh0rez I've added support for an indeterminate depth of arrays as suggested, as well as creating a duplicate of testDataArray with an additional level of depth at the beginning.

As mentioned in my commit messages, I've brought in the reflect module, to supplement the type switch. Unfortunately it seems that this is needed to support the indeterminate depth, but I've tried to make the implementation as clean as possible.

We should still get around to refactoring some of the code a la @dyson's changes, but for now I think this may be the smallest set of changes I could think of to add the functionality we wanted, with a suitable test.

Let me know if there are any more changes, happy to work on getting this in ASAP.

[sh0rez]

Thanks so much, this looks very good! I'd suggest a slight refactoring of tryCoerceList and walkJSON to keep their concerns separate:

From 045999cb169af3e3e4a690efa04b039e5fad44fc Mon Sep 17 00:00:00 2001
From: sh0rez <[email protected]>
Date: Sun, 19 Jan 2020 12:18:13 +0100
Subject: [PATCH] refactor(kubernetes): separate concerns of walkJSON and
 tryCoerceSlice

---
 pkg/kubernetes/reconcile.go | 51 ++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go
index 729fca9..fee55bc 100644
--- a/pkg/kubernetes/reconcile.go
+++ b/pkg/kubernetes/reconcile.go
@@ -71,21 +71,13 @@ func extract(deep interface{}) (map[string]manifest.Manifest, error) {
 	return extracted, nil
 }
 
-// tryCoerceSlice will test that an input is an array or a slice,
-// and then construct a slice of empty interface.
-//
-// This is useful to handle recursion in walkJSON.
-// Types not specified exactly in a type switch (such as highly nested arrays),
-// will not be matched.
-func tryCoerceSlice(input interface{}, path trace) ([]interface{}, error) {
+// tryCoerceSlice attempts to construct a []interface{} from any other kind of
+// array/slice.
+// If the argument is not a list at all, ok will be false
+func tryCoerceSlice(input interface{}, path trace) (list []interface{}, ok bool) {
 	v := reflect.ValueOf(input)
-	v.Kind()
 	if v.Kind() != reflect.Slice && v.Kind() != reflect.Array {
-		return nil, ErrorPrimitiveReached{
-			path:      path.Base(),
-			key:       path.Name(),
-			primitive: input,
-		}
+		return nil, false
 	}
 
 	l := v.Len()
@@ -95,7 +87,7 @@ func tryCoerceSlice(input interface{}, path trace) ([]interface{}, error) {
 		s[i] = v.Index(i).Interface()
 	}
 
-	return s, nil
+	return s, true
 }
 
 // walkJSON recurses into either a map or list, returning a list of all objects that look
@@ -106,24 +98,35 @@ func tryCoerceSlice(input interface{}, path trace) ([]interface{}, error) {
 // walkJSON, and then walkObj/walkList to handle the two different types of collection we
 // support.
 func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trace) error {
+	// check for known types
 	switch v := ptr.(type) {
 	case map[string]interface{}:
 		return walkObj(v, extracted, path)
-	case []interface{}: // Duplicated to handle multiple list types
-		for idx, value := range v {
-			err := walkJSON(value, extracted, append(path, fmt.Sprintf("[%d]", idx)))
-			if err != nil {
-				return err
-			}
+	case []interface{}:
+		return walkList(v, extracted, path)
+	}
+
+	// Lists other than []interface{} need to be handled separately
+	s, ok := tryCoerceSlice(ptr, path)
+	if !ok {
+		// object and list tried, so not a collection.
+		return ErrorPrimitiveReached{
+			path:      path.Base(),
+			key:       path.Name(),
+			primitive: ptr,
 		}
-		return nil
-	default:
-		s, err := tryCoerceSlice(ptr, path)
+	}
+	return walkJSON(s, extracted, path)
+}
+
+func walkList(list []interface{}, extracted map[string]manifest.Manifest, path trace) error {
+	for idx, value := range list {
+		err := walkJSON(value, extracted, append(path, fmt.Sprintf("[%d]", idx)))
 		if err != nil {
 			return err
 		}
-		return walkJSON(s, extracted, path)
 	}
+	return nil
 }
 
 func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) error {
-- 
2.24.1

[sh0rez]

Also, could tryCoerceList be moved below walkJSON? That way it resembles the order of use in the file, so it can be read from top to bottom

[sh0rez]

Wow, this turned out awesome 🚀