From 9a03d79a841a753a74db31c24e3884eb05e0ec94 Mon Sep 17 00:00:00 2001 From: KUOKA Yusuke Date: Sat, 8 Aug 2020 21:22:57 +0900 Subject: [PATCH] Fix chart fetched by go-getter not to fail due to missing dependencies (#1408) Ref https://github.com/roboll/helmfile/issues/1401#issuecomment-670915272 --- pkg/state/state.go | 71 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 10 deletions(-) diff --git a/pkg/state/state.go b/pkg/state/state.go index 36504628..04126cab 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -819,6 +819,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre releaseName string chartPath string err error + diagnostic string } errs := []error{} @@ -838,6 +839,8 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre } *st = *updated + var diagnostics []string + st.scatterGather( concurrency, len(releases), @@ -867,6 +870,8 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre return } + chartFetchedByGoGetter := chartPath != chartName + chartification, clean, err := st.PrepareChartify(helm, release, chartPath, workerIndex) defer clean() if err != nil { @@ -874,6 +879,8 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre return } + var diagnostic string + if chartification != nil { c := chartify.New( chartify.HelmBin(st.DefaultHelmBinary), @@ -888,6 +895,50 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre // TODO Chartify chartPath = out } + } else if pathExists(normalizeChart(st.basePath, chartPath)) { + // 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 + // + // The chart may have Chart.yaml(and requirements.yaml for Helm 2), and optionally Chart.lock/requirements.lock, + // but no `charts/` directory populated at all, or a subet of chart dependencies are missing in the directory. + // + // In such situation, Helm fails with an error like: + // Error: found in Chart.yaml, but missing in charts/ directory: cert-manager, prometheus, postgresql, gitlab-runner, grafana, redis + // + // (See also https://github.com/roboll/helmfile/issues/1401#issuecomment-670854495) + // + // To avoid it, we need to call a `helm dep build` command on the chart. + // But the command may consistently fail when an outdated Chart.lock exists. + // + // (I've mentioned about such case in https://github.com/roboll/helmfile/pull/1400.) + // + // Trying to run `helm dep build` on the chart regardless of if it's from local or remote is + // problematic, as usually the user would have no way to fix the remote chart on their own. + // + // 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) + + if !skipDeps { + if err := helm.BuildDeps(release.Name, chartPath); err != nil { + if chartFetchedByGoGetter { + diagnostic = fmt.Sprintf( + "WARN: `helm dep build` failed. While processing release %q, Helmfile observed that remote chart %q fetched by go-getter is seemingly broken. "+ + "One of well-known causes of this is that the chart has outdated Chart.lock, which needs the chart maintainer needs to run `helm dep up`. "+ + "Helmfile is tolerating the error to avoid blocking you until the remote chart gets fixed. "+ + "But this may result in any failure later if the chart is broken badly. FYI, the tolerated error was: %v", + release.Name, + chartName, + err.Error(), + ) + } else { + results <- &downloadResults{err: err} + return + } + } + } } else if !forceDownload { // At this point, we are sure that either: // 1. It is a local chart and we can use it in later process (helm upgrade/template/lint/etc) @@ -895,15 +946,6 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre // 2. It is a remote chart which can be safely handed over to helm, // because the version of Helm used in this transaction support downloading the chart instead, // and we don't need any modification to the chart - } else if pathExists(normalizeChart(st.basePath, chartPath)) { - chartPath = normalizeChart(st.basePath, chartPath) - - if !skipDeps { - if err := helm.BuildDeps(release.Name, chartPath); err != nil { - results <- &downloadResults{err: err} - return - } - } } else { pathElems := []string{ dir, @@ -943,13 +985,17 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre } } - results <- &downloadResults{releaseName: release.Name, chartPath: chartPath} + results <- &downloadResults{releaseName: release.Name, chartPath: chartPath, diagnostic: diagnostic} } }, func() { for i := 0; i < len(releases); i++ { downloadRes := <-results + if d := downloadRes.diagnostic; d != "" { + diagnostics = append(diagnostics, d) + } + if downloadRes.err != nil { errs = append(errs, downloadRes.err) @@ -960,6 +1006,11 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre }, ) + sort.Strings(diagnostics) + for _, d := range diagnostics { + st.logger.Warn(d) + } + if len(errs) > 0 { return nil, errs }