refactor: remove crd from pipeline factory

- crd can be obtained directly from the promise
- also: remove rubyism from tests 🤠

Co-authored-by: Chunyi Lyu <chunyi@syntasso.io>

api/v1alpha1/pipeline_types.go

@@ -30,7 +30,6 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
-	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -68,18 +67,14 @@ type Pipeline struct {
 }
 
 type PipelineFactory struct {
-	ID        string
-	Promise   *Promise
-	Pipeline  *Pipeline
-	Namespace string
-
-	ResourceRequest *unstructured.Unstructured
-	CRD             *apiextensionsv1.CustomResourceDefinition
-
+	ID               string
+	Promise          *Promise
+	Pipeline         *Pipeline
+	Namespace        string
+	ResourceRequest  *unstructured.Unstructured
 	ResourceWorkflow bool
-
-	WorkflowAction Action
-	WorkflowType   Type
+	WorkflowAction   Action
+	WorkflowType     Type
 }
 
 // +kubebuilder:object:generate=false
@@ -140,15 +135,14 @@ func (p *Pipeline) ForPromise(promise *Promise, action Action) *PipelineFactory
 	}
 }
 
-func (p *Pipeline) ForResource(promise *Promise, action Action, crd *apiextensionsv1.CustomResourceDefinition, resourceRequest *unstructured.Unstructured) *PipelineFactory {
+func (p *Pipeline) ForResource(promise *Promise, action Action, resourceRequest *unstructured.Unstructured) *PipelineFactory {
 	return &PipelineFactory{
 		ID:               promise.GetName() + "-resource-pipeline",
 		Promise:          promise,
 		Pipeline:         p,
 		ResourceRequest:  resourceRequest,
 		Namespace:        resourceRequest.GetNamespace(),
 		ResourceWorkflow: true,
-		CRD:              crd,
 		WorkflowType:     WorkflowTypeResource,
 		WorkflowAction:   action,
 	}
@@ -163,7 +157,10 @@ func (p *PipelineFactory) Resources(jobEnv []corev1.EnvVar) (PipelineJobResource
 
 	serviceAccount := p.ServiceAccount()
 
-	role := p.ObjectRole()
+	role, err := p.ObjectRole()
+	if err != nil {
+		return PipelineJobResources{}, err
+	}
 	roleBinding := p.ObjectRoleBinding(role.GetName(), serviceAccount)
 
 	job, err := p.PipelineJob(schedulingConfigMap, serviceAccount, jobEnv)
@@ -193,11 +190,11 @@ func (p *PipelineFactory) ServiceAccount() *corev1.ServiceAccount {
 	}
 }
 
-func (p *PipelineFactory) ObjectRole() client.Object {
+func (p *PipelineFactory) ObjectRole() (client.Object, error) {
 	if p.ResourceWorkflow {
 		return p.role()
 	}
-	return p.clusterRole()
+	return p.clusterRole(), nil
 }
 
 func (p *PipelineFactory) ObjectRoleBinding(roleName string, serviceAccount *corev1.ServiceAccount) client.Object {
@@ -481,8 +478,12 @@ func (p *PipelineFactory) getObjAndHash() (*unstructured.Unstructured, string, e
 	return p.ResourceRequest, hash.ComputeHash(fmt.Sprintf("%s-%s", promiseHash, resourceHash)), nil
 }
 
-func (p *PipelineFactory) role() *rbacv1.Role {
-	plural := p.CRD.Spec.Names.Plural
+func (p *PipelineFactory) role() (*rbacv1.Role, error) {
+	crd, err := p.Promise.GetAPIAsCRD()
+	if err != nil {
+		return nil, err
+	}
+	plural := crd.Spec.Names.Plural
 	return &rbacv1.Role{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      p.ID,
@@ -491,7 +492,7 @@ func (p *PipelineFactory) role() *rbacv1.Role {
 		},
 		Rules: []rbacv1.PolicyRule{
 			{
-				APIGroups: []string{p.CRD.Spec.Group},
+				APIGroups: []string{crd.Spec.Group},
 				Resources: []string{plural, plural + "/status"},
 				Verbs:     []string{"get", "list", "update", "create", "patch"},
 			},
@@ -501,7 +502,7 @@ func (p *PipelineFactory) role() *rbacv1.Role {
 				Verbs:     []string{"*"},
 			},
 		},
-	}
+	}, nil
 }
 
 func (p *PipelineFactory) roleBinding(roleName string, serviceAccount *corev1.ServiceAccount) *rbacv1.RoleBinding {
@@ -591,7 +592,9 @@ func WorkflowLabels(workflowType Type, workflowAction Action, pipelineName strin
 	}
 
 	if workflowAction != "" {
-		ls = labels.Merge(ls, map[string]string{})
+		ls = labels.Merge(ls, map[string]string{
+			WorkActionLabel: string(workflowAction),
+		})
 	}
 	return ls
 }

api/v1alpha1/pipeline_types_test.go

@@ -1,6 +1,7 @@
 package v1alpha1_test
 
 import (
+	"encoding/json"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	. "github.com/onsi/gomega/gstruct"
@@ -11,6 +12,7 @@ import (
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	"strings"
 )
 
@@ -47,6 +49,17 @@ var _ = Describe("Pipeline", func() {
 				ImagePullSecrets: []corev1.LocalObjectReference{{Name: "imagePullSecret"}},
 			},
 		}
+		promiseCrd = &apiextensionsv1.CustomResourceDefinition{
+			Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+				Group: "promise.crd.group",
+				Names: apiextensionsv1.CustomResourceDefinitionNames{
+					Plural: "promiseCrdPlural",
+				},
+			},
+		}
+
+		rawCrd, err := json.Marshal(promiseCrd)
+		Expect(err).ToNot(HaveOccurred())
 		promise = &v1alpha1.Promise{
 			TypeMeta: metav1.TypeMeta{
 				APIVersion: "fake.promise.group/v1",
@@ -60,17 +73,10 @@ var _ = Describe("Pipeline", func() {
 					{MatchLabels: map[string]string{"label": "value"}},
 					{MatchLabels: map[string]string{"another-label": "another-value"}},
 				},
+				API: &runtime.RawExtension{Raw: rawCrd},
 			},
 		}
 
-		promiseCrd = &apiextensionsv1.CustomResourceDefinition{
-			Spec: apiextensionsv1.CustomResourceDefinitionSpec{
-				Group: "promise.crd.group",
-				Names: apiextensionsv1.CustomResourceDefinitionNames{
-					Plural: "promiseCrdPlural",
-				},
-			},
-		}
 		resourceRequest = &unstructured.Unstructured{
 			Object: map[string]interface{}{
 				"apiVersion": "fake.resource.group/v1",
@@ -83,7 +89,7 @@ var _ = Describe("Pipeline", func() {
 	})
 
 	Describe("Pipeline Factory Constructors", func() {
-		Describe("#ForPromise", func() {
+		Describe("ForPromise", func() {
 			It("sets the appropriate fields", func() {
 				f := pipeline.ForPromise(promise, v1alpha1.WorkflowActionConfigure)
 				Expect(f).ToNot(BeNil())
@@ -98,9 +104,9 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#ForResource", func() {
+		Describe("ForResource", func() {
 			It("sets the appropriate fields", func() {
-				f := pipeline.ForResource(promise, v1alpha1.WorkflowActionConfigure, promiseCrd, resourceRequest)
+				f := pipeline.ForResource(promise, v1alpha1.WorkflowActionConfigure, resourceRequest)
 				Expect(f).ToNot(BeNil())
 				Expect(f.ID).To(Equal(promise.GetName() + "-resource-pipeline"))
 				Expect(f.Promise).To(Equal(promise))
@@ -124,20 +130,20 @@ var _ = Describe("Pipeline", func() {
 				ID:              "factoryID",
 				Namespace:       "factoryNamespace",
 				Promise:         promise,
-				CRD:             promiseCrd,
 				WorkflowAction:  "fakeAction",
 				WorkflowType:    "fakeType",
 				ResourceRequest: resourceRequest,
 				Pipeline:        pipeline,
 			}
 		})
 
-		Describe("#Resources", func() {
+		Describe("Resources", func() {
 			When("building resources for the configure action", func() {
 				It("should return a list of resources", func() {
 					factory.WorkflowAction = v1alpha1.WorkflowActionConfigure
 					env := []corev1.EnvVar{{Name: "env1", Value: "value1"}}
-					role := factory.ObjectRole()
+					role, err := factory.ObjectRole()
+					Expect(err).ToNot(HaveOccurred())
 					serviceAccount := factory.ServiceAccount()
 					configMap, err := factory.ConfigMap(promise.GetWorkloadGroupScheduling())
 					Expect(err).ToNot(HaveOccurred())
@@ -161,7 +167,8 @@ var _ = Describe("Pipeline", func() {
 				It("should return a list of resources", func() {
 					factory.WorkflowAction = v1alpha1.WorkflowActionDelete
 					env := []corev1.EnvVar{{Name: "env1", Value: "value1"}}
-					role := factory.ObjectRole()
+					role, err := factory.ObjectRole()
+					Expect(err).ToNot(HaveOccurred())
 					serviceAccount := factory.ServiceAccount()
 					configMap, err := factory.ConfigMap(promise.GetWorkloadGroupScheduling())
 					Expect(err).ToNot(HaveOccurred())
@@ -183,7 +190,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#ServiceAccount", func() {
+		Describe("ServiceAccount", func() {
 			It("should return a service account", func() {
 				sa := factory.ServiceAccount()
 				Expect(sa).ToNot(BeNil())
@@ -193,10 +200,11 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#ObjectRole", func() {
+		Describe("ObjectRole", func() {
 			When("building a role for a promise pipeline", func() {
 				It("returns a cluster role", func() {
-					objectRole := factory.ObjectRole()
+					objectRole, err := factory.ObjectRole()
+					Expect(err).ToNot(HaveOccurred())
 					Expect(objectRole).ToNot(BeNil())
 					Expect(objectRole).To(BeAssignableToTypeOf(&rbacv1.ClusterRole{}))
 
@@ -216,7 +224,8 @@ var _ = Describe("Pipeline", func() {
 				It("returns a role", func() {
 					factory.ResourceWorkflow = true
 
-					objectRole := factory.ObjectRole()
+					objectRole, err := factory.ObjectRole()
+					Expect(err).ToNot(HaveOccurred())
 					Expect(objectRole).ToNot(BeNil())
 					Expect(objectRole).To(BeAssignableToTypeOf(&rbacv1.Role{}))
 
@@ -238,7 +247,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#ObjectRoleBinding", func() {
+		Describe("ObjectRoleBinding", func() {
 			var serviceAccount *corev1.ServiceAccount
 			BeforeEach(func() {
 				serviceAccount = &corev1.ServiceAccount{
@@ -300,7 +309,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#ConfigMap", func() {
+		Describe("ConfigMap", func() {
 			It("should return a config map", func() {
 				workloadGroupScheduling := []v1alpha1.WorkloadGroupScheduling{
 					{MatchLabels: map[string]string{"label": "value"}, Source: "promise"},
@@ -316,7 +325,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#DefaultVolumes", func() {
+		Describe("DefaultVolumes", func() {
 			It("should return a list of default volumes", func() {
 				configMap := &corev1.ConfigMap{
 					ObjectMeta: metav1.ObjectMeta{
@@ -339,7 +348,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#DefaultPipelineVolumes", func() {
+		Describe("DefaultPipelineVolumes", func() {
 			It("should return a list of default pipeline volumes", func() {
 				volumes, volumeMounts := factory.DefaultPipelineVolumes()
 				Expect(volumes).To(HaveLen(3))
@@ -358,7 +367,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#DefaultEnvVars", func() {
+		Describe("DefaultEnvVars", func() {
 			It("should return a list of default environment variables", func() {
 				envVars := factory.DefaultEnvVars()
 				Expect(envVars).To(HaveLen(3))
@@ -370,7 +379,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#ReaderContainer", func() {
+		Describe("ReaderContainer", func() {
 			When("building the reader container for a promise pipeline", func() {
 				It("returns a the reader container with the promise information", func() {
 					container := factory.ReaderContainer()
@@ -414,7 +423,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#WorkCreatorContainer", func() {
+		Describe("WorkCreatorContainer", func() {
 			When("building the work creator container for a promise pipeline", func() {
 				It("returns a the work creator container with the appropriate command", func() {
 					expectedFlags := strings.Join([]string{
@@ -463,7 +472,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#PipelineContainers", func() {
+		Describe("PipelineContainers", func() {
 			var defaultEnvVars []corev1.EnvVar
 			var defaultVolumes []corev1.Volume
 			var defaultVolumeMounts []corev1.VolumeMount
@@ -504,7 +513,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#StatusWriterContainer", func() {
+		Describe("StatusWriterContainer", func() {
 			var obj *unstructured.Unstructured
 			var envVars []corev1.EnvVar
 			BeforeEach(func() {
@@ -546,7 +555,7 @@ var _ = Describe("Pipeline", func() {
 			})
 		})
 
-		Describe("#PipelineJob", func() {
+		Describe("PipelineJob", func() {
 			var (
 				serviceAccount *corev1.ServiceAccount
 				configMap      *corev1.ConfigMap
@@ -576,6 +585,7 @@ var _ = Describe("Pipeline", func() {
 							Expect(definedLabels).To(SatisfyAll(
 								HaveKeyWithValue(v1alpha1.PromiseNameLabel, promise.GetName()),
 								HaveKeyWithValue(v1alpha1.WorkTypeLabel, string(factory.WorkflowType)),
+								HaveKeyWithValue(v1alpha1.WorkActionLabel, string(factory.WorkflowAction)),
 								HaveKeyWithValue(v1alpha1.PipelineNameLabel, pipeline.GetName()),
 								HaveKeyWithValue(v1alpha1.KratixResourceHashLabel, promiseHash(promise)),
 								Not(HaveKey(v1alpha1.ResourceNameLabel)),
@@ -660,6 +670,7 @@ var _ = Describe("Pipeline", func() {
 							Expect(definedLabels).To(SatisfyAll(
 								HaveKeyWithValue(v1alpha1.PromiseNameLabel, promise.GetName()),
 								HaveKeyWithValue(v1alpha1.WorkTypeLabel, string(factory.WorkflowType)),
+								HaveKeyWithValue(v1alpha1.WorkActionLabel, string(factory.WorkflowAction)),
 								HaveKeyWithValue(v1alpha1.PipelineNameLabel, pipeline.GetName()),
 								HaveKeyWithValue(v1alpha1.KratixResourceHashLabel, combinedHash(promiseHash(promise), resourceHash(resourceRequest))),
 								HaveKeyWithValue(v1alpha1.ResourceNameLabel, resourceRequest.GetName()),

api/v1alpha1/promise_types.go

@@ -283,7 +283,7 @@ func (p *Promise) generatePipelinesObjects(workflowType Type, workflowAction Act
 		return nil, err
 	}
 
-	allResources := []PipelineJobResources{}
+	var allResources []PipelineJobResources
 	pipelines := promisePipelines[workflowType][workflowAction]
 
 	lastIndex := len(pipelines) - 1
@@ -297,7 +297,7 @@ func (p *Promise) generatePipelinesObjects(workflowType Type, workflowAction Act
 		var err error
 		switch workflowType {
 		case WorkflowTypeResource:
-			resources, err = pipe.ForResource(p, workflowAction, crd, resourceRequest).Resources(additionalJobEnv)
+			resources, err = pipe.ForResource(p, workflowAction, resourceRequest).Resources(additionalJobEnv)
 		case WorkflowTypePromise:
 			resources, err = pipe.ForPromise(p, workflowAction).Resources(additionalJobEnv)
 		}

api/v1alpha1/work_types.go

@@ -30,6 +30,7 @@ const (
 	ResourceNameLabel = KratixPrefix + "resource-name"
 	PipelineNameLabel = KratixPrefix + "pipeline-name"
 	WorkTypeLabel     = KratixPrefix + "work-type"
+	WorkActionLabel   = KratixPrefix + "work-action"
 
 	WorkTypePromise          = "promise"
 	WorkTypeResource         = "resource"