From a0df812d31ba07d1f38615f1596ce958b331d3db Mon Sep 17 00:00:00 2001
From: Aaran McGuire <aaran@cloudflare.com>
Date: Fri, 23 Aug 2024 12:58:52 +0100
Subject: [PATCH] Improve YAML parsing to handle list sequences within SLI spec

This PR will allow more YAML configurations within the SLI spec as lists were previously not parsed.
---
 pkg/manifest/v1/alerts.go         |  2 +-
 pkg/manifest/v1/indicator.go      | 22 ++++++++++++----------
 pkg/manifest/v1/indicator_test.go | 18 ++++++++++++++++++
 3 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/pkg/manifest/v1/alerts.go b/pkg/manifest/v1/alerts.go
index 5467883..19694d7 100644
--- a/pkg/manifest/v1/alerts.go
+++ b/pkg/manifest/v1/alerts.go
@@ -26,7 +26,7 @@ type AlertCondition struct {
 // different from the AlertCondition type because it does not have an APIVersion.
 type AlertConditionInline struct {
 	Kind     string             `yaml:"kind" validate:"required"`
-	Metadata Metadata           `yaml:"metadata" validate:"required"`
+	Metadata Metadata           `yaml:"metadata,omitempty" validate:"omitempty"`
 	Spec     AlertConditionSpec `yaml:"spec" validate:"required"`
 }
 
diff --git a/pkg/manifest/v1/indicator.go b/pkg/manifest/v1/indicator.go
index 844456e..c6f27a2 100644
--- a/pkg/manifest/v1/indicator.go
+++ b/pkg/manifest/v1/indicator.go
@@ -40,7 +40,7 @@ type SLI struct {
 
 // SLIInline represents the SLI inline.
 type SLIInline struct {
-	Metadata Metadata `yaml:"metadata" validate:"required"`
+	Metadata Metadata `yaml:"metadata,omitempty" validate:"omitempty"`
 	Spec     SLISpec  `yaml:"spec" validate:"required"`
 }
 
@@ -70,15 +70,15 @@ type MetricSource struct {
 	MetricSourceSpec map[string]string `yaml:"spec" validate:"required_without=MetricSourceRef"`
 }
 
-// UnmarshalYAML is used to override the default unmarshal behavior
-// Since MetricSources don't have a determined structure, we need to do a few things here:
-//  1. Pull out the MetricSourceRef and Type separately, and add them to the MetricSource
-//  2. Attempt to unmarshal the MetricSourceSpec, which can be either a string or an array.
-//     2a.  If its a string, add it as a single string
-//     2b.  If its an array, flatten it to a single string
-//
-// This also assumes a certain flat structure that we can revisit if the need arises.
+// UnmarshalYAML is used to override the default unmarshal behavior.
+// MetricSources can have varying structures, so this method performs the following tasks:
+//  1. Extracts the MetricSourceRef and Type separately, and assigns them to the MetricSource.
+//  2. Attempts to unmarshal the MetricSourceSpec, which can be either a string, a list, or a more complex structure:
+//     2a. If it's a scalar string, it is added directly as a single string.
+//     2b. If it's a list of scalar values, they are concatenated into a single semicolon separated string.
+//     2c. If it's a more complex sequence (e.g., containing mappings), the values are processed and flattened appropriately.
 //
+// The current implementation assumes a relatively flat structure but can be adapted if more complex nesting becomes necessary.
 
 func (m *MetricSource) UnmarshalYAML(value *yaml.Node) error {
 	// temp struct to unmarshal the string values
@@ -108,7 +108,9 @@ func (m *MetricSource) UnmarshalYAML(value *yaml.Node) error {
 			// top level string that we will join with a semicolon
 			seqStrings := []string{}
 			for _, node := range v.Content {
-				if node.Kind == yaml.MappingNode {
+				if node.Kind == yaml.ScalarNode {
+					seqStrings = append(seqStrings, node.Value)
+				} else if node.Kind == yaml.MappingNode {
 					// each of these are k/v pairs that we will join with a comma
 					kvPairs := []string{}
 					for i := 0; i < len(node.Content); i += 2 {
diff --git a/pkg/manifest/v1/indicator_test.go b/pkg/manifest/v1/indicator_test.go
index 4e8bbde..b987faf 100644
--- a/pkg/manifest/v1/indicator_test.go
+++ b/pkg/manifest/v1/indicator_test.go
@@ -81,6 +81,24 @@ spec:
 				},
 			},
 		},
+		{
+			name: "Prometheus - Two labels",
+			yaml: `metricSourceRef: thanos
+type: Prometheus
+spec:
+  query: http_requests_total{}
+  dimensions:
+    - following
+    - another`,
+			want: MetricSource{
+				MetricSourceRef: "thanos",
+				Type:            "Prometheus",
+				MetricSourceSpec: map[string]string{
+					"query":      "http_requests_total{}",
+					"dimensions": "following;another",
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {