Implement PackageVariantSet injector template (#3952)

kptdev · May 11, 2023 · fca5a91 · fca5a91
1 parent ef09e04
commit fca5a91
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 10 deletions.
diff --git a/docs/design-docs/08-package-variant.md b/docs/design-docs/08-package-variant.md
@@ -957,7 +957,7 @@ type PackageVariantTemplate struct {
 
         // Injectors allows specifying the spec.Injectors field of the generated PackageVariant
         // +optional
-        Injectors     *InfectionSelectorTemplate `json:"injectors,omitempty"`
+        Injectors     []InjectionSelectorTemplate `json:"injectors,omitempty"`
 }
 
 // DownstreamTemplate is used to calculate the downstream field of the resulting

diff --git a/porch/controllers/packagevariantsets/api/v1alpha2/packagevariantset_types.go b/porch/controllers/packagevariantsets/api/v1alpha2/packagevariantset_types.go
@@ -136,7 +136,7 @@ type PackageVariantTemplate struct {
 
 	// Injectors allows specifying the spec.Injectors field of the generated PackageVariant
 	// +optional
-	//Injectors     *InfectionSelectorTemplate `json:"injectors,omitempty"`
+	Injectors []InjectionSelectorTemplate `json:"injectors,omitempty"`
 }
 
 // DownstreamTemplate is used to calculate the downstream field of the resulting
@@ -160,8 +160,8 @@ type PackageContextTemplate struct {
 }
 
 // InjectionSelectorTemplate is used to calculate the injectors field of the
-// resulting package variants. Only one of the Name and NameExpr fields may be
-// specified.
+// resulting package variants. Exactly one of the Name and NameExpr fields must
+// be specified. The other fields are optional.
 type InjectionSelectorTemplate struct {
 	Group   *string `json:"group,omitempty"`
 	Version *string `json:"version,omitempty"`

diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/render.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/render.go
@@ -74,7 +74,7 @@ func renderPackageVariantSpec(ctx context.Context, pvs *api.PackageVariantSet, r
 		if pvt.Downstream.RepoExpr != nil && *pvt.Downstream.RepoExpr != "" {
 			repo, err = evalExpr(*pvt.Downstream.RepoExpr, inputs)
 			if err != nil {
-				return nil, fmt.Errorf("spec.downstream.repoExpr: %s", err.Error())
+				return nil, fmt.Errorf("template.downstream.repoExpr: %s", err.Error())
 			}
 		}
 
@@ -104,7 +104,7 @@ func renderPackageVariantSpec(ctx context.Context, pvs *api.PackageVariantSet, r
 		if pvt.Downstream.PackageExpr != nil && *pvt.Downstream.PackageExpr != "" {
 			spec.Downstream.Package, err = evalExpr(*pvt.Downstream.PackageExpr, inputs)
 			if err != nil {
-				return nil, fmt.Errorf("spec.downstream.packageExpr: %s", err.Error())
+				return nil, fmt.Errorf("template.downstream.packageExpr: %s", err.Error())
 			}
 		}
 	}
@@ -116,23 +116,23 @@ func renderPackageVariantSpec(ctx context.Context, pvs *api.PackageVariantSet, r
 	if pvt.DeletionPolicy != nil {
 		spec.DeletionPolicy = *pvt.DeletionPolicy
 	}
-	spec.Labels, err = copyAndOverlayMapExpr("spec.labelExprs", pvt.Labels, pvt.LabelExprs, inputs)
+	spec.Labels, err = copyAndOverlayMapExpr("template.labelExprs", pvt.Labels, pvt.LabelExprs, inputs)
 	if err != nil {
 		return nil, err
 	}
 
-	spec.Annotations, err = copyAndOverlayMapExpr("spec.annotationExprs", pvt.Annotations, pvt.AnnotationExprs, inputs)
+	spec.Annotations, err = copyAndOverlayMapExpr("template.annotationExprs", pvt.Annotations, pvt.AnnotationExprs, inputs)
 	if err != nil {
 		return nil, err
 	}
 
 	if pvt.PackageContext != nil {
-		data, err := copyAndOverlayMapExpr("spec.packageContext.dataExprs", pvt.PackageContext.Data, pvt.PackageContext.DataExprs, inputs)
+		data, err := copyAndOverlayMapExpr("template.packageContext.dataExprs", pvt.PackageContext.Data, pvt.PackageContext.DataExprs, inputs)
 		if err != nil {
 			return nil, err
 		}
 
-		removeKeys, err := copyAndOverlayStringSlice("spec.packageContext.removeKeyExprs", pvt.PackageContext.RemoveKeys,
+		removeKeys, err := copyAndOverlayStringSlice("template.packageContext.removeKeyExprs", pvt.PackageContext.RemoveKeys,
 			pvt.PackageContext.RemoveKeyExprs, inputs)
 		if err != nil {
 			return nil, err
@@ -142,6 +142,27 @@ func renderPackageVariantSpec(ctx context.Context, pvs *api.PackageVariantSet, r
 			RemoveKeys: removeKeys,
 		}
 	}
+
+	for i, injTemplate := range pvt.Injectors {
+		injector := pkgvarapi.InjectionSelector{
+			Group:   injTemplate.Group,
+			Version: injTemplate.Version,
+			Kind:    injTemplate.Kind,
+		}
+		if injTemplate.Name != nil && *injTemplate.Name != "" {
+			injector.Name = *injTemplate.Name
+		}
+
+		if injTemplate.NameExpr != nil && *injTemplate.NameExpr != "" {
+			injector.Name, err = evalExpr(*injTemplate.NameExpr, inputs)
+			if err != nil {
+				return nil, fmt.Errorf("template.injectors[%d].nameExpr: %s", i, err.Error())
+			}
+		}
+
+		spec.Injectors = append(spec.Injectors, injector)
+	}
+
 	return spec, nil
 }
 

diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/render_test.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/render_test.go
@@ -346,6 +346,56 @@ func TestRenderPackageVariantSpec(t *testing.T) {
 			},
 			expectedErrs: nil,
 		},
+		"template injectors": {
+			downstream: pvContext{
+				repoDefault:    "my-repo-1",
+				packageDefault: "p",
+				template: &api.PackageVariantTemplate{
+					Injectors: []api.InjectionSelectorTemplate{
+						{
+							Group:   pointer.String("kpt.dev"),
+							Version: pointer.String("v1alpha1"),
+							Kind:    pointer.String("Foo"),
+							Name:    pointer.String("bar"),
+						},
+						{
+							Group:    pointer.String("kpt.dev"),
+							Version:  pointer.String("v1alpha1"),
+							Kind:     pointer.String("Foo"),
+							NameExpr: pointer.String("repository.labels['abc']"),
+						},
+						{
+							NameExpr: pointer.String("repository.name + '-test'"),
+						},
+					},
+				},
+			},
+			expectedSpec: pkgvarapi.PackageVariantSpec{
+				Upstream: pvs.Spec.Upstream,
+				Downstream: &pkgvarapi.Downstream{
+					Repo:    "my-repo-1",
+					Package: "p",
+				},
+				Injectors: []pkgvarapi.InjectionSelector{
+					{
+						Group:   pointer.String("kpt.dev"),
+						Version: pointer.String("v1alpha1"),
+						Kind:    pointer.String("Foo"),
+						Name:    "bar",
+					},
+					{
+						Group:   pointer.String("kpt.dev"),
+						Version: pointer.String("v1alpha1"),
+						Kind:    pointer.String("Foo"),
+						Name:    "def",
+					},
+					{
+						Name: "my-repo-1-test",
+					},
+				},
+			},
+			expectedErrs: nil,
+		},
 	}
 
 	for tn, tc := range testCases {

diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/validate.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/validate.go
@@ -131,6 +131,15 @@ func validateTemplate(template *api.PackageVariantTemplate, field string) []erro
 		allErrs = append(allErrs, validateMapExpr(template.PackageContext.DataExprs, fmt.Sprintf("%s.packageContext.dataExprs", field))...)
 	}
 
+	for i, injector := range template.Injectors {
+		if injector.Name != nil && injector.NameExpr != nil {
+			allErrs = append(allErrs, fmt.Errorf("%s.injectors[%d] may specify only one of `name` and `nameExpr`", field, i))
+		}
+
+		if injector.Name == nil && injector.NameExpr == nil {
+			allErrs = append(allErrs, fmt.Errorf("%s.injectors[%d] must specify either `name` or `nameExpr`", field, i))
+		}
+	}
 	return allErrs
 }
 

diff --git a/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/validate_test.go b/porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/validate_test.go
@@ -169,6 +169,23 @@ spec:
 				"spec.targets[0].template.packageContext.dataExprs[1] may specify only one of `value` and `valueExpr`",
 			},
 		},
+		"injectors must specify exactly one of name or nameexpr": {
+			packageVariant: packageVariantHeader + `
+spec:
+  targets:
+  - repositories:
+    - name: bar
+    template:
+      injectors:
+      - name: foo
+        nameExpr: bar
+      - group: foo
+`,
+			expectedErrs: []string{"spec.upstream is a required field",
+				"spec.targets[0].template.injectors[0] may specify only one of `name` and `nameExpr`",
+				"spec.targets[0].template.injectors[1] must specify either `name` or `nameExpr`",
+			},
+		},
 	}
 
 	for tn, tc := range testCases {