Fix some helm gen bugs (#435)

* Fix some helm gen bugs

* changelog

* typo

* Value path was not respected in doc gen

changelog/v0.29.2/helm-gen-bugs.yaml

@@ -0,0 +1,4 @@
+changelog:
+  - type: NON_USER_FACING
+    description: Fix some helm generation bugs.
+    issueLink: https://github.com/solo-io/skv2/issues/434

codegen/cmd_test.go

@@ -275,7 +275,6 @@ var _ = Describe("Cmd", func() {
 			},
 			AnyVendorConfig: skv2Imports,
 			RenderProtos:    true,
-
 			Chart: &Chart{
 				Operators: []Operator{
 					{
@@ -308,6 +307,10 @@ var _ = Describe("Cmd", func() {
 						"https://github.com/solo-io/skv2",
 					},
 				},
+				ValuesReferenceDocs: ValuesReferenceDocs{
+					Title:    "Name Override Chart",
+					Filename: "name_override_chart_reference.md",
+				},
 			},
 
 			ManifestRoot: "codegen/test/name_override_chart",
@@ -329,6 +332,13 @@ var _ = Describe("Cmd", func() {
 
 		Expect(deploymentString).NotTo(ContainSubstring("$.Values.painterOriginalName"))
 		Expect(deploymentString).To(ContainSubstring("$.Values.overrideName"))
+
+		fileContents, err = os.ReadFile("codegen/test/name_override_chart/name_override_chart_reference.md")
+		Expect(err).NotTo(HaveOccurred())
+		docString := string(fileContents)
+
+		Expect(docString).NotTo(ContainSubstring("painterOriginalName"))
+		Expect(docString).To(ContainSubstring("overrideName"))
 	})
 
 	It("generates json schema for the values file", func() {

codegen/doc/helm_values.go

@@ -69,7 +69,7 @@ func GenerateHelmValuesDoc(s interface{}, topLevelKey string, topLevelDesc strin
 
 	var path []string
 	if topLevelKey != "" {
-		path = []string{topLevelKey}
+		path = strings.Split(topLevelKey, ".")
 	}
 
 	docReflect(addValue, path, topLevelDesc, cfgT.Type(), cfgT)

codegen/model/chart.go

@@ -84,6 +84,14 @@ type Operator struct {
 	Values interface{}
 }
 
+func (o Operator) FormattedName() string {
+	formattedName := strcase.ToLowerCamel(o.Name)
+	if o.ValuesFileNameOverride != "" {
+		formattedName = strcase.ToLowerCamel(o.ValuesFileNameOverride)
+	}
+	return formattedName
+}
+
 // values for Deployment template
 type Deployment struct {
 	// TODO support use of a DaemonSet instead of a Deployment
@@ -215,7 +223,8 @@ func (c Chart) GenerateHelmDoc() string {
 
 	// generate documentation for operator values
 	for _, operatorWithValues := range helmValues.Operators {
-		name := strcase.ToLowerCamel(operatorWithValues.Name)
+
+		name := operatorWithValues.FormattedName()
 		values := operatorWithValues.Values
 
 		// clear image tag so it doesn't show build time commit hashes
@@ -225,8 +234,13 @@ func (c Chart) GenerateHelmDoc() string {
 			values.Sidecars[name] = container
 		}
 
-		helmValuesForDoc = append(helmValuesForDoc, doc.GenerateHelmValuesDoc(operatorWithValues.CustomValues, name, fmt.Sprintf("Configuration for the %s deployment.", name))...)
-		helmValuesForDoc = append(helmValuesForDoc, doc.GenerateHelmValuesDoc(values, name, fmt.Sprintf("Configuration for the %s deployment.", name))...)
+		keyPath := name
+		if operatorWithValues.ValuePath != "" {
+			keyPath = fmt.Sprintf("%s.%s", operatorWithValues.ValuePath, name)
+		}
+
+		helmValuesForDoc = append(helmValuesForDoc, doc.GenerateHelmValuesDoc(operatorWithValues.CustomValues, keyPath, fmt.Sprintf("Configuration for the %s deployment.", name))...)
+		helmValuesForDoc = append(helmValuesForDoc, doc.GenerateHelmValuesDoc(values, keyPath, fmt.Sprintf("Configuration for the %s deployment.", name))...)
 	}
 
 	return helmValuesForDoc.ToMarkdown(c.ValuesReferenceDocs.Title)

codegen/model/values/helm_chart_values.go

@@ -3,6 +3,7 @@ package values
 import (
 	"reflect"
 
+	"github.com/iancoleman/strcase"
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 )
@@ -23,6 +24,14 @@ type UserOperatorValues struct {
 	CustomValues           interface{}
 }
 
+func (o UserOperatorValues) FormattedName() string {
+	formattedName := strcase.ToLowerCamel(o.Name)
+	if o.ValuesFileNameOverride != "" {
+		formattedName = strcase.ToLowerCamel(o.ValuesFileNameOverride)
+	}
+	return formattedName
+}
+
 type UserValuesInlineDocs struct {
 	LineLengthLimit int
 }

codegen/render/funcs.go

@@ -94,7 +94,7 @@ func makeTemplateFuncs(customFuncs template.FuncMap) template.FuncMap {
 
 		"get_operator_values": func(o values.UserOperatorValues) map[string]interface{} {
 			opValues := map[string]interface{}{
-				opName(o.Name, o.ValuesFileNameOverride): o.Values,
+				o.FormattedName(): o.Values,
 			}
 			if o.ValuePath != "" {
 				splitPath := strings.Split(o.ValuePath, ".")
@@ -112,7 +112,7 @@ func makeTemplateFuncs(customFuncs template.FuncMap) template.FuncMap {
 			opValues := map[string]interface{}{}
 			if o.CustomValues != nil {
 				opValues = map[string]interface{}{
-					opName(o.Name, o.ValuesFileNameOverride): o.CustomValues,
+					o.FormattedName(): o.CustomValues,
 				}
 				if o.ValuePath != "" {
 					splitPath := strings.Split(o.ValuePath, ".")
@@ -169,22 +169,14 @@ func containerConfigs(op model.Operator) []containerConfig {
 }
 
 func opVar(op model.Operator) string {
-	name := opName(op.Name, op.ValuesFileNameOverride)
+	name := op.FormattedName()
 	opVar := fmt.Sprintf("$.Values.%s", name)
 	if op.ValuePath != "" {
 		opVar = fmt.Sprintf("$.Values.%s.%s", op.ValuePath, name)
 	}
 	return opVar
 }
 
-func opName(name, valuesFileNameOverride string) string {
-	formattedName := strcase.ToLowerCamel(name)
-	if valuesFileNameOverride != "" {
-		formattedName = strcase.ToLowerCamel(valuesFileNameOverride)
-	}
-	return formattedName
-}
-
 /*
 Find the proto messages for a given set of descriptors which need proto_deepcopoy funcs and whose types are not in
 the API root package
@@ -929,20 +921,37 @@ func mergeJsonSchema(
 	target *jsonschema.Schema,
 	src *jsonschema.Schema,
 ) {
-	if target.Properties == nil {
-		target.Properties = orderedmap.New()
-	}
 
 	if target.Required == nil {
 		target.Required = []string{}
 	}
 
-	for _, prop := range src.Properties.Keys() {
-		val, _ := src.Properties.Get(prop)
-		target.Properties.Set(prop, val)
+	target.Required = append(target.Required, src.Required...)
+
+	if target.Properties == nil {
+		target.Properties = orderedmap.New()
 	}
 
-	target.Required = append(target.Required, src.Required...)
+	for _, prop := range src.Properties.Keys() {
+		sourceVal, _ := src.Properties.Get(prop)
+		sourceValSchema, sourceIsSchema := sourceVal.(*jsonschema.Schema)
+		merged := false
+		if sourceIsSchema {
+			targetValue, targetHasProp := target.Properties.Get(prop)
+			if sourceValSchema.Properties != nil && len(sourceValSchema.Properties.Keys()) > 0 && targetHasProp {
+				targetValSchema, targetIsSchema := targetValue.(*jsonschema.Schema)
+				if targetIsSchema {
+					mergeJsonSchema(targetValSchema, sourceValSchema)
+					target.Properties.Set(prop, targetValSchema)
+					merged = true
+				}
+			}
+		}
+
+		if !merged {
+			target.Properties.Set(prop, sourceVal)
+		}
+	}
 }
 
 // jsonSchemaForOperator creates the json schema for the operator's values.
@@ -963,11 +972,11 @@ func jsonSchemaForOperator(
 	}
 
 	// nest the operator values schema in the correct place
-	path := []string{strcase.ToLowerCamel(o.Name)}
+	path := []string{o.FormattedName()}
 	if o.ValuePath != "" {
 		splitPath := strings.Split(o.ValuePath, ".")
 		reverse(splitPath)
-		path = append(splitPath, path...)
+		path = append(path, splitPath...)
 	}
 	schema = nestSchemaAtPath(schema, path...)
 

codegen/render/funcs_test.go

@@ -398,10 +398,11 @@ var _ = Describe("toJSONSchema", func() {
 					CustomValues: &Type2{Field2a: "hello2"},
 				},
 				{
-					Name:         "My_Operator_Two",
-					ValuePath:    "my.values",
-					Values:       values.UserValues{},
-					CustomValues: &Type2{Field2a: "hello2"},
+					Name:                   "My_Operator_Two",
+					ValuesFileNameOverride: "myOperatorNameOverride",
+					ValuePath:              "my.values",
+					Values:                 values.UserValues{},
+					CustomValues:           &Type2{Field2a: "hello2"},
 				},
 			},
 		})
@@ -418,11 +419,11 @@ var _ = Describe("toJSONSchema", func() {
 							Properties *struct {
 								Values *struct {
 									Properties map[string]map[string]interface{} `json:"properties"`
-								} `json:"values"`
+								} `json:"myOperatorNameOverride"`
 							} `json:"properties"`
-						} `json:"my"`
+						} `json:"values"`
 					} `json:"properties"`
-				} `json:"myOperatorTwo"`
+				} `json:"my"`
 			} `json:"properties"`
 		}{}
 

codegen/test/name_override_chart/name_override_chart_reference.md

@@ -0,0 +1,39 @@
+
+---
+title: "Name Override Chart"
+description: Reference for Helm values.
+weight: 2
+---
+
+|Option|Type|Default Value|Description|
+|------|----|-----------|-------------|
+|overrideName|struct| |Configuration for the overrideName deployment.|
+|overrideName|struct| ||
+|overrideName.image|struct| |Container image.|
+|overrideName.image.tag|string| |Version tag for the container image.|
+|overrideName.image.repository|string|painter|Image name (repository).|
+|overrideName.image.registry|string|quay.io/solo-io|Image registry.|
+|overrideName.image.pullPolicy|string|IfNotPresent|Image pull policy.|
+|overrideName.image.pullSecret|string| |Image pull secret.|
+|overrideName.env[]|slice|null|Environment variables for the container. For more info, see the [Kubernetes documentation](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#envvarsource-v1-core).|
+|overrideName.resources|struct| |Container resource requirements. For more info, see the [Kubernetes documentation](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#resourcerequirements-v1-core).|
+|overrideName.securityContext|struct| |Container security context. Set to 'false' to omit the security context entirely. For more info, see the [Kubernetes documentation](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#securitycontext-v1-core).|
+|overrideName.sidecars|map[string, struct]|{}|Optional configuration for the deployed containers.|
+|overrideName.sidecars.<MAP_KEY>|struct| |Optional configuration for the deployed containers.|
+|overrideName.sidecars.<MAP_KEY>.image|struct| |Container image.|
+|overrideName.sidecars.<MAP_KEY>.image.tag|string| |Version tag for the container image.|
+|overrideName.sidecars.<MAP_KEY>.image.repository|string| |Image name (repository).|
+|overrideName.sidecars.<MAP_KEY>.image.registry|string| |Image registry.|
+|overrideName.sidecars.<MAP_KEY>.image.pullPolicy|string| |Image pull policy.|
+|overrideName.sidecars.<MAP_KEY>.image.pullSecret|string| |Image pull secret.|
+|overrideName.sidecars.<MAP_KEY>.env[]|slice| |Environment variables for the container. For more info, see the [Kubernetes documentation](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#envvarsource-v1-core).|
+|overrideName.sidecars.<MAP_KEY>.resources|struct| |Container resource requirements. For more info, see the [Kubernetes documentation](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#resourcerequirements-v1-core).|
+|overrideName.sidecars.<MAP_KEY>.securityContext|struct| |Container security context. Set to 'false' to omit the security context entirely. For more info, see the [Kubernetes documentation](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.26/#securitycontext-v1-core).|
+|overrideName.floatingUserId|bool|false|Allow the pod to be assigned a dynamic user ID. Required for OpenShift installations.|
+|overrideName.runAsUser|uint32|10101|Static user ID to run the containers as. Unused if floatingUserId is 'true'.|
+|overrideName.serviceType|string| |Kubernetes service type. Can be either "ClusterIP", "NodePort", "LoadBalancer", or "ExternalName".|
+|overrideName.ports|map[string, uint32]|{}|Service ports as a map from port name to port number.|
+|overrideName.ports.<MAP_KEY>|uint32| |Service ports as a map from port name to port number.|
+|overrideName.deploymentOverrides|struct| |Arbitrary overrides for the component's [deployment template](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/deployment-v1/)|
+|overrideName.serviceOverrides|struct| |Arbitrary overrides for the component's [service template](https://kubernetes.io/docs/reference/kubernetes-api/service-resources/service-v1/).|
+|overrideName.enabled|bool|true|Enable creation of the deployment/service.|