From 4e485219d7cf076052ceb1efef00fd56149992d2 Mon Sep 17 00:00:00 2001 From: Wi1dcard Date: Thu, 19 Nov 2020 08:29:59 +0800 Subject: [PATCH] Fix the logic of helmfile deps and add tests. (#1588) --- pkg/app/app.go | 44 ++++++-- pkg/app/app_test.go | 153 +++++++++++++++++++++++++++ pkg/app/desired_state_file_loader.go | 18 ++-- pkg/remote/remote_test.go | 25 ++--- pkg/state/create.go | 36 ++++--- pkg/state/create_test.go | 9 +- pkg/state/state.go | 32 +++--- pkg/state/state_test.go | 65 ++++++++---- pkg/testhelper/testfs.go | 10 +- 9 files changed, 298 insertions(+), 94 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index 26a18e87..64eb8b45 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -105,6 +105,7 @@ func (a *App) Deps(c DepsConfigProvider) error { return a.ForEachState(func(run *Run) (_ bool, errs []error) { prepErr := run.withPreparedCharts("deps", state.ChartPrepareOptions{ SkipRepos: c.SkipRepos(), + SkipDeps: true, SkipResolve: true, }, func() { errs = run.Deps(c) @@ -122,7 +123,10 @@ func (a *App) Repos(c ReposConfigProvider) error { return a.ForEachState(func(run *Run) (_ bool, errs []error) { var reposErr error - err := run.withPreparedCharts("repos", state.ChartPrepareOptions{SkipRepos: true}, func() { + err := run.withPreparedCharts("repos", state.ChartPrepareOptions{ + SkipRepos: true, + SkipDeps: true, + }, func() { reposErr = run.Repos(c) }) @@ -140,7 +144,10 @@ func (a *App) Repos(c ReposConfigProvider) error { func (a *App) DeprecatedSyncCharts(c DeprecatedChartsConfigProvider) error { return a.ForEachState(func(run *Run) (_ bool, errs []error) { - err := run.withPreparedCharts("charts", state.ChartPrepareOptions{SkipRepos: true}, func() { + err := run.withPreparedCharts("charts", state.ChartPrepareOptions{ + SkipRepos: true, + SkipDeps: true, + }, func() { errs = run.DeprecatedSyncCharts(c) }) @@ -166,7 +173,10 @@ func (a *App) Diff(c DiffConfigProvider) error { var errs []error - prepErr := run.withPreparedCharts("diff", state.ChartPrepareOptions{SkipRepos: c.SkipDeps()}, func() { + prepErr := run.withPreparedCharts("diff", state.ChartPrepareOptions{ + SkipRepos: c.SkipDeps(), + SkipDeps: c.SkipDeps(), + }, func() { msg, matched, affected, errs = a.diff(run, c) }) @@ -227,6 +237,7 @@ func (a *App) Template(c TemplateConfigProvider) error { prepErr := run.withPreparedCharts("template", state.ChartPrepareOptions{ ForceDownload: !run.helm.IsHelm3(), SkipRepos: c.SkipDeps(), + SkipDeps: c.SkipDeps(), }, func() { ok, errs = a.template(run, c) }) @@ -246,6 +257,7 @@ func (a *App) WriteValues(c WriteValuesConfigProvider) error { prepErr := run.withPreparedCharts("write-values", state.ChartPrepareOptions{ ForceDownload: !run.helm.IsHelm3(), SkipRepos: c.SkipDeps(), + SkipDeps: c.SkipDeps(), }, func() { ok, errs = a.writeValues(run, c) }) @@ -264,6 +276,7 @@ func (a *App) Lint(c LintConfigProvider) error { prepErr := run.withPreparedCharts("lint", state.ChartPrepareOptions{ ForceDownload: true, SkipRepos: c.SkipDeps(), + SkipDeps: c.SkipDeps(), }, func() { errs = run.Lint(c) }) @@ -280,6 +293,7 @@ func (a *App) Sync(c SyncConfigProvider) error { return a.ForEachState(func(run *Run) (ok bool, errs []error) { prepErr := run.withPreparedCharts("sync", state.ChartPrepareOptions{ SkipRepos: c.SkipDeps(), + SkipDeps: c.SkipDeps(), }, func() { ok, errs = a.sync(run, c) }) @@ -304,6 +318,7 @@ func (a *App) Apply(c ApplyConfigProvider) error { err := a.ForEachState(func(run *Run) (ok bool, errs []error) { prepErr := run.withPreparedCharts("apply", state.ChartPrepareOptions{ SkipRepos: c.SkipDeps(), + SkipDeps: c.SkipDeps(), }, func() { matched, updated, es := a.apply(run, c) @@ -338,6 +353,7 @@ func (a *App) Status(c StatusesConfigProvider) error { return a.ForEachState(func(run *Run) (_ bool, errs []error) { err := run.withPreparedCharts("status", state.ChartPrepareOptions{ SkipRepos: true, + SkipDeps: true, }, func() { errs = run.Status(c) }) @@ -354,6 +370,7 @@ func (a *App) Delete(c DeleteConfigProvider) error { return a.ForEachState(func(run *Run) (ok bool, errs []error) { err := run.withPreparedCharts("delete", state.ChartPrepareOptions{ SkipRepos: true, + SkipDeps: true, }, func() { ok, errs = a.delete(run, c.Purge(), c) }) @@ -370,6 +387,7 @@ func (a *App) Destroy(c DestroyConfigProvider) error { return a.ForEachState(func(run *Run) (ok bool, errs []error) { err := run.withPreparedCharts("destroy", state.ChartPrepareOptions{ SkipRepos: true, + SkipDeps: true, }, func() { ok, errs = a.delete(run, true, c) }) @@ -392,6 +410,7 @@ func (a *App) Test(c TestConfigProvider) error { err := run.withPreparedCharts("test", state.ChartPrepareOptions{ SkipRepos: true, + SkipDeps: true, }, func() { errs = a.test(run, c) }) @@ -408,6 +427,7 @@ func (a *App) PrintState(c StateConfigProvider) error { return a.ForEachState(func(run *Run) (_ bool, errs []error) { err := run.withPreparedCharts("build", state.ChartPrepareOptions{ SkipRepos: true, + SkipDeps: true, }, func() { if c.EmbedValues() { for i := range run.state.Releases { @@ -456,6 +476,7 @@ func (a *App) ListReleases(c ListConfigProvider) error { err := a.ForEachState(func(run *Run) (_ bool, errs []error) { err := run.withPreparedCharts("list", state.ChartPrepareOptions{ SkipRepos: true, + SkipDeps: true, }, func() { //var releases m @@ -587,14 +608,15 @@ func (a *App) loadDesiredStateFromYaml(file string, opts ...LoadOpts) (*state.He } ld := &desiredStateLoader{ - readFile: a.readFile, - deleteFile: a.deleteFile, - fileExists: a.fileExists, - env: a.Env, - namespace: a.Namespace, - logger: a.Logger, - abs: a.abs, - remote: a.remote, + readFile: a.readFile, + deleteFile: a.deleteFile, + fileExists: a.fileExists, + directoryExistsAt: a.directoryExistsAt, + env: a.Env, + namespace: a.Namespace, + logger: a.Logger, + abs: a.abs, + remote: a.remote, overrideKubeContext: a.OverrideKubeContext, overrideHelmBinary: a.OverrideHelmBinary, diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 741c7ae2..13dd3cac 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -2380,6 +2380,18 @@ func (a applyConfig) RetainValuesFiles() bool { return a.retainValuesFiles } +type depsConfig struct { + skipRepos bool +} + +func (d depsConfig) SkipRepos() bool { + return d.skipRepos +} + +func (d depsConfig) Args() string { + return "" +} + // Mocking the command-line runner type mockRunner struct { @@ -3961,6 +3973,147 @@ err: "foo" depends on nonexistent release "bar" } } +func TestDeps(t *testing.T) { + testcases := []struct { + name string + loc string + error string + files map[string]string + log string + charts []string + }{ + // + // complex test cases for smoke testing + // + { + name: "smoke", + loc: location(), + files: map[string]string{ + "/path/to/helmfile.yaml": ` +repositories: +- name: bitnami + url: https://charts.bitnami.com/bitnami/ +releases: +- name: example + chart: /path/to/charts/example +`, + "/path/to/charts/example/Chart.yaml": `foo: FOO`, + }, + log: `processing file "helmfile.yaml" in directory "." +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: repositories: + 2: - name: bitnami + 3: url: https://charts.bitnami.com/bitnami/ + 4: releases: + 5: - name: example + 6: chart: /path/to/charts/example + 7: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: repositories: + 2: - name: bitnami + 3: url: https://charts.bitnami.com/bitnami/ + 4: releases: + 5: - name: example + 6: chart: /path/to/charts/example + 7: + +merged environment: &{default map[] map[]} +There are no repositories defined in your helmfile.yaml. +This means helmfile cannot update your dependencies or create a lock file. +See https://github.com/roboll/helmfile/issues/878 for more information. +`, + charts: []string{"/path/to/charts/example"}, + }, + } + + for i := range testcases { + tc := testcases[i] + t.Run(tc.name, func(t *testing.T) { + + var helm = &exectest.Helm{ + DiffMutex: &sync.Mutex{}, + ChartsMutex: &sync.Mutex{}, + ReleasesMutex: &sync.Mutex{}, + } + + bs := &bytes.Buffer{} + + func() { + logReader, logWriter := io.Pipe() + + logFlushed := &sync.WaitGroup{} + // Ensure all the log is consumed into `bs` by calling `logWriter.Close()` followed by `logFlushed.Wait()` + logFlushed.Add(1) + go func() { + scanner := bufio.NewScanner(logReader) + for scanner.Scan() { + bs.Write(scanner.Bytes()) + bs.WriteString("\n") + } + logFlushed.Done() + }() + + defer func() { + // This is here to avoid data-trace on bytes buffer `bs` to capture logs + if err := logWriter.Close(); err != nil { + panic(err) + } + logFlushed.Wait() + }() + + logger := helmexec.NewLogger(logWriter, "debug") + + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + glob: filepath.Glob, + abs: filepath.Abs, + OverrideKubeContext: "default", + Env: "default", + Logger: logger, + helms: map[helmKey]helmexec.Interface{ + createHelmKey("helm", "default"): helm, + }, + }, tc.files) + + depsErr := app.Deps(depsConfig{ + skipRepos: false, + }) + + if tc.error == "" && depsErr != nil { + t.Fatalf("unexpected error for data defined at %s: %v", tc.loc, depsErr) + } else if tc.error != "" && depsErr == nil { + t.Fatalf("expected error did not occur for data defined at %s", tc.loc) + } else if tc.error != "" && depsErr != nil && tc.error != depsErr.Error() { + t.Fatalf("invalid error: expected %q, got %q", tc.error, depsErr.Error()) + } + + if !reflect.DeepEqual(helm.Charts, tc.charts) { + t.Fatalf("expected charts %v, got %v", helm.Charts, tc.charts) + } + }() + + if tc.log != "" { + actual := bs.String() + + diff, exists := testhelper.Diff(tc.log, actual, 3) + if exists { + t.Errorf("unexpected log for data defined %s:\nDIFF\n%s\nEOD", tc.loc, diff) + } + } + }) + } +} + func captureStdout(f func()) string { reader, writer, err := os.Pipe() if err != nil { diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index b9d675ad..401228eb 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -4,6 +4,8 @@ import ( "bytes" "errors" "fmt" + "path/filepath" + "github.com/imdario/mergo" "github.com/roboll/helmfile/pkg/environment" "github.com/roboll/helmfile/pkg/helmexec" @@ -11,7 +13,6 @@ import ( "github.com/roboll/helmfile/pkg/state" "github.com/variantdev/vals" "go.uber.org/zap" - "path/filepath" ) const ( @@ -25,12 +26,13 @@ type desiredStateLoader struct { env string namespace string - readFile func(string) ([]byte, error) - deleteFile func(string) error - fileExists func(string) (bool, error) - abs func(string) (string, error) - glob func(string) ([]string, error) - getHelm func(*state.HelmState) helmexec.Interface + readFile func(string) ([]byte, error) + deleteFile func(string) error + fileExists func(string) (bool, error) + abs func(string) (string, error) + glob func(string) ([]string, error) + directoryExistsAt func(string) bool + getHelm func(*state.HelmState) helmexec.Interface remote *remote.Remote logger *zap.SugaredLogger @@ -144,7 +146,7 @@ func (ld *desiredStateLoader) loadFileWithOverrides(inheritedEnv, overrodeEnv *e } func (a *desiredStateLoader) underlying() *state.StateCreator { - c := state.NewCreator(a.logger, a.readFile, a.fileExists, a.abs, a.glob, a.valsRuntime, a.getHelm, a.overrideHelmBinary, a.remote) + c := state.NewCreator(a.logger, a.readFile, a.fileExists, a.abs, a.glob, a.directoryExistsAt, a.valsRuntime, a.getHelm, a.overrideHelmBinary, a.remote) c.DeleteFile = a.deleteFile c.LoadFile = a.loadFile return c diff --git a/pkg/remote/remote_test.go b/pkg/remote/remote_test.go index 0f0cc6a3..70186c9b 100644 --- a/pkg/remote/remote_test.go +++ b/pkg/remote/remote_test.go @@ -2,19 +2,20 @@ package remote import ( "fmt" + "os" + "testing" + "github.com/google/go-cmp/cmp" "github.com/roboll/helmfile/pkg/helmexec" "github.com/roboll/helmfile/pkg/testhelper" - "os" - "testing" ) func TestRemote_HttpsGitHub(t *testing.T) { cleanfs := map[string]string{ - "path/to/home": "", + "/path/to/home": "", } cachefs := map[string]string{ - "path/to/home/.helmfile/cache/https_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml": "foo: bar", + "/path/to/home/.helmfile/cache/https_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml": "foo: bar", } type testcase struct { @@ -36,7 +37,7 @@ func TestRemote_HttpsGitHub(t *testing.T) { hit := true get := func(wd, src, dst string) error { - if wd != "path/to/home" { + if wd != "/path/to/home" { return fmt.Errorf("unexpected wd: %s", wd) } if src != "git::https://github.com/cloudposse/helmfiles.git?ref=0.40.0" { @@ -53,7 +54,7 @@ func TestRemote_HttpsGitHub(t *testing.T) { } remote := &Remote{ Logger: helmexec.NewLogger(os.Stderr, "debug"), - Home: "path/to/home", + Home: "/path/to/home", Getter: getter, ReadFile: testfs.ReadFile, FileExists: testfs.FileExistsAt, @@ -72,7 +73,7 @@ func TestRemote_HttpsGitHub(t *testing.T) { t.Fatalf("unexpected error: %v", err) } - if file != "path/to/home/.helmfile/cache/https_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml" { + if file != "/path/to/home/.helmfile/cache/https_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml" { t.Errorf("unexpected file located: %s", file) } @@ -88,10 +89,10 @@ func TestRemote_HttpsGitHub(t *testing.T) { func TestRemote_SShGitHub(t *testing.T) { cleanfs := map[string]string{ - "path/to/home": "", + "/path/to/home": "", } cachefs := map[string]string{ - "path/to/home/.helmfile/cache/ssh_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml": "foo: bar", + "/path/to/home/.helmfile/cache/ssh_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml": "foo: bar", } type testcase struct { @@ -113,7 +114,7 @@ func TestRemote_SShGitHub(t *testing.T) { hit := true get := func(wd, src, dst string) error { - if wd != "path/to/home" { + if wd != "/path/to/home" { return fmt.Errorf("unexpected wd: %s", wd) } if src != "git::ssh://git@github.com/cloudposse/helmfiles.git?ref=0.40.0" { @@ -130,7 +131,7 @@ func TestRemote_SShGitHub(t *testing.T) { } remote := &Remote{ Logger: helmexec.NewLogger(os.Stderr, "debug"), - Home: "path/to/home", + Home: "/path/to/home", Getter: getter, ReadFile: testfs.ReadFile, FileExists: testfs.FileExistsAt, @@ -143,7 +144,7 @@ func TestRemote_SShGitHub(t *testing.T) { t.Fatalf("unexpected error: %v", err) } - if file != "path/to/home/.helmfile/cache/ssh_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml" { + if file != "/path/to/home/.helmfile/cache/ssh_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml" { t.Errorf("unexpected file located: %s", file) } diff --git a/pkg/state/create.go b/pkg/state/create.go index e8784336..2706ab3f 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -4,11 +4,12 @@ import ( "bytes" "errors" "fmt" - "github.com/roboll/helmfile/pkg/helmexec" - "github.com/roboll/helmfile/pkg/remote" "io" "os" + "github.com/roboll/helmfile/pkg/helmexec" + "github.com/roboll/helmfile/pkg/remote" + "github.com/imdario/mergo" "github.com/roboll/helmfile/pkg/environment" "github.com/roboll/helmfile/pkg/maputil" @@ -39,12 +40,15 @@ func (e *UndefinedEnvError) Error() string { } type StateCreator struct { - logger *zap.SugaredLogger - readFile func(string) ([]byte, error) - fileExists func(string) (bool, error) - abs func(string) (string, error) - glob func(string) ([]string, error) - DeleteFile func(string) error + logger *zap.SugaredLogger + + readFile func(string) ([]byte, error) + fileExists func(string) (bool, error) + abs func(string) (string, error) + glob func(string) ([]string, error) + DeleteFile func(string) error + directoryExistsAt func(string) bool + valsRuntime vals.Evaluator Strict bool @@ -58,13 +62,16 @@ type StateCreator struct { remote *remote.Remote } -func NewCreator(logger *zap.SugaredLogger, readFile func(string) ([]byte, error), fileExists func(string) (bool, error), abs func(string) (string, error), glob func(string) ([]string, error), valsRuntime vals.Evaluator, getHelm func(*HelmState) helmexec.Interface, overrideHelmBinary string, remote *remote.Remote) *StateCreator { +func NewCreator(logger *zap.SugaredLogger, readFile func(string) ([]byte, error), fileExists func(string) (bool, error), abs func(string) (string, error), glob func(string) ([]string, error), directoryExistsAt func(string) bool, valsRuntime vals.Evaluator, getHelm func(*HelmState) helmexec.Interface, overrideHelmBinary string, remote *remote.Remote) *StateCreator { return &StateCreator{ - logger: logger, - readFile: readFile, - fileExists: fileExists, - abs: abs, - glob: glob, + logger: logger, + + readFile: readFile, + fileExists: fileExists, + abs: abs, + glob: glob, + directoryExistsAt: directoryExistsAt, + Strict: true, valsRuntime: valsRuntime, getHelm: getHelm, @@ -131,6 +138,7 @@ func (c *StateCreator) Parse(content []byte, baseDir, file string) (*HelmState, state.removeFile = os.Remove state.fileExists = c.fileExists state.glob = c.glob + state.directoryExistsAt = c.directoryExistsAt state.valsRuntime = c.valsRuntime return &state, nil diff --git a/pkg/state/create_test.go b/pkg/state/create_test.go index d9c113aa..656fc119 100644 --- a/pkg/state/create_test.go +++ b/pkg/state/create_test.go @@ -1,13 +1,14 @@ package state import ( - "github.com/roboll/helmfile/pkg/remote" "io/ioutil" "os" "path/filepath" "reflect" "testing" + "github.com/roboll/helmfile/pkg/remote" + "github.com/roboll/helmfile/pkg/testhelper" "go.uber.org/zap" @@ -83,7 +84,7 @@ func (testEnv stateTestEnv) MustLoadState(t *testing.T, file, envName string) *H } r := remote.NewRemote(logger, testFs.Cwd, testFs.ReadFile, testFs.DirectoryExistsAt, testFs.FileExistsAt) - state, err := NewCreator(logger, testFs.ReadFile, testFs.FileExists, testFs.Abs, testFs.Glob, nil, nil, "", r). + state, err := NewCreator(logger, testFs.ReadFile, testFs.FileExists, testFs.Abs, testFs.Glob, testFs.DirectoryExistsAt, nil, nil, "", r). ParseAndLoad([]byte(yamlContent), filepath.Dir(file), file, envName, true, nil) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -148,7 +149,7 @@ releaseNamespace: mynamespace testFs.Cwd = "/example/path/to" r := remote.NewRemote(logger, testFs.Cwd, testFs.ReadFile, testFs.DirectoryExistsAt, testFs.FileExistsAt) - state, err := NewCreator(logger, testFs.ReadFile, testFs.FileExists, testFs.Abs, testFs.Glob, nil, nil, "", r). + state, err := NewCreator(logger, testFs.ReadFile, testFs.FileExists, testFs.Abs, testFs.Glob, testFs.DirectoryExistsAt, nil, nil, "", r). ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, nil) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -235,7 +236,7 @@ overrideNamespace: myns testFs.Cwd = "/example/path/to" r := remote.NewRemote(logger, testFs.Cwd, testFs.ReadFile, testFs.DirectoryExistsAt, testFs.FileExistsAt) - state, err := NewCreator(logger, testFs.ReadFile, testFs.FileExists, testFs.Abs, testFs.Glob, nil, nil, "", r). + state, err := NewCreator(logger, testFs.ReadFile, testFs.FileExists, testFs.Abs, testFs.Glob, testFs.DirectoryExistsAt, nil, nil, "", r). ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, nil) if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/pkg/state/state.go b/pkg/state/state.go index 450e4345..e03f74f6 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -82,12 +82,12 @@ type HelmState struct { logger *zap.SugaredLogger - readFile func(string) ([]byte, error) - - removeFile func(string) error - fileExists func(string) (bool, error) - glob func(string) ([]string, error) - tempDir func(string, string) (string, error) + readFile func(string) ([]byte, error) + removeFile func(string) error + fileExists func(string) (bool, error) + glob func(string) ([]string, error) + tempDir func(string, string) (string, error) + directoryExistsAt func(string) bool runner helmexec.Runner valsRuntime vals.Evaluator @@ -833,6 +833,7 @@ func releasesNeedCharts(releases []ReleaseSpec) []ReleaseSpec { type ChartPrepareOptions struct { ForceDownload bool SkipRepos bool + SkipDeps bool SkipResolve bool } @@ -923,7 +924,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre chartName := release.Chart - isLocal := pathExists(normalizeChart(st.basePath, chartName)) + isLocal := st.directoryExistsAt(normalizeChart(st.basePath, chartName)) chartPath, err := st.goGetterChart(chartName, release.Directory, release.ForceGoGetter) if err != nil { @@ -942,7 +943,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre var buildDeps bool - skipDepsGlobal := opts.SkipRepos + skipDepsGlobal := opts.SkipDeps skipDepsRelease := release.SkipDeps != nil && *release.SkipDeps skipDepsDefault := release.SkipDeps == nil && st.HelmDefaults.SkipDeps skipDeps := !isLocal || skipDepsGlobal || skipDepsRelease || skipDepsDefault @@ -970,7 +971,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre // Skip `helm dep build` and `helm dep up` altogether when the chart is from remote or the dep is // explicitly skipped. buildDeps = !skipDeps - } else if pathExists(normalizeChart(st.basePath, chartPath)) { + } else if normalizedChart := normalizeChart(st.basePath, chartPath); st.directoryExistsAt(normalizedChart) { // At this point, we are sure that chartPath is a local directory containing either: // - A remote chart fetched by go-getter or // - A local chart @@ -994,7 +995,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre // Given that, we always run `helm dep build` on the chart here, but tolerate any error caused by it // for a remote chart, so that the user can notice/fix the issue in a local chart while // a broken remote chart won't completely block their job. - chartPath = normalizeChart(st.basePath, chartPath) + chartPath = normalizedChart buildDeps = !skipDeps } else if !opts.ForceDownload { @@ -1929,10 +1930,12 @@ func (st *HelmState) UpdateDeps(helm helmexec.Interface) []error { var errs []error for _, release := range st.Releases { - if isLocalChart(release.Chart) { - if err := helm.UpdateDeps(normalizeChart(st.basePath, release.Chart)); err != nil { + if st.directoryExistsAt(release.Chart) { + if err := helm.UpdateDeps(release.Chart); err != nil { errs = append(errs, err) } + } else { + st.logger.Debugf("skipped updating dependencies for remote chart %s", release.Chart) } } @@ -1953,11 +1956,6 @@ func (st *HelmState) UpdateDeps(helm helmexec.Interface) []error { return nil } -func pathExists(chart string) bool { - _, err := os.Stat(chart) - return err == nil -} - func chartNameWithoutRepository(chart string) string { chartSplit := strings.Split(chart, "/") return chartSplit[len(chartSplit)-1] diff --git a/pkg/state/state_test.go b/pkg/state/state_test.go index 7b28ea62..c46ccebd 100644 --- a/pkg/state/state_test.go +++ b/pkg/state/state_test.go @@ -22,6 +22,7 @@ func injectFs(st *HelmState, fs *testhelper.TestFs) *HelmState { st.glob = fs.Glob st.readFile = fs.ReadFile st.fileExists = fs.FileExists + st.directoryExistsAt = fs.DirectoryExistsAt return st } @@ -786,15 +787,23 @@ func Test_normalizeChart(t *testing.T) { { name: "construct local chart path", args: args{ - basePath: "/Users/jane/code/deploy/charts", + basePath: "/src", chart: "./app", }, - want: "/Users/jane/code/deploy/charts/app", + want: "/src/app", + }, + { + name: "construct local chart path, without leading dot", + args: args{ + basePath: "/src", + chart: "published", + }, + want: "/src/published", }, { name: "repo path", args: args{ - basePath: "/Users/jane/code/deploy/charts", + basePath: "/src", chart: "remote/app", }, want: "remote/app", @@ -802,18 +811,26 @@ func Test_normalizeChart(t *testing.T) { { name: "chartcenter repo path", args: args{ - basePath: "/Users/jane/code/deploy/charts", + basePath: "/src", chart: "center/stable/myapp", }, want: "center/stable/myapp", }, { - name: "construct local chart path, parent dir", + name: "construct local chart path, sibling dir", args: args{ - basePath: "/Users/jane/code/deploy/charts", + basePath: "/src", chart: "../app", }, - want: "/Users/jane/code/deploy/app", + want: "/app", + }, + { + name: "construct local chart path, parent dir", + args: args{ + basePath: "/src", + chart: "./..", + }, + want: "/", }, { name: "too much parent levels", @@ -1752,22 +1769,17 @@ generated: 2019-05-16T15:42:45.50486+09:00 } logger := helmexec.NewLogger(os.Stderr, "debug") + basePath := "/src" state := &HelmState{ - basePath: "/src", + basePath: basePath, FilePath: "/src/helmfile.yaml", ReleaseSetSpec: ReleaseSetSpec{ Releases: []ReleaseSpec{ { - Chart: "./..", + Chart: "/example", }, { - Chart: "../examples", - }, - { - Chart: "../../helmfile", - }, - { - Chart: "published", + Chart: "./example", }, { Chart: "published/deeper", @@ -1792,25 +1804,32 @@ generated: 2019-05-16T15:42:45.50486+09:00 logger: logger, } + fs := testhelper.NewTestFs(map[string]string{ + "/example/Chart.yaml": `foo: FOO`, + "/src/example/Chart.yaml": `foo: FOO`, + }) + fs.Cwd = basePath + state = injectFs(state, fs) errs := state.UpdateDeps(helm) - want := []string{"/", "/examples", "/helmfile", "/src/published", generatedDir} + + want := []string{"/example", "./example", generatedDir} if !reflect.DeepEqual(helm.Charts, want) { t.Errorf("HelmState.UpdateDeps() = %v, want %v", helm.Charts, want) } if len(errs) != 0 { - t.Errorf("HelmState.UpdateDeps() - no errors, but got %d: %v", len(errs), errs) + t.Errorf("HelmState.UpdateDeps() - unexpected %d errors: %v", len(errs), errs) } resolved, err := state.ResolveDeps() if err != nil { - t.Errorf("unexpected error: %v", err) + t.Errorf("HelmState.ResolveDeps() - unexpected error: %v", err) } - if resolved.Releases[5].Version != "1.5.0" { - t.Errorf("unexpected version number: expected=1.5.0, got=%s", resolved.Releases[5].Version) + if resolved.Releases[3].Version != "1.5.0" { + t.Errorf("HelmState.ResolveDeps() - unexpected version number: expected=1.5.0, got=%s", resolved.Releases[5].Version) } - if resolved.Releases[6].Version != "1.4.0" { - t.Errorf("unexpected version number: expected=1.4.0, got=%s", resolved.Releases[6].Version) + if resolved.Releases[4].Version != "1.4.0" { + t.Errorf("HelmState.ResolveDeps() - unexpected version number: expected=1.4.0, got=%s", resolved.Releases[6].Version) } } diff --git a/pkg/testhelper/testfs.go b/pkg/testhelper/testfs.go index f28d818a..90b58842 100644 --- a/pkg/testhelper/testfs.go +++ b/pkg/testhelper/testfs.go @@ -39,7 +39,7 @@ func NewTestFs(files map[string]string) *TestFs { func (f *TestFs) FileExistsAt(path string) bool { var ok bool - if strings.Contains(path, "/") { + if strings.HasPrefix(path, "/") { _, ok = f.files[path] } else { _, ok = f.files[filepath.Join(f.Cwd, path)] @@ -53,7 +53,7 @@ func (f *TestFs) FileExists(path string) (bool, error) { func (f *TestFs) DirectoryExistsAt(path string) bool { var ok bool - if strings.Contains(path, "/") { + if strings.HasPrefix(path, "/") { _, ok = f.dirs[path] } else { _, ok = f.dirs[filepath.Join(f.Cwd, path)] @@ -64,7 +64,7 @@ func (f *TestFs) DirectoryExistsAt(path string) bool { func (f *TestFs) ReadFile(filename string) ([]byte, error) { var str string var ok bool - if filename[0] == '/' { + if strings.HasPrefix(filename, "/") { str, ok = f.files[filename] } else { str, ok = f.files[filepath.Join(f.Cwd, filename)] @@ -90,7 +90,7 @@ func (f *TestFs) FileReaderCalls() int { func (f *TestFs) Glob(relPattern string) ([]string, error) { var pattern string - if relPattern[0] == '/' { + if strings.HasPrefix(relPattern, "/") { pattern = relPattern } else { pattern = filepath.Join(f.Cwd, relPattern) @@ -116,7 +116,7 @@ func (f *TestFs) Glob(relPattern string) ([]string, error) { func (f *TestFs) Abs(path string) (string, error) { var p string - if path[0] == '/' { + if strings.HasPrefix(path, "/") { p = path } else { p = filepath.Join(f.Cwd, path)