From 653c938edc3047bef6725cddc84c0eebbd016e12 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Fri, 10 Jul 2020 13:33:45 -0400
Subject: [PATCH] watch: extract makeWatchPlan to facilitate testing

There is a bug in here now that slices in opaque config are unsliced. But to test that bug fix we
need a function that can be easily tested.
---
 agent/agent.go         | 38 ++------------------------------------
 agent/watch_handler.go | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 36 deletions(-)

diff --git a/agent/agent.go b/agent/agent.go
index f3dff12784..f0ddfa5a6b 100644
--- a/agent/agent.go
+++ b/agent/agent.go
@@ -1336,44 +1336,10 @@ func (a *Agent) reloadWatches(cfg *config.RuntimeConfig) error {
 			}
 		}
 
-		// Parse the watches, excluding 'handler' and 'args'
-		wp, err := watch.ParseExempt(params, []string{"handler", "args"})
+		wp, err := makeWatchPlan(a.logger, params)
 		if err != nil {
-			return fmt.Errorf("Failed to parse watch (%#v): %v", params, err)
+			return err
 		}
-
-		// Get the handler and subprocess arguments
-		handler, hasHandler := wp.Exempt["handler"]
-		args, hasArgs := wp.Exempt["args"]
-		if hasHandler {
-			a.logger.Warn("The 'handler' field in watches has been deprecated " +
-				"and replaced with the 'args' field. See https://www.consul.io/docs/agent/watches.html")
-		}
-		if _, ok := handler.(string); hasHandler && !ok {
-			return fmt.Errorf("Watch handler must be a string")
-		}
-		if raw, ok := args.([]interface{}); hasArgs && ok {
-			var parsed []string
-			for _, arg := range raw {
-				v, ok := arg.(string)
-				if !ok {
-					return fmt.Errorf("Watch args must be a list of strings")
-				}
-
-				parsed = append(parsed, v)
-			}
-			wp.Exempt["args"] = parsed
-		} else if hasArgs && !ok {
-			return fmt.Errorf("Watch args must be a list of strings")
-		}
-		if hasHandler && hasArgs || hasHandler && wp.HandlerType == "http" || hasArgs && wp.HandlerType == "http" {
-			return fmt.Errorf("Only one watch handler allowed")
-		}
-		if !hasHandler && !hasArgs && wp.HandlerType != "http" {
-			return fmt.Errorf("Must define a watch handler")
-		}
-
-		// Store the watch plan
 		watchPlans = append(watchPlans, wp)
 	}
 
diff --git a/agent/watch_handler.go b/agent/watch_handler.go
index a98acd68c8..82431a43dd 100644
--- a/agent/watch_handler.go
+++ b/agent/watch_handler.go
@@ -184,3 +184,44 @@ func makeHTTPWatchHandler(logger hclog.Logger, config *watch.HttpHandlerConfig)
 	}
 	return fn
 }
+
+// TODO: return a fully constructed watch.Plan with a Plan.Handler, so that Except
+// can be ignored by the caller.
+func makeWatchPlan(logger hclog.Logger, params map[string]interface{}) (*watch.Plan, error) {
+	wp, err := watch.ParseExempt(params, []string{"handler", "args"})
+	if err != nil {
+		return nil, fmt.Errorf("Failed to parse watch (%#v): %v", params, err)
+	}
+
+	// Get the handler and subprocess arguments
+	handler, hasHandler := wp.Exempt["handler"]
+	args, hasArgs := wp.Exempt["args"]
+	if hasHandler {
+		logger.Warn("The 'handler' field in watches has been deprecated " +
+			"and replaced with the 'args' field. See https://www.consul.io/docs/agent/watches.html")
+	}
+	if _, ok := handler.(string); hasHandler && !ok {
+		return nil, fmt.Errorf("Watch handler must be a string")
+	}
+	if raw, ok := args.([]interface{}); hasArgs && ok {
+		var parsed []string
+		for _, arg := range raw {
+			v, ok := arg.(string)
+			if !ok {
+				return nil, fmt.Errorf("Watch args must be a list of strings")
+			}
+
+			parsed = append(parsed, v)
+		}
+		wp.Exempt["args"] = parsed
+	} else if hasArgs && !ok {
+		return nil, fmt.Errorf("Watch args must be a list of strings")
+	}
+	if hasHandler && hasArgs || hasHandler && wp.HandlerType == "http" || hasArgs && wp.HandlerType == "http" {
+		return nil, fmt.Errorf("Only one watch handler allowed")
+	}
+	if !hasHandler && !hasArgs && wp.HandlerType != "http" {
+		return nil, fmt.Errorf("Must define a watch handler")
+	}
+	return wp, nil
+}