From 76b264035fc2744b88a549dab0eec2c2c462e807 Mon Sep 17 00:00:00 2001
From: Eric Beard <ericzbeard@gmail.com>
Date: Fri, 1 Mar 2024 15:25:28 -0800
Subject: [PATCH 1/2] new test

---
 cft/pkg/module.go               | 14 +++++++-------
 cft/pkg/module_test.go          |  4 ++++
 cft/pkg/pkg.go                  |  7 +++----
 cft/pkg/tmpl/many-expect.yaml   | 20 ++++++++++++++++++++
 cft/pkg/tmpl/many-module.yaml   |  9 +++++++++
 cft/pkg/tmpl/many-template.yaml | 18 ++++++++++++++++++
 cft/pkg/util.go                 |  4 ++--
 internal/s11n/match.go          | 13 +++++++++++++
 8 files changed, 76 insertions(+), 13 deletions(-)
 create mode 100644 cft/pkg/tmpl/many-expect.yaml
 create mode 100644 cft/pkg/tmpl/many-module.yaml
 create mode 100644 cft/pkg/tmpl/many-template.yaml

diff --git a/cft/pkg/module.go b/cft/pkg/module.go
index 92015b6c..12f61212 100644
--- a/cft/pkg/module.go
+++ b/cft/pkg/module.go
@@ -133,8 +133,8 @@ type refctx struct {
 
 func replaceProp(prop *yaml.Node, parentName string, v *yaml.Node, outNode *yaml.Node, sidx int) error {
 
-	config.Debugf("replaceProp prop: %s, parentName: %s, v: %s, sidx: %d",
-		node.ToSJson(prop), parentName, node.ToSJson(v), sidx)
+	// config.Debugf("replaceProp prop: %s, parentName: %s, v: %s, sidx: %d",
+	//  	node.ToSJson(prop), parentName, node.ToSJson(v), sidx)
 
 	if sidx > -1 {
 		// The node is a sequence element
@@ -146,7 +146,7 @@ func replaceProp(prop *yaml.Node, parentName string, v *yaml.Node, outNode *yaml
 
 		if v.Kind == yaml.MappingNode {
 			parentNode := node.GetParent(prop, outNode, nil)
-			config.Debugf("replaceProp parentNode: %s", parentNode.String())
+			//config.Debugf("replaceProp parentNode: %s", parentNode.String())
 			*parentNode.Value = *newVal
 		} else {
 			*prop = *newVal
@@ -227,7 +227,7 @@ func resolveModuleRef(parentName string, prop *yaml.Node, sidx int, ctx *refctx)
 					prop.Value)
 			}
 
-			config.Debugf("resolveModuleRef about to call replaceProp")
+			//config.Debugf("resolveModuleRef about to call replaceProp")
 
 			replaceProp(prop, parentName, parentVal, outNode, sidx)
 
@@ -380,8 +380,8 @@ func resolveModuleSub(parentName string, prop *yaml.Node, sidx int, ctx *refctx)
 // If sidx is > -1, this prop is in a sequence
 func renamePropRefs(parentName string, propName string, prop *yaml.Node, sidx int, ctx *refctx) error {
 
-	config.Debugf("renamePropRefs parentName: %s, propName: %s, prop: %s, sidx: %d",
-		parentName, propName, node.ToSJson(prop), sidx)
+	//config.Debugf("renamePropRefs parentName: %s, propName: %s, prop: %s, sidx: %d",
+	//	parentName, propName, node.ToSJson(prop), sidx)
 
 	logicalId := ctx.logicalId
 
@@ -419,7 +419,7 @@ func renamePropRefs(parentName string, propName string, prop *yaml.Node, sidx in
 			for i, p := range prop.Content {
 				// propName is blank so the next parentName is blank
 
-				config.Debugf("About to call renamePropRefs, propName is %s", propName)
+				//config.Debugf("About to call renamePropRefs, propName is %s", propName)
 
 				//result := renamePropRefs(parentName, p.Value, prop.Content[i], i, ctx)
 				result := renamePropRefs(propName, p.Value, prop.Content[i], i, ctx)
diff --git a/cft/pkg/module_test.go b/cft/pkg/module_test.go
index aac7b4cf..008856e2 100644
--- a/cft/pkg/module_test.go
+++ b/cft/pkg/module_test.go
@@ -26,6 +26,10 @@ func TestSub(t *testing.T) {
 	runTest("sub", t)
 }
 
+func TestMany(t *testing.T) {
+	runTest("many", t)
+}
+
 // TODO: This was broken in the refactor, come back to it later
 //func TestForeach(t *testing.T) {
 //	runTest("foreach", t)
diff --git a/cft/pkg/pkg.go b/cft/pkg/pkg.go
index 18fc0279..56f7d513 100644
--- a/cft/pkg/pkg.go
+++ b/cft/pkg/pkg.go
@@ -54,12 +54,11 @@ func transform(ctx *transformContext) (bool, error) {
 	// registry is a map of functions defined in rain.go
 	for path, fn := range registry {
 		for found := range s11n.MatchAll(ctx.nodeToTransform, path) {
-			//config.Debugf("pkg transform path: %v, nodeToTransform:\n%v", path,
-			//	node.ToSJson(ctx.nodeToTransform))
-			//config.Debugf("pkg transform found: %v", node.ToSJson(found))
 			nodeParent := node.GetParent(found, ctx.nodeToTransform, nil)
 			nodeParent.Parent = ctx.parent
-			//config.Debugf("pkg transform nodeParent: %s", nodeParent.String())
+			if path == "**/*|Rain::Module" {
+				config.Debugf("found a Module at line %v", nodeParent.Value.Line)
+			}
 			c, err := fn(&directiveContext{found, ctx.rootDir, ctx.t, nodeParent, ctx.fs})
 			if err != nil {
 				config.Debugf("Error packaging template: %s\n", err)
diff --git a/cft/pkg/tmpl/many-expect.yaml b/cft/pkg/tmpl/many-expect.yaml
new file mode 100644
index 00000000..9c46957e
--- /dev/null
+++ b/cft/pkg/tmpl/many-expect.yaml
@@ -0,0 +1,20 @@
+Resources:
+  My1Bucket1:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: foo1
+
+  My2Bucket1:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: foo2
+
+  My3Bucket1:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: foo3
+
+  My4Bucket1:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: foo4
diff --git a/cft/pkg/tmpl/many-module.yaml b/cft/pkg/tmpl/many-module.yaml
new file mode 100644
index 00000000..5931440e
--- /dev/null
+++ b/cft/pkg/tmpl/many-module.yaml
@@ -0,0 +1,9 @@
+Parameters:
+  Name:
+    Type: String
+Resources:
+  Bucket1:
+    Type: AWS::S3::Bucket
+    Properties:
+      BucketName: !Ref Name
+
diff --git a/cft/pkg/tmpl/many-template.yaml b/cft/pkg/tmpl/many-template.yaml
new file mode 100644
index 00000000..26369264
--- /dev/null
+++ b/cft/pkg/tmpl/many-template.yaml
@@ -0,0 +1,18 @@
+Resources:
+  My1:
+    Type: !Rain::Module "./many-module.yaml"
+    Properties:
+      Name: foo1
+  My2:
+    Type: !Rain::Module "./many-module.yaml"
+    Properties:
+      Name: foo2
+  My3:
+    Type: !Rain::Module "./many-module.yaml"
+    Properties:
+      Name: foo3
+  My4:
+    Type: !Rain::Module "./many-module.yaml"
+    Properties:
+      Name: foo4
+
diff --git a/cft/pkg/util.go b/cft/pkg/util.go
index 31304aca..1df6477d 100644
--- a/cft/pkg/util.go
+++ b/cft/pkg/util.go
@@ -184,13 +184,13 @@ func expectFile(n *yaml.Node, root string) ([]byte, string, error) {
 		return nil, "", err
 	}
 
-	config.Debugf("root: %v, path: %v", root, path)
+	//config.Debugf("root: %v, path: %v", root, path)
 
 	if !filepath.IsAbs(path) {
 		path = filepath.Join(root, path)
 	}
 
-	config.Debugf("path: %v", path)
+	//config.Debugf("path: %v", path)
 
 	info, err := os.Stat(path)
 	if err != nil {
diff --git a/internal/s11n/match.go b/internal/s11n/match.go
index 030855d3..39994d2d 100644
--- a/internal/s11n/match.go
+++ b/internal/s11n/match.go
@@ -25,6 +25,19 @@ func MatchOne(node *yaml.Node, path string) *yaml.Node {
 	return results[0]
 }
 
+// TODO: There's a bug in here that's missing some nodes
+// also it's super annoying to troubleshoot since things
+// happen in an unpredictable order on goroutines.
+// Do we *really* need the channel?
+//
+// I *think* what's happening is that Modules modify the document by
+// adding new nodes, and MatchAll gets confused because it wasn't
+// expecting new nodes to be added. All of the other directives only
+// add or change content underneath where the directive appears.
+// Probably an array index thing... when we started there were
+// n resources, now there are n+m, so it doesn't fail, it just doesn't finish.
+// This is likely to be unsafe on a goroutine
+
 // MatchAll returns all yaml nodes that match the provided path.
 // The path is a `/`-separated string that describes a path into the template's tree.
 // Wildcard elements (which can be map keys or array indices) are represented by a `*`.

From 6f22e4765f368aa8d522bd9859b469c5c09a666a Mon Sep 17 00:00:00 2001
From: Eric Beard <ericzbeard@gmail.com>
Date: Fri, 1 Mar 2024 15:59:22 -0800
Subject: [PATCH 2/2] Fixed a bug with modules that was missing some directives

---
 cft/pkg/pkg.go             | 29 +++++++++++++++++++++++++----
 internal/config/version.go |  2 +-
 internal/s11n/match.go     | 13 -------------
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/cft/pkg/pkg.go b/cft/pkg/pkg.go
index 56f7d513..5ae917b7 100644
--- a/cft/pkg/pkg.go
+++ b/cft/pkg/pkg.go
@@ -26,6 +26,7 @@ package pkg
 
 import (
 	"embed"
+	"errors"
 	"fmt"
 	"path/filepath"
 
@@ -88,14 +89,34 @@ func Template(t cft.Template, rootDir string, fs *embed.FS) (cft.Template, error
 		parent:          &node.NodePair{Key: t.Node, Value: t.Node},
 		fs:              fs,
 	}
-	changed, err := transform(ctx)
-	if err != nil {
-		return t, err
+	var changed bool = false
+	passes := 0
+	maxPasses := 100
+	for {
+		passes += 1
+		// Modules can add new nodes to the template, which
+		// breaks s11n.MatchAll, since it expects the length to stay the same.
+		// Just start over a transform the whole template again.
+		changedThisPass, err := transform(ctx)
+		if err != nil {
+			return t, err
+		}
+		if changedThisPass {
+			config.Debugf("Need another pass: %d", passes)
+			changed = true
+		}
+		if !changedThisPass {
+			config.Debugf("No changes this pass: %d", passes)
+			break
+		}
+		if passes > maxPasses {
+			return t, errors.New("reached maxPasses while transforming")
+		}
 	}
 
 	// j, _ = json.MarshalIndent(templateNode, "", "  ")
 	// config.Debugf("Transformed template: %v", string(j))
-
+	var err error
 	if changed {
 		t, err = parse.Node(templateNode)
 		if err != nil {
diff --git a/internal/config/version.go b/internal/config/version.go
index af725831..352ea40d 100644
--- a/internal/config/version.go
+++ b/internal/config/version.go
@@ -5,5 +5,5 @@ const (
 	NAME = "Rain"
 
 	// VERSION is the application's version string
-	VERSION = "v1.8.0"
+	VERSION = "v1.8.1"
 )
diff --git a/internal/s11n/match.go b/internal/s11n/match.go
index 39994d2d..030855d3 100644
--- a/internal/s11n/match.go
+++ b/internal/s11n/match.go
@@ -25,19 +25,6 @@ func MatchOne(node *yaml.Node, path string) *yaml.Node {
 	return results[0]
 }
 
-// TODO: There's a bug in here that's missing some nodes
-// also it's super annoying to troubleshoot since things
-// happen in an unpredictable order on goroutines.
-// Do we *really* need the channel?
-//
-// I *think* what's happening is that Modules modify the document by
-// adding new nodes, and MatchAll gets confused because it wasn't
-// expecting new nodes to be added. All of the other directives only
-// add or change content underneath where the directive appears.
-// Probably an array index thing... when we started there were
-// n resources, now there are n+m, so it doesn't fail, it just doesn't finish.
-// This is likely to be unsafe on a goroutine
-
 // MatchAll returns all yaml nodes that match the provided path.
 // The path is a `/`-separated string that describes a path into the template's tree.
 // Wildcard elements (which can be map keys or array indices) are represented by a `*`.