Merge pull request #10 from norwoodj/document-lists-and-objects

feat: updates values table generation allowing for non-empty lists/ma…

.goreleaser.yml

@@ -1,6 +1,7 @@
 before:
   hooks:
     - go mod download
+    - go test ./...
 builds:
   - main: ./cmd/helm-docs
     env:

Makefile

@@ -0,0 +1,15 @@
+helm-docs:
+	cd cmd/helm-docs && go build
+	mv cmd/helm-docs/helm-docs .
+
+.PHONY: fmt
+fmt:
+	go fmt ./...
+
+.PHONY: test
+test:
+	go test -v ./...
+
+.PHONY: clean
+clean:
+	rm helm-docs

README.md

@@ -99,17 +99,55 @@ controller:
   replicas: 2
 ```
 
-The descriptions will be picked up and put in the table in the README. The comment need not be near the parameter it
-explains, although this is probably preferable.
+The following rules are used to determine which values will be added to the values table in the README:
+
+* By default, only _leaf nodes_, that is, fields of type `int`, `string`, `float`, `bool`, empty lists, and empty maps
+  are added as rows in the values table. These fields will be added even if they do not have a description comment
+* Lists and maps which contain elements will not be added as rows in the values table _unless_ they have a description
+  comment which refers to them
+* Adding a description comment for a non-empty list or map in this way makes it so that leaf nodes underneath the
+  described field will _not_ be automatically added to the values table. In order to document both a non-empty list/map
+  _and_ a leaf node within that field, description comments must be added for both
+
+e.g. In this case, both `controller.livenessProbe` and `controller.livenessProbe.httpGet.path` will be added as rows in
+the values table, but `controller.livenessProbe.httpGet.port` will not
+```yaml
+controller:
+  # controller.livenessProbe -- Configure the healthcheck for the ingress controller
+  livenessProbe:
+    httpGet:
+      # controller.livenessProbe.httpGet.path -- This is the liveness check endpoint
+      path: /healthz
+      port: http
+```
+
+Results in:
+
+| Key | Type | Default | Description |
+|-----|------|---------|-------------|
+| controller.livenessProbe | object | `{"httpGet":{"path":"/healthz","port":8080}}` | Configure the healthcheck for the ingress controller |
+| controller.livenessProbe.httpGet.path | string | `"/healthz"` | This is the liveness check endpoint |
+
+If we remove the comment for `controller.livenessProbe` however, both leaf nodes `controller.livenessProbe.httpGet.path`
+and `controller.livenessProbe.httpGet.port` will be added to the table, with our without description comments:
 
-_Note:_ if the value in question contains any `.` characters, that section of the path must be quoted e.g.
 ```yaml
-service:
-  annotations:
-    # ingress.annotations."external-dns.alpha.kubernetes.io/hostname" -- Hostname to be assigned to the ELB for the service
-    external-dns.alpha.kubernetes.io/hostname: stupidchess.jmn23.com
+controller:
+  livenessProbe:
+    httpGet:
+      # controller.livenessProbe.httpGet.path -- This is the liveness check endpoint
+      path: /healthz
+      port: http
 ```
 
+Results in:
+
+| Key | Type | Default | Description |
+|-----|------|---------|-------------|
+| controller.livenessProbe.httpGet.path | string | `"/healthz"` | This is the liveness check endpoint |
+| controller.livenessProbe.httpGet.port | string | `"http"` | |
+
+
 ### nil values
 If you would like to define a key for a value, but leave the default empty, you can still specify a description for it
 as well as a type. Like so:
@@ -119,3 +157,18 @@ controller:
   replicas:
 ```
 This could be useful when wanting to enforce user-defined values for the chart, where there are no sensible defaults.
+
+### Spaces and Dots in keys
+If a key name contains any "." or " " characters, that section of the path must be quoted in description comments e.g.
+
+```yaml
+service:
+  annotations:
+    # service.annotations."external-dns.alpha.kubernetes.io/hostname" -- Hostname to be assigned to the ELB for the service
+    external-dns.alpha.kubernetes.io/hostname: stupidchess.jmn23.com
+
+configMap:
+  # configMap."not real config param" -- A completely fake config parameter for a useful example
+  not real config param: value
+```
+

cmd/helm-docs/main.go

@@ -12,7 +12,7 @@ import (
 	"github.com/spf13/viper"
 )
 
