From 3759a161a5e50a3c07fcc6df93e25dc764b7389a Mon Sep 17 00:00:00 2001
From: Gerd Oberlechner <goberlec@redhat.com>
Date: Wed, 4 Dec 2024 22:46:21 +0100
Subject: [PATCH] fix pipeline.yaml step dependency validation

Signed-off-by: Gerd Oberlechner <goberlec@redhat.com>
---
 tooling/templatize/pkg/pipeline/run.go      | 37 +++++----
 tooling/templatize/pkg/pipeline/run_test.go | 91 ++++++++++++++++++++-
 2 files changed, 110 insertions(+), 18 deletions(-)

diff --git a/tooling/templatize/pkg/pipeline/run.go b/tooling/templatize/pkg/pipeline/run.go
index b1c9b989f..492522665 100644
--- a/tooling/templatize/pkg/pipeline/run.go
+++ b/tooling/templatize/pkg/pipeline/run.go
@@ -162,6 +162,29 @@ func (s *Step) description() string {
 }
 
 func (p *Pipeline) Validate() error {
+	// collect all steps from all resourcegroups and fail if there are duplicates
+	stepMap := make(map[string]*Step)
+	for _, rg := range p.ResourceGroups {
+		for _, step := range rg.Steps {
+			if _, ok := stepMap[step.Name]; ok {
+				return fmt.Errorf("duplicate step name %q", step.Name)
+			}
+			stepMap[step.Name] = step
+		}
+	}
+
+	// validate dependsOn for a step exists
+	for _, step := range stepMap {
+		for _, dep := range step.DependsOn {
+			if _, ok := stepMap[dep]; !ok {
+				return fmt.Errorf("invalid dependency on step %s: dependency %s does not exist", step.Name, dep)
+			}
+		}
+	}
+
+	// todo check for circular dependencies
+
+	// validate resource groups
 	for _, rg := range p.ResourceGroups {
 		err := rg.Validate()
 		if err != nil {
@@ -178,19 +201,5 @@ func (rg *ResourceGroup) Validate() error {
 	if rg.Subscription == "" {
 		return fmt.Errorf("subscription is required")
 	}
-
-	// validate step dependencies
-	// todo - check for circular dependencies
-	stepMap := make(map[string]bool)
-	for _, step := range rg.Steps {
-		stepMap[step.Name] = true
-	}
-	for _, step := range rg.Steps {
-		for _, dep := range step.DependsOn {
-			if !stepMap[dep] {
-				return fmt.Errorf("invalid dependency from step %s to %s", step.Name, dep)
-			}
-		}
-	}
 	return nil
 }
diff --git a/tooling/templatize/pkg/pipeline/run_test.go b/tooling/templatize/pkg/pipeline/run_test.go
index f61c33369..573f9bc26 100644
--- a/tooling/templatize/pkg/pipeline/run_test.go
+++ b/tooling/templatize/pkg/pipeline/run_test.go
@@ -76,11 +76,94 @@ func TestRGValidate(t *testing.T) {
 }
 
 func TestPipelineValidate(t *testing.T) {
-	p := &Pipeline{
-		ResourceGroups: []*ResourceGroup{{}},
+	testCases := []struct {
+		name     string
+		pipeline *Pipeline
+		err      string
+	}{
+		{
+			name: "missing name",
+			pipeline: &Pipeline{
+				ResourceGroups: []*ResourceGroup{{}},
+			},
+			err: "resource group name is required",
+		},
+		{
+			name: "missing subscription",
+			pipeline: &Pipeline{
+				ResourceGroups: []*ResourceGroup{
+					{
+						Name: "rg",
+					},
+				},
+			},
+			err: "subscription is required",
+		},
+		{
+			name: "missing step dependency",
+			pipeline: &Pipeline{
+				ResourceGroups: []*ResourceGroup{
+					{
+						Name:         "rg1",
+						Subscription: "sub1",
+						Steps: []*Step{
+							{
+								Name: "step1",
+							},
+						},
+					},
+					{
+						Name:         "rg2",
+						Subscription: "sub1",
+						Steps: []*Step{
+							{
+								Name:      "step2",
+								DependsOn: []string{"step3"},
+							},
+						},
+					},
+				},
+			},
+			err: "invalid dependency on step step2: dependency step3 does not exist",
+		},
+		{
+			name: "valid step dependencies",
+			pipeline: &Pipeline{
+				ResourceGroups: []*ResourceGroup{
+					{
+						Name:         "rg1",
+						Subscription: "sub1",
+						Steps: []*Step{
+							{
+								Name: "step1",
+							},
+						},
+					},
+					{
+						Name:         "rg2",
+						Subscription: "sub1",
+						Steps: []*Step{
+							{
+								Name:      "step2",
+								DependsOn: []string{"step1"},
+							},
+						},
+					},
+				},
+			},
+			err: "",
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.pipeline.Validate()
+			if tc.err == "" {
+				assert.NilError(t, err)
+			} else {
+				assert.Error(t, err, tc.err)
+			}
+		})
 	}
-	err := p.Validate()
-	assert.Error(t, err, "resource group name is required")
 }
 
 func TestResourceGroupRun(t *testing.T) {