fix(ref): use explicit child/scoped loggers, avoid variable shadowing

I even know about variable shadowing, but here we are. In any case, while Go 1.22 should fix the loop variable shadowing issue, I find it much cleaner and clearer to use explicitly scoped, named loggers where possible. Fixes a few issues where log statements would continuously grow and add more keys on in each iteration of loops because I was reusing the same logger reference in each iteration. Ex (note the `module` field added on from all of the various module run loops): ``` Apr 08 04:10:52 testbox-arch mango[1744]: {"time":"2024-04-08T04:10:52.970064415Z","level":"DEBUG","msg":"No module variables","hostname":{"system":"testbox-arch","inventory":"testbox-arch"},"worker":"manager","manager":{"inventory":"/opt/mango/inventory","hostname":"testbox-arch"},"manager":{"enrolled":true,"runID":"01HTXYDQ54N7BNNFQSEC42NC9M"},"module":{"id":"/opt/mango/inventory/modules/test-requires-3"},"module":{"id":"/opt/mango/inventory/modules/test-requires-4"},"module":{"id":"/opt/mango/inventory/modules/test-requires-5"},"module":{"id":"/opt/mango/inventory/modules/test-reload"},"module":{"id":"/opt/mango/inventory/modules/test-env-vars"},"module":{"id":"/opt/mango/inventory/modules/test-template"},"module":{"id":"/opt/mango/inventory/modules/test-requires-1"},"module":{"id":"/opt/mango/inventory/modules/test-requires-2"}} ```
tjhop · Apr 9, 2024 · 7375f23 · 7375f23
1 parent 6ea9131
commit 7375f23
Show file tree

Hide file tree

Showing 10 changed files with 63 additions and 63 deletions.
diff --git a/cmd/mango/mango.go b/cmd/mango/mango.go
@@ -537,14 +537,14 @@ func main() {
 		}
 	}
 
-	logger = logger.With(
+	mainLogger := logger.With(
 		slog.Group("hostname",
 			slog.String("system", utils.GetHostname()),
 			slog.String("inventory", me),
 		),
 	)
 
-	logger.LogAttrs(
+	mainLogger.LogAttrs(
 		rootCtx,
 		slog.LevelInfo,
 		"Mango build information",
@@ -556,9 +556,9 @@ func main() {
 		),
 	)
 
-	// set logger as default
-	slog.SetDefault(logger)
+	// set mainLogger as default
+	slog.SetDefault(mainLogger)
 
 	// run mango daemon
-	mango(rootCtx, logger, inventoryPath, me)
+	mango(rootCtx, mainLogger, inventoryPath, me)
 }
diff --git a/internal/inventory/directive.go b/internal/inventory/directive.go
@@ -30,7 +30,7 @@ func (i *Inventory) ParseDirectives(ctx context.Context, logger *slog.Logger) er
 		"inventory": i.inventoryPath,
 		"component": "directives",
 	}
