diff --git a/cmd/helm/repo_update.go b/cmd/helm/repo_update.go index 27661674c..a05379cc4 100644 --- a/cmd/helm/repo_update.go +++ b/cmd/helm/repo_update.go @@ -41,7 +41,8 @@ To update all the repositories, use 'helm repo update'. var errNoRepositories = errors.New("no repositories found. You must add one before updating") type repoUpdateOptions struct { - update func([]*repo.ChartRepository, io.Writer, bool) error + showValidationErrors bool + update func([]*repo.ChartRepository, io.Writer, bool, bool) error repoFile string repoCache string names []string @@ -64,7 +65,7 @@ func newRepoUpdateCmd(out io.Writer) *cobra.Command { o.repoFile = settings.RepositoryConfig o.repoCache = settings.RepositoryCache o.names = args - return o.run(out) + return o.run(out, args) }, } @@ -74,10 +75,12 @@ func newRepoUpdateCmd(out io.Writer) *cobra.Command { // This should be deprecated in Helm 4 by update to the behaviour of `helm repo update` command. f.BoolVar(&o.failOnRepoUpdateFail, "fail-on-repo-update-fail", false, "update fails if any of the repository updates fail") + f.BoolVar(&o.showValidationErrors, "show-repo-validation-errors", false, "show every invalid chart while indexing repository") + return cmd } -func (o *repoUpdateOptions) run(out io.Writer) error { +func (o *repoUpdateOptions) run(out io.Writer, args []string) error { f, err := repo.LoadFile(o.repoFile) switch { case isNotExist(err): @@ -111,10 +114,10 @@ func (o *repoUpdateOptions) run(out io.Writer) error { } } - return o.update(repos, out, o.failOnRepoUpdateFail) + return o.update(repos, out, o.failOnRepoUpdateFail, o.showValidationErrors) } -func updateCharts(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error { +func updateCharts(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool, showValidationErrors bool) error { fmt.Fprintln(out, "Hang tight while we grab the latest from your chart repositories...") var wg sync.WaitGroup var repoFailList []string @@ -122,7 +125,13 @@ func updateCharts(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdate wg.Add(1) go func(re *repo.ChartRepository) { defer wg.Done() - if _, err := re.DownloadIndexFile(); err != nil { + var err error + if showValidationErrors { + _, err = re.ValidateAndDownloadIndexFile() + } else { + _, err = re.DownloadIndexFile() + } + if err != nil { fmt.Fprintf(out, "...Unable to get an update from the %q chart repository (%s):\n\t%s\n", re.Config.Name, re.Config.URL, err) repoFailList = append(repoFailList, re.Config.URL) } else { diff --git a/cmd/helm/repo_update_test.go b/cmd/helm/repo_update_test.go index cf2136ff0..08d6e29b1 100644 --- a/cmd/helm/repo_update_test.go +++ b/cmd/helm/repo_update_test.go @@ -35,7 +35,7 @@ func TestUpdateCmd(t *testing.T) { var out bytes.Buffer // Instead of using the HTTP updater, we provide our own for this test. // The TestUpdateCharts test verifies the HTTP behavior independently. - updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error { + updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool, showValidationErrors bool) error { for _, re := range repos { fmt.Fprintln(out, re.Config.Name) } @@ -45,7 +45,7 @@ func TestUpdateCmd(t *testing.T) { update: updater, repoFile: "testdata/repositories.yaml", } - if err := o.run(&out); err != nil { + if err := o.run(&out, []string{}); err != nil { t.Fatal(err) } @@ -60,7 +60,7 @@ func TestUpdateCmdMultiple(t *testing.T) { var out bytes.Buffer // Instead of using the HTTP updater, we provide our own for this test. // The TestUpdateCharts test verifies the HTTP behavior independently. - updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error { + updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool, showValidationErrors bool) error { for _, re := range repos { fmt.Fprintln(out, re.Config.Name) } @@ -71,7 +71,7 @@ func TestUpdateCmdMultiple(t *testing.T) { repoFile: "testdata/repositories.yaml", names: []string{"firstexample", "charts"}, } - if err := o.run(&out); err != nil { + if err := o.run(&out, []string{}); err != nil { t.Fatal(err) } @@ -86,7 +86,7 @@ func TestUpdateCmdInvalid(t *testing.T) { var out bytes.Buffer // Instead of using the HTTP updater, we provide our own for this test. // The TestUpdateCharts test verifies the HTTP behavior independently. - updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error { + updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool, showValidationErrors bool) error { for _, re := range repos { fmt.Fprintln(out, re.Config.Name) } @@ -97,7 +97,7 @@ func TestUpdateCmdInvalid(t *testing.T) { repoFile: "testdata/repositories.yaml", names: []string{"firstexample", "invalid"}, } - if err := o.run(&out); err == nil { + if err := o.run(&out, []string{}); err == nil { t.Fatal("expected error but did not get one") } } @@ -120,7 +120,7 @@ func TestUpdateCustomCacheCmd(t *testing.T) { repoCache: cachePath, } b := ioutil.Discard - if err := o.run(b); err != nil { + if err := o.run(b, []string{}); err != nil { t.Fatal(err) } if _, err := os.Stat(filepath.Join(cachePath, "test-index.yaml")); err != nil { @@ -147,7 +147,7 @@ func TestUpdateCharts(t *testing.T) { } b := bytes.NewBuffer(nil) - updateCharts([]*repo.ChartRepository{r}, b, false) + updateCharts([]*repo.ChartRepository{r}, b, false, false) got := b.String() if strings.Contains(got, "Unable to get an update") { @@ -183,7 +183,7 @@ func TestUpdateChartsFail(t *testing.T) { } b := bytes.NewBuffer(nil) - if err := updateCharts([]*repo.ChartRepository{r}, b, false); err != nil { + if err := updateCharts([]*repo.ChartRepository{r}, b, false, false); err != nil { t.Error("Repo update should not return error if update of repository fails") } @@ -216,7 +216,50 @@ func TestUpdateChartsFailWithError(t *testing.T) { } b := bytes.NewBuffer(nil) - err = updateCharts([]*repo.ChartRepository{r}, b, true) + err = updateCharts([]*repo.ChartRepository{r}, b, true, false) + if err == nil { + t.Error("Repo update should return error because update of repository fails and 'fail-on-repo-update-fail' flag set") + return + } + var expectedErr = "Failed to update the following repositories" + var receivedErr = err.Error() + if !strings.Contains(receivedErr, expectedErr) { + t.Errorf("Expected error (%s) but got (%s) instead", expectedErr, receivedErr) + } + if !strings.Contains(receivedErr, invalidURL) { + t.Errorf("Expected invalid URL (%s) in error message but got (%s) instead", invalidURL, receivedErr) + } + + got := b.String() + if !strings.Contains(got, "Unable to get an update") { + t.Errorf("Repo should have failed update but instead got: %q", got) + } + if strings.Contains(got, "Update Complete.") { + t.Error("Update was not successful and should return error message because 'fail-on-repo-update-fail' flag set") + } +} + +func TestUpdateChartsShowValidationErrors(t *testing.T) { + defer resetEnv()() + defer ensure.HelmHome(t)() + + ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") + if err != nil { + t.Fatal(err) + } + defer ts.Stop() + + var invalidURL = ts.URL() + "55" + r, err := repo.NewChartRepository(&repo.Entry{ + Name: "charts", + URL: invalidURL, + }, getter.All(settings)) + if err != nil { + t.Error(err) + } + + b := bytes.NewBuffer(nil) + err = updateCharts([]*repo.ChartRepository{r}, b, true, true) if err == nil { t.Error("Repo update should return error because update of repository fails and 'fail-on-repo-update-fail' flag set") return diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index 48fbe3856..06798c5ff 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -116,6 +116,32 @@ func (r *ChartRepository) Load() error { // DownloadIndexFile fetches the index from a repository. func (r *ChartRepository) DownloadIndexFile() (string, error) { + return r.downloadIndexFile(true) +} + +// ValidateAndDownloadIndexFile fetches the index without consolidating validation warnings. +func (r *ChartRepository) ValidateAndDownloadIndexFile() (string, error) { + return r.downloadIndexFile(false) +} + +// Index generates an index for the chart repository and writes an index.yaml file. +func (r *ChartRepository) Index() error { + err := r.generateIndex() + if err != nil { + return err + } + return r.saveIndexFile() +} + +func (r *ChartRepository) saveIndexFile() error { + index, err := yaml.Marshal(r.IndexFile) + if err != nil { + return err + } + return ioutil.WriteFile(filepath.Join(r.Config.Name, indexPath), index, 0644) +} + +func (r *ChartRepository) downloadIndexFile(consolidateWarnings bool) (string, error) { parsedURL, err := url.Parse(r.Config.URL) if err != nil { return "", err @@ -141,7 +167,7 @@ func (r *ChartRepository) DownloadIndexFile() (string, error) { return "", err } - indexFile, err := loadIndex(index, r.Config.URL) + indexFile, err := loadIndex(index, r.Config.URL, consolidateWarnings) if err != nil { return "", err } @@ -161,23 +187,6 @@ func (r *ChartRepository) DownloadIndexFile() (string, error) { return fname, ioutil.WriteFile(fname, index, 0644) } -// Index generates an index for the chart repository and writes an index.yaml file. -func (r *ChartRepository) Index() error { - err := r.generateIndex() - if err != nil { - return err - } - return r.saveIndexFile() -} - -func (r *ChartRepository) saveIndexFile() error { - index, err := yaml.Marshal(r.IndexFile) - if err != nil { - return err - } - return ioutil.WriteFile(filepath.Join(r.Config.Name, indexPath), index, 0644) -} - func (r *ChartRepository) generateIndex() error { for _, path := range r.ChartPaths { ch, err := loader.Load(path) diff --git a/pkg/repo/index.go b/pkg/repo/index.go index 60cfe5801..f8118a23e 100644 --- a/pkg/repo/index.go +++ b/pkg/repo/index.go @@ -108,7 +108,7 @@ func LoadIndexFile(path string) (*IndexFile, error) { if err != nil { return nil, err } - i, err := loadIndex(b, path) + i, err := loadIndex(b, path, true) if err != nil { return nil, errors.Wrapf(err, "error loading %s", path) } @@ -330,7 +330,7 @@ func IndexDirectory(dir, baseURL string) (*IndexFile, error) { // // The source parameter is only used for logging. // This will fail if API Version is not set (ErrNoAPIVersion) or if the unmarshal fails. -func loadIndex(data []byte, source string) (*IndexFile, error) { +func loadIndex(data []byte, source string, consolidateValidationErrors bool) (*IndexFile, error) { i := &IndexFile{} if len(data) == 0 { @@ -341,6 +341,7 @@ func loadIndex(data []byte, source string) (*IndexFile, error) { return i, err } + invalidCharts := 0 for name, cvs := range i.Entries { for idx := len(cvs) - 1; idx >= 0; idx-- { if cvs[idx] == nil { @@ -351,11 +352,17 @@ func loadIndex(data []byte, source string) (*IndexFile, error) { cvs[idx].APIVersion = chart.APIVersionV1 } if err := cvs[idx].Validate(); err != nil { - log.Printf("skipping loading invalid entry for chart %q %q from %s: %s", name, cvs[idx].Version, source, err) + invalidCharts++ + if !consolidateValidationErrors { + log.Printf("skipping loading invalid entry for chart %q %q from %s: %s", name, cvs[idx].Version, source, err) + } cvs = append(cvs[:idx], cvs[idx+1:]...) } } } + if invalidCharts > 0 && consolidateValidationErrors { + log.Printf("skipped loading %d invalid chart entries from %s\nrun 'helm repo update --show-repo-validation-errors' for full list of invalid entries", invalidCharts, source) + } i.SortEntries() if i.APIVersion == "" { return i, ErrNoAPIVersion diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go index 2403e9a71..2ba5b8b64 100644 --- a/pkg/repo/index_test.go +++ b/pkg/repo/index_test.go @@ -156,7 +156,7 @@ func TestLoadIndex(t *testing.T) { // TestLoadIndex_Duplicates is a regression to make sure that we don't non-deterministically allow duplicate packages. func TestLoadIndex_Duplicates(t *testing.T) { - if _, err := loadIndex([]byte(indexWithDuplicates), "indexWithDuplicates"); err == nil { + if _, err := loadIndex([]byte(indexWithDuplicates), "indexWithDuplicates", false); err == nil { t.Errorf("Expected an error when duplicate entries are present") } } @@ -168,7 +168,7 @@ func TestLoadIndex_EmptyEntry(t *testing.T) { } func TestLoadIndex_Empty(t *testing.T) { - if _, err := loadIndex([]byte(""), "indexWithEmpty"); err == nil { + if _, err := loadIndex([]byte(""), "indexWithEmpty", false); err == nil { t.Errorf("Expected an error when index.yaml is empty.") } }