From f9d8645c173af224742eac3c21153504d91ef65f Mon Sep 17 00:00:00 2001 From: Matheus Hunsche Date: Fri, 2 Oct 2020 01:58:48 -0300 Subject: [PATCH 01/12] add include file Signed-off-by: Matheus Hunsche --- cmd/helm/flags.go | 6 ++ cmd/helm/helm.go | 5 ++ cmd/helm/install.go | 51 +++++++++++ cmd/helm/install_test.go | 18 ++++ cmd/helm/template_test.go | 15 ++++ cmd/helm/testdata/files/external.1.conf | 1 + cmd/helm/testdata/files/external.2.conf | 1 + cmd/helm/testdata/files/external.txt | 1 + .../output/install-with-external-files.txt | 6 ++ .../output/template-with-external-dir.txt | 22 +++++ .../output/template-with-external-file.txt | 20 +++++ .../output/template-with-external-glob.txt | 21 +++++ .../testdata/testcharts/external/Chart.yaml | 8 ++ .../testdata/testcharts/external/external.txt | 1 + .../external/templates/config-map.yaml | 23 +++++ .../testdata/testcharts/external/values.yaml | 4 + cmd/helm/upgrade.go | 7 ++ cmd/helm/upgrade_test.go | 69 +++++++++++++++ pkg/action/install.go | 2 + pkg/action/upgrade.go | 2 + pkg/chart/loader/local.go | 85 +++++++++++++++++++ pkg/chart/loader/local_test.go | 46 ++++++++++ pkg/cli/files/files.go | 20 +++++ pkg/cli/files/parser.go | 66 ++++++++++++++ pkg/cli/files/parser_test.go | 76 +++++++++++++++++ pkg/cli/files/testdata/foo/bar.txt | 1 + pkg/cli/files/testdata/foo/foo.txt | 1 + 27 files changed, 578 insertions(+) create mode 100644 cmd/helm/testdata/files/external.1.conf create mode 100644 cmd/helm/testdata/files/external.2.conf create mode 100644 cmd/helm/testdata/files/external.txt create mode 100644 cmd/helm/testdata/output/install-with-external-files.txt create mode 100644 cmd/helm/testdata/output/template-with-external-dir.txt create mode 100644 cmd/helm/testdata/output/template-with-external-file.txt create mode 100644 cmd/helm/testdata/output/template-with-external-glob.txt create mode 100644 cmd/helm/testdata/testcharts/external/Chart.yaml create mode 100644 cmd/helm/testdata/testcharts/external/external.txt create mode 100644 cmd/helm/testdata/testcharts/external/templates/config-map.yaml create mode 100644 cmd/helm/testdata/testcharts/external/values.yaml create mode 100644 pkg/chart/loader/local.go create mode 100644 pkg/chart/loader/local_test.go create mode 100644 pkg/cli/files/files.go create mode 100644 pkg/cli/files/parser.go create mode 100644 pkg/cli/files/parser_test.go create mode 100644 pkg/cli/files/testdata/foo/bar.txt create mode 100644 pkg/cli/files/testdata/foo/foo.txt diff --git a/cmd/helm/flags.go b/cmd/helm/flags.go index 76d6e0476..6453e523d 100644 --- a/cmd/helm/flags.go +++ b/cmd/helm/flags.go @@ -29,6 +29,7 @@ import ( "k8s.io/klog/v2" "helm.sh/helm/v3/pkg/action" + "helm.sh/helm/v3/pkg/cli/files" "helm.sh/helm/v3/pkg/cli/output" "helm.sh/helm/v3/pkg/cli/values" "helm.sh/helm/v3/pkg/helmpath" @@ -64,6 +65,11 @@ func addChartPathOptionsFlags(f *pflag.FlagSet, c *action.ChartPathOptions) { f.BoolVar(&c.PassCredentialsAll, "pass-credentials", false, "pass credentials to all domains") } +func addExternalFilesFlags(f *pflag.FlagSet, v *files.ExternalFiles) { + f.StringArrayVar(&v.Files, "include-file", []string{}, "paths to local files to add during chart installation") + f.StringArrayVar(&v.Globs, "include-dir", []string{}, "paths or globs to local dirs to add during chart installation") +} + // bindOutputFlag will add the output flag to the given command and bind the // value to the given format pointer func bindOutputFlag(cmd *cobra.Command, varRef *output.Format) { diff --git a/cmd/helm/helm.go b/cmd/helm/helm.go index 15b0d5c76..a98af62e4 100644 --- a/cmd/helm/helm.go +++ b/cmd/helm/helm.go @@ -80,6 +80,11 @@ func main() { } }) + if err := actionConfig.Init(settings.RESTClientGetter(), settings.Namespace(), os.Getenv("HELM_DRIVER"), debug); err != nil { + debug("%+v", err) + os.Exit(1) + } + if err := cmd.Execute(); err != nil { debug("%+v", err) switch e := err.(type) { diff --git a/cmd/helm/install.go b/cmd/helm/install.go index 044d1f2b5..53da84b86 100644 --- a/cmd/helm/install.go +++ b/cmd/helm/install.go @@ -20,9 +20,11 @@ import ( "context" "fmt" "io" + "io/ioutil" "log" "os" "os/signal" + "strings" "syscall" "time" @@ -34,6 +36,7 @@ import ( "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" + "helm.sh/helm/v3/pkg/cli/files" "helm.sh/helm/v3/pkg/cli/output" "helm.sh/helm/v3/pkg/cli/values" "helm.sh/helm/v3/pkg/downloader" @@ -171,6 +174,7 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal f.BoolVar(&client.SubNotes, "render-subchart-notes", false, "if set, render subchart notes along with the parent") addValueOptionsFlags(f, valueOpts) addChartPathOptionsFlags(f, &client.ChartPathOptions) + addExternalFilesFlags(f, &client.ExternalFiles) err := cmd.RegisterFlagCompletionFunc("version", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { requiredArgs := 2 @@ -258,6 +262,11 @@ func runInstall(args []string, client *action.Install, valueOpts *values.Options } } + err = loadExternalFiles(chartRequested, client.ExternalFiles) + if err != nil { + return nil, err + } + client.Namespace = settings.Namespace() // Create context and prepare the handle of SIGTERM @@ -289,6 +298,48 @@ func checkIfInstallable(ch *chart.Chart) error { return errors.Errorf("%s charts are not installable", ch.Metadata.Type) } +func loadExternalFiles(ch *chart.Chart, exFiles files.ExternalFiles) error { + var errs []string + fs := make(map[string]string) + + for _, s := range exFiles.Files { + if err := files.ParseIntoString(s, fs); err != nil { + debug(fmt.Sprintf("error parsing include-file option %s: %v", s, err)) + errs = append(errs, fmt.Sprintf("%s (parse error)", s)) + } + } + + for _, g := range exFiles.Globs { + if err := files.ParseIntoString(g, fs); err != nil { + debug(fmt.Sprintf("error parsing include-dir option %s: %v", g, err)) + errs = append(errs, fmt.Sprintf("%s (parse error)", g)) + } + } + + for n, p := range fs { + allPaths, err := loader.ExpandLocalPath(n, p) + debug(fmt.Sprintf("%s expanded to: %v", p, allPaths)) + if err != nil { + debug(fmt.Sprintf("error loading external path %s: %v", p, err)) + errs = append(errs, fmt.Sprintf("%s (path not accessible)", p)) + } + + for name, fp := range allPaths { + byt, err := ioutil.ReadFile(fp) + if err != nil { + errs = append(errs, fmt.Sprintf("%s (not readable)", fp)) + } else { + ch.Files = append(ch.Files, &chart.File{Name: name, Data: byt}) + } + } + } + + if len(errs) > 0 { + return errors.New(fmt.Sprint("Failed to load external files: ", strings.Join(errs, "; "))) + } + return nil +} + // Provide dynamic auto-completion for the install and template commands func compInstall(args []string, toComplete string, client *action.Install) ([]string, cobra.ShellCompDirective) { requiredArgs := 1 diff --git a/cmd/helm/install_test.go b/cmd/helm/install_test.go index b34d1455c..a1a97445d 100644 --- a/cmd/helm/install_test.go +++ b/cmd/helm/install_test.go @@ -77,6 +77,24 @@ func TestInstall(t *testing.T) { cmd: "install virgil testdata/testcharts/alpine -f testdata/testcharts/alpine/extra_values.yaml", golden: "output/install-with-values-file.txt", }, + // Install, external file + { + name: "install with external files", + cmd: "install virgil testdata/testcharts/external --include-file external.txt=testdata/files/external.txt", + golden: "output/install-with-external-files.txt", + }, + // Install, external dir + { + name: "install with external dir", + cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-dir glob=testdata/files/", + golden: "output/install-with-external-files.txt", + }, + // Install, external glob files + { + name: "install with external globbed files", + cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-dir glob=testdata/files/external.*.conf", + golden: "output/install-with-external-files.txt", + }, // Install, no hooks { name: "install without hooks", diff --git a/cmd/helm/template_test.go b/cmd/helm/template_test.go index d1f17fe98..91a68acec 100644 --- a/cmd/helm/template_test.go +++ b/cmd/helm/template_test.go @@ -131,6 +131,21 @@ func TestTemplateCmd(t *testing.T) { cmd: fmt.Sprintf(`template '%s' --skip-tests`, chartPath), golden: "output/template-skip-tests.txt", }, + { + name: "chart with template with external file", + cmd: fmt.Sprintf("template '%s' --set external=external.txt --include-file external.txt=testdata/files/external.txt", "testdata/testcharts/external"), + golden: "output/template-with-external-file.txt", + }, + { + name: "chart with template with external dir", + cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-dir glob=testdata/files/", "testdata/testcharts/external"), + golden: "output/template-with-external-dir.txt", + }, + { + name: "chart with template with external globbed files", + cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-dir glob=testdata/files/external.*.conf", "testdata/testcharts/external"), + golden: "output/template-with-external-glob.txt", + }, } runTestCmd(t, tests) } diff --git a/cmd/helm/testdata/files/external.1.conf b/cmd/helm/testdata/files/external.1.conf new file mode 100644 index 000000000..065cf1bd9 --- /dev/null +++ b/cmd/helm/testdata/files/external.1.conf @@ -0,0 +1 @@ +glob-external-1 diff --git a/cmd/helm/testdata/files/external.2.conf b/cmd/helm/testdata/files/external.2.conf new file mode 100644 index 000000000..92c587e84 --- /dev/null +++ b/cmd/helm/testdata/files/external.2.conf @@ -0,0 +1 @@ +glob-external-2 diff --git a/cmd/helm/testdata/files/external.txt b/cmd/helm/testdata/files/external.txt new file mode 100644 index 000000000..068c4db58 --- /dev/null +++ b/cmd/helm/testdata/files/external.txt @@ -0,0 +1 @@ +out-of-chart-dir diff --git a/cmd/helm/testdata/output/install-with-external-files.txt b/cmd/helm/testdata/output/install-with-external-files.txt new file mode 100644 index 000000000..406e522a9 --- /dev/null +++ b/cmd/helm/testdata/output/install-with-external-files.txt @@ -0,0 +1,6 @@ +NAME: virgil +LAST DEPLOYED: Fri Sep 2 22:04:05 1977 +NAMESPACE: default +STATUS: deployed +REVISION: 1 +TEST SUITE: None diff --git a/cmd/helm/testdata/output/template-with-external-dir.txt b/cmd/helm/testdata/output/template-with-external-dir.txt new file mode 100644 index 000000000..34645fde8 --- /dev/null +++ b/cmd/helm/testdata/output/template-with-external-dir.txt @@ -0,0 +1,22 @@ +--- +# Source: configmap/templates/config-map.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: "RELEASE-NAME-" + labels: + # The "app.kubernetes.io/managed-by" label is used to track which tool + # deployed a given chart. It is useful for admins who want to see what + # releases a particular tool is responsible for. + app.kubernetes.io/managed-by: "Helm" + # The "app.kubernetes.io/instance" convention makes it easy to tie a release + # to all of the Kubernetes resources that were created as part of that + # release. + app.kubernetes.io/instance: "RELEASE-NAME" + app.kubernetes.io/version: 1.0 + # This makes it easy to audit chart usage. + helm.sh/chart: "configmap-0.1.0" +data: + external.1.conf: glob-external-1 + external.2.conf: glob-external-2 + external.txt: out-of-chart-dir diff --git a/cmd/helm/testdata/output/template-with-external-file.txt b/cmd/helm/testdata/output/template-with-external-file.txt new file mode 100644 index 000000000..9550768c0 --- /dev/null +++ b/cmd/helm/testdata/output/template-with-external-file.txt @@ -0,0 +1,20 @@ +--- +# Source: configmap/templates/config-map.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: "RELEASE-NAME-" + labels: + # The "app.kubernetes.io/managed-by" label is used to track which tool + # deployed a given chart. It is useful for admins who want to see what + # releases a particular tool is responsible for. + app.kubernetes.io/managed-by: "Helm" + # The "app.kubernetes.io/instance" convention makes it easy to tie a release + # to all of the Kubernetes resources that were created as part of that + # release. + app.kubernetes.io/instance: "RELEASE-NAME" + app.kubernetes.io/version: 1.0 + # This makes it easy to audit chart usage. + helm.sh/chart: "configmap-0.1.0" +data: + external.txt: out-of-chart-dir diff --git a/cmd/helm/testdata/output/template-with-external-glob.txt b/cmd/helm/testdata/output/template-with-external-glob.txt new file mode 100644 index 000000000..7796e1c7e --- /dev/null +++ b/cmd/helm/testdata/output/template-with-external-glob.txt @@ -0,0 +1,21 @@ +--- +# Source: configmap/templates/config-map.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: "RELEASE-NAME-" + labels: + # The "app.kubernetes.io/managed-by" label is used to track which tool + # deployed a given chart. It is useful for admins who want to see what + # releases a particular tool is responsible for. + app.kubernetes.io/managed-by: "Helm" + # The "app.kubernetes.io/instance" convention makes it easy to tie a release + # to all of the Kubernetes resources that were created as part of that + # release. + app.kubernetes.io/instance: "RELEASE-NAME" + app.kubernetes.io/version: 1.0 + # This makes it easy to audit chart usage. + helm.sh/chart: "configmap-0.1.0" +data: + external.1.conf: glob-external-1 + external.2.conf: glob-external-2 diff --git a/cmd/helm/testdata/testcharts/external/Chart.yaml b/cmd/helm/testdata/testcharts/external/Chart.yaml new file mode 100644 index 000000000..0b070f43e --- /dev/null +++ b/cmd/helm/testdata/testcharts/external/Chart.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +appVersion: "1.0" +description: Deploy a basic Config Map from an external file +home: https://helm.sh/helm +name: configmap +sources: +- https://github.com/helm/helm +version: 0.1.0 diff --git a/cmd/helm/testdata/testcharts/external/external.txt b/cmd/helm/testdata/testcharts/external/external.txt new file mode 100644 index 000000000..d8e8e4de4 --- /dev/null +++ b/cmd/helm/testdata/testcharts/external/external.txt @@ -0,0 +1 @@ +in-chart diff --git a/cmd/helm/testdata/testcharts/external/templates/config-map.yaml b/cmd/helm/testdata/testcharts/external/templates/config-map.yaml new file mode 100644 index 000000000..f644c307e --- /dev/null +++ b/cmd/helm/testdata/testcharts/external/templates/config-map.yaml @@ -0,0 +1,23 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: "{{.Release.Name}}-{{.Values.Name}}" + labels: + # The "app.kubernetes.io/managed-by" label is used to track which tool + # deployed a given chart. It is useful for admins who want to see what + # releases a particular tool is responsible for. + app.kubernetes.io/managed-by: {{.Release.Service | quote }} + # The "app.kubernetes.io/instance" convention makes it easy to tie a release + # to all of the Kubernetes resources that were created as part of that + # release. + app.kubernetes.io/instance: {{.Release.Name | quote }} + app.kubernetes.io/version: {{ .Chart.AppVersion }} + # This makes it easy to audit chart usage. + helm.sh/chart: "{{.Chart.Name}}-{{.Chart.Version}}" +data: +{{- if .Values.external }} +{{ (.Files.Glob .Values.external).AsConfig | indent 2 }} +{{- end }} +{{- if .Values.glob.enabled }} +{{ (.Files.Glob .Values.glob.path).AsConfig | indent 2 }} +{{- end }} diff --git a/cmd/helm/testdata/testcharts/external/values.yaml b/cmd/helm/testdata/testcharts/external/values.yaml new file mode 100644 index 000000000..ee4f44aed --- /dev/null +++ b/cmd/helm/testdata/testcharts/external/values.yaml @@ -0,0 +1,4 @@ +external: false +glob: + enabled: false + path: "glob/*" diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index 2ceb31a81..45a35c679 100644 --- a/cmd/helm/upgrade.go +++ b/cmd/helm/upgrade.go @@ -116,6 +116,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { instClient.PostRenderer = client.PostRenderer instClient.DisableOpenAPIValidation = client.DisableOpenAPIValidation instClient.SubNotes = client.SubNotes + instClient.ExternalFiles = client.ExternalFiles instClient.Description = client.Description instClient.DependencyUpdate = client.DependencyUpdate @@ -181,6 +182,11 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { warning("This chart is deprecated") } + err = loadExternalFiles(ch, client.ExternalFiles) + if err != nil { + return err + } + // Create context and prepare the handle of SIGTERM ctx := context.Background() ctx, cancel := context.WithCancel(ctx) @@ -235,6 +241,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { addValueOptionsFlags(f, valueOpts) bindOutputFlag(cmd, &outfmt) bindPostRenderFlag(cmd, &client.PostRenderer) + addExternalFilesFlags(f, &client.ExternalFiles) err := cmd.RegisterFlagCompletionFunc("version", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { if len(args) != 2 { diff --git a/cmd/helm/upgrade_test.go b/cmd/helm/upgrade_test.go index 8afcb139b..26a459a79 100644 --- a/cmd/helm/upgrade_test.go +++ b/cmd/helm/upgrade_test.go @@ -357,6 +357,38 @@ func TestUpgradeInstallWithValuesFromStdin(t *testing.T) { } +func TestUpgradeWithExternalFile(t *testing.T) { + + releaseName := "funny-bunny-v7" + + exFiles := []*chart.File{ + {Name: "external.txt", Data: []byte("from-external-file")}, + } + + relMock, ch, chartPath := prepareMockReleaseWithExternal(releaseName, exFiles, t) + + defer resetEnv()() + + store := storageFixture() + + store.Create(relMock(releaseName, 3, ch)) + + cmd := fmt.Sprintf("upgrade %s --set glob.enabled=false --set external=external.txt '%s'", releaseName, chartPath) + _, _, err := executeActionCommandC(store, cmd) + if err != nil { + t.Errorf("unexpected error, got '%v'", err) + } + + updatedRel, err := store.Get(releaseName, 4) + if err != nil { + t.Errorf("unexpected error, got '%v'", err) + } + + if !strings.Contains(updatedRel.Manifest, "from-external-file") { + t.Errorf("The value is not set correctly. manifest: %s", updatedRel.Manifest) + } +} + func prepareMockRelease(releaseName string, t *testing.T) (func(n string, v int, ch *chart.Chart) *release.Release, *chart.Chart, string) { tmpChart := ensure.TempDir(t) configmapData, err := ioutil.ReadFile("testdata/testcharts/upgradetest/templates/configmap.yaml") @@ -392,6 +424,43 @@ func prepareMockRelease(releaseName string, t *testing.T) (func(n string, v int, return relMock, ch, chartPath } +func prepareMockReleaseWithExternal(releaseName string, exFiles []*chart.File, t *testing.T) (func(n string, v int, ch *chart.Chart) *release.Release, *chart.Chart, string) { + tmpChart := ensure.TempDir(t) + configmapData, err := ioutil.ReadFile("testdata/testcharts/external/templates/config-map.yaml") + + if err != nil { + t.Fatalf("Error loading template yaml %v", err) + } + cfile := &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: chart.APIVersionV1, + Name: "testUpgradeChart", + Description: "A Helm chart for Kubernetes", + Version: "0.1.0", + }, + Templates: []*chart.File{{Name: "templates/configmap.yaml", Data: configmapData}}, + Files: exFiles, + } + chartPath := filepath.Join(tmpChart, cfile.Metadata.Name) + if err := chartutil.SaveDir(cfile, tmpChart); err != nil { + t.Fatalf("Error creating chart for upgrade: %v", err) + } + ch, err := loader.Load(chartPath) + if err != nil { + t.Fatalf("Error loading chart: %v", err) + } + _ = release.Mock(&release.MockReleaseOptions{ + Name: releaseName, + Chart: ch, + }) + + relMock := func(n string, v int, ch *chart.Chart) *release.Release { + return release.Mock(&release.MockReleaseOptions{Name: n, Version: v, Chart: ch}) + } + + return relMock, ch, chartPath +} + func TestUpgradeOutputCompletion(t *testing.T) { outputFlagCompletionTest(t, "upgrade") } diff --git a/pkg/action/install.go b/pkg/action/install.go index fa5508234..92e14ce9e 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -41,6 +41,7 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/cli" + "helm.sh/helm/v3/pkg/cli/files" "helm.sh/helm/v3/pkg/downloader" "helm.sh/helm/v3/pkg/getter" "helm.sh/helm/v3/pkg/kube" @@ -100,6 +101,7 @@ type Install struct { // OutputDir/ UseReleaseName bool PostRenderer postrender.PostRenderer + ExternalFiles files.ExternalFiles // Lock to control raceconditions when the process receives a SIGTERM Lock sync.Mutex } diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 690397d4a..e2befe2a1 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -30,6 +30,7 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" + "helm.sh/helm/v3/pkg/cli/files" "helm.sh/helm/v3/pkg/kube" "helm.sh/helm/v3/pkg/postrender" "helm.sh/helm/v3/pkg/release" @@ -101,6 +102,7 @@ type Upgrade struct { DisableOpenAPIValidation bool // Get missing dependencies DependencyUpdate bool + ExternalFiles files.ExternalFiles // Lock to control raceconditions when the process receives a SIGTERM Lock sync.Mutex } diff --git a/pkg/chart/loader/local.go b/pkg/chart/loader/local.go new file mode 100644 index 000000000..9537c2742 --- /dev/null +++ b/pkg/chart/loader/local.go @@ -0,0 +1,85 @@ +/* +Copyright The Helm Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package loader + +import ( + "errors" + "os" + "path/filepath" + "strings" +) + +// ExpandLocalPath expands a local file, dir or glob path to a list of files +func ExpandLocalPath(name string, path string) (map[string]string, error) { + if strings.Contains(path, "*") { + // if this is a glob, we expand it and return a list of files + return expandGlob(name, path) + } + + fi, err := os.Stat(path) + if err != nil { + return nil, err + } + + if fi.IsDir() { + // if this is a valid dir, we return all files within + return expandDir(name, path) + } + + // finally, this is a file, so we return it + return map[string]string{name: path}, nil +} + +func expandGlob(name string, path string) (map[string]string, error) { + fmap := make(map[string]string) + paths, err := filepath.Glob(path) + if err != nil { + return nil, err + } + if len(paths) == 0 { + return nil, errors.New("empty glob") + } + + namePrefix := strings.TrimRight(name, "/") + "/" + for _, p := range paths { + key := namePrefix + filepath.Base(p) + fmap[key] = p + } + + return fmap, nil +} + +func expandDir(name string, path string) (map[string]string, error) { + fmap := make(map[string]string) + + f, err := os.Open(path) + + if err != nil { + return nil, err + } + defer f.Close() + + files, err := f.Readdir(-1) + if err != nil { + return nil, err + } + + localDirName := strings.TrimRight(path, "/") + "/" + namePrefix := strings.TrimRight(name, "/") + "/" + for _, file := range files { + key := namePrefix + file.Name() + fmap[key] = localDirName + file.Name() + } + return fmap, nil +} diff --git a/pkg/chart/loader/local_test.go b/pkg/chart/loader/local_test.go new file mode 100644 index 000000000..79b81f9fe --- /dev/null +++ b/pkg/chart/loader/local_test.go @@ -0,0 +1,46 @@ +/* +Copyright The Helm Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package loader + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestExpandLocalPath(t *testing.T) { + need := require.New(t) + is := assert.New(t) + + glob, err := ExpandLocalPath("glob", "testdata/frobnitz/*.yaml") + need.NoError(err) + need.Contains(glob, "glob/Chart.yaml") + need.Contains(glob, "glob/values.yaml") + is.Equal("testdata/frobnitz/Chart.yaml", glob["glob/Chart.yaml"]) + is.Equal("testdata/frobnitz/values.yaml", glob["glob/values.yaml"]) + + dir, err := ExpandLocalPath("dir", "testdata/albatross/") + need.NoError(err) + need.Contains(dir, "dir/Chart.yaml") + need.Contains(dir, "dir/values.yaml") + is.Equal("testdata/albatross/Chart.yaml", dir["dir/Chart.yaml"]) + is.Equal("testdata/albatross/values.yaml", dir["dir/values.yaml"]) + + file, err := ExpandLocalPath("file", "testdata/albatross/Chart.yaml") + need.NoError(err) + need.Contains(file, "file") + is.Equal("testdata/albatross/Chart.yaml", file["file"]) + +} diff --git a/pkg/cli/files/files.go b/pkg/cli/files/files.go new file mode 100644 index 000000000..f8f781742 --- /dev/null +++ b/pkg/cli/files/files.go @@ -0,0 +1,20 @@ +/* +Copyright The Helm Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package files + +// ExternalFiles holds the list of external files or globs +type ExternalFiles struct { + Files []string + Globs []string +} diff --git a/pkg/cli/files/parser.go b/pkg/cli/files/parser.go new file mode 100644 index 000000000..d3dd68345 --- /dev/null +++ b/pkg/cli/files/parser.go @@ -0,0 +1,66 @@ +/* +Copyright The Helm Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package files + +import ( + "errors" + "fmt" + "path/filepath" + "strings" +) + +// ParseIntoString parses a include-file line and merges the result into dest. +func ParseIntoString(s string, dest map[string]string) error { + for _, val := range strings.Split(s, ",") { + val = strings.TrimSpace(val) + splt := strings.SplitN(val, "=", 2) + + if len(splt) != 2 { + return errors.New("Could not parse line") + } + + name := strings.TrimSpace(splt[0]) + path := strings.TrimSpace(splt[1]) + dest[name] = path + } + + return nil +} + +//ParseGlobIntoString parses an include-dir file line and merges all files found into dest. +func ParseGlobIntoString(g string, dest map[string]string) error { + globs := make(map[string]string) + err := ParseIntoString(g, globs) + if err != nil { + return err + } + for k, g := range globs { + if !strings.Contains(g, "*") { + // force glob style on simple directories + g = strings.TrimRight(g, "/") + "/*" + } + + paths, err := filepath.Glob(g) + if err != nil { + return err + } + + k = strings.TrimRight(k, "/") + for _, path := range paths { + dest[fmt.Sprintf("%s/%s", k, filepath.Base(path))] = path + } + } + + return nil +} diff --git a/pkg/cli/files/parser_test.go b/pkg/cli/files/parser_test.go new file mode 100644 index 000000000..ac3a9d46f --- /dev/null +++ b/pkg/cli/files/parser_test.go @@ -0,0 +1,76 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package files + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseIntoString(t *testing.T) { + need := require.New(t) + is := assert.New(t) + + dest := make(map[string]string) + goodFlag := "foo.txt=../foo.txt" + anotherFlag := " bar.txt=~/bar.txt, baz.txt=/path/to/baz.txt" + + err := ParseIntoString(goodFlag, dest) + need.NoError(err) + + err = ParseIntoString(anotherFlag, dest) + need.NoError(err) + + is.Contains(dest, "foo.txt") + is.Contains(dest, "bar.txt") + is.Contains(dest, "baz.txt") + + is.Equal(dest["foo.txt"], "../foo.txt", "foo.txt not mapped properly") + is.Equal(dest["bar.txt"], "~/bar.txt", "bar.txt not mapped properly") + is.Equal(dest["baz.txt"], "/path/to/baz.txt", "baz.txt not mapped properly") + + overwriteFlag := "foo.txt=../new_foo.txt" + err = ParseIntoString(overwriteFlag, dest) + need.NoError(err) + + is.Equal(dest["foo.txt"], "../new_foo.txt") + + badFlag := "empty.txt" + err = ParseIntoString(badFlag, dest) + is.NotNil(err) +} + +func TestParseGlobIntoString(t *testing.T) { + need := require.New(t) + is := assert.New(t) + + dest := make(map[string]string) + globFlagSlash := "glob/=testdata/foo/foo.*" + dirFlagNoSlash := "dir=testdata/foo/" + + err := ParseGlobIntoString(globFlagSlash, dest) + need.NoError(err) + need.Contains(dest, "glob/foo.txt") + is.Equal("testdata/foo/foo.txt", dest["glob/foo.txt"]) + + err = ParseGlobIntoString(dirFlagNoSlash, dest) + need.NoError(err) + need.Contains(dest, "dir/foo.txt") + need.Contains(dest, "dir/bar.txt") + is.Equal("testdata/foo/foo.txt", dest["dir/foo.txt"]) + is.Equal("testdata/foo/bar.txt", dest["dir/bar.txt"]) +} diff --git a/pkg/cli/files/testdata/foo/bar.txt b/pkg/cli/files/testdata/foo/bar.txt new file mode 100644 index 000000000..5716ca598 --- /dev/null +++ b/pkg/cli/files/testdata/foo/bar.txt @@ -0,0 +1 @@ +bar diff --git a/pkg/cli/files/testdata/foo/foo.txt b/pkg/cli/files/testdata/foo/foo.txt new file mode 100644 index 000000000..257cc5642 --- /dev/null +++ b/pkg/cli/files/testdata/foo/foo.txt @@ -0,0 +1 @@ +foo From 907eb8c8bfea9b1a6e081e396f5e8593e2f5dcce Mon Sep 17 00:00:00 2001 From: Matheus Hunsche Date: Fri, 2 Oct 2020 02:16:26 -0300 Subject: [PATCH 02/12] fix tests templates Signed-off-by: Matheus Hunsche --- cmd/helm/testdata/output/template-with-external-dir.txt | 9 ++++++--- cmd/helm/testdata/output/template-with-external-file.txt | 3 ++- cmd/helm/testdata/output/template-with-external-glob.txt | 6 ++++-- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/cmd/helm/testdata/output/template-with-external-dir.txt b/cmd/helm/testdata/output/template-with-external-dir.txt index 34645fde8..d12220663 100644 --- a/cmd/helm/testdata/output/template-with-external-dir.txt +++ b/cmd/helm/testdata/output/template-with-external-dir.txt @@ -17,6 +17,9 @@ metadata: # This makes it easy to audit chart usage. helm.sh/chart: "configmap-0.1.0" data: - external.1.conf: glob-external-1 - external.2.conf: glob-external-2 - external.txt: out-of-chart-dir + external.1.conf: | + glob-external-1 + external.2.conf: | + glob-external-2 + external.txt: | + out-of-chart-dir diff --git a/cmd/helm/testdata/output/template-with-external-file.txt b/cmd/helm/testdata/output/template-with-external-file.txt index 9550768c0..fcbdad5fe 100644 --- a/cmd/helm/testdata/output/template-with-external-file.txt +++ b/cmd/helm/testdata/output/template-with-external-file.txt @@ -17,4 +17,5 @@ metadata: # This makes it easy to audit chart usage. helm.sh/chart: "configmap-0.1.0" data: - external.txt: out-of-chart-dir + external.txt: | + out-of-chart-dir diff --git a/cmd/helm/testdata/output/template-with-external-glob.txt b/cmd/helm/testdata/output/template-with-external-glob.txt index 7796e1c7e..5d663af0d 100644 --- a/cmd/helm/testdata/output/template-with-external-glob.txt +++ b/cmd/helm/testdata/output/template-with-external-glob.txt @@ -17,5 +17,7 @@ metadata: # This makes it easy to audit chart usage. helm.sh/chart: "configmap-0.1.0" data: - external.1.conf: glob-external-1 - external.2.conf: glob-external-2 + external.1.conf: | + glob-external-1 + external.2.conf: | + glob-external-2 From 5c6c660ac55da21e015389acec5277552e0d2a2c Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Sun, 22 Aug 2021 22:17:50 +0300 Subject: [PATCH 03/12] Require passing only file name without alias. E.g: helm install myrelease mychart --include-file /opt/my_file.txt Signed-off-by: itaispiegel --- cmd/helm/install.go | 35 +++------ cmd/helm/install_test.go | 6 +- cmd/helm/template_test.go | 6 +- .../testdata/testcharts/external/values.yaml | 2 +- cmd/helm/upgrade_test.go | 4 +- pkg/chart/loader/local.go | 40 ++++------ pkg/chart/loader/local_test.go | 32 +++----- pkg/cli/files/parser.go | 66 ---------------- pkg/cli/files/parser_test.go | 76 ------------------- pkg/cli/files/testdata/foo/bar.txt | 1 - pkg/cli/files/testdata/foo/foo.txt | 1 - 11 files changed, 48 insertions(+), 221 deletions(-) delete mode 100644 pkg/cli/files/parser.go delete mode 100644 pkg/cli/files/parser_test.go delete mode 100644 pkg/cli/files/testdata/foo/bar.txt delete mode 100644 pkg/cli/files/testdata/foo/foo.txt diff --git a/cmd/helm/install.go b/cmd/helm/install.go index 53da84b86..17d11c099 100644 --- a/cmd/helm/install.go +++ b/cmd/helm/install.go @@ -300,36 +300,25 @@ func checkIfInstallable(ch *chart.Chart) error { func loadExternalFiles(ch *chart.Chart, exFiles files.ExternalFiles) error { var errs []string - fs := make(map[string]string) + var fs []string - for _, s := range exFiles.Files { - if err := files.ParseIntoString(s, fs); err != nil { - debug(fmt.Sprintf("error parsing include-file option %s: %v", s, err)) - errs = append(errs, fmt.Sprintf("%s (parse error)", s)) - } - } - - for _, g := range exFiles.Globs { - if err := files.ParseIntoString(g, fs); err != nil { - debug(fmt.Sprintf("error parsing include-dir option %s: %v", g, err)) - errs = append(errs, fmt.Sprintf("%s (parse error)", g)) - } - } + fs = append(fs, exFiles.Files...) + fs = append(fs, exFiles.Globs...) - for n, p := range fs { - allPaths, err := loader.ExpandLocalPath(n, p) - debug(fmt.Sprintf("%s expanded to: %v", p, allPaths)) + for _, path := range fs { + allPaths, err := loader.ExpandFilePath(path) + debug(fmt.Sprintf("%s expanded to: %v", path, allPaths)) if err != nil { - debug(fmt.Sprintf("error loading external path %s: %v", p, err)) - errs = append(errs, fmt.Sprintf("%s (path not accessible)", p)) + debug(fmt.Sprintf("error loading external path %s: %v", path, err)) + errs = append(errs, fmt.Sprintf("%s (path not accessible)", path)) } - for name, fp := range allPaths { - byt, err := ioutil.ReadFile(fp) + for _, name := range allPaths { + fileContentBytes, err := ioutil.ReadFile(name) if err != nil { - errs = append(errs, fmt.Sprintf("%s (not readable)", fp)) + errs = append(errs, fmt.Sprintf("%s (not readable)", name)) } else { - ch.Files = append(ch.Files, &chart.File{Name: name, Data: byt}) + ch.Files = append(ch.Files, &chart.File{Name: name, Data: fileContentBytes}) } } } diff --git a/cmd/helm/install_test.go b/cmd/helm/install_test.go index a1a97445d..d7e13dfc7 100644 --- a/cmd/helm/install_test.go +++ b/cmd/helm/install_test.go @@ -80,19 +80,19 @@ func TestInstall(t *testing.T) { // Install, external file { name: "install with external files", - cmd: "install virgil testdata/testcharts/external --include-file external.txt=testdata/files/external.txt", + cmd: "install virgil testdata/testcharts/external --include-file testdata/files/external.txt --set external=testdata/files/external.txt", golden: "output/install-with-external-files.txt", }, // Install, external dir { name: "install with external dir", - cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-dir glob=testdata/files/", + cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-dir testdata/files/", golden: "output/install-with-external-files.txt", }, // Install, external glob files { name: "install with external globbed files", - cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-dir glob=testdata/files/external.*.conf", + cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-dir testdata/files/external.*.conf", golden: "output/install-with-external-files.txt", }, // Install, no hooks diff --git a/cmd/helm/template_test.go b/cmd/helm/template_test.go index 91a68acec..5ffdd8bc8 100644 --- a/cmd/helm/template_test.go +++ b/cmd/helm/template_test.go @@ -133,17 +133,17 @@ func TestTemplateCmd(t *testing.T) { }, { name: "chart with template with external file", - cmd: fmt.Sprintf("template '%s' --set external=external.txt --include-file external.txt=testdata/files/external.txt", "testdata/testcharts/external"), + cmd: fmt.Sprintf("template '%s' --set external=testdata/files/external.txt --include-file testdata/files/external.txt", "testdata/testcharts/external"), golden: "output/template-with-external-file.txt", }, { name: "chart with template with external dir", - cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-dir glob=testdata/files/", "testdata/testcharts/external"), + cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-dir testdata/files/", "testdata/testcharts/external"), golden: "output/template-with-external-dir.txt", }, { name: "chart with template with external globbed files", - cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-dir glob=testdata/files/external.*.conf", "testdata/testcharts/external"), + cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-dir testdata/files/external.*.conf", "testdata/testcharts/external"), golden: "output/template-with-external-glob.txt", }, } diff --git a/cmd/helm/testdata/testcharts/external/values.yaml b/cmd/helm/testdata/testcharts/external/values.yaml index ee4f44aed..422adf0ef 100644 --- a/cmd/helm/testdata/testcharts/external/values.yaml +++ b/cmd/helm/testdata/testcharts/external/values.yaml @@ -1,4 +1,4 @@ external: false glob: enabled: false - path: "glob/*" + path: "testdata/files/*" diff --git a/cmd/helm/upgrade_test.go b/cmd/helm/upgrade_test.go index 26a459a79..29a397e2f 100644 --- a/cmd/helm/upgrade_test.go +++ b/cmd/helm/upgrade_test.go @@ -362,7 +362,7 @@ func TestUpgradeWithExternalFile(t *testing.T) { releaseName := "funny-bunny-v7" exFiles := []*chart.File{ - {Name: "external.txt", Data: []byte("from-external-file")}, + {Name: "testdata/files/external.txt", Data: []byte("from-external-file")}, } relMock, ch, chartPath := prepareMockReleaseWithExternal(releaseName, exFiles, t) @@ -373,7 +373,7 @@ func TestUpgradeWithExternalFile(t *testing.T) { store.Create(relMock(releaseName, 3, ch)) - cmd := fmt.Sprintf("upgrade %s --set glob.enabled=false --set external=external.txt '%s'", releaseName, chartPath) + cmd := fmt.Sprintf("upgrade %s --set glob.enabled=false --set external=testdata/files/external.txt '%s'", releaseName, chartPath) _, _, err := executeActionCommandC(store, cmd) if err != nil { t.Errorf("unexpected error, got '%v'", err) diff --git a/pkg/chart/loader/local.go b/pkg/chart/loader/local.go index 9537c2742..38f080827 100644 --- a/pkg/chart/loader/local.go +++ b/pkg/chart/loader/local.go @@ -20,29 +20,28 @@ import ( "strings" ) -// ExpandLocalPath expands a local file, dir or glob path to a list of files -func ExpandLocalPath(name string, path string) (map[string]string, error) { +// ExpandFilePath expands a local file, dir or glob path to a list of files +func ExpandFilePath(path string) ([]string, error) { if strings.Contains(path, "*") { // if this is a glob, we expand it and return a list of files - return expandGlob(name, path) + return expandGlob(path) } - fi, err := os.Stat(path) + fileInfo, err := os.Stat(path) if err != nil { return nil, err } - if fi.IsDir() { + if fileInfo.IsDir() { // if this is a valid dir, we return all files within - return expandDir(name, path) + return expandDir(path) } // finally, this is a file, so we return it - return map[string]string{name: path}, nil + return []string{path}, nil } -func expandGlob(name string, path string) (map[string]string, error) { - fmap := make(map[string]string) +func expandGlob(path string) ([]string, error) { paths, err := filepath.Glob(path) if err != nil { return nil, err @@ -51,18 +50,10 @@ func expandGlob(name string, path string) (map[string]string, error) { return nil, errors.New("empty glob") } - namePrefix := strings.TrimRight(name, "/") + "/" - for _, p := range paths { - key := namePrefix + filepath.Base(p) - fmap[key] = p - } - - return fmap, nil + return paths, err } -func expandDir(name string, path string) (map[string]string, error) { - fmap := make(map[string]string) - +func expandDir(path string) ([]string, error) { f, err := os.Open(path) if err != nil { @@ -70,16 +61,15 @@ func expandDir(name string, path string) (map[string]string, error) { } defer f.Close() - files, err := f.Readdir(-1) + filesInfos, err := f.Readdir(-1) if err != nil { return nil, err } + var filesPaths []string localDirName := strings.TrimRight(path, "/") + "/" - namePrefix := strings.TrimRight(name, "/") + "/" - for _, file := range files { - key := namePrefix + file.Name() - fmap[key] = localDirName + file.Name() + for _, file := range filesInfos { + filesPaths = append(filesPaths, localDirName+file.Name()) } - return fmap, nil + return filesPaths, nil } diff --git a/pkg/chart/loader/local_test.go b/pkg/chart/loader/local_test.go index 79b81f9fe..456d0dddf 100644 --- a/pkg/chart/loader/local_test.go +++ b/pkg/chart/loader/local_test.go @@ -16,31 +16,23 @@ package loader import ( "testing" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestExpandLocalPath(t *testing.T) { - need := require.New(t) - is := assert.New(t) + req := require.New(t) - glob, err := ExpandLocalPath("glob", "testdata/frobnitz/*.yaml") - need.NoError(err) - need.Contains(glob, "glob/Chart.yaml") - need.Contains(glob, "glob/values.yaml") - is.Equal("testdata/frobnitz/Chart.yaml", glob["glob/Chart.yaml"]) - is.Equal("testdata/frobnitz/values.yaml", glob["glob/values.yaml"]) + glob, err := ExpandFilePath("testdata/frobnitz/*.yaml") + req.NoError(err) + req.Contains(glob, "testdata/frobnitz/Chart.yaml") + req.Contains(glob, "testdata/frobnitz/values.yaml") - dir, err := ExpandLocalPath("dir", "testdata/albatross/") - need.NoError(err) - need.Contains(dir, "dir/Chart.yaml") - need.Contains(dir, "dir/values.yaml") - is.Equal("testdata/albatross/Chart.yaml", dir["dir/Chart.yaml"]) - is.Equal("testdata/albatross/values.yaml", dir["dir/values.yaml"]) - - file, err := ExpandLocalPath("file", "testdata/albatross/Chart.yaml") - need.NoError(err) - need.Contains(file, "file") - is.Equal("testdata/albatross/Chart.yaml", file["file"]) + dir, err := ExpandFilePath("testdata/albatross/") + req.NoError(err) + req.Contains(dir, "testdata/albatross/Chart.yaml") + req.Contains(dir, "testdata/albatross/values.yaml") + file, err := ExpandFilePath("testdata/albatross/Chart.yaml") + req.NoError(err) + req.Contains(file, "testdata/albatross/Chart.yaml") } diff --git a/pkg/cli/files/parser.go b/pkg/cli/files/parser.go deleted file mode 100644 index d3dd68345..000000000 --- a/pkg/cli/files/parser.go +++ /dev/null @@ -1,66 +0,0 @@ -/* -Copyright The Helm Authors. -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - http://www.apache.org/licenses/LICENSE-2.0 -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package files - -import ( - "errors" - "fmt" - "path/filepath" - "strings" -) - -// ParseIntoString parses a include-file line and merges the result into dest. -func ParseIntoString(s string, dest map[string]string) error { - for _, val := range strings.Split(s, ",") { - val = strings.TrimSpace(val) - splt := strings.SplitN(val, "=", 2) - - if len(splt) != 2 { - return errors.New("Could not parse line") - } - - name := strings.TrimSpace(splt[0]) - path := strings.TrimSpace(splt[1]) - dest[name] = path - } - - return nil -} - -//ParseGlobIntoString parses an include-dir file line and merges all files found into dest. -func ParseGlobIntoString(g string, dest map[string]string) error { - globs := make(map[string]string) - err := ParseIntoString(g, globs) - if err != nil { - return err - } - for k, g := range globs { - if !strings.Contains(g, "*") { - // force glob style on simple directories - g = strings.TrimRight(g, "/") + "/*" - } - - paths, err := filepath.Glob(g) - if err != nil { - return err - } - - k = strings.TrimRight(k, "/") - for _, path := range paths { - dest[fmt.Sprintf("%s/%s", k, filepath.Base(path))] = path - } - } - - return nil -} diff --git a/pkg/cli/files/parser_test.go b/pkg/cli/files/parser_test.go deleted file mode 100644 index ac3a9d46f..000000000 --- a/pkg/cli/files/parser_test.go +++ /dev/null @@ -1,76 +0,0 @@ -/* -Copyright The Helm Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - http://www.apache.org/licenses/LICENSE-2.0 -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package files - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestParseIntoString(t *testing.T) { - need := require.New(t) - is := assert.New(t) - - dest := make(map[string]string) - goodFlag := "foo.txt=../foo.txt" - anotherFlag := " bar.txt=~/bar.txt, baz.txt=/path/to/baz.txt" - - err := ParseIntoString(goodFlag, dest) - need.NoError(err) - - err = ParseIntoString(anotherFlag, dest) - need.NoError(err) - - is.Contains(dest, "foo.txt") - is.Contains(dest, "bar.txt") - is.Contains(dest, "baz.txt") - - is.Equal(dest["foo.txt"], "../foo.txt", "foo.txt not mapped properly") - is.Equal(dest["bar.txt"], "~/bar.txt", "bar.txt not mapped properly") - is.Equal(dest["baz.txt"], "/path/to/baz.txt", "baz.txt not mapped properly") - - overwriteFlag := "foo.txt=../new_foo.txt" - err = ParseIntoString(overwriteFlag, dest) - need.NoError(err) - - is.Equal(dest["foo.txt"], "../new_foo.txt") - - badFlag := "empty.txt" - err = ParseIntoString(badFlag, dest) - is.NotNil(err) -} - -func TestParseGlobIntoString(t *testing.T) { - need := require.New(t) - is := assert.New(t) - - dest := make(map[string]string) - globFlagSlash := "glob/=testdata/foo/foo.*" - dirFlagNoSlash := "dir=testdata/foo/" - - err := ParseGlobIntoString(globFlagSlash, dest) - need.NoError(err) - need.Contains(dest, "glob/foo.txt") - is.Equal("testdata/foo/foo.txt", dest["glob/foo.txt"]) - - err = ParseGlobIntoString(dirFlagNoSlash, dest) - need.NoError(err) - need.Contains(dest, "dir/foo.txt") - need.Contains(dest, "dir/bar.txt") - is.Equal("testdata/foo/foo.txt", dest["dir/foo.txt"]) - is.Equal("testdata/foo/bar.txt", dest["dir/bar.txt"]) -} diff --git a/pkg/cli/files/testdata/foo/bar.txt b/pkg/cli/files/testdata/foo/bar.txt deleted file mode 100644 index 5716ca598..000000000 --- a/pkg/cli/files/testdata/foo/bar.txt +++ /dev/null @@ -1 +0,0 @@ -bar diff --git a/pkg/cli/files/testdata/foo/foo.txt b/pkg/cli/files/testdata/foo/foo.txt deleted file mode 100644 index 257cc5642..000000000 --- a/pkg/cli/files/testdata/foo/foo.txt +++ /dev/null @@ -1 +0,0 @@ -foo From 7bceab3fc764548e5292157d3c0c879463052f90 Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Mon, 23 Aug 2021 14:27:54 +0300 Subject: [PATCH 04/12] Merge --include-file and --include-dir flags to --include-path Signed-off-by: itaispiegel --- cmd/helm/flags.go | 6 ++---- cmd/helm/install.go | 15 +++++---------- cmd/helm/install_test.go | 8 ++++---- cmd/helm/template_test.go | 6 +++--- cmd/helm/upgrade.go | 6 +++--- pkg/action/install.go | 3 +-- pkg/action/upgrade.go | 3 +-- pkg/cli/files/files.go | 20 -------------------- 8 files changed, 19 insertions(+), 48 deletions(-) delete mode 100644 pkg/cli/files/files.go diff --git a/cmd/helm/flags.go b/cmd/helm/flags.go index 6453e523d..378730f32 100644 --- a/cmd/helm/flags.go +++ b/cmd/helm/flags.go @@ -29,7 +29,6 @@ import ( "k8s.io/klog/v2" "helm.sh/helm/v3/pkg/action" - "helm.sh/helm/v3/pkg/cli/files" "helm.sh/helm/v3/pkg/cli/output" "helm.sh/helm/v3/pkg/cli/values" "helm.sh/helm/v3/pkg/helmpath" @@ -65,9 +64,8 @@ func addChartPathOptionsFlags(f *pflag.FlagSet, c *action.ChartPathOptions) { f.BoolVar(&c.PassCredentialsAll, "pass-credentials", false, "pass credentials to all domains") } -func addExternalFilesFlags(f *pflag.FlagSet, v *files.ExternalFiles) { - f.StringArrayVar(&v.Files, "include-file", []string{}, "paths to local files to add during chart installation") - f.StringArrayVar(&v.Globs, "include-dir", []string{}, "paths or globs to local dirs to add during chart installation") +func addExternalPathsFlags(f *pflag.FlagSet, v *[]string) { + f.StringArrayVar(v, "include-path", []string{}, "paths or globs to local files and directories to add during chart installation") } // bindOutputFlag will add the output flag to the given command and bind the diff --git a/cmd/helm/install.go b/cmd/helm/install.go index 17d11c099..b287b1f72 100644 --- a/cmd/helm/install.go +++ b/cmd/helm/install.go @@ -36,7 +36,6 @@ import ( "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" - "helm.sh/helm/v3/pkg/cli/files" "helm.sh/helm/v3/pkg/cli/output" "helm.sh/helm/v3/pkg/cli/values" "helm.sh/helm/v3/pkg/downloader" @@ -174,7 +173,7 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal f.BoolVar(&client.SubNotes, "render-subchart-notes", false, "if set, render subchart notes along with the parent") addValueOptionsFlags(f, valueOpts) addChartPathOptionsFlags(f, &client.ChartPathOptions) - addExternalFilesFlags(f, &client.ExternalFiles) + addExternalPathsFlags(f, &client.ExternalPaths) err := cmd.RegisterFlagCompletionFunc("version", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { requiredArgs := 2 @@ -262,7 +261,7 @@ func runInstall(args []string, client *action.Install, valueOpts *values.Options } } - err = loadExternalFiles(chartRequested, client.ExternalFiles) + err = loadExternalPaths(chartRequested, client.ExternalPaths) if err != nil { return nil, err } @@ -298,14 +297,10 @@ func checkIfInstallable(ch *chart.Chart) error { return errors.Errorf("%s charts are not installable", ch.Metadata.Type) } -func loadExternalFiles(ch *chart.Chart, exFiles files.ExternalFiles) error { +func loadExternalPaths(ch *chart.Chart, externalPaths []string) error { var errs []string - var fs []string - fs = append(fs, exFiles.Files...) - fs = append(fs, exFiles.Globs...) - - for _, path := range fs { + for _, path := range externalPaths { allPaths, err := loader.ExpandFilePath(path) debug(fmt.Sprintf("%s expanded to: %v", path, allPaths)) if err != nil { @@ -324,7 +319,7 @@ func loadExternalFiles(ch *chart.Chart, exFiles files.ExternalFiles) error { } if len(errs) > 0 { - return errors.New(fmt.Sprint("Failed to load external files: ", strings.Join(errs, "; "))) + return errors.New(fmt.Sprint("Failed to load external paths: ", strings.Join(errs, "; "))) } return nil } diff --git a/cmd/helm/install_test.go b/cmd/helm/install_test.go index d7e13dfc7..54079d17d 100644 --- a/cmd/helm/install_test.go +++ b/cmd/helm/install_test.go @@ -77,22 +77,22 @@ func TestInstall(t *testing.T) { cmd: "install virgil testdata/testcharts/alpine -f testdata/testcharts/alpine/extra_values.yaml", golden: "output/install-with-values-file.txt", }, - // Install, external file + // Install, external files { name: "install with external files", - cmd: "install virgil testdata/testcharts/external --include-file testdata/files/external.txt --set external=testdata/files/external.txt", + cmd: "install virgil testdata/testcharts/external --include-path testdata/files/external.txt --set external=testdata/files/external.txt", golden: "output/install-with-external-files.txt", }, // Install, external dir { name: "install with external dir", - cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-dir testdata/files/", + cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-path testdata/files/", golden: "output/install-with-external-files.txt", }, // Install, external glob files { name: "install with external globbed files", - cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-dir testdata/files/external.*.conf", + cmd: "install virgil testdata/testcharts/external --set glob.enabled=true --include-path testdata/files/external.*.conf", golden: "output/install-with-external-files.txt", }, // Install, no hooks diff --git a/cmd/helm/template_test.go b/cmd/helm/template_test.go index 5ffdd8bc8..25bf05c4c 100644 --- a/cmd/helm/template_test.go +++ b/cmd/helm/template_test.go @@ -133,17 +133,17 @@ func TestTemplateCmd(t *testing.T) { }, { name: "chart with template with external file", - cmd: fmt.Sprintf("template '%s' --set external=testdata/files/external.txt --include-file testdata/files/external.txt", "testdata/testcharts/external"), + cmd: fmt.Sprintf("template '%s' --set external=testdata/files/external.txt --include-path testdata/files/external.txt", "testdata/testcharts/external"), golden: "output/template-with-external-file.txt", }, { name: "chart with template with external dir", - cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-dir testdata/files/", "testdata/testcharts/external"), + cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-path testdata/files/", "testdata/testcharts/external"), golden: "output/template-with-external-dir.txt", }, { name: "chart with template with external globbed files", - cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-dir testdata/files/external.*.conf", "testdata/testcharts/external"), + cmd: fmt.Sprintf("template '%s' --set glob.enabled=true --include-path testdata/files/external.*.conf", "testdata/testcharts/external"), golden: "output/template-with-external-glob.txt", }, } diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index 45a35c679..d1ce7624e 100644 --- a/cmd/helm/upgrade.go +++ b/cmd/helm/upgrade.go @@ -116,7 +116,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { instClient.PostRenderer = client.PostRenderer instClient.DisableOpenAPIValidation = client.DisableOpenAPIValidation instClient.SubNotes = client.SubNotes - instClient.ExternalFiles = client.ExternalFiles + instClient.ExternalPaths = client.ExternalPaths instClient.Description = client.Description instClient.DependencyUpdate = client.DependencyUpdate @@ -182,7 +182,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { warning("This chart is deprecated") } - err = loadExternalFiles(ch, client.ExternalFiles) + err = loadExternalPaths(ch, client.ExternalPaths) if err != nil { return err } @@ -241,7 +241,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { addValueOptionsFlags(f, valueOpts) bindOutputFlag(cmd, &outfmt) bindPostRenderFlag(cmd, &client.PostRenderer) - addExternalFilesFlags(f, &client.ExternalFiles) + addExternalPathsFlags(f, &client.ExternalPaths) err := cmd.RegisterFlagCompletionFunc("version", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { if len(args) != 2 { diff --git a/pkg/action/install.go b/pkg/action/install.go index 92e14ce9e..e16e69aeb 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -41,7 +41,6 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/cli" - "helm.sh/helm/v3/pkg/cli/files" "helm.sh/helm/v3/pkg/downloader" "helm.sh/helm/v3/pkg/getter" "helm.sh/helm/v3/pkg/kube" @@ -101,7 +100,7 @@ type Install struct { // OutputDir/ UseReleaseName bool PostRenderer postrender.PostRenderer - ExternalFiles files.ExternalFiles + ExternalPaths []string // Lock to control raceconditions when the process receives a SIGTERM Lock sync.Mutex } diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index e2befe2a1..2826076d2 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -30,7 +30,6 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" - "helm.sh/helm/v3/pkg/cli/files" "helm.sh/helm/v3/pkg/kube" "helm.sh/helm/v3/pkg/postrender" "helm.sh/helm/v3/pkg/release" @@ -102,7 +101,7 @@ type Upgrade struct { DisableOpenAPIValidation bool // Get missing dependencies DependencyUpdate bool - ExternalFiles files.ExternalFiles + ExternalPaths []string // Lock to control raceconditions when the process receives a SIGTERM Lock sync.Mutex } diff --git a/pkg/cli/files/files.go b/pkg/cli/files/files.go deleted file mode 100644 index f8f781742..000000000 --- a/pkg/cli/files/files.go +++ /dev/null @@ -1,20 +0,0 @@ -/* -Copyright The Helm Authors. -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - http://www.apache.org/licenses/LICENSE-2.0 -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package files - -// ExternalFiles holds the list of external files or globs -type ExternalFiles struct { - Files []string - Globs []string -} From 143cd93e39b8125ba8160e1d71333453cc001839 Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Mon, 23 Aug 2021 15:09:37 +0300 Subject: [PATCH 05/12] Make file functions fail if file isn't included or passed Signed-off-by: itaispiegel --- pkg/engine/files.go | 38 +++++++++++++++++------------- pkg/engine/files_test.go | 51 +++++++++++++++++++++++++++++++++------- 2 files changed, 65 insertions(+), 24 deletions(-) diff --git a/pkg/engine/files.go b/pkg/engine/files.go index d7e62da5a..ed9ad757e 100644 --- a/pkg/engine/files.go +++ b/pkg/engine/files.go @@ -18,6 +18,7 @@ package engine import ( "encoding/base64" + "fmt" "path" "strings" @@ -46,11 +47,11 @@ func newFiles(from []*chart.File) files { // // This is intended to be accessed from within a template, so a missed key returns // an empty []byte. -func (f files) GetBytes(name string) []byte { +func (f files) GetBytes(name string) ([]byte, error) { if v, ok := f[name]; ok { - return v + return v, nil } - return []byte{} + return []byte{}, fmt.Errorf("file %s not included", name) } // Get returns a string representation of the given file. @@ -59,8 +60,13 @@ func (f files) GetBytes(name string) []byte { // template. // // {{.Files.Get "foo"}} -func (f files) Get(name string) string { - return string(f.GetBytes(name)) +func (f files) Get(name string) (string, error) { + content, err := f.GetBytes(name) + if err != nil { + return "", err + } + + return string(content), nil } // Glob takes a glob pattern and returns another files object only containing @@ -101,9 +107,9 @@ func (f files) Glob(pattern string) files { // // data: // {{ .Files.Glob("config/**").AsConfig() | indent 4 }} -func (f files) AsConfig() string { - if f == nil { - return "" +func (f files) AsConfig() (string, error) { + if f == nil || len(f) == 0 { + return "", fmt.Errorf("must pass files") } m := make(map[string]string) @@ -113,7 +119,7 @@ func (f files) AsConfig() string { m[path.Base(k)] = string(v) } - return toYAML(m) + return toYAML(m), nil } // AsSecrets returns the base64-encoded value of a Files object suitable for @@ -130,9 +136,9 @@ func (f files) AsConfig() string { // // data: // {{ .Files.Glob("secrets/*").AsSecrets() }} -func (f files) AsSecrets() string { - if f == nil { - return "" +func (f files) AsSecrets() (string, error) { + if f == nil || len(f) == 0 { + return "", fmt.Errorf("must pass files") } m := make(map[string]string) @@ -141,7 +147,7 @@ func (f files) AsSecrets() string { m[path.Base(k)] = base64.StdEncoding.EncodeToString(v) } - return toYAML(m) + return toYAML(m), nil } // Lines returns each line of a named file (split by "\n") as a slice, so it can @@ -151,10 +157,10 @@ func (f files) AsSecrets() string { // // {{ range .Files.Lines "foo/bar.html" }} // {{ . }}{{ end }} -func (f files) Lines(path string) []string { +func (f files) Lines(path string) ([]string, error) { if f == nil || f[path] == nil { - return []string{} + return nil, fmt.Errorf("must pass files") } - return strings.Split(string(f[path]), "\n") + return strings.Split(string(f[path]), "\n"), nil } diff --git a/pkg/engine/files_test.go b/pkg/engine/files_test.go index 4b37724f9..debb3089e 100644 --- a/pkg/engine/files_test.go +++ b/pkg/engine/files_test.go @@ -21,6 +21,8 @@ import ( "github.com/stretchr/testify/assert" ) +const NonExistingFileName = "no_such_file.txt" + var cases = []struct { path, data string }{ @@ -46,15 +48,30 @@ func TestNewFiles(t *testing.T) { } for i, f := range cases { - if got := string(files.GetBytes(f.path)); got != f.data { + gotBytes, err := files.GetBytes(f.path) + got := string(gotBytes) + if err != nil || got != f.data { t.Errorf("%d: expected %q, got %q", i, f.data, got) } - if got := files.Get(f.path); got != f.data { + + gotBytes, err = files.GetBytes(f.path) + got = string(gotBytes) + if err != nil || got != f.data { t.Errorf("%d: expected %q, got %q", i, f.data, got) } } } +func TestGetNonExistingFile(t *testing.T) { + as := assert.New(t) + + f := getTestFiles() + + content, err := f.Get(NonExistingFileName) + as.Empty(content) + as.Error(err, "not included") +} + func TestFileGlob(t *testing.T) { as := assert.New(t) @@ -63,18 +80,27 @@ func TestFileGlob(t *testing.T) { matched := f.Glob("story/**") as.Len(matched, 2, "Should be two files in glob story/**") - as.Equal("Joseph Conrad", matched.Get("story/author.txt")) + + content, err := matched.Get("story/author.txt") + as.Equal("Joseph Conrad", content) + as.NoError(err) } func TestToConfig(t *testing.T) { as := assert.New(t) f := getTestFiles() - out := f.Glob("**/captain.txt").AsConfig() + out, err := f.Glob("**/captain.txt").AsConfig() as.Equal("captain.txt: The Captain", out) + as.NoError(err) - out = f.Glob("ship/**").AsConfig() + out, err = f.Glob("ship/**").AsConfig() as.Equal("captain.txt: The Captain\nstowaway.txt: Legatt", out) + as.NoError(err) + + out, err = f.Glob(NonExistingFileName).AsConfig() + as.Empty(out) + as.Error(err, "must pass files") } func TestToSecret(t *testing.T) { @@ -82,8 +108,13 @@ func TestToSecret(t *testing.T) { f := getTestFiles() - out := f.Glob("ship/**").AsSecrets() + out, err := f.Glob("ship/**").AsSecrets() as.Equal("captain.txt: VGhlIENhcHRhaW4=\nstowaway.txt: TGVnYXR0", out) + as.NoError(err) + + out, err = f.Glob(NonExistingFileName).AsSecrets() + as.Empty(out) + as.Errorf(err, "must pass files") } func TestLines(t *testing.T) { @@ -91,8 +122,12 @@ func TestLines(t *testing.T) { f := getTestFiles() - out := f.Lines("multiline/test.txt") + out, err := f.Lines("multiline/test.txt") as.Len(out, 2) - as.Equal("bar", out[0]) + as.NoError(err) + + out, err = f.Lines(NonExistingFileName) + as.Nil(out) + as.Error(err, "must pass files") } From 9e12d57680db75752d5863e3cd67eaf6d2e3381c Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Sat, 28 Aug 2021 13:55:51 +0300 Subject: [PATCH 06/12] Move loadExternalPaths to pkg/action and implement tests for install and upgrade Signed-off-by: itaispiegel --- cmd/helm/install.go | 34 --------- cmd/helm/upgrade.go | 5 -- pkg/action/action_test.go | 12 ++++ pkg/action/install.go | 36 ++++++++++ pkg/action/install_test.go | 96 ++++++++++++++++++++++++++ pkg/action/testdata/files/external.txt | 1 + pkg/action/upgrade.go | 4 ++ pkg/action/upgrade_test.go | 93 +++++++++++++++++++++++++ 8 files changed, 242 insertions(+), 39 deletions(-) create mode 100644 pkg/action/testdata/files/external.txt diff --git a/cmd/helm/install.go b/cmd/helm/install.go index b287b1f72..d4a768e15 100644 --- a/cmd/helm/install.go +++ b/cmd/helm/install.go @@ -20,11 +20,9 @@ import ( "context" "fmt" "io" - "io/ioutil" "log" "os" "os/signal" - "strings" "syscall" "time" @@ -261,11 +259,6 @@ func runInstall(args []string, client *action.Install, valueOpts *values.Options } } - err = loadExternalPaths(chartRequested, client.ExternalPaths) - if err != nil { - return nil, err - } - client.Namespace = settings.Namespace() // Create context and prepare the handle of SIGTERM @@ -297,33 +290,6 @@ func checkIfInstallable(ch *chart.Chart) error { return errors.Errorf("%s charts are not installable", ch.Metadata.Type) } -func loadExternalPaths(ch *chart.Chart, externalPaths []string) error { - var errs []string - - for _, path := range externalPaths { - allPaths, err := loader.ExpandFilePath(path) - debug(fmt.Sprintf("%s expanded to: %v", path, allPaths)) - if err != nil { - debug(fmt.Sprintf("error loading external path %s: %v", path, err)) - errs = append(errs, fmt.Sprintf("%s (path not accessible)", path)) - } - - for _, name := range allPaths { - fileContentBytes, err := ioutil.ReadFile(name) - if err != nil { - errs = append(errs, fmt.Sprintf("%s (not readable)", name)) - } else { - ch.Files = append(ch.Files, &chart.File{Name: name, Data: fileContentBytes}) - } - } - } - - if len(errs) > 0 { - return errors.New(fmt.Sprint("Failed to load external paths: ", strings.Join(errs, "; "))) - } - return nil -} - // Provide dynamic auto-completion for the install and template commands func compInstall(args []string, toComplete string, client *action.Install) ([]string, cobra.ShellCompDirective) { requiredArgs := 1 diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index d1ce7624e..72f29c637 100644 --- a/cmd/helm/upgrade.go +++ b/cmd/helm/upgrade.go @@ -182,11 +182,6 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { warning("This chart is deprecated") } - err = loadExternalPaths(ch, client.ExternalPaths) - if err != nil { - return err - } - // Create context and prepare the handle of SIGTERM ctx := context.Background() ctx, cancel := context.WithCancel(ctx) diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index c816c84af..f0c8a0c51 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -17,6 +17,7 @@ package action import ( "flag" + "fmt" "io/ioutil" "testing" @@ -32,6 +33,8 @@ import ( "helm.sh/helm/v3/pkg/time" ) +const withExternalPathsTemplatePath = "templates/with-external-paths" + var verbose = flag.Bool("test.log", false, "enable test logging") func actionConfigFixture(t *testing.T) *Configuration { @@ -209,6 +212,15 @@ func withSampleIncludingIncorrectTemplates() chartOption { } } +func withExternalFileTemplate(externalPath string) chartOption { + return func(opts *chartOptions) { + externalFilesTemplates := []*chart.File{ + {Name: withExternalPathsTemplatePath, Data: []byte(fmt.Sprintf(`data: {{ .Files.Get "%s" }}`, externalPath))}, + } + opts.Templates = append(opts.Templates, externalFilesTemplates...) + } +} + func withMultipleManifestTemplate() chartOption { return func(opts *chartOptions) { sampleTemplates := []*chart.File{ diff --git a/pkg/action/install.go b/pkg/action/install.go index e16e69aeb..10291d3e7 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -177,6 +177,38 @@ func (i *Install) installCRDs(crds []chart.CRD) error { return nil } +func loadExternalPaths(ch *chart.Chart, externalPaths []string) error { + var errs []string + + for _, p := range externalPaths { + allPaths, err := expandFilePath(p) + if err != nil { + errs = append(errs, fmt.Sprintf("%s (path not accessible)", p)) + } + + for _, currentPath := range allPaths { + fileContentBytes, err := os.ReadFile(currentPath) + if err != nil { + errs = append(errs, fmt.Sprintf("%s (not readable)", currentPath)) + continue + } + + newFile := chart.File{Name: currentPath, Data: fileContentBytes} + for _, file := range ch.Files { + if file.Name == newFile.Name && bytes.Equal(file.Data, newFile.Data) { + continue + } + } + ch.Files = append(ch.Files, &newFile) + } + } + + if len(errs) > 0 { + return errors.New(fmt.Sprint("Failed to load external paths: ", strings.Join(errs, "; "))) + } + return nil +} + // Run executes the installation // // If DryRun is set to true, this will prepare the release, but not install it @@ -188,6 +220,10 @@ func (i *Install) Run(chrt *chart.Chart, vals map[string]interface{}) (*release. // Run executes the installation with Context func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals map[string]interface{}) (*release.Release, error) { + if err := loadExternalPaths(chrt, i.ExternalPaths); err != nil { + return nil, err + } + // Check reachability of cluster unless in client-only mode (e.g. `helm template` without `--validate`) if !i.ClientOnly { if err := i.cfg.KubeClient.IsReachable(); err != nil { diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 45e5a2670..1fde091a3 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -39,12 +39,20 @@ import ( helmtime "helm.sh/helm/v3/pkg/time" ) +const ExternalFileRelPath = "testdata/files/external.txt" + type nameTemplateTestCase struct { tpl string expected string expectedErrorStr string } +type includeExternalPathTestCase struct { + Name string + IncludedFilePath string + ExternalPath string +} + func installAction(t *testing.T) *Install { config := actionConfigFixture(t) instAction := NewInstall(config) @@ -54,6 +62,11 @@ func installAction(t *testing.T) *Install { return instAction } +func getAbsPath(path string) string { + absPath, _ := filepath.Abs(path) + return absPath +} + func TestInstallRelease(t *testing.T) { is := assert.New(t) req := require.New(t) @@ -717,3 +730,86 @@ func TestNameAndChartGenerateName(t *testing.T) { }) } } + +func TestInstallFailsWhenWrongPathsIncluded(t *testing.T) { + is := assert.New(t) + vals := map[string]interface{}{} + + tests := []includeExternalPathTestCase{ + { + Name: "included paths not passed", + IncludedFilePath: "", + ExternalPath: ExternalFileRelPath, + }, + { + Name: "absolute path of file is included and external file is relative", + IncludedFilePath: getAbsPath(ExternalFileRelPath), + ExternalPath: ExternalFileRelPath, + }, + { + Name: "relative path of file is included and external file is absolute", + IncludedFilePath: ExternalFileRelPath, + ExternalPath: getAbsPath(ExternalFileRelPath), + }, + { + Name: "absolute path of directory is included and external file is relative", + IncludedFilePath: getAbsPath("testdata/files"), + ExternalPath: ExternalFileRelPath, + }, + { + Name: "relative path of directory is included and external file is absolute", + IncludedFilePath: "testdata/files", + ExternalPath: getAbsPath(ExternalFileRelPath), + }, + } + + for _, tc := range tests { + t.Run(tc.Name, func(t *testing.T) { + instAction := installAction(t) + instAction.ExternalPaths = append(instAction.ExternalPaths, tc.IncludedFilePath) + + _, err := instAction.Run(buildChart(withExternalFileTemplate(tc.ExternalPath)), vals) + expectedErr := fmt.Sprintf("<.Files.Get>: error calling Get: file %s not included", tc.ExternalPath) + is.Error(err, expectedErr) + }) + } +} + +func TestInstallWhenIncludePathsPassed(t *testing.T) { + is := assert.New(t) + vals := map[string]interface{}{} + + tests := []includeExternalPathTestCase{ + { + Name: "relative path of file is included and external file is relative", + IncludedFilePath: ExternalFileRelPath, + ExternalPath: ExternalFileRelPath, + }, + { + Name: "absolute path of file is included and external file is absolute", + IncludedFilePath: getAbsPath(ExternalFileRelPath), + ExternalPath: getAbsPath(ExternalFileRelPath), + }, + { + Name: "relative path of directory is included and external file is relative", + IncludedFilePath: "testdata/files", + ExternalPath: ExternalFileRelPath, + }, + { + Name: "absolute path of directory is included and external file is absolute", + IncludedFilePath: getAbsPath("testdata/files"), + ExternalPath: getAbsPath(ExternalFileRelPath), + }, + } + + for _, tc := range tests { + t.Run(tc.Name, func(t *testing.T) { + instAction := installAction(t) + instAction.ExternalPaths = append(instAction.ExternalPaths, tc.IncludedFilePath) + + installRelease, err := instAction.Run(buildChart(withExternalFileTemplate(tc.ExternalPath)), vals) + is.Contains(installRelease.Manifest, "out-of-chart-dir") + is.NoError(err) + }) + } +} diff --git a/pkg/action/testdata/files/external.txt b/pkg/action/testdata/files/external.txt new file mode 100644 index 000000000..068c4db58 --- /dev/null +++ b/pkg/action/testdata/files/external.txt @@ -0,0 +1 @@ +out-of-chart-dir diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 2826076d2..17defcd1e 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -180,6 +180,10 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin return nil, nil, err } + if err := loadExternalPaths(chart, u.ExternalPaths); err != nil { + return nil, nil, err + } + // Concurrent `helm upgrade`s will either fail here with `errPending` or when creating the release with "already exists". This should act as a pessimistic lock. if lastRelease.Info.Status.IsPending() { return nil, nil, errPending diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index 62922b373..b64145f51 100644 --- a/pkg/action/upgrade_test.go +++ b/pkg/action/upgrade_test.go @@ -388,3 +388,96 @@ func TestUpgradeRelease_Interrupted_Atomic(t *testing.T) { is.Equal(updatedRes.Info.Status, release.StatusDeployed) } + +func TestUpgradeFailsWhenWrongPathsIncluded(t *testing.T) { + is := assert.New(t) + vals := map[string]interface{}{} + + tests := []includeExternalPathTestCase{ + { + Name: "included paths not passed", + IncludedFilePath: "", + ExternalPath: "testdata/files/external.txt", + }, + { + Name: "absolute path of file is included and external file is relative", + IncludedFilePath: getAbsPath("testdata/files/external.txt"), + ExternalPath: "testdata/files/external.txt", + }, + { + Name: "relative path of file is included and external file is absolute", + IncludedFilePath: "testdata/files/external.txt", + ExternalPath: getAbsPath("testdata/files/external.txt"), + }, + { + Name: "absolute path of directory is included and external file is relative", + IncludedFilePath: getAbsPath("testdata/files"), + ExternalPath: "testdata/files/external.txt", + }, + { + Name: "relative path of directory is included and external file is absolute", + IncludedFilePath: "testdata/files", + ExternalPath: getAbsPath("testdata/files/external.txt"), + }, + } + + for _, tc := range tests { + t.Run(tc.Name, func(t *testing.T) { + upAction := upgradeAction(t) + upAction.ExternalPaths = append(upAction.ExternalPaths, tc.IncludedFilePath) + + rel := releaseStub() + rel.Name = "test" + rel.Info.Status = release.StatusDeployed + upAction.cfg.Releases.Create(rel) + + _, err := upAction.Run(rel.Name, buildChart(withExternalFileTemplate(tc.ExternalPath)), vals) + expectedErr := fmt.Sprintf("<.Files.Get>: error calling Get: file %s not included", tc.ExternalPath) + is.Error(err, expectedErr) + }) + } +} + +func TestUpgradeWhenIncludePathsPassed(t *testing.T) { + is := assert.New(t) + vals := map[string]interface{}{} + + tests := []includeExternalPathTestCase{ + { + Name: "relative path of file is included and external file is relative", + IncludedFilePath: "testdata/files/external.txt", + ExternalPath: "testdata/files/external.txt", + }, + { + Name: "relative path of file is included and external file is absolute", + IncludedFilePath: getAbsPath("testdata/files/external.txt"), + ExternalPath: getAbsPath("testdata/files/external.txt"), + }, + { + Name: "relative path of directory is included and external file is relative", + IncludedFilePath: "testdata/files", + ExternalPath: "testdata/files/external.txt", + }, + { + Name: "absolute path of directory is included and external file is absolute", + IncludedFilePath: getAbsPath("testdata/files"), + ExternalPath: getAbsPath("testdata/files/external.txt"), + }, + } + + for _, tc := range tests { + t.Run(tc.Name, func(t *testing.T) { + upAction := upgradeAction(t) + upAction.ExternalPaths = append(upAction.ExternalPaths, tc.IncludedFilePath) + + rel := releaseStub() + rel.Name = "test" + rel.Info.Status = release.StatusDeployed + upAction.cfg.Releases.Create(rel) + + upgradeRelease, err := upAction.Run(rel.Name, buildChart(withExternalFileTemplate(tc.ExternalPath)), vals) + is.Contains(upgradeRelease.Manifest, "out-of-chart-dir") + is.NoError(err) + }) + } +} From 4a3deb883501b5b63308093bb92b3724a5ef8e37 Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Sat, 28 Aug 2021 17:13:21 +0300 Subject: [PATCH 07/12] Add test for rollback to release with external file Signed-off-by: itaispiegel --- pkg/action/rollback_test.go | 63 +++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 pkg/action/rollback_test.go diff --git a/pkg/action/rollback_test.go b/pkg/action/rollback_test.go new file mode 100644 index 000000000..1fa705718 --- /dev/null +++ b/pkg/action/rollback_test.go @@ -0,0 +1,63 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package action + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func rollbackAction(t *testing.T) *Rollback { + config := actionConfigFixture(t) + rollbackAction := NewRollback(config) + + return rollbackAction +} + +func TestRollbackToReleaseWithExternalFile(t *testing.T) { + is := assert.New(t) + vals := map[string]interface{}{} + + chartVersion1 := buildChart(withExternalFileTemplate(ExternalFileRelPath)) + chartVersion2 := buildChart() + + instAction := installAction(t) + instAction.ExternalPaths = append(instAction.ExternalPaths, ExternalFileRelPath) + relVersion1, err := instAction.Run(chartVersion1, vals) + is.Contains(relVersion1.Manifest, "out-of-chart-dir") + is.NoError(err) + + upAction := upgradeAction(t) + err = upAction.cfg.Releases.Create(relVersion1) + is.NoError(err) + relVersion2, err := upAction.Run(relVersion1.Name, chartVersion2, vals) + is.NotContains(relVersion2.Manifest, "out-out-chart-dir") + is.NoError(err) + + rollAction := rollbackAction(t) + err = rollAction.cfg.Releases.Create(relVersion1) + is.NoError(err) + err = rollAction.cfg.Releases.Create(relVersion2) + is.NoError(err) + currentRelease, targetRelease, err := rollAction.prepareRollback(relVersion2.Name) + is.NoError(err) + relVersion3, err := rollAction.performRollback(currentRelease, targetRelease) + is.NoError(err) + + is.Contains(relVersion3.Manifest, "out-of-chart-dir") +} From 0061f66d3794e45b2be20f9fd1ece3569c1d13f7 Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Mon, 30 Aug 2021 22:56:26 +0300 Subject: [PATCH 08/12] Print warning instead of fail when accessing non-included file Signed-off-by: itaispiegel --- pkg/action/install_test.go | 25 ++++++++++++++++++++----- pkg/action/upgrade_test.go | 24 ++++++++++++++++++++---- pkg/engine/files.go | 18 ++++++++---------- pkg/engine/files_test.go | 24 ++++++++++++++++-------- 4 files changed, 64 insertions(+), 27 deletions(-) diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index 1fde091a3..f5e842076 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -17,9 +17,11 @@ limitations under the License. package action import ( + "bytes" "context" "fmt" "io/ioutil" + "log" "os" "path/filepath" "regexp" @@ -731,7 +733,7 @@ func TestNameAndChartGenerateName(t *testing.T) { } } -func TestInstallFailsWhenWrongPathsIncluded(t *testing.T) { +func TestInstallUsesEmptyContentWrongPathsIncluded(t *testing.T) { is := assert.New(t) vals := map[string]interface{}{} @@ -763,14 +765,27 @@ func TestInstallFailsWhenWrongPathsIncluded(t *testing.T) { }, } + var buf bytes.Buffer + log.SetOutput(&buf) + defer func() { + log.SetOutput(os.Stderr) + }() + for _, tc := range tests { t.Run(tc.Name, func(t *testing.T) { instAction := installAction(t) - instAction.ExternalPaths = append(instAction.ExternalPaths, tc.IncludedFilePath) + if tc.IncludedFilePath != "" { + instAction.ExternalPaths = append(instAction.ExternalPaths, tc.IncludedFilePath) + } - _, err := instAction.Run(buildChart(withExternalFileTemplate(tc.ExternalPath)), vals) - expectedErr := fmt.Sprintf("<.Files.Get>: error calling Get: file %s not included", tc.ExternalPath) - is.Error(err, expectedErr) + rel, err := instAction.Run(buildChart(withExternalFileTemplate(tc.ExternalPath)), vals) + is.Contains(buf.String(), "not included") + is.NoError(err) + is.Equal( + rel.Manifest, + "---\n# Source: hello/templates/hello\nhello: world\n---\n# Source: hello/templates/with-external-paths\ndata:\n", + ) + buf.Reset() }) } } diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index b64145f51..6ef058555 100644 --- a/pkg/action/upgrade_test.go +++ b/pkg/action/upgrade_test.go @@ -17,8 +17,11 @@ limitations under the License. package action import ( + "bytes" "context" "fmt" + "log" + "os" "testing" "time" @@ -421,19 +424,32 @@ func TestUpgradeFailsWhenWrongPathsIncluded(t *testing.T) { }, } + var buf bytes.Buffer + log.SetOutput(&buf) + defer func() { + log.SetOutput(os.Stderr) + }() + for _, tc := range tests { t.Run(tc.Name, func(t *testing.T) { upAction := upgradeAction(t) - upAction.ExternalPaths = append(upAction.ExternalPaths, tc.IncludedFilePath) + if tc.IncludedFilePath != "" { + upAction.ExternalPaths = append(upAction.ExternalPaths, tc.IncludedFilePath) + } rel := releaseStub() rel.Name = "test" rel.Info.Status = release.StatusDeployed upAction.cfg.Releases.Create(rel) - _, err := upAction.Run(rel.Name, buildChart(withExternalFileTemplate(tc.ExternalPath)), vals) - expectedErr := fmt.Sprintf("<.Files.Get>: error calling Get: file %s not included", tc.ExternalPath) - is.Error(err, expectedErr) + rel, err := upAction.Run(rel.Name, buildChart(withExternalFileTemplate(tc.ExternalPath)), vals) + is.Contains(buf.String(), "not included") + is.NoError(err) + is.Equal( + rel.Manifest, + "---\n# Source: hello/templates/hello\nhello: world\n---\n# Source: hello/templates/with-external-paths\ndata:\n", + ) + buf.Reset() }) } } diff --git a/pkg/engine/files.go b/pkg/engine/files.go index ed9ad757e..cd45d7a07 100644 --- a/pkg/engine/files.go +++ b/pkg/engine/files.go @@ -19,6 +19,7 @@ package engine import ( "encoding/base64" "fmt" + "log" "path" "strings" @@ -47,11 +48,12 @@ func newFiles(from []*chart.File) files { // // This is intended to be accessed from within a template, so a missed key returns // an empty []byte. -func (f files) GetBytes(name string) ([]byte, error) { +func (f files) GetBytes(name string) []byte { if v, ok := f[name]; ok { - return v, nil + return v } - return []byte{}, fmt.Errorf("file %s not included", name) + log.Printf("WARNING: %s not included", name) + return nil } // Get returns a string representation of the given file. @@ -60,13 +62,9 @@ func (f files) GetBytes(name string) ([]byte, error) { // template. // // {{.Files.Get "foo"}} -func (f files) Get(name string) (string, error) { - content, err := f.GetBytes(name) - if err != nil { - return "", err - } - - return string(content), nil +func (f files) Get(name string) string { + content := f.GetBytes(name) + return string(content) } // Glob takes a glob pattern and returns another files object only containing diff --git a/pkg/engine/files_test.go b/pkg/engine/files_test.go index debb3089e..17142d25f 100644 --- a/pkg/engine/files_test.go +++ b/pkg/engine/files_test.go @@ -16,6 +16,9 @@ limitations under the License. package engine import ( + "bytes" + "log" + "os" "testing" "github.com/stretchr/testify/assert" @@ -48,15 +51,15 @@ func TestNewFiles(t *testing.T) { } for i, f := range cases { - gotBytes, err := files.GetBytes(f.path) + gotBytes := files.GetBytes(f.path) got := string(gotBytes) - if err != nil || got != f.data { + if got != f.data { t.Errorf("%d: expected %q, got %q", i, f.data, got) } - gotBytes, err = files.GetBytes(f.path) + gotBytes = files.GetBytes(f.path) got = string(gotBytes) - if err != nil || got != f.data { + if got != f.data { t.Errorf("%d: expected %q, got %q", i, f.data, got) } } @@ -67,9 +70,15 @@ func TestGetNonExistingFile(t *testing.T) { f := getTestFiles() - content, err := f.Get(NonExistingFileName) + var buf bytes.Buffer + log.SetOutput(&buf) + defer func() { + log.SetOutput(os.Stderr) + }() + + content := f.Get(NonExistingFileName) as.Empty(content) - as.Error(err, "not included") + as.Contains(buf.String(), "not included") } func TestFileGlob(t *testing.T) { @@ -81,9 +90,8 @@ func TestFileGlob(t *testing.T) { as.Len(matched, 2, "Should be two files in glob story/**") - content, err := matched.Get("story/author.txt") + content := matched.Get("story/author.txt") as.Equal("Joseph Conrad", content) - as.NoError(err) } func TestToConfig(t *testing.T) { From b0c802439b9b715c56fd9bfc4381888c18cd24cb Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Fri, 22 Jul 2022 20:41:46 +0300 Subject: [PATCH 09/12] Fix broken compatibility Signed-off-by: itaispiegel --- pkg/engine/files.go | 26 +++++++++++++----------- pkg/engine/files_test.go | 43 +++++++++++++++++++++++++++------------- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/pkg/engine/files.go b/pkg/engine/files.go index cd45d7a07..427664850 100644 --- a/pkg/engine/files.go +++ b/pkg/engine/files.go @@ -18,7 +18,6 @@ package engine import ( "encoding/base64" - "fmt" "log" "path" "strings" @@ -105,9 +104,10 @@ func (f files) Glob(pattern string) files { // // data: // {{ .Files.Glob("config/**").AsConfig() | indent 4 }} -func (f files) AsConfig() (string, error) { - if f == nil || len(f) == 0 { - return "", fmt.Errorf("must pass files") +func (f files) AsConfig() string { + if len(f) == 0 { + log.Printf("must pass files") + return "" } m := make(map[string]string) @@ -117,7 +117,7 @@ func (f files) AsConfig() (string, error) { m[path.Base(k)] = string(v) } - return toYAML(m), nil + return toYAML(m) } // AsSecrets returns the base64-encoded value of a Files object suitable for @@ -134,9 +134,10 @@ func (f files) AsConfig() (string, error) { // // data: // {{ .Files.Glob("secrets/*").AsSecrets() }} -func (f files) AsSecrets() (string, error) { - if f == nil || len(f) == 0 { - return "", fmt.Errorf("must pass files") +func (f files) AsSecrets() string { + if len(f) == 0 { + log.Printf("must pass files") + return "" } m := make(map[string]string) @@ -145,7 +146,7 @@ func (f files) AsSecrets() (string, error) { m[path.Base(k)] = base64.StdEncoding.EncodeToString(v) } - return toYAML(m), nil + return toYAML(m) } // Lines returns each line of a named file (split by "\n") as a slice, so it can @@ -155,10 +156,11 @@ func (f files) AsSecrets() (string, error) { // // {{ range .Files.Lines "foo/bar.html" }} // {{ . }}{{ end }} -func (f files) Lines(path string) ([]string, error) { +func (f files) Lines(path string) []string { if f == nil || f[path] == nil { - return nil, fmt.Errorf("must pass files") + log.Printf("must pass files") + return nil } - return strings.Split(string(f[path]), "\n"), nil + return strings.Split(string(f[path]), "\n") } diff --git a/pkg/engine/files_test.go b/pkg/engine/files_test.go index 17142d25f..80a71478f 100644 --- a/pkg/engine/files_test.go +++ b/pkg/engine/files_test.go @@ -98,17 +98,22 @@ func TestToConfig(t *testing.T) { as := assert.New(t) f := getTestFiles() - out, err := f.Glob("**/captain.txt").AsConfig() + + var buf bytes.Buffer + log.SetOutput(&buf) + defer func() { + log.SetOutput(os.Stderr) + }() + + out := f.Glob("**/captain.txt").AsConfig() as.Equal("captain.txt: The Captain", out) - as.NoError(err) - out, err = f.Glob("ship/**").AsConfig() + out = f.Glob("ship/**").AsConfig() as.Equal("captain.txt: The Captain\nstowaway.txt: Legatt", out) - as.NoError(err) - out, err = f.Glob(NonExistingFileName).AsConfig() + out = f.Glob(NonExistingFileName).AsConfig() as.Empty(out) - as.Error(err, "must pass files") + as.Contains(buf.String(), "must pass files") } func TestToSecret(t *testing.T) { @@ -116,13 +121,18 @@ func TestToSecret(t *testing.T) { f := getTestFiles() - out, err := f.Glob("ship/**").AsSecrets() + var buf bytes.Buffer + log.SetOutput(&buf) + defer func() { + log.SetOutput(os.Stderr) + }() + + out := f.Glob("ship/**").AsSecrets() as.Equal("captain.txt: VGhlIENhcHRhaW4=\nstowaway.txt: TGVnYXR0", out) - as.NoError(err) - out, err = f.Glob(NonExistingFileName).AsSecrets() + out = f.Glob(NonExistingFileName).AsSecrets() as.Empty(out) - as.Errorf(err, "must pass files") + as.Contains(buf.String(), "must pass files") } func TestLines(t *testing.T) { @@ -130,12 +140,17 @@ func TestLines(t *testing.T) { f := getTestFiles() - out, err := f.Lines("multiline/test.txt") + var buf bytes.Buffer + log.SetOutput(&buf) + defer func() { + log.SetOutput(os.Stderr) + }() + + out := f.Lines("multiline/test.txt") as.Len(out, 2) as.Equal("bar", out[0]) - as.NoError(err) - out, err = f.Lines(NonExistingFileName) + out = f.Lines(NonExistingFileName) as.Nil(out) - as.Error(err, "must pass files") + as.Contains(buf.String(), "must pass files") } From df457bf826a7f1e9c823fa756787337da7d10ac1 Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Thu, 22 Dec 2022 23:27:35 +0200 Subject: [PATCH 10/12] Move expandFilePath to pkg/action Signed-off-by: itaispiegel --- pkg/{chart/loader => action}/local.go | 6 +++--- pkg/{chart/loader => action}/local_test.go | 18 ++++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) rename pkg/{chart/loader => action}/local.go (92%) rename pkg/{chart/loader => action}/local_test.go (60%) diff --git a/pkg/chart/loader/local.go b/pkg/action/local.go similarity index 92% rename from pkg/chart/loader/local.go rename to pkg/action/local.go index 38f080827..21d6c6ca4 100644 --- a/pkg/chart/loader/local.go +++ b/pkg/action/local.go @@ -11,7 +11,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package loader +package action import ( "errors" @@ -20,8 +20,8 @@ import ( "strings" ) -// ExpandFilePath expands a local file, dir or glob path to a list of files -func ExpandFilePath(path string) ([]string, error) { +// expandFilePath expands a local file, dir or glob path to a list of files +func expandFilePath(path string) ([]string, error) { if strings.Contains(path, "*") { // if this is a glob, we expand it and return a list of files return expandGlob(path) diff --git a/pkg/chart/loader/local_test.go b/pkg/action/local_test.go similarity index 60% rename from pkg/chart/loader/local_test.go rename to pkg/action/local_test.go index 456d0dddf..29ac7c107 100644 --- a/pkg/chart/loader/local_test.go +++ b/pkg/action/local_test.go @@ -11,7 +11,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package loader +package action import ( "testing" @@ -22,17 +22,15 @@ import ( func TestExpandLocalPath(t *testing.T) { req := require.New(t) - glob, err := ExpandFilePath("testdata/frobnitz/*.yaml") + glob, err := expandFilePath("testdata/output/*.txt") req.NoError(err) - req.Contains(glob, "testdata/frobnitz/Chart.yaml") - req.Contains(glob, "testdata/frobnitz/values.yaml") + req.Contains(glob, "testdata/output/list-compressed-deps.txt") + req.Contains(glob, "testdata/output/list-missing-deps.txt") - dir, err := ExpandFilePath("testdata/albatross/") + dir, err := expandFilePath("testdata/files/") req.NoError(err) - req.Contains(dir, "testdata/albatross/Chart.yaml") - req.Contains(dir, "testdata/albatross/values.yaml") + req.Contains(dir, "testdata/files/external.txt") - file, err := ExpandFilePath("testdata/albatross/Chart.yaml") - req.NoError(err) - req.Contains(file, "testdata/albatross/Chart.yaml") + _, err = expandFilePath("testdata/non_existing") + req.Error(err) } From b8df6fda7f26915aaa86f9fef5d1efdd2abd0c22 Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Thu, 22 Dec 2022 23:34:29 +0200 Subject: [PATCH 11/12] Revert: actionConfig initialization in helm.go Signed-off-by: itaispiegel --- cmd/helm/helm.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cmd/helm/helm.go b/cmd/helm/helm.go index a98af62e4..15b0d5c76 100644 --- a/cmd/helm/helm.go +++ b/cmd/helm/helm.go @@ -80,11 +80,6 @@ func main() { } }) - if err := actionConfig.Init(settings.RESTClientGetter(), settings.Namespace(), os.Getenv("HELM_DRIVER"), debug); err != nil { - debug("%+v", err) - os.Exit(1) - } - if err := cmd.Execute(); err != nil { debug("%+v", err) switch e := err.(type) { From 24f643364de7fc8dc0a294b74febe85a105f979f Mon Sep 17 00:00:00 2001 From: itaispiegel Date: Sun, 8 Jan 2023 11:03:31 +0200 Subject: [PATCH 12/12] Fix chart with template with external files tests Signed-off-by: itaispiegel --- cmd/helm/testdata/output/template-with-external-dir.txt | 4 ++-- cmd/helm/testdata/output/template-with-external-file.txt | 4 ++-- cmd/helm/testdata/output/template-with-external-glob.txt | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cmd/helm/testdata/output/template-with-external-dir.txt b/cmd/helm/testdata/output/template-with-external-dir.txt index d12220663..469ff133f 100644 --- a/cmd/helm/testdata/output/template-with-external-dir.txt +++ b/cmd/helm/testdata/output/template-with-external-dir.txt @@ -3,7 +3,7 @@ apiVersion: v1 kind: ConfigMap metadata: - name: "RELEASE-NAME-" + name: "release-name-" labels: # The "app.kubernetes.io/managed-by" label is used to track which tool # deployed a given chart. It is useful for admins who want to see what @@ -12,7 +12,7 @@ metadata: # The "app.kubernetes.io/instance" convention makes it easy to tie a release # to all of the Kubernetes resources that were created as part of that # release. - app.kubernetes.io/instance: "RELEASE-NAME" + app.kubernetes.io/instance: "release-name" app.kubernetes.io/version: 1.0 # This makes it easy to audit chart usage. helm.sh/chart: "configmap-0.1.0" diff --git a/cmd/helm/testdata/output/template-with-external-file.txt b/cmd/helm/testdata/output/template-with-external-file.txt index fcbdad5fe..047b30e2d 100644 --- a/cmd/helm/testdata/output/template-with-external-file.txt +++ b/cmd/helm/testdata/output/template-with-external-file.txt @@ -3,7 +3,7 @@ apiVersion: v1 kind: ConfigMap metadata: - name: "RELEASE-NAME-" + name: "release-name-" labels: # The "app.kubernetes.io/managed-by" label is used to track which tool # deployed a given chart. It is useful for admins who want to see what @@ -12,7 +12,7 @@ metadata: # The "app.kubernetes.io/instance" convention makes it easy to tie a release # to all of the Kubernetes resources that were created as part of that # release. - app.kubernetes.io/instance: "RELEASE-NAME" + app.kubernetes.io/instance: "release-name" app.kubernetes.io/version: 1.0 # This makes it easy to audit chart usage. helm.sh/chart: "configmap-0.1.0" diff --git a/cmd/helm/testdata/output/template-with-external-glob.txt b/cmd/helm/testdata/output/template-with-external-glob.txt index 5d663af0d..026cd0f7e 100644 --- a/cmd/helm/testdata/output/template-with-external-glob.txt +++ b/cmd/helm/testdata/output/template-with-external-glob.txt @@ -3,7 +3,7 @@ apiVersion: v1 kind: ConfigMap metadata: - name: "RELEASE-NAME-" + name: "release-name-" labels: # The "app.kubernetes.io/managed-by" label is used to track which tool # deployed a given chart. It is useful for admins who want to see what @@ -12,7 +12,7 @@ metadata: # The "app.kubernetes.io/instance" convention makes it easy to tie a release # to all of the Kubernetes resources that were created as part of that # release. - app.kubernetes.io/instance: "RELEASE-NAME" + app.kubernetes.io/instance: "release-name" app.kubernetes.io/version: 1.0 # This makes it easy to audit chart usage. helm.sh/chart: "configmap-0.1.0"