Fix the logic of helmfile deps and add tests.

[wi1dcard]

Context

helmfile.yaml:

repositories:
  - name: bitnami
    url: https://charts.bitnami.com/bitnami/

releases:
  - name: example # this chart contains a dep
    chart: ./charts/example

./charts/example/Chart.yaml:

apiVersion: v2
name: example
description: A Helm chart for Kubernetes

type: application
version: 0.1.0
appVersion: 1.16.0

dependencies:
  - name: redis
    version: ^10.7.0
    repository: alias:bitnami

When I run helmfile deps --skip-repos with the helmfile.yaml above, it skips updating the deps of the local chart example:

$ helmfile deps --skip-repos
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.

What I was expecting is that helmfile updates all the dependencies of the chart, like what helmfiles does without the option --skip-repos (but skips adding repos of course):

$ helmfile deps
Adding repo bitnami https://charts.bitnami.com/bitnami/
"bitnami" has been added to your repositories

Building dependency release=darwinia-bridge, chart=charts/darwinia-bridge
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "bitnami" chart repository
Update Complete. ⎈Happy Helming!⎈
Saving 1 charts
Downloading redis from repo https://charts.bitnami.com/bitnami/
Deleting outdated charts

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.

The Problem

While I was digging into this issue, I found that the problem is not caused by --skip-repos itself.

In fact, logics in UpdateDeps:

helmfile/pkg/state/state.go

Lines 1928 to 1937 in f6bf885

	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 {
	errs = append(errs, err)
	}
	}
	}

will never get executed, as release.Chart is already "normalized path", which means, it won't be considered as a local chart anymore. Here is where the chart get "normalized":

helmfile/pkg/app/run.go

Lines 62 to 74 in f6bf885

	releaseToChart, errs := r.state.PrepareCharts(r.helm, dir, 2, helmfileCommand, opts)
	if len(errs) > 0 {
	return fmt.Errorf("%v", errs)
	}
	for i := range r.state.Releases {
	rel := &r.state.Releases[i]
	if chart := releaseToChart[rel.Name]; chart != "" {
	rel.Chart = chart
	}
	}

releaseToChart contains the paths proceeded by normalizeChart(st.basePath, chartPath), and rel.Chart = chart overrides the original chart paths in releases.

In order to fix that, I've decided not to break the current behavior of PrepareCharts but change the logic of determining if it's a local chart.

Firstly, I changed if isLocalChart(release.Chart) to if pathExists(release.Chart). However, I changed it again to the func DirectoryExistsAt to make it mockable in unit tests.

Other Changes in the PR

Add SkipDeps in struct ChartPrepareOptions

PrepareCharts skips executing helm dep build when SkipRepos is true:

helmfile/pkg/state/state.go

Lines 943 to 948 in f6bf885

	var buildDeps bool
	skipDepsGlobal := opts.SkipRepos
	skipDepsRelease := release.SkipDeps != nil && *release.SkipDeps
	skipDepsDefault := release.SkipDeps == nil && st.HelmDefaults.SkipDeps
	skipDeps := !isLocal || skipDepsGlobal || skipDepsRelease || skipDepsDefault

helmfile/pkg/state/state.go

Lines 1086 to 1090 in f6bf885

	if len(builds) > 0 {
	if err := st.runHelmDepBuilds(helm, concurrency, builds); err != nil {
	return nil, []error{err}
	}
	}

Therefore, if I run helmfile deps --skip-repos:

1. PrepareCharts not executing st.runHelmDepBuilds. ✅
2. Then execute UpdateDeps. ✅

If run helmfile deps:

1. PrepareCharts executing st.runHelmDepBuilds. ❌ (Unnecessary)
2. Then execute UpdateDeps. ✅

PrepareCharts shouldn't use SkipRepos as the condition of running helm dep build or not. So I added another flag SkipDeps in struct to control whether PrepareCharts needs to build the deps.

Helper functions in testfs

helmfile/pkg/testhelper/testfs.go

Lines 54 to 62 in f6bf885

	func (f *TestFs) DirectoryExistsAt(path string) bool {
	var ok bool
	if strings.Contains(path, "/") {
	_, ok = f.dirs[path]
	} else {
	_, ok = f.dirs[filepath.Join(f.Cwd, path)]
	}
	return ok
	}

Determining absolute paths with strings.Contains sounds not ideal. Changed to strings.HasPrefix(path, "/").

helmfile/pkg/testhelper/testfs.go

Lines 64 to 74 in f6bf885

	func (f *TestFs) ReadFile(filename string) ([]byte, error) {
	var str string
	var ok bool
	if filename[0] == '/' {
	str, ok = f.files[filename]
	} else {
	str, ok = f.files[filepath.Join(f.Cwd, filename)]
	}
	if !ok {
	return []byte(nil), os.ErrNotExist
	}

path[0] == '/' can lead to nil pointer error with empty string path. Changed to strings.HasPrefix(path, "/").

Add and fix tests

• Move several test cases from TestHelmState_UpdateDeps to Test_normalizeChart.
• Mock the fs and add test cases in TestHelmState_UpdateDeps.
• Add TestDeps in pkg/app/app_test.go.
• Fix tests in pkg/remote/remote_test.go with respect to the changes of testfs.
• Output logs when skipping updating deps for remote chart.

[mumoshu]

LGTM. Thank you so much for your continued effort @wi1dcard ☺️

Firstly, I changed if isLocalChart(release.Chart) to if pathExists(release.Chart). However, I changed it again to the func DirectoryExistsAt to make it mockable in unit tests.

Note: I initially thought this can break it when chart referenced a local tarball e.g. chart: envoy-1.9.3.tgz. But after seeingin L974 in pkg/state/state.go, the move to directoryExistsAt might be a good thing, as you can't run helm dep build on a tarball, obviously.. 😃