-	logger = logger.With(
+	iLogger := logger.With(
 		slog.Group(
 			"inventory",
 			slog.String("component", "directives"),
@@ -40,7 +40,7 @@ func (i *Inventory) ParseDirectives(ctx context.Context, logger *slog.Logger) er
 	path := filepath.Join(i.inventoryPath, "directives")
 	files, err := utils.GetFilesInDirectory(path)
 	if err != nil {
-		logger.LogAttrs(
+		iLogger.LogAttrs(
 			ctx,
 			slog.LevelError,
 			"Failed to parse directives",

diff --git a/internal/inventory/group.go b/internal/inventory/group.go
@@ -42,7 +42,7 @@ func (i *Inventory) ParseGroups(ctx context.Context, logger *slog.Logger) error
 		"inventory": i.inventoryPath,
 		"component": "groups",
 	}
-	logger = logger.With(
+	iLogger := logger.With(
 		slog.Group(
 			"inventory",
 			slog.String("component", "groups"),
@@ -52,7 +52,7 @@ func (i *Inventory) ParseGroups(ctx context.Context, logger *slog.Logger) error
 	path := filepath.Join(i.inventoryPath, "groups")
 	groupDirs, err := utils.GetFilesInDirectory(path)
 	if err != nil {
-		logger.LogAttrs(
+		iLogger.LogAttrs(
 			ctx,
 			slog.LevelError,
 			"Failed to get files in directory",
@@ -73,7 +73,7 @@ func (i *Inventory) ParseGroups(ctx context.Context, logger *slog.Logger) error
 			groupPath := filepath.Join(path, groupDir.Name())
 			groupFiles, err := utils.GetFilesInDirectory(groupPath)
 			if err != nil {
-				logger.LogAttrs(
+				iLogger.LogAttrs(
 					ctx,
 					slog.LevelError,
 					"Failed to parse group files",
@@ -100,7 +100,7 @@ func (i *Inventory) ParseGroups(ctx context.Context, logger *slog.Logger) error
 
 						for line := range lines {
 							if line.Err != nil {
-								logger.LogAttrs(
+								iLogger.LogAttrs(
 									ctx,
 									slog.LevelError,
 									"Failed to read globs for group",
@@ -120,7 +120,7 @@ func (i *Inventory) ParseGroups(ctx context.Context, logger *slog.Logger) error
 
 						for line := range lines {
 							if line.Err != nil {
-								logger.LogAttrs(
+								iLogger.LogAttrs(
 									ctx,
 									slog.LevelError,
 									"Failed to read regexs for group",
@@ -140,7 +140,7 @@ func (i *Inventory) ParseGroups(ctx context.Context, logger *slog.Logger) error
 
 						for line := range lines {
 							if line.Err != nil {
-								logger.LogAttrs(
+								iLogger.LogAttrs(
 									ctx,
 									slog.LevelError,
 									"Failed to read roles for group",
@@ -160,7 +160,7 @@ func (i *Inventory) ParseGroups(ctx context.Context, logger *slog.Logger) error
 
 						for line := range lines {
 							if line.Err != nil {
-								logger.LogAttrs(
+								iLogger.LogAttrs(
 									ctx,
 									slog.LevelError,
 									"Failed to read modules for group",
@@ -176,7 +176,7 @@ func (i *Inventory) ParseGroups(ctx context.Context, logger *slog.Logger) error
 					case "variables":
 						group.variables = filepath.Join(groupPath, "variables")
 					default:
-						logger.LogAttrs(
+						iLogger.LogAttrs(
 							ctx,
 							slog.LevelWarn,
 							"Skipping file while parsing inventory",

diff --git a/internal/inventory/host.go b/internal/inventory/host.go
@@ -35,7 +35,7 @@ func (i *Inventory) ParseHosts(ctx context.Context, logger *slog.Logger) error {
 		"inventory": i.inventoryPath,
 		"component": "hosts",
 	}
-	logger = logger.With(
+	iLogger := logger.With(
 		slog.Group(
 			"inventory",
 			slog.String("component", "hosts"),
@@ -45,7 +45,7 @@ func (i *Inventory) ParseHosts(ctx context.Context, logger *slog.Logger) error {
 	path := filepath.Join(i.inventoryPath, "hosts")
 	hostDirs, err := utils.GetFilesInDirectory(path)
 	if err != nil {
-		logger.LogAttrs(
+		iLogger.LogAttrs(
 			ctx,
 			slog.LevelError,
 			"Failed to get files in directory",
@@ -66,7 +66,7 @@ func (i *Inventory) ParseHosts(ctx context.Context, logger *slog.Logger) error {
 			hostPath := filepath.Join(path, hostDir.Name())
 			hostFiles, err := utils.GetFilesInDirectory(hostPath)
 			if err != nil {
-				logger.LogAttrs(
+				iLogger.LogAttrs(
 					ctx,
 					slog.LevelError,
 					"Failed to parse host files",
@@ -93,7 +93,7 @@ func (i *Inventory) ParseHosts(ctx context.Context, logger *slog.Logger) error {
 
 						for line := range lines {
 							if line.Err != nil {
-								logger.LogAttrs(
+								iLogger.LogAttrs(
 									ctx,
 									slog.LevelError,
 									"Failed to read roles for host",
@@ -113,7 +113,7 @@ func (i *Inventory) ParseHosts(ctx context.Context, logger *slog.Logger) error {
 
 						for line := range lines {
 							if line.Err != nil {
-								logger.LogAttrs(
+								iLogger.LogAttrs(
 									ctx,
 									slog.LevelError,
 									"Failed to read modules for host",
@@ -129,7 +129,7 @@ func (i *Inventory) ParseHosts(ctx context.Context, logger *slog.Logger) error {
 					case "variables":
 						host.variables = filepath.Join(hostPath, "variables")
 					default:
-						logger.LogAttrs(
+						iLogger.LogAttrs(
 							ctx,
 							slog.LevelWarn,
 							"Skipping file while parsing inventory",

diff --git a/internal/inventory/module.go b/internal/inventory/module.go
@@ -37,7 +37,7 @@ func (i *Inventory) ParseModules(ctx context.Context, logger *slog.Logger) error
 		"inventory": i.inventoryPath,
 		"component": "modules",
 	}
-	logger = logger.With(
+	iLogger := logger.With(
 		slog.Group(
 			"inventory",
 			slog.String("component", "modules"),
@@ -47,7 +47,7 @@ func (i *Inventory) ParseModules(ctx context.Context, logger *slog.Logger) error
 	path := filepath.Join(i.inventoryPath, "modules")
 	modDirs, err := utils.GetFilesInDirectory(path)
 	if err != nil {
-		logger.LogAttrs(
+		iLogger.LogAttrs(
 			ctx,
 			slog.LevelError,
 			"Failed to get files in directory",
@@ -68,7 +68,7 @@ func (i *Inventory) ParseModules(ctx context.Context, logger *slog.Logger) error
 			modPath := filepath.Join(path, modDir.Name())
 			modFiles, err := utils.GetFilesInDirectory(modPath)
 			if err != nil {
-				logger.LogAttrs(
+				iLogger.LogAttrs(
 					ctx,
 					slog.LevelError,
 					"Failed to parse module files",
@@ -97,7 +97,7 @@ func (i *Inventory) ParseModules(ctx context.Context, logger *slog.Logger) error
 					case "requires":
 						mod.Requires = filepath.Join(modPath, "requires")
 					default:
-						logger.LogAttrs(
+						iLogger.LogAttrs(
 							ctx,
 							slog.LevelWarn,
 							"Skipping file while parsing inventory",

diff --git a/internal/inventory/role.go b/internal/inventory/role.go
@@ -30,7 +30,7 @@ func (i *Inventory) ParseRoles(ctx context.Context, logger *slog.Logger) error {
 		"inventory": i.inventoryPath,
 		"component": "roles",
 	}
-	logger = logger.With(
+	iLogger := logger.With(
 		slog.Group(
 			"inventory",
 			slog.String("component", "roles"),
@@ -40,7 +40,7 @@ func (i *Inventory) ParseRoles(ctx context.Context, logger *slog.Logger) error {
 	path := filepath.Join(i.inventoryPath, "roles")
 	roleDirs, err := utils.GetFilesInDirectory(path)
 	if err != nil {
-		logger.LogAttrs(
+		iLogger.LogAttrs(
 			ctx,
 			slog.LevelError,
 			"Failed to parse roles",
@@ -61,7 +61,7 @@ func (i *Inventory) ParseRoles(ctx context.Context, logger *slog.Logger) error {
 			rolePath := filepath.Join(path, roleDir.Name())
 			roleFiles, err := utils.GetFilesInDirectory(rolePath)
 			if err != nil {
-				logger.LogAttrs(
+				iLogger.LogAttrs(
 					ctx,
 					slog.LevelError,
 					"Failed to parse role files",
@@ -88,7 +88,7 @@ func (i *Inventory) ParseRoles(ctx context.Context, logger *slog.Logger) error {
 
 						for line := range lines {
 							if line.Err != nil {
-								logger.LogAttrs(
+								iLogger.LogAttrs(
 									ctx,
 									slog.LevelError,
 									"Failed to read modules in role",
@@ -106,7 +106,7 @@ func (i *Inventory) ParseRoles(ctx context.Context, logger *slog.Logger) error {
 						role.modules = mods
 
 					default:
-						logger.LogAttrs(
+						iLogger.LogAttrs(
 							ctx,
 							slog.LevelWarn,
 							"Skipping file while parsing inventory",

diff --git a/internal/manager/directive.go b/internal/manager/directive.go
@@ -86,18 +86,18 @@ func (mgr *Manager) RunDirectives(ctx context.Context, logger *slog.Logger) {
 	logger.InfoContext(ctx, "Directive run started")
 	defer logger.InfoContext(ctx, "Directive run finished")
 	for _, d := range mgr.directives {
-		logger = logger.With(
+		dLogger := logger.With(
 			slog.Group(
 				"directive",
 				slog.String("id", d.String()),
 			),
 		)
 
-		logger.InfoContext(ctx, "Directive started")
-		defer logger.InfoContext(ctx, "Directive finished")
+		dLogger.InfoContext(ctx, "Directive started")
+		defer dLogger.InfoContext(ctx, "Directive finished")
 
 		if err := mgr.RunDirective(ctx, d); err != nil {
-			logger.LogAttrs(
+			dLogger.LogAttrs(
 				ctx,
 				slog.LevelError,
 				"Directive failed",

diff --git a/internal/manager/manager.go b/internal/manager/manager.go
@@ -85,16 +85,16 @@ func (mgr *Manager) ReloadAndRunAll(ctx context.Context, logger *slog.Logger, in
 	ctx = context.WithValue(ctx, contextKeyInventoryPath, inv.GetInventoryPath())
 	ctx = context.WithValue(ctx, contextKeyHostname, inv.GetHostname())
 
-	logger = logger.With(
+	mLogger := logger.With(
 		slog.Group(
 			"manager",
 			slog.Bool(string(contextKeyEnrolled), enrolled),
 			slog.String(string(contextKeyRunID), runID.String()),
 		),
 	)
 
-	mgr.Reload(ctx, logger, inv)
-	mgr.RunAll(ctx, logger)
+	mgr.Reload(ctx, mLogger, inv)
+	mgr.RunAll(ctx, mLogger)
 }
 
 // Reload accepts a struct that fulfills the inventory.Store interface and

diff --git a/internal/manager/module.go b/internal/manager/module.go
@@ -44,7 +44,7 @@ func (mgr *Manager) ReloadModules(ctx context.Context, logger *slog.Logger) {
 	modGraph := graph.New(moduleHash, graph.Directed(), graph.PreventCycles())
 	for _, mod := range rawMods {
 		newMod := Module{m: mod}
-		logger = logger.With(
+		modLogger := logger.With(
 			slog.Group(
 				"module",
 				slog.String("id", mod.String()),
@@ -54,14 +54,14 @@ func (mgr *Manager) ReloadModules(ctx context.Context, logger *slog.Logger) {
 		// if the module has a variables file set, source it and store
 		// the expanded variables
 		if mod.Variables != "" {
-			newMod.Variables = mgr.ReloadVariables(ctx, logger, []string{mod.Variables}, shell.MakeVariableMap(mgr.hostVariables))
+			newMod.Variables = mgr.ReloadVariables(ctx, modLogger, []string{mod.Variables}, shell.MakeVariableMap(mgr.hostVariables))
 		} else {
-			logger.DebugContext(ctx, "No module variables")
+			modLogger.DebugContext(ctx, "No module variables")
 		}
 
 		err := modGraph.AddVertex(newMod)
 		if err != nil {
-			logger.LogAttrs(
+			modLogger.LogAttrs(
 				ctx,
 				slog.LevelError,
 				"Failed to add module to DAG",
@@ -203,7 +203,7 @@ func (mgr *Manager) RunModules(ctx context.Context, logger *slog.Logger) {
 	}
 
 	for _, v := range order {
-		logger = logger.With(
+		vLogger := logger.With(
 			slog.Group(
 				"module",
 				slog.String("id", v),
@@ -212,19 +212,19 @@ func (mgr *Manager) RunModules(ctx context.Context, logger *slog.Logger) {
 
 		mod, err := mgr.modules.Vertex(v)
 		if err != nil {
-			logger.LogAttrs(
+			vLogger.LogAttrs(
 				ctx,
 				slog.LevelError,
 				"Failed to retreive module from DAG vertex",
 				slog.String("err", err.Error()),
 			)
 		}
 
-		logger.InfoContext(ctx, "Module started")
-		defer logger.InfoContext(ctx, "Module finished")
+		vLogger.InfoContext(ctx, "Module started")
+		defer vLogger.InfoContext(ctx, "Module finished")
 
-		if err := mgr.RunModule(ctx, logger, mod); err != nil {
-			logger.LogAttrs(
+		if err := mgr.RunModule(ctx, vLogger, mod); err != nil {
+			vLogger.LogAttrs(
 				ctx,
 				slog.LevelError,
 				"Module failed",