Repository: grafana/tanka
Pull request #851 (2023-04-27 17:30:21)

helm: Set `includeCrds` to true by default
This is what `helm install` does. With Helm 3, we aren't installing CRDs by default any more and we expect this to be what most Tanka + Helm users want.

Closes: #829

github-actions[bot]: [PR Preview Action](https://github.com/rossjrw/pr-preview-action) bf7f7d51346171c6ac2e9c0fe8e2e2b6c48afc7a
:---:
:rocket: Deployed preview to https://grafana.github.io/tanka/pr-preview/pr-851/
on branch [`gh-pages`](https://github.com/grafana/tanka/tree/gh-pages) at 2023-04-27 17:34 UTC
<!-- Sticky Pull Request Commentpr-preview -->

Patch:
diff --git a/docs/docs/helm.mdx b/docs/docs/helm.mdx
index 20546564..1881de3a 100644
--- a/docs/docs/helm.mdx
+++ b/docs/docs/helm.mdx
@@ -79,8 +79,6 @@ The following options control how the command is invoked:
     values: {
       persistence: { enabled: true }
     },
-    // Equivalent to: --include-crds; only relevant for Helm v3+.
-    includeCrds: true,
     // Equivalent to: --api-versions v1 --api-versions apps/v1
     apiVersions: ['v1', 'apps/v1']
     // Equivalent to: --kube-version v1.20.0
@@ -90,6 +88,17 @@ The following options control how the command is invoked:
 }
 ```
 
+Tanka will install Custom Resource Definitions (CRDs) automatically, if the
+Helm Chart requires them and ships them in `crds/`. This is equivalent to `helm
+template --include-crds`. This can be disabled using `includeCrds: false`:
+
+```jsonnet
+{
+  grafana: helm.template("grafana", "./charts/grafana", {
+    includeCrds: false
+  })
+}
+```
 
 ## Vendoring Helm Charts
 
diff --git a/pkg/helm/helm.go b/pkg/helm/helm.go
index b68f8a38..edaa7631 100644
--- a/pkg/helm/helm.go
+++ b/pkg/helm/helm.go
@@ -21,6 +21,9 @@ type Helm interface {
 
 	// Template returns the individual resources of a Helm Chart
 	Template(name, chart string, opts TemplateOpts) (manifest.List, error)
+
+	// ChartExists checks if a chart exists in the provided calledFromPath
+	ChartExists(chart string, opts *JsonnetOpts) (string, error)
 }
 
 // PullOpts are additional, non-required options for Helm.Pull
@@ -103,6 +106,17 @@ func (e ExecHelm) RepoUpdate(opts Opts) error {
 	return nil
 }
 
+func (e ExecHelm) ChartExists(chart string, opts *JsonnetOpts) (string, error) {
+	// resolve the Chart relative to the caller
+	callerDir := filepath.Dir(opts.CalledFrom)
+	chart = filepath.Join(callerDir, chart)
+	if _, err := os.Stat(chart); err != nil {
+		return "", fmt.Errorf("helmTemplate: Failed to find a chart at '%s': %s. See https://tanka.dev/helm#failed-to-find-chart", chart, err)
+	}
+
+	return chart, nil
+}
+
 // cmd returns a prepared exec.Cmd to use the `helm` binary
 func (e ExecHelm) cmd(action string, args ...string) *exec.Cmd {
 	argv := []string{action}
diff --git a/pkg/helm/jsonnet.go b/pkg/helm/jsonnet.go
index e0d7111e..f7cfe156 100644
--- a/pkg/helm/jsonnet.go
+++ b/pkg/helm/jsonnet.go
@@ -3,8 +3,6 @@ package helm
 import (
 	"encoding/json"
 	"fmt"
-	"os"
-	"path/filepath"
 
 	"github.com/google/go-jsonnet"
 	"github.com/google/go-jsonnet/ast"
@@ -54,10 +52,8 @@ func NativeFunc(h Helm) *jsonnet.NativeFunction {
 				return "", err
 			}
 
-			// resolve the Chart relative to the caller
-			callerDir := filepath.Dir(opts.CalledFrom)
-			chart := filepath.Join(callerDir, chartpath)
-			if _, err := os.Stat(chart); err != nil {
+			chart, err := h.ChartExists(chartpath, opts)
+			if err != nil {
 				return nil, fmt.Errorf("helmTemplate: Failed to find a chart at '%s': %s. See https://tanka.dev/helm#failed-to-find-chart", chart, err)
 			}
 
@@ -83,7 +79,16 @@ func parseOpts(data interface{}) (*JsonnetOpts, error) {
 	if err != nil {
 		return nil, err
 	}
-	var opts JsonnetOpts
+
+	// default IncludeCRDs to true, as this is the default in the `helm install`
+	// command. Needs to be specified here because the zero value of bool is
+	// false.
+	opts := JsonnetOpts{
+		TemplateOpts: TemplateOpts{
+			IncludeCRDs: true,
+		},
+	}
+
 	if err := json.Unmarshal(c, &opts); err != nil {
 		return nil, err
 	}
diff --git a/pkg/helm/template.go b/pkg/helm/template.go
index 7aa70439..f395da0e 100644
--- a/pkg/helm/template.go
+++ b/pkg/helm/template.go
@@ -10,13 +10,18 @@ import (
 	yaml "gopkg.in/yaml.v3"
 )
 
-// Template expands a Helm Chart into a regular manifest.List using the `helm
-// template` command
-func (e ExecHelm) Template(name, chart string, opts TemplateOpts) (manifest.List, error) {
+func (e ExecHelm) templateCommandArgs(name, chart string, opts TemplateOpts) []string {
 	args := []string{name, chart,
 		"--values", "-", // values from stdin
 	}
 	args = append(args, opts.Flags()...)
+	return args
+}
+
+// Template expands a Helm Chart into a regular manifest.List using the `helm
+// template` command
+func (e ExecHelm) Template(name, chart string, opts TemplateOpts) (manifest.List, error) {
+	args := e.templateCommandArgs(name, chart, opts)
 
 	cmd := e.cmd("template", args...)
 	var buf bytes.Buffer


Test patch:
diff --git a/pkg/helm/jsonnet_test.go b/pkg/helm/jsonnet_test.go
new file mode 100644
index 00000000..7607e836
--- /dev/null
+++ b/pkg/helm/jsonnet_test.go
@@ -0,0 +1,142 @@
+package helm
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/tanka/pkg/kubernetes/manifest"
+)
+
+const calledFrom = "/my/path/here"
+
+type MockHelm struct {
+	mock.Mock
+}
+
+// fulfill the Helm interface
+func (m *MockHelm) Pull(chart, version string, opts PullOpts) error {
+	args := m.Called(chart, version, opts)
+	return args.Error(0)
+}
+
+func (m *MockHelm) RepoUpdate(opts Opts) error {
+	args := m.Called(opts)
+	return args.Error(0)
+}
+
+func (m *MockHelm) Template(name, chart string, opts TemplateOpts) (manifest.List, error) {
+	args := m.Called(name, chart, opts)
+
+	// figure out what arguments `helm template` would be called with and save
+	// them
+	execHelm := &ExecHelm{}
+	cmdArgs := execHelm.templateCommandArgs(name, chart, opts)
+	m.TestData().Set("templateCommandArgs", cmdArgs)
+
+	return args.Get(0).(manifest.List), args.Error(1)
+}
+
+func (m *MockHelm) ChartExists(chart string, opts *JsonnetOpts) (string, error) {
+	args := m.Called(chart, opts)
+	return args.String(0), args.Error(1)
+}
+
+func callNativeFunction(t *testing.T, expectedHelmTemplateOptions TemplateOpts, inputOptionsFromJsonnet map[string]interface{}) []string {
+	t.Helper()
+
+	helmMock := &MockHelm{}
+
+	helmMock.On(
+		"ChartExists",
+		"exampleChartPath",
+		mock.AnythingOfType("*helm.JsonnetOpts")).
+		Return("/full/chart/path", nil).
+		Once()
+
+	// this verifies that the helmMock.Template() method is called with the
+	// correct arguments, i.e. includeCrds: true is set by default
+	helmMock.On("Template", "exampleChartName", "/full/chart/path", expectedHelmTemplateOptions).
+		Return(manifest.List{}, nil).
+		Once()
+
+	nf := NativeFunc(helmMock)
+	require.NotNil(t, nf)
+
+	// the mandatory parameters to helm.template() in Jsonnet
+	params := []string{
+		"exampleChartName",
+		"exampleChartPath",
+	}
+
+	// mandatory parameters + the k-v pairs from the Jsonnet input
+	paramsInterface := make([]interface{}, 3)
+	paramsInterface[0] = params[0]
+	paramsInterface[1] = params[1]
+	paramsInterface[2] = inputOptionsFromJsonnet
+
+	_, err := nf.Func(paramsInterface)
+
+	require.NoError(t, err)
+
+	helmMock.AssertExpectations(t)
+
+	return helmMock.TestData().Get("templateCommandArgs").StringSlice()
+}
+
+// TestDefaultCommandineFlagsIncludeCrds tests that the includeCrds flag is set
+// to true by default
+func TestDefaultCommandLineFlagsIncludeCrds(t *testing.T) {
+	kubeVersion := "1.18.0"
+
+	// we will check that the template function is called with these options,
+	// i.e. that includeCrds got set to true. This is not us passing an input,
+	// we are asserting here that the template function is called with these
+	// options.
+	expectedHelmTemplateOptions := TemplateOpts{
+		KubeVersion: kubeVersion,
+		IncludeCRDs: true,
+	}
+
+	// the options to helm.template(), which are a Jsonnet object, turned into a
+	// go map[string]interface{}. we do not set includeCrds here, so it should
+	// be true by default
+	inputOptionsFromJsonnet := make(map[string]interface{})
+	inputOptionsFromJsonnet["calledFrom"] = calledFrom
+	inputOptionsFromJsonnet["kubeVersion"] = kubeVersion
+
+	args := callNativeFunction(t, expectedHelmTemplateOptions, inputOptionsFromJsonnet)
+
+	// finally check that the actual command line arguments we will pass to
+	// `helm template` contain the --include-crds flag
+	require.Contains(t, args, "--include-crds")
+}
+
+// TestIncludeCrdsFalse tests that the includeCrds flag is can be set to false,
+// and this makes it to the helm.Template() method call
+func TestIncludeCrdsFalse(t *testing.T) {
+	kubeVersion := "1.18.0"
+
+	// we will check that the template function is called with these options,
+	// i.e. that includeCrds got set to false. This is not us passing an input,
+	// we are asserting here that the template function is called with these
+	// options.
+	expectedHelmTemplateOptions := TemplateOpts{
+		KubeVersion: kubeVersion,
+		IncludeCRDs: false,
+	}
+
+	// the options to helm.template(), which are a Jsonnet object, turned into a
+	// go map[string]interface{}. we explicitly set includeCrds to false here
+	inputOptionsFromJsonnet := make(map[string]interface{})
+	inputOptionsFromJsonnet["calledFrom"] = calledFrom
+	inputOptionsFromJsonnet["kubeVersion"] = kubeVersion
+	inputOptionsFromJsonnet["includeCrds"] = false
+
+	args := callNativeFunction(t, expectedHelmTemplateOptions, inputOptionsFromJsonnet)
+
+	// finally check that the actual command line arguments we will pass to
+	// `helm template` don't contain the --include-crds flag
+	require.NotContains(t, args, "--include-crds")
+}
diff --git a/pkg/jsonnet/find_importers_test.go b/pkg/jsonnet/find_importers_test.go
index d295f821..58080276 100644
--- a/pkg/jsonnet/find_importers_test.go
+++ b/pkg/jsonnet/find_importers_test.go
@@ -8,24 +8,28 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestFindImportersForFiles(t *testing.T) {
-	// Make sure the main files all eval correctly
-	// We want to make sure that the importers command works correctly,
-	// but there's no point in testing on invalid jsonnet files
-	files, err := FindFiles("testdata", nil)
-	require.NoError(t, err)
-	require.NotEmpty(t, files)
-	for _, file := range files {
-		_, err := EvaluateFile(file, Opts{})
-		require.NoError(t, err, "failed to eval %s", file)
+type findImportersTestCase struct {
+	name              string
+	files             []string
+	expectedImporters []string
+	expectedErr       error
+}
+
+func (tc findImportersTestCase) run(t testing.TB) {
+	importers, err := FindImporterForFiles("testdata/findImporters", tc.files)
+
+	if tc.expectedErr != nil {
+		require.EqualError(t, err, tc.expectedErr.Error())
+	} else {
+		require.NoError(t, err)
+		require.Equal(t, tc.expectedImporters, importers)
 	}
+}
 
-	cases := []struct {
-		name              string
-		files             []string
-		expectedImporters []string
-		expectedErr       error
-	}{
+func findImportersTestCases(t testing.TB) []findImportersTestCase {
+	t.Helper()
+
+	return []findImportersTestCase{
 		{
 			name:  "no files",
 			files: []string{},
@@ -115,17 +119,23 @@ func TestFindImportersForFiles(t *testing.T) {
 			},
 		},
 	}
+}
 
-	for _, c := range cases {
-		t.Run(c.name, func(t *testing.T) {
-			importers, err := FindImporterForFiles("testdata/findImporters", c.files)
+func TestFindImportersForFiles(t *testing.T) {
+	// Make sure the main files all eval correctly
+	// We want to make sure that the importers command works correctly,
+	// but there's no point in testing on invalid jsonnet files
+	files, err := FindFiles("testdata", nil)
+	require.NoError(t, err)
+	require.NotEmpty(t, files)
+	for _, file := range files {
+		_, err := EvaluateFile(file, Opts{})
+		require.NoError(t, err, "failed to eval %s", file)
+	}
 
-			if c.expectedErr != nil {
-				require.EqualError(t, err, c.expectedErr.Error())
-			} else {
-				require.NoError(t, err)
-				require.Equal(t, c.expectedImporters, importers)
-			}
+	for _, c := range findImportersTestCases(t) {
+		t.Run(c.name, func(t *testing.T) {
+			c.run(t)
 		})
 	}
 }
@@ -149,7 +159,17 @@ func BenchmarkFindImporters(b *testing.B) {
 	}
 }
 
-func absPath(t *testing.T, path string) string {
+func BenchmarkFindImporters_StaticCases(b *testing.B) {
+	for _, c := range findImportersTestCases(b) {
+		b.Run(c.name, func(b *testing.B) {
+			for i := 0; i < b.N; i++ {
+				c.run(b)
+			}
+		})
+	}
+}
+
+func absPath(t testing.TB, path string) string {
 	t.Helper()
 
 	abs, err := filepath.Abs(path)