-func retrieveInfoAndPrintDocumentation(chartDirectory string, waitGroup *sync.WaitGroup) {
+func retrieveInfoAndPrintDocumentation(chartDirectory string, waitGroup *sync.WaitGroup, dryRun bool) {
 	defer waitGroup.Done()
 	chartDocumentationInfo, err := helm.ParseChartInformation(chartDirectory)
 
@@ -21,7 +21,7 @@ func retrieveInfoAndPrintDocumentation(chartDirectory string, waitGroup *sync.Wa
 		return
 	}
 
-	document.PrintDocumentation(chartDocumentationInfo, viper.GetBool("dry-run"))
+	document.PrintDocumentation(chartDocumentationInfo, dryRun)
 
 }
 
@@ -35,11 +35,18 @@ func helmDocs(_ *cobra.Command, _ []string) {
 	}
 
 	log.Infof("Found Chart directories [%s]", strings.Join(chartDirs, ", "))
+	dryRun := viper.GetBool("dry-run")
 	waitGroup := sync.WaitGroup{}
 
 	for _, c := range chartDirs {
 		waitGroup.Add(1)
-		go retrieveInfoAndPrintDocumentation(c, &waitGroup)
+
+		// On dry runs all output goes to stdout, and so as to not jumble things, generate serially
+		if dryRun {
+			retrieveInfoAndPrintDocumentation(c, &waitGroup, dryRun)
+		} else {
+			go retrieveInfoAndPrintDocumentation(c, &waitGroup, dryRun)
+		}
 	}
 
 	waitGroup.Wait()

example-charts/custom-template/README.md

@@ -23,15 +23,15 @@ culpa qui officia deserunt mollit anim id est laborum.
 
 | Key | Type | Default | Description |
 |-----|------|---------|-------------|
-| controller.extraVolumes[0].configMap.name | string | "nginx-ingress-config" | Uses the name of the configmap created by this chart |
-| controller.extraVolumes[0].name | string | "config-volume" |  |
-| controller.image.repository | string | "nginx-ingress-controller" |  |
-| controller.image.tag | string | "18.0831" |  |
-| controller.ingressClass | string | "nginx" | Name of the ingress class to route through this controller |
-| controller.name | string | "controller" |  |
-| controller.persistentVolumeClaims | list | [] | List of persistent volume claims to create |
-| controller.podLabels | object | {} | The labels to be applied to instances of the controller pod |
-| controller.publishService.enabled | bool | false | Whether to expose the ingress controller to the public world |
-| controller.replicas | int | \<nil\> | Number of nginx-ingress pods to load balance between |
-| controller.service.annotations."external-dns.alpha.kubernetes.io/hostname" | string | "stupidchess.jmn23.com" | Hostname to be assigned to the ELB for the service |
-| controller.service.type | string | "LoadBalancer" |  |
+| controller.extraVolumes[0].configMap.name | string | `"nginx-ingress-config"` | Uses the name of the configmap created by this chart |
+| controller.extraVolumes[0].name | string | `"config-volume"` |  |
+| controller.image.repository | string | `"nginx-ingress-controller"` |  |
+| controller.image.tag | string | `"18.0831"` |  |
+| controller.ingressClass | string | `"nginx"` | Name of the ingress class to route through this controller |
+| controller.name | string | `"controller"` |  |
+| controller.persistentVolumeClaims | list | `[]` | List of persistent volume claims to create |
+| controller.podLabels | object | `{}` | The labels to be applied to instances of the controller pod |
+| controller.publishService.enabled | bool | `false` | Whether to expose the ingress controller to the public world |
+| controller.replicas | int | `nil` | Number of nginx-ingress pods to load balance between |
+| controller.service.annotations."external-dns.alpha.kubernetes.io/hostname" | string | `"stupidchess.jmn23.com"` | Hostname to be assigned to the ELB for the service |
+| controller.service.type | string | `"LoadBalancer"` |  |

example-charts/nginx-ingress/README.md

@@ -16,15 +16,16 @@ Source code can be found [here](https://github.com/norwoodj/helm-docs/example-ch
 
 | Key | Type | Default | Description |
 |-----|------|---------|-------------|
-| controller.extraVolumes[0].configMap.name | string | "nginx-ingress-config" | Uses the name of the configmap created by this chart |
-| controller.extraVolumes[0].name | string | "config-volume" |  |
-| controller.image.repository | string | "nginx-ingress-controller" |  |
-| controller.image.tag | string | "18.0831" |  |
-| controller.ingressClass | string | "nginx" | Name of the ingress class to route through this controller |
-| controller.name | string | "controller" |  |
-| controller.persistentVolumeClaims | list | [] | List of persistent volume claims to create |
-| controller.podLabels | object | {} | The labels to be applied to instances of the controller pod |
-| controller.publishService.enabled | bool | false | Whether to expose the ingress controller to the public world |
-| controller.replicas | int | \<nil\> | Number of nginx-ingress pods to load balance between |
-| controller.service.annotations."external-dns.alpha.kubernetes.io/hostname" | string | "stupidchess.jmn23.com" | Hostname to be assigned to the ELB for the service |
-| controller.service.type | string | "LoadBalancer" |  |
+| controller.extraVolumes | list | `[{"configMap":{"name":"nginx-ingress-config"},"name":"config-volume"}]` | Additional volumes to be mounted into the ingress controller container |
+| controller.image.repository | string | `"nginx-ingress-controller"` |  |
+| controller.image.tag | string | `"18.0831"` |  |
+| controller.ingressClass | string | `"nginx"` | Name of the ingress class to route through this controller |
+| controller.livenessProbe | object | `{"httpGet":{"path":"/healthz","port":8080}}` | Configure the healthcheck for the ingress controller |
+| controller.livenessProbe.httpGet.path | string | `"/healthz"` | This is the liveness check endpoint |
+| controller.name | string | `"controller"` |  |
+| controller.persistentVolumeClaims | list | `[]` | List of persistent volume claims to create |
+| controller.podLabels | object | `{}` | The labels to be applied to instances of the controller pod |
+| controller.publishService.enabled | bool | `false` | Whether to expose the ingress controller to the public world |
+| controller.replicas | int | `nil` | Number of nginx-ingress pods to load balance between |
+| controller.service.annotations."external-dns.alpha.kubernetes.io/hostname" | string | `"stupidchess.jmn23.com"` | Hostname to be assigned to the ELB for the service |
+| controller.service.type | string | `"LoadBalancer"` |  |

example-charts/nginx-ingress/values.yaml

@@ -7,12 +7,19 @@ controller:
   # controller.persistentVolumeClaims -- List of persistent volume claims to create
   persistentVolumeClaims: []
 
+  # controller.extraVolumes -- Additional volumes to be mounted into the ingress controller container
   extraVolumes:
     - name: config-volume
       configMap:
-        # controller.extraVolumes[0].configMap.name -- Uses the name of the configmap created by this chart
         name: nginx-ingress-config
 
+  # controller.livenessProbe -- Configure the healthcheck for the ingress controller
+  livenessProbe:
+    httpGet:
+      # controller.livenessProbe.httpGet.path -- This is the liveness check endpoint
+      path: /healthz
+      port: 8080
+
   # controller.ingressClass -- Name of the ingress class to route through this controller
   ingressClass: nginx
 

example-charts/no-requirements/README.md

@@ -12,7 +12,7 @@ Source code can be found [here](https://github.com/norwoodj/helm-docs/example-ch
 
 | Key | Type | Default | Description |
 |-----|------|---------|-------------|
-| rules.latency.percentiles.99.duration | string | "5m" | Duration for which the 99th percentile must be above the threshold to alert |
-| rules.latency.percentiles.99.threshold | float | 1.5 | Threshold in seconds for our 99th percentile latency above which the alert will fire |
-| rules.statusCodes.codes.5xx.duration | string | "5m" | Duration for which the percent of 5xx responses must be above the threshold to alert |
-| rules.statusCodes.codes.5xx.threshold | float | 1.5 | Threshold percentage of 5xx responses above which the alert will fire |
+| rules.latency.percentiles.99.duration | string | `"5m"` | Duration for which the 99th percentile must be above the threshold to alert |
+| rules.latency.percentiles.99.threshold | float | `1.5` | Threshold in seconds for our 99th percentile latency above which the alert will fire |
+| rules.statusCodes.codes.5xx.duration | string | `"5m"` | Duration for which the percent of 5xx responses must be above the threshold to alert |
+| rules.statusCodes.codes.5xx.threshold | float | `1.5` | Threshold percentage of 5xx responses above which the alert will fire |

go.mod

@@ -11,5 +11,6 @@ require (
 	github.com/spf13/cobra v0.0.5
 	github.com/spf13/pflag v1.0.3
 	github.com/spf13/viper v1.4.0
+	github.com/stretchr/testify v1.2.2
 	gopkg.in/yaml.v2 v2.2.2
 )

pkg/document/generate.go

@@ -26,25 +26,29 @@ func getOutputFile(chartDirectory string, dryRun bool) (*os.File, error) {
 func PrintDocumentation(chartDocumentationInfo helm.ChartDocumentationInfo, dryRun bool) {
 	log.Infof("Generating README Documentation for chart %s", chartDocumentationInfo.ChartDirectory)
 
-	outputFile, err := getOutputFile(chartDocumentationInfo.ChartDirectory, dryRun)
+	chartDocumentationTemplate, err := newChartDocumentationTemplate(chartDocumentationInfo)
 	if err != nil {
-		log.Warnf("Could not open chart README file %s, skipping chart", filepath.Join(chartDocumentationInfo.ChartDirectory, "README.md"))
+		log.Warnf("Error generating gotemplates for chart %s: %s", chartDocumentationInfo.ChartDirectory, err)
 		return
 	}
 
-	if !dryRun {
-		defer outputFile.Close()
+	chartTemplateDataObject, err := getChartTemplateData(chartDocumentationInfo)
+	if err != nil {
+		log.Warnf("Error generating template data for chart %s: %s", chartDocumentationInfo.ChartDirectory, err)
+		return
 	}
 
-	chartDocumentationTemplate, err := newChartDocumentationTemplate(chartDocumentationInfo)
+	outputFile, err := getOutputFile(chartDocumentationInfo.ChartDirectory, dryRun)
 	if err != nil {
-		log.Warnf("Error generating templates for chart %s: %s", chartDocumentationInfo.ChartDirectory, err)
+		log.Warnf("Could not open chart README file %s, skipping chart", filepath.Join(chartDocumentationInfo.ChartDirectory, "README.md"))
 		return
 	}
 
-	chartTemplateDataObject := getChartTemplateData(chartDocumentationInfo)
-	err = chartDocumentationTemplate.Execute(outputFile, chartTemplateDataObject)
+	if !dryRun {
+		defer outputFile.Close()
+	}
 
+	err = chartDocumentationTemplate.Execute(outputFile, chartTemplateDataObject)
 	if err != nil {
 		log.Warnf("Error generating documentation for chart %s: %s", chartDocumentationInfo.ChartDirectory, err)
 	}

pkg/document/model.go

@@ -16,11 +16,20 @@ type chartTemplateData struct {
 	Values []valueRow
 }
 
-func getChartTemplateData(chartDocumentationInfo helm.ChartDocumentationInfo) chartTemplateData {
-	valuesTableRows := createValueRows("", chartDocumentationInfo.ChartValues, chartDocumentationInfo.ChartValuesDescriptions)
+func getChartTemplateData(chartDocumentationInfo helm.ChartDocumentationInfo) (chartTemplateData, error) {
+	valuesTableRows, err := createValueRowsFromObject(
+		"",
+		chartDocumentationInfo.ChartValues,
+		chartDocumentationInfo.ChartValuesDescriptions,
+		true,
+	)
+
+	if err != nil {
+		return chartTemplateData{}, err
+	}
 
 	return chartTemplateData{
 		ChartDocumentationInfo: chartDocumentationInfo,
 		Values:                 valuesTableRows,
-	}
+	}, nil
 }

pkg/document/util.go

@@ -0,0 +1,49 @@
+package document
+
+import (
+	"fmt"
+)
+
+type jsonableMap map[string]interface{}
+
+func convertMapKeyToString(key interface{}) string {
+	switch key.(type) {
+	case string:
+		return key.(string)
+	case int:
+		return fmt.Sprintf("int(%d)", key)
+	case float64:
+		return fmt.Sprintf("float(%f)", key)
+	case bool:
+		return fmt.Sprintf("bool(%t)", key)
+	}
+
+	return fmt.Sprintf("?(%+v)", key)
+}
+
+// The json library can only marshal maps with string keys, and so all of our lists and maps that go into documentation
+// must be converted to have only string keys before marshalling
+func convertHelmValuesToJsonable(values interface{}) interface{} {
+	switch values.(type) {
+	case map[interface{}]interface{}:
+		convertedMap := make(jsonableMap)
+
+		for key, value := range values.(map[interface{}]interface{}) {
+			convertedMap[convertMapKeyToString(key)] = convertHelmValuesToJsonable(value)
+		}
+
+		return convertedMap
+
+	case []interface{}:
+		convertedList := make([]interface{}, 0)
+
+		for _, value := range values.([]interface{}) {
+			convertedList = append(convertedList, convertHelmValuesToJsonable(value))
+		}
+
+		return convertedList
+
+	default:
+		return values
+	}
